Add secrets config: ignore/only paths, gitignore, generated files, max file size - #942
Add secrets config: ignore/only paths, gitignore, generated files, max file size#942MikaYuoadas wants to merge 5 commits into
Conversation
Split the monolithic CliConfiguration into RunConfiguration, SastConfiguration, and SecretsConfiguration, and break main() into per-product functions (resolve_sast/resolve_secrets, select_sast_files/select_secrets_files, run_sast/run_secrets, etc). SAST's and secrets' settings can no longer leak into one another. This groundwork will make it easier to fork the binary into separate SAST-only and secrets-only codebases.
|
🔄 Datadog auto-retried 1 job - 1 passed on retry 🎯 Code Coverage (details) 🔗 Commit SHA: 186b77a | Docs | Datadog PR Page | Give us feedback! |
There was a problem hiding this comment.
Pull request overview
This PR refactors CLI configuration so SAST and secrets have independent config parsing and independent file-selection controls, preventing one product’s settings from silently affecting the other. It also factors shared primitives (schema version + path include/exclude) into common and updates binaries/server call sites accordingly.
Changes:
- Split former shared CLI config into
RunConfiguration,SastConfiguration, andSecretsConfiguration, and update consumers to use per-product configs. - Add an independent secrets YAML parser for the
secretssection and wire it into local/remote config loading. - Introduce product-scoped file selection (
ignore/only paths, gitignore, generated files, max size) so SAST and secrets build separate file lists.
Reviewed changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/static-analysis-server/src/request.rs | Use SAST-only config when building rule config provider for server requests. |
| crates/static-analysis-kernel/src/rule_overrides.rs | Make overrides builder accept SAST section directly (optional). |
| crates/static-analysis-kernel/src/rule_config.rs | Build rule config provider from optional SAST config section only. |
| crates/static-analysis-kernel/src/config/file_v1.rs | Re-home YAML path config/schema imports; remove duplicated YamlPathConfig. |
| crates/static-analysis-kernel/src/config/common.rs | Re-export shared config/path primitives from common; simplify parsing logic. |
| crates/static-analysis-kernel/src/arguments.rs | Read rule arguments from optional SAST config section. |
| crates/static-analysis-kernel/src/analysis/analyze.rs | Update tests to build rule config provider from local_config.sast(). |
| crates/secrets/src/lib.rs | Export new config module. |
| crates/secrets/src/config/file_v1.rs | Add secrets-only v1.x parser for secrets.global-config options. |
| crates/secrets/src/config.rs | Add secrets config module root. |
| crates/secrets/Cargo.toml | Add serde_yaml and thiserror for secrets config parsing. |
| crates/common/src/model/path_config.rs | Add shared PathConfig / PathPattern / YamlPathConfig for all products. |
| crates/common/src/model/config_method.rs | Add shared ConfigMethod + YamlSchemaVersion (and tests). |
| crates/common/src/model.rs | Export new config_method and path_config modules. |
| crates/common/Cargo.toml | Add globset + dev serde_yaml for new shared model modules. |
| crates/cli/src/utils.rs | Split configuration printing into run/SAST/secrets sections. |
| crates/cli/src/sarif/sarif_utils.rs | Update SARIF generation to accept run/SAST/secrets configs separately. |
| crates/cli/src/rule_utils.rs | Make rule conversion consume SAST config (rules + perf flag). |
| crates/cli/src/model/secrets_configuration.rs | Add secrets-only CLI configuration struct (rules + path selection + max size). |
| crates/cli/src/model/sast_configuration.rs | Rename/re-scope former CLI config into SAST-only config and update diff-aware hashing. |
| crates/cli/src/model/run_configuration.rs | Add run-level config shared across products (paths/output/threads/flags). |
| crates/cli/src/model.rs | Update model exports for new configuration split. |
| crates/cli/src/file_utils.rs | Introduce ProductFileSelection + select_files to build per-product file lists. |
| crates/cli/src/config_file.rs | Add ConfigFile wrapper and parse SAST/secrets sections independently based on enabled products. |
| crates/bins/src/lib.rs | Split static vs secrets analysis function signatures to take run + per-product configs. |
| crates/bins/src/git_history.rs | Use secrets-specific max file size for history scanning; adapt to new config split. |
| crates/bins/src/bin/datadog-static-analyzer.rs | Major CLI refactor: parse args, load config, resolve per-product configs, run products independently, build report. |
| crates/bins/src/bin/datadog-static-analyzer-git-hook.rs | Update git-hook binary to new config split and per-product file selection. |
| Cargo.lock | Lockfile updates for new dependencies (globset, serde_yaml, thiserror). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
df86dd8 to
186b77a
Compare
| use kernel::analysis::ddsa_lib::v8_platform::{initialize_v8, Initialized, V8Platform}; | ||
| use kernel::classifiers::ArtifactClassification; | ||
| use kernel::config::common::{ConfigMethod, PathConfig}; | ||
| use kernel::config::file_v1; |
There was a problem hiding this comment.
Please don't change the import to remove this qualifier. It's done to disambiguate from a "legacy" config.
| pub fn from_config(cfg: &file_v1::ConfigFile) -> RuleConfigProvider { | ||
| /// Builds a provider from SAST's own configuration section. Secrets doesn't have (or need) | ||
| /// an equivalent, since it doesn't support per-rule path/argument/severity overrides. | ||
| pub fn from_config(sast_config: Option<&file_v1::SastConfig>) -> RuleConfigProvider { |
There was a problem hiding this comment.
Please don't change the function signature to accept an Option<T>. It's a code smell
By doing this, you are encoding business logic ("doesn't support per-rule path/argument/severity overrides") into a RuleConfigProvider, which shouldn't know anything about that.
If you need to split these two, change it to from_sast_config, and add from_secrets_config.
| /// `parse_sast`/`parse_secrets` should match whether each product is enabled: a product's | ||
| /// section is only ever read and validated when that product is enabled. An invalid section for | ||
| /// a disabled product never causes a failure, because it's never parsed at all. | ||
| pub fn get_config( |
There was a problem hiding this comment.
Hmm, we're separating some functions but not others...it doesn't feel consistent. The code smell here is needing to thread parse_sast and parse_secrets in as parameters.
Why not get_config(...) -> Result<(String, ConfigMethod)>? Small, contained
And then the consumer can do whatever they want with the (merged) config yaml
| } | ||
| } | ||
|
|
||
| /// Secrets' own `global-config.ignore-paths`/`only-paths` are parsed independently of SAST. |
There was a problem hiding this comment.
Not sure I fully understand this test, but if the intent is to ensure
Secrets' own
global-config.ignore-paths/only-pathsare parsed independently of SAST.
This doesn't do that. It's only checking secrets. I would expect such a test to include both secrets and sast.
But I am not sure I understand the point of it -- the type system already guarantees this statically, no?
| @@ -74,31 +51,40 @@ impl DiffAware for CliConfiguration { | |||
| self.ignore_gitignore, | |||
| rules_string.join(","), | |||
| self.max_file_size_kb, | |||
| self.source_subdirectories.join(","), | |||
| let max_blob_bytes = secrets_config | ||
| .max_file_size_kb | ||
| .map(|kb| kb as usize * 1024) | ||
| .unwrap_or(usize::MAX); |
There was a problem hiding this comment.
Seems like a footgun. Why not have a sensible default?
| /// underlying configuration file: each product owns its own parser, so an invalid section in | ||
| /// one never affects the other. | ||
| #[derive(Debug, Clone, Default, PartialEq)] | ||
| pub struct ConfigFile { |
There was a problem hiding this comment.
Why have this? If the intent is that the products exist independently of each other, then I'm not sure why we'd want to keep them together like this
| assert_not_contains_files!(&base_path, files, ["src/b/main.rs", "test/a/main.rs"]); | ||
| } | ||
|
|
||
| // Two products with different `ignore_gitignore` settings must get independent results: |
There was a problem hiding this comment.
I don't know off the top of my head, but where did we used to test all this behavior? This just seems like a surprising amount of tests for what is a refactor. (Possible I missed where these used to be)
Also
// Two products with different
ignore_gitignoresettings must get independent results:
That's not what's being tested. Is this just an LLM-prompt-leaking-into-code-thing or what was the intent here?
What
Splits
CliConfigurationintoRunConfiguration,SastConfiguration, andSecretsConfiguration, breaksmain()into per-product functions, and gives secrets its own independent configuration-file parser. Adds foursecrets.global-configoptions, mirroring the existingsastschema:Why
Previously SAST's file-selection settings (paths, gitignore, generated-files, size limits) could silently govern secrets scanning too, since both products shared one configuration struct and one file-selection code path. There was also no way to configure secrets scanning independently at all.
Behavior
max-file-size-kbinstead of SAST's, so it's unbounded until a limit is configured.