diff --git a/.keywatch-baseline.json b/.keywatch-baseline.json index 3ba181c..7eb9e67 100644 --- a/.keywatch-baseline.json +++ b/.keywatch-baseline.json @@ -269,14 +269,14 @@ }, { "file_path": "./src/baseline.rs", - "line_number": 267, + "line_number": 313, "finding_type": "Random String", "matched_content_hash": "c40c9141cd77d55f1ba9d21fb045f8e7c5e900c16f2e563779e7ed75cee6b15b", "plugin_name": "RandomString" }, { "file_path": "./src/config/tests/application.rs", - "line_number": 298, + "line_number": 317, "finding_type": "Credit Card Number", "matched_content_hash": "4541206d542811878a9374508fe296fa321a8b56c2902736a02f388836f6e108", "plugin_name": "CreditCardDetector" @@ -304,28 +304,28 @@ }, { "file_path": "./src/detector.rs", - "line_number": 614, + "line_number": 669, "finding_type": "Credit Card Number", "matched_content_hash": "4541206d542811878a9374508fe296fa321a8b56c2902736a02f388836f6e108", "plugin_name": "CreditCardDetector" }, { "file_path": "./src/detector.rs", - "line_number": 615, + "line_number": 670, "finding_type": "Credit Card Number", "matched_content_hash": "13ae894eedbfba2dbd06400ba5b215ffd661885646ab86e050fb1a0d192c1c5b", "plugin_name": "CreditCardDetector" }, { "file_path": "./src/detector.rs", - "line_number": 616, + "line_number": 671, "finding_type": "Credit Card Number", "matched_content_hash": "0d30829f4cbd240de78f8dc72d0a5ed0a77887656aa572ee8fb1392cf9ae34a1", "plugin_name": "CreditCardDetector" }, { "file_path": "./src/detector.rs", - "line_number": 631, + "line_number": 686, "finding_type": "Random String", "matched_content_hash": "e51298df0e431de2bfdf6180e3a7b9f3f092c3e9a912facd350e8c7179936e75", "plugin_name": "RandomString" @@ -906,7 +906,7 @@ }, { "file_path": "./tests/report_tests.rs", - "line_number": 327, + "line_number": 329, "finding_type": "AWS Access Key", "matched_content_hash": "3f733150de7916d4778298d7f90493889c38b76876b80c058e439851ce60cb2b", "plugin_name": "AWSKeyDetector" @@ -1137,140 +1137,140 @@ }, { "file_path": "./tests/scanner_tests.rs", - "line_number": 665, + "line_number": 660, "finding_type": "Aadhaar Card Number", "matched_content_hash": "4714ac4f70659987acae4a4760aedecd3924449f855e3be8115a51eb1ad35f93", "plugin_name": "AadhaarCardDetector" }, { "file_path": "./tests/scanner_tests.rs", - "line_number": 665, + "line_number": 660, "finding_type": "Aadhaar Card Number", "matched_content_hash": "73e8a0879ef4b4acf94b2c188c0b620e4dd198bef454f6270118d63fd87ff4a3", "plugin_name": "AadhaarCardDetector" }, { "file_path": "./tests/scanner_tests.rs", - "line_number": 665, + "line_number": 660, "finding_type": "Aadhaar Card Number", "matched_content_hash": "c941e88b8d0be7288dcc792a8ecfae22482b8a83ea4494f1ca0e36293009730b", "plugin_name": "AadhaarCardDetector" }, { "file_path": "./tests/scanner_tests.rs", - "line_number": 692, + "line_number": 687, "finding_type": "Voter ID (EPIC)", "matched_content_hash": "e530110a549dd903837a0e2af06b406b01367b4572d07bffee9387334c027f20", "plugin_name": "VoterIDDetector" }, { "file_path": "./tests/scanner_tests.rs", - "line_number": 692, + "line_number": 687, "finding_type": "Voter ID (EPIC)", "matched_content_hash": "8d6bf65189e5bdf52955b4a1592eb9c2fb0560709152317a1dabc36971c38f30", "plugin_name": "VoterIDDetector" }, { "file_path": "./tests/scanner_tests.rs", - "line_number": 716, + "line_number": 711, "finding_type": "PAN Card Number", "matched_content_hash": "75d7a58ed9996980fee805fb623d81193cdb5e9efddf2fa12b595171ca16530e", "plugin_name": "PANCardDetector" }, { "file_path": "./tests/scanner_tests.rs", - "line_number": 716, + "line_number": 711, "finding_type": "PAN Card Number", "matched_content_hash": "4dd22fad4bf96036ceea29ca253be8d72851cc289d7e694c9b208b7370812ee6", "plugin_name": "PANCardDetector" }, { "file_path": "./tests/scanner_tests.rs", - "line_number": 740, + "line_number": 735, "finding_type": "ABHA Health ID", "matched_content_hash": "25b618101c3bfdb4438894aa540e108cd150903bd24da57ac24f0d4174c0fe7e", "plugin_name": "ABHADetector" }, { "file_path": "./tests/scanner_tests.rs", - "line_number": 740, + "line_number": 735, "finding_type": "ABHA Health ID", "matched_content_hash": "397d0bc36d77ccfc6f74c2eaa6615bb75c3fe3bdc3df26e6f53f413cfcdacdb3", "plugin_name": "ABHADetector" }, { "file_path": "./tests/scanner_tests.rs", - "line_number": 765, + "line_number": 760, "finding_type": "Aadhaar Card Number", "matched_content_hash": "8d7ab03162973f8bc2baf7c8556af862841d23c4d66f7f18f8cf5c5777f8b263", "plugin_name": "AadhaarCardDetector" }, { "file_path": "./tests/scanner_tests.rs", - "line_number": 765, + "line_number": 760, "finding_type": "PAN Card Number", "matched_content_hash": "18fe123b5e00bbed7228c4ededc8a4e6e0bfd59169af97ec2e62b595eac11dde", "plugin_name": "PANCardDetector" }, { "file_path": "./tests/scanner_tests.rs", - "line_number": 765, + "line_number": 760, "finding_type": "ABHA Health ID", "matched_content_hash": "89deb2b58721d6303daa4a62f2dee30b9a388afb905fc4d3595ec88ca875fb15", "plugin_name": "ABHADetector" }, { "file_path": "./tests/scanner_tests.rs", - "line_number": 870, + "line_number": 865, "finding_type": "Google API Key", "matched_content_hash": "aec6660855470791a5b4f5a6c1ac4b61e61d2b942b1cdec400fb7811743798d0", "plugin_name": "GoogleAPIKeyDetector" }, { "file_path": "./tests/scanner_tests.rs", - "line_number": 870, + "line_number": 865, "finding_type": "Random String", "matched_content_hash": "cbe1ce18b874bc08437699d863cd422c49e104462e7d5ac6bd390acc0d7c973a", "plugin_name": "RandomString" }, { "file_path": "./tests/scanner_tests.rs", - "line_number": 917, + "line_number": 912, "finding_type": "Password", "matched_content_hash": "67ef748345ad7f183084a7449ec05906e648c882400b9d940f2fadec23a7b197", "plugin_name": "PasswordDetector" }, { "file_path": "./tests/scanner_tests.rs", - "line_number": 1012, + "line_number": 1007, "finding_type": "AWS Access Key", "matched_content_hash": "05c0aace2b76ca255ed3a7a953016d981477226dccc3b0e709d00174c8bc48b5", "plugin_name": "AWSKeyDetector" }, { "file_path": "./tests/scanner_tests.rs", - "line_number": 1655, + "line_number": 1650, "finding_type": "Generic Key/Secret", "matched_content_hash": "97894929682fc00219686473cbcfa3731d73b23e88ffa7c19a511c1bbfa18aa5", "plugin_name": "GenericKeyValueDetector" }, { "file_path": "./tests/scanner_tests.rs", - "line_number": 1859, + "line_number": 1854, "finding_type": "AWS Access Key", "matched_content_hash": "357b7fb7890985d4c94a43012d1f7aefe25757f36d8810388357993bb38bd8e7", "plugin_name": "AWSKeyDetector" }, { "file_path": "./tests/scanner_tests.rs", - "line_number": 2111, + "line_number": 2106, "finding_type": "AWS Access Key", "matched_content_hash": "0cbae582394e61dc81d9946ed87ec44f4c83cf1167181f62cd1454eb5e2e5469", "plugin_name": "AWSKeyDetector" }, { "file_path": "./tests/scanner_tests.rs", - "line_number": 2412, + "line_number": 2407, "finding_type": "Password", "matched_content_hash": "6121378fbc25183476235d474ee367f7dbd0bbe3d96642daaeb483b5e9108cdb", "plugin_name": "PasswordDetector" @@ -1333,7 +1333,7 @@ }, { "file_path": "./tests/scanner_tests.rs", - "line_number": 2526, + "line_number": 2521, "finding_type": "Base64 Encoded String", "matched_content_hash": "37f93597692f90e64e009b94ce81237f0f5a6d03ffce8085e91814efe71e4431", "plugin_name": "Base64Detector" @@ -1823,7 +1823,7 @@ }, { "file_path": "./tests/scanner_tests.rs", - "line_number": 1655, + "line_number": 1650, "finding_type": "AWS Secret Access Key", "matched_content_hash": "58db90a4f2acf80492ed15e73ad77de6c84b9aace839d821931aa0474c898386", "plugin_name": "AWSSecretKeyDetector" diff --git a/src/baseline.rs b/src/baseline.rs index e5c92bf..66fec3d 100644 --- a/src/baseline.rs +++ b/src/baseline.rs @@ -65,10 +65,52 @@ fn normalize_fingerprint_path(path: &str) -> String { normalized.to_string() } +/// Anchors finding paths for fingerprinting. Scan modes spell the same file +/// differently — the staged diff is repository-root-relative while a +/// filesystem scan is relative to the invocation directory — so inside a +/// repository every fingerprint path is re-anchored to the repository root. +/// Outside one (or with `Default`), paths are normalized as-is. +#[derive(Clone, Default)] +pub struct PathAnchor { + /// Directory that relative finding paths are resolved against. + pub scan_dir: PathBuf, + /// Enclosing repository root, when there is one. + pub repo_root: Option, +} + +/// Repository-root-relative spelling of a finding path, resolved lexically +/// (the file may no longer exist, e.g. a staged-only or historical path). +/// Falls back to plain normalization when the path leaves the repository or +/// no repository is known. +fn anchored_fingerprint_path(path: &str, anchor: &PathAnchor) -> String { + if let Some(root) = &anchor.repo_root { + let candidate = Path::new(path); + let absolute = if candidate.is_absolute() { + candidate.to_path_buf() + } else { + anchor.scan_dir.join(candidate) + }; + let mut resolved = PathBuf::new(); + for component in absolute.components() { + match component { + std::path::Component::CurDir => {} + std::path::Component::ParentDir => { + resolved.pop(); + } + other => resolved.push(other), + } + } + if let Ok(relative) = resolved.strip_prefix(root) { + return normalize_fingerprint_path(&relative.to_string_lossy()); + } + } + normalize_fingerprint_path(path) +} + impl BaselineFingerprint { - fn from_finding(finding: &Finding) -> Self { + fn from_finding(finding: &Finding, anchor: &PathAnchor) -> Self { Self { - file_path: normalize_fingerprint_path(&finding.file_path), + file_path: anchored_fingerprint_path(&finding.file_path, anchor), finding_type: finding.finding_type.clone(), matched_content_hash: hash_content(&finding.matched_content), plugin_name: finding.detector_name.clone(), @@ -186,9 +228,11 @@ impl Baseline { self.entries.iter().map(BaselineFingerprint::from).collect() } - fn entry_from_finding(finding: &Finding) -> BaselineEntry { + fn entry_from_finding(finding: &Finding, anchor: &PathAnchor) -> BaselineEntry { BaselineEntry { - file_path: finding.file_path.clone(), + // Stored anchored so the committed baseline is stable no matter + // which directory or scan mode produced the entry. + file_path: anchored_fingerprint_path(&finding.file_path, anchor), line_number: finding.line_number, finding_type: finding.finding_type.clone(), matched_content_hash: hash_content(&finding.matched_content), @@ -196,22 +240,24 @@ impl Baseline { } } - pub fn filter_findings(&self, findings: Vec) -> Vec { + pub fn filter_findings(&self, findings: Vec, anchor: &PathAnchor) -> Vec { let fingerprints = self.build_fingerprints(); findings .into_iter() - .filter(|finding| !fingerprints.contains(&BaselineFingerprint::from_finding(finding))) + .filter(|finding| { + !fingerprints.contains(&BaselineFingerprint::from_finding(finding, anchor)) + }) .collect() } - pub fn from_findings(findings: &[Finding]) -> Self { + pub fn from_findings(findings: &[Finding], anchor: &PathAnchor) -> Self { let mut entries = Vec::new(); let mut fingerprints = HashSet::new(); for finding in findings { - let fingerprint = BaselineFingerprint::from_finding(finding); + let fingerprint = BaselineFingerprint::from_finding(finding, anchor); if fingerprints.insert(fingerprint) { - entries.push(Self::entry_from_finding(finding)); + entries.push(Self::entry_from_finding(finding, anchor)); } } @@ -221,7 +267,7 @@ impl Baseline { } } - pub fn update_with_findings(&mut self, findings: &[Finding]) { + pub fn update_with_findings(&mut self, findings: &[Finding], anchor: &PathAnchor) { // First occurrence wins within one scan, matching `from_findings`. // Pre-existing entries refresh their recorded line number — code // edits move findings around — while entries added by this call keep @@ -234,7 +280,7 @@ impl Baseline { .collect(); let mut seen: HashSet = HashSet::new(); for finding in findings { - let fingerprint = BaselineFingerprint::from_finding(finding); + let fingerprint = BaselineFingerprint::from_finding(finding, anchor); if !seen.insert(fingerprint.clone()) { continue; } @@ -242,7 +288,7 @@ impl Baseline { Some(&entry_index) => self.entries[entry_index].line_number = finding.line_number, None => { index.insert(fingerprint, self.entries.len()); - self.entries.push(Self::entry_from_finding(finding)); + self.entries.push(Self::entry_from_finding(finding, anchor)); } } } @@ -297,7 +343,7 @@ mod tests { matched_content: "AKIAIOSFODNN7EXAMPLE".to_string(), detector_name: "AWSKeyDetector".to_string(), }; - let baseline = Baseline::from_findings(&[recorded]); + let baseline = Baseline::from_findings(&[recorded], &super::PathAnchor::default()); let seen_again = Finding { file_path: "secrets.txt".to_string(), @@ -309,35 +355,81 @@ mod tests { }; assert!( - baseline.filter_findings(vec![seen_again]).is_empty(), + baseline + .filter_findings(vec![seen_again], &super::PathAnchor::default()) + .is_empty(), "a baseline entry must suppress the same finding under any path spelling" ); } #[test] fn update_with_findings_refreshes_line_numbers_of_known_entries() { - let mut baseline = Baseline::from_findings(&[Finding { - file_path: "secrets.txt".to_string(), - line_number: 7, - finding_type: "AWS".to_string(), - severity: crate::report::Severity::High, - matched_content: "AKIAIOSFODNN7EXAMPLE".to_string(), - detector_name: "AWSKeyDetector".to_string(), - }]); + let mut baseline = Baseline::from_findings( + &[Finding { + file_path: "secrets.txt".to_string(), + line_number: 7, + finding_type: "AWS".to_string(), + severity: crate::report::Severity::High, + matched_content: "AKIAIOSFODNN7EXAMPLE".to_string(), + detector_name: "AWSKeyDetector".to_string(), + }], + &super::PathAnchor::default(), + ); // The secret moved down after an edit; the update must move the // recorded line with it instead of adding a duplicate entry. - baseline.update_with_findings(&[Finding { - file_path: "secrets.txt".to_string(), - line_number: 42, + baseline.update_with_findings( + &[Finding { + file_path: "secrets.txt".to_string(), + line_number: 42, + finding_type: "AWS".to_string(), + severity: crate::report::Severity::High, + matched_content: "AKIAIOSFODNN7EXAMPLE".to_string(), + detector_name: "AWSKeyDetector".to_string(), + }], + &super::PathAnchor::default(), + ); + + assert_eq!(baseline.entries.len(), 1, "no duplicate entry"); + assert_eq!(baseline.entries[0].line_number, 42); + } + + #[test] + fn anchored_fingerprints_unify_subdirectory_and_staged_spellings() { + // The bug this guards against: a baseline written by `key-watch + // scan .` from repo/sub records "creds.txt", while the staged diff + // reports the same file as "sub/creds.txt" — and the baseline + // suppressed nothing. + let repo_root = std::path::PathBuf::from("/repo"); + let file_scan_from_subdir = super::PathAnchor { + scan_dir: std::path::PathBuf::from("/repo/sub"), + repo_root: Some(repo_root.clone()), + }; + let staged_scan = super::PathAnchor { + scan_dir: repo_root.clone(), + repo_root: Some(repo_root), + }; + + let make = |path: &str| Finding { + file_path: path.to_string(), + line_number: 1, finding_type: "AWS".to_string(), severity: crate::report::Severity::High, matched_content: "AKIAIOSFODNN7EXAMPLE".to_string(), detector_name: "AWSKeyDetector".to_string(), - }]); + }; - assert_eq!(baseline.entries.len(), 1, "no duplicate entry"); - assert_eq!(baseline.entries[0].line_number, 42); + let baseline = Baseline::from_findings(&[make("./creds.txt")], &file_scan_from_subdir); + assert_eq!( + baseline.entries[0].file_path, "sub/creds.txt", + "entries are stored repository-root-relative" + ); + assert!( + baseline + .filter_findings(vec![make("sub/creds.txt")], &staged_scan) + .is_empty(), + "a subdirectory-created baseline must suppress the staged-mode finding" + ); } #[test] diff --git a/src/cli.rs b/src/cli.rs index f1a1608..fed4d5a 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -16,6 +16,8 @@ pub enum CliValidationError { StdinWithPaths, #[error("Must specify paths, use --stdin, --staged, or --git-history")] MissingScanInput, + #[error("--rev-range must be a revision or range, not a flag (got '{range}')")] + RevRangeLooksLikeFlag { range: String }, #[error("--allowed-repos and --blocked-repos are only supported for pre-push hooks")] PreCommitRepositoryFilters, #[error("--exclude is only supported for pre-commit hooks")] @@ -71,6 +73,11 @@ pub struct ScanArgs { #[arg(long, default_value_t = false)] pub git_history: bool, + /// Restrict --git-history to a revision range (e.g. "abc123..def456"); + /// without it, every ref is walked (git log --all) + #[arg(long, requires = "git_history")] + pub rev_range: Option, + /// Scan only the lines staged for commit (paths narrow the staged diff) #[arg(long, default_value_t = false)] pub staged: bool, @@ -135,6 +142,15 @@ pub struct ScanArgs { impl ScanArgs { pub fn validate(&self) -> Result<(), CliValidationError> { + // The range is placed on the git command line; a value starting with + // "-" would be parsed by git as a flag (argument injection). + if let Some(range) = self.rev_range.as_deref() { + if range.starts_with('-') { + return Err(CliValidationError::RevRangeLooksLikeFlag { + range: range.to_string(), + }); + } + } match (self.git_history, self.staged, self.stdin) { (true, true, _) => Err(CliValidationError::StagedWithGitHistory), (true, _, true) => Err(CliValidationError::GitHistoryWithStdin), diff --git a/src/config.rs b/src/config.rs index 5e42ec0..157d5f5 100644 --- a/src/config.rs +++ b/src/config.rs @@ -18,6 +18,7 @@ mod tests; /// directory. Merges with `detectors.toml`: custom rules are appended, /// overrides are applied by detector name. #[derive(Deserialize, Default)] +#[serde(deny_unknown_fields)] pub struct KeywatchConfig { pub rules: Option>, pub overrides: Option>, @@ -86,10 +87,17 @@ impl KeywatchConfig { detectors.extend(staged_detectors); if let Some(overrides) = &self.overrides { + let before = detectors.len(); detectors.retain(|detector| match overrides.get(&detector.name) { Some(detector_override) => detector_override.enabled != Some(false), None => true, }); + let disabled = before - detectors.len(); + if disabled > 0 { + // A config can legitimately disable detectors, but doing so + // must be visible to whoever reads the scan output. + eprintln!("keywatch: {disabled} detector(s) disabled by config overrides"); + } for detector in detectors.iter_mut() { if let Some(severity) = overrides .get(&detector.name) @@ -157,7 +165,10 @@ impl KeywatchConfig { } } +// deny_unknown_fields: a misspelled key (`[[custom_rules]]` instead of +// `[[rules]]`) must fail loudly, not silently weaken the scan. #[derive(Deserialize, Clone)] +#[serde(deny_unknown_fields)] pub struct CustomRule { pub name: String, pub pattern: String, @@ -174,9 +185,13 @@ pub struct CustomRule { pub entropy: Option, /// Extra structural check applied to each match, e.g. `validate = "luhn"`. pub validate: Option, + /// Accepted for configuration compatibility and otherwise ignored; earlier + /// releases parsed the key but never surfaced it. + pub description: Option, } #[derive(Deserialize, Clone)] +#[serde(deny_unknown_fields)] pub struct DetectorOverride { pub enabled: Option, pub severity: Option, diff --git a/src/config/tests/application.rs b/src/config/tests/application.rs index 6ae6844..5c0e95b 100644 --- a/src/config/tests/application.rs +++ b/src/config/tests/application.rs @@ -200,6 +200,25 @@ severity = "HIGH" assert!(config.overrides.is_none()); } +#[test] +fn test_rule_description_is_accepted_and_ignored() { + let toml_str = r#" +[[rules]] +name = "DescribedDetector" +pattern = "DESCRIBED_[A-Z]+" +finding_type = "Test Secret" +severity = "HIGH" +description = "kept for configuration compatibility" +"#; + + let config: KeywatchConfig = toml::from_str(toml_str).expect("description should parse"); + let rules = config.rules.expect("rules present"); + assert_eq!( + rules[0].description.as_deref(), + Some("kept for configuration compatibility") + ); +} + #[test] fn test_parse_config_with_overrides() { let toml_str = r#" diff --git a/src/detector.rs b/src/detector.rs index 7cd765e..9969135 100644 --- a/src/detector.rs +++ b/src/detector.rs @@ -424,11 +424,13 @@ fn shannon_entropy(input: &str) -> f64 { } #[derive(Deserialize)] +#[serde(deny_unknown_fields)] struct DetectorsConfig { detectors: Vec, } #[derive(Deserialize)] +#[serde(deny_unknown_fields)] struct DetectorConfig { name: String, pattern: String, @@ -478,31 +480,70 @@ pub(crate) fn untrusted_root(scan_path: &str, cwd: &std::path::Path) -> std::pat start } -fn find_detectors_config( +/// Resolves `KEYWATCH_CONFIG_PATH`, warning on stderr whenever a set value +/// is ignored: a typo'd operator path silently falling back to another +/// detector set is indistinguishable from working configuration. +fn env_detectors_config( include_repository_config: bool, untrusted_roots: &[std::path::PathBuf], ) -> Option { - std::env::var("KEYWATCH_CONFIG_PATH") - .map(std::path::PathBuf::from) - .ok() - .filter(|path| path.exists()) - // KEYWATCH_CONFIG_PATH is an operator channel. A repository can reach - // it through .envrc/direnv or a devcontainer, so in trusted mode a - // a value pointing back into the tree being scanned — at or below any - // untrusted root — is ignored. - .filter(|path| { - include_repository_config - || untrusted_roots.is_empty() - || !untrusted_roots.iter().any(|root| is_within(path, root)) - }) - .filter(|path| !crate::utils::is_world_writable(path)) + let path = std::path::PathBuf::from(std::env::var_os("KEYWATCH_CONFIG_PATH")?); + if !path.exists() { + eprintln!( + "keywatch: KEYWATCH_CONFIG_PATH '{}' does not exist; ignoring it", + path.display() + ); + return None; + } + // KEYWATCH_CONFIG_PATH is an operator channel. A repository can reach + // it through .envrc/direnv or a devcontainer, so in trusted mode a + // value pointing back into the tree being scanned — at or below any + // untrusted root — is ignored. + if !include_repository_config + && !untrusted_roots.is_empty() + && untrusted_roots.iter().any(|root| is_within(&path, root)) + { + eprintln!( + "keywatch: KEYWATCH_CONFIG_PATH '{}' points inside the scanned tree; ignoring it in trusted mode", + path.display() + ); + return None; + } + if crate::utils::is_world_writable(&path) { + eprintln!( + "keywatch: KEYWATCH_CONFIG_PATH '{}' is world-writable; ignoring it", + path.display() + ); + return None; + } + Some(path) +} + +/// Which channel supplied an external detector file. Only the repository +/// channel draws a stderr warning: the tree being scanned replacing the +/// detector set is the hijack case, while `KEYWATCH_CONFIG_PATH`, the user +/// config directory and the binary directory are operator-managed (the +/// Docker image sets the env var on every run). +enum DetectorConfigChannel { + Operator, + Repository, +} + +fn find_detectors_config( + include_repository_config: bool, + untrusted_roots: &[std::path::PathBuf], +) -> Option<(std::path::PathBuf, DetectorConfigChannel)> { + env_detectors_config(include_repository_config, untrusted_roots) + .map(|path| (path, DetectorConfigChannel::Operator)) .or_else(|| { if !include_repository_config { return None; } let repository_config = std::path::PathBuf::from(DETECTORS_FILE_NAME); - repository_config.exists().then_some(repository_config) + repository_config + .exists() + .then_some((repository_config, DetectorConfigChannel::Repository)) }) // Trusted mode uses the embedded detector set. A repository can // redirect HOME or XDG_CONFIG_HOME (.envrc, devcontainer) and drop a @@ -517,6 +558,7 @@ fn find_detectors_config( dirs::config_dir() .map(|config_directory| config_directory.join("keywatch").join(DETECTORS_FILE_NAME)) .filter(|path| path.exists()) + .map(|path| (path, DetectorConfigChannel::Operator)) }) .or_else(|| { if !include_repository_config { @@ -531,6 +573,7 @@ fn find_detectors_config( .map(|directory| directory.join(DETECTORS_FILE_NAME)) }) .filter(|path| path.exists()) + .map(|path| (path, DetectorConfigChannel::Operator)) }) } @@ -549,12 +592,24 @@ fn initialize_detectors_from_config( untrusted_roots: &[std::path::PathBuf], ) -> Result, DetectorInitError> { let toml_contents = match find_detectors_config(include_repository_config, untrusted_roots) { - Some(config_path) => Cow::Owned(fs::read_to_string(&config_path).map_err(|source| { - DetectorInitError::ReadConfig { - path: config_path, - source, + Some((config_path, channel)) => { + // A repository-supplied file REPLACES the embedded set. Said out + // loud on stderr: it would otherwise silently disable detection + // for whoever scans the clone. Operator channels stay quiet so + // the warning keeps meaning something. + if matches!(channel, DetectorConfigChannel::Repository) { + eprintln!( + "keywatch: using the scanned repository's '{}' instead of the embedded detector set", + config_path.display() + ); } - })?), + Cow::Owned(fs::read_to_string(&config_path).map_err(|source| { + DetectorInitError::ReadConfig { + path: config_path, + source, + } + })?) + } None => Cow::Borrowed(EMBEDDED_DETECTORS_CONFIG), }; diff --git a/src/lib.rs b/src/lib.rs index fda2d10..dd7c8f5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -69,6 +69,7 @@ fn run_scan_command(args: &ScanArgs) -> Result { // plain update must refresh the recorded line numbers of known findings, // so neither may filter the findings first. let prune = args.prune_baseline && args.update_baseline; + let anchor = baseline_anchor(&args); let mut loaded_baseline = match args.baseline.as_deref() { Some(path) => Some(baseline::Baseline::load(std::path::Path::new(path))?), None => None, @@ -78,12 +79,12 @@ fn run_scan_command(args: &ScanArgs) -> Result { .filter(|_| !prune && !args.update_baseline) { let before = findings.len(); - findings = baseline.filter_findings(findings); + findings = baseline.filter_findings(findings, &anchor); scan_metadata.suppressed_by_baseline = before - findings.len(); } if args.update_baseline { - update_baseline(&args, &findings, &mut loaded_baseline, prune)?; + update_baseline(&args, &findings, &mut loaded_baseline, prune, &anchor)?; return Ok(0); } @@ -127,6 +128,45 @@ fn load_scan_config(args: &ScanArgs) -> Result, R } } +/// Where fingerprint paths are anchored for this scan: relative finding +/// paths come from the repository root in git-backed modes and from the +/// invocation directory otherwise. The repository is probed at the first +/// scan operand, not the process directory, so `key-watch scan some/repo` +/// run from outside the repository still anchors to it. Canonicalized so +/// macOS `/tmp` vs `/private/tmp` spellings cannot break the root prefix +/// match. +fn baseline_anchor(args: &ScanArgs) -> baseline::PathAnchor { + let cwd = env::current_dir() + .ok() + .and_then(|dir| std::fs::canonicalize(dir).ok()) + .unwrap_or_else(|| std::path::PathBuf::from(".")); + let probe = match args.paths.first() { + Some(first) if !args.staged => { + let joined = cwd.join(first); + if joined.is_dir() { + joined + } else { + joined + .parent() + .map(std::path::Path::to_path_buf) + .unwrap_or_else(|| cwd.clone()) + } + } + _ => cwd.clone(), + }; + let repo_root = + scanner::git_repo_root(&probe).and_then(|root| std::fs::canonicalize(root).ok()); + let scan_dir = if args.staged || args.git_history { + repo_root.clone().unwrap_or(cwd) + } else { + cwd + }; + baseline::PathAnchor { + scan_dir, + repo_root, + } +} + /// Writes the baseline after the scan. /// /// Pruning rebuilds it from what the scan actually found. The drop count and @@ -137,6 +177,7 @@ fn update_baseline( findings: &[Finding], loaded_baseline: &mut Option, prune: bool, + anchor: &baseline::PathAnchor, ) -> Result<(), RunCliError> { let baseline_path = args .baseline @@ -148,7 +189,7 @@ fn update_baseline( if prune { let stale = baseline.entries.len(); - *baseline = baseline::Baseline::from_findings(findings); + *baseline = baseline::Baseline::from_findings(findings, anchor); let dropped = stale.saturating_sub(baseline.entries.len()); if dropped > 0 { let noun = if dropped == 1 { "entry" } else { "entries" }; @@ -162,7 +203,7 @@ fn update_baseline( )?; } } else { - baseline.update_with_findings(findings); + baseline.update_with_findings(findings, anchor); } baseline.save(std::path::Path::new(baseline_path))?; @@ -181,13 +222,35 @@ fn emit_scan_result( let suppressed = scan_metadata.suppressed_by_baseline; let severity_counts = report::get_severity_counts(&findings); let mut exit_code = calculate_exit_code(&findings, &args.exit_mode); - if args.fail_on_unscannable + let unscannable_failure = args.fail_on_unscannable && matches!(args.exit_mode, ExitMode::Strict) - && !scan_metadata.unscannable_files.is_empty() - { + && !scan_metadata.unscannable_files.is_empty(); + if unscannable_failure { exit_code = 1; } + let unscannable_count = scan_metadata.unscannable_files.len(); let findings_count = findings.len(); + // Non-verbose runs still need to say WHERE each finding is; a bare count + // forces a second scan with --verbose to act on anything. Matched text + // stays redacted on the console regardless of --show-secrets. + let finding_lines: Vec = if args.verbose { + Vec::new() + } else { + findings + .iter() + .map(|finding| { + format!( + "{}: {} at {}:{} ({}) [{}]", + finding.severity, + finding.finding_type, + finding.file_path, + finding.line_number, + report::redact(&finding.matched_content), + finding.detector_name + ) + }) + .collect() + }; let report_out = match args.format { OutputFormat::Json => { report::create_report(findings, scan_metadata, scan_time, args.show_secrets) @@ -207,8 +270,16 @@ fn emit_scan_result( ))?; } + for line in &finding_lines { + emit(line)?; + } let summary = match findings_count { _ if args.verbose => report_out.clone(), + // "No secrets found." next to exit code 1 is contradictory; name the + // actual failure instead. + 0 if unscannable_failure => format!( + "WARNING: {unscannable_count} file(s) could not be scanned (--fail-on-unscannable)" + ), 0 => "No secrets found.".to_string(), count => format!( "WARNING: {} potential secret(s) detected (CRITICAL: {}, HIGH: {}, MEDIUM: {}, LOW: {})", diff --git a/src/report.rs b/src/report.rs index d84f86a..866e975 100644 --- a/src/report.rs +++ b/src/report.rs @@ -126,7 +126,7 @@ pub struct Finding { pub detector_name: String, } -#[derive(Serialize, Clone, Default)] +#[derive(Serialize, Clone, Debug, Default)] pub struct ScanMetadata { pub files_scanned: usize, pub total_lines: usize, diff --git a/src/report/sarif.rs b/src/report/sarif.rs index f6fcff1..9a27c53 100644 --- a/src/report/sarif.rs +++ b/src/report/sarif.rs @@ -38,6 +38,7 @@ pub fn create_sarif_report( #[serde(skip_serializing_if = "Option::is_none")] version: Option, information_uri: &'static str, + #[serde(skip_serializing_if = "Option::is_none")] semantic_version: Option, } @@ -100,11 +101,10 @@ pub fn create_sarif_report( let uri = finding.file_path; let start_line = finding.line_number; + // No per-rule confidence model exists, so no `precision` claim is + // made: a blanket "very-high" on entropy-gated LOW rules was a + // false statement to SARIF consumers. let mut properties = BTreeMap::new(); - properties.insert( - "precision".to_string(), - serde_json::Value::String("very-high".to_string()), - ); properties.insert( "severity".to_string(), serde_json::Value::String(severity_str.to_string()), diff --git a/src/scanner.rs b/src/scanner.rs index af6553c..efe0645 100644 --- a/src/scanner.rs +++ b/src/scanner.rs @@ -156,11 +156,23 @@ fn scan_git_history( "--no-textconv", "--no-color", ]); + // Without a range, walk every ref: a secret committed on a side branch + // is exactly as leaked as one on the checked-out branch. An explicit + // --rev-range (the pre-push hook passes the pushed range) narrows the + // walk instead. + match args.rev_range.as_deref() { + Some(range) => { + command.arg(range); + } + None => { + command.arg("--all"); + } + } let exclude_patterns = compile_exclude_patterns(args, config)?; let history = scan_git_output( command, - ScannerError::GitLogNonZero, + |stderr| ScannerError::GitLogNonZero { stderr }, |reader| { scan_staged_diff( reader, @@ -210,7 +222,7 @@ fn scan_staged( let staged = scan_git_output( command, - ScannerError::GitDiffNonZero, + |stderr| ScannerError::GitDiffNonZero { stderr }, |reader| { scan_staged_diff( reader, @@ -311,15 +323,32 @@ fn scan_filesystem( line_detectors: &[&Detector], ) -> Result<(Vec, ScanMetadata), ScannerError> { let mut target_paths: Vec = Vec::new(); + let mut unlistable_dirs: Vec = Vec::new(); + // Explicit operands are validated strictly: a typo'd path or an operand + // the scanner will not read (symlink, device, FIFO) must not produce a + // silent "No secrets found" pass. for path_str in &args.paths { let path = Path::new(path_str); - let Ok(metadata) = fs::symlink_metadata(path) else { - continue; + let metadata = match fs::symlink_metadata(path) { + Ok(metadata) => metadata, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + return Err(ScannerError::ScanPathMissing { + path: path_str.clone(), + }); + } + Err(source) => { + return Err(ScannerError::ScanPathUnreadable { + path: path_str.clone(), + source, + }); + } }; let file_type = metadata.file_type(); if file_type.is_symlink() { - continue; + return Err(ScannerError::ScanPathSymlink { + path: path_str.clone(), + }); } if file_type.is_file() { target_paths.push(ScanTarget { @@ -327,21 +356,39 @@ fn scan_filesystem( root: None, }); } else if file_type.is_dir() { - collect_files(path_str, &mut target_paths, path_str); + // An explicit operand that cannot be listed is the same silent + // clean pass as a missing one; nested unlistable directories + // stay unscannable entries instead. + if let Err(source) = fs::read_dir(path) { + return Err(ScannerError::ScanPathUnreadable { + path: path_str.clone(), + source, + }); + } + collect_files(path_str, &mut target_paths, path_str, &mut unlistable_dirs); + } else { + return Err(ScannerError::ScanPathUnsupported { + path: path_str.clone(), + }); } } target_paths.sort_by(|a, b| a.path.cmp(&b.path)); - let mut unique_paths: std::collections::BTreeMap>> = + // Keyed by a lexically normalized form so `dup.txt` and `./dup.txt` are + // one scan target, not two duplicated findings. The first spelling seen + // is the one reported. + let mut unique_paths: std::collections::BTreeMap>)> = std::collections::BTreeMap::new(); for ScanTarget { path, root } in target_paths { - let roots = unique_paths.entry(path).or_default(); + let (_, roots) = unique_paths + .entry(normalized_path_key(&path)) + .or_insert_with(|| (path, Vec::new())); if !roots.contains(&root) { roots.push(root); } } - let unique_paths: Vec<_> = unique_paths.into_iter().collect(); + let unique_paths: Vec<_> = unique_paths.into_values().collect(); let exclude_patterns = compile_exclude_patterns(args, config)?; let line_scan_context = LineScanContext::new(line_detectors); @@ -362,7 +409,32 @@ fn scan_filesystem( }) .collect(); - Ok(aggregate_file_outcomes(results)) + let (findings, mut metadata) = aggregate_file_outcomes(results); + metadata.unscannable_files.extend(unlistable_dirs); + Ok((findings, metadata)) +} + +/// Lexically normalized dedup key for a scan target: `.` components and +/// empty segments drop out, absoluteness is preserved (so `.//x` keys as +/// `x`, never as the absolute `/x`), and `\` folds to `/` on Windows only — +/// on unix a backslash is an ordinary filename character, and folding it +/// would collide `a\b` with `a/b` and silently drop one of them from the +/// scan. Purely a key — the path reported to the user keeps its original +/// spelling. +fn normalized_path_key(path: &str) -> String { + #[cfg(windows)] + let path = &path.replace('\\', "/"); + let absolute = path.starts_with('/'); + let segments: Vec<&str> = path + .split('/') + .filter(|segment| !segment.is_empty() && *segment != ".") + .collect(); + let joined = segments.join("/"); + if absolute { + format!("/{joined}") + } else { + joined + } } /// Scans a single path and classifies the outcome. Streamed: memory stays @@ -498,7 +570,7 @@ fn untrusted_roots(args: &ScanArgs) -> Vec { /// The enclosing repository's root, via `git rev-parse --show-toplevel`. /// `None` when the directory is not inside a working tree. -fn git_repo_root(dir: &Path) -> Option { +pub(crate) fn git_repo_root(dir: &Path) -> Option { let output = std::process::Command::new("git") .args(["rev-parse", "--show-toplevel"]) .current_dir(dir) @@ -565,9 +637,26 @@ pub(crate) mod test_support { #[cfg(test)] mod tests { - use super::dedupe_findings; + use super::{dedupe_findings, normalized_path_key}; use crate::report::{Finding, Severity}; + #[test] + fn normalized_path_key_never_turns_relative_into_absolute() { + // `.//x` must key as the relative `x`; keying it as `/x` would + // collide with a genuine absolute operand and silently drop one of + // the two files from the scan. + assert_eq!(normalized_path_key(".//x"), "x"); + assert_eq!(normalized_path_key("./x"), "x"); + assert_eq!(normalized_path_key("a/./b"), "a/b"); + assert_eq!(normalized_path_key("a//b"), "a/b"); + assert_eq!(normalized_path_key("/x"), "/x"); + // `..` is kept: resolving it lexically could alias distinct paths. + assert_eq!(normalized_path_key("a/../b"), "a/../b"); + // On unix a backslash is a filename character, not a separator. + #[cfg(not(windows))] + assert_eq!(normalized_path_key("a\\b"), "a\\b"); + } + fn finding(detector: &str, severity: Severity, matched: &str, line: usize) -> Finding { Finding { file_path: "src/lib.rs".to_string(), diff --git a/src/scanner/error.rs b/src/scanner/error.rs index 832e190..643ac47 100644 --- a/src/scanner/error.rs +++ b/src/scanner/error.rs @@ -22,10 +22,18 @@ pub enum ScannerError { CaptureGitStdout, #[error("git process error: {source}")] GitProcess { source: io::Error }, - #[error("git log exited with non-zero status")] - GitLogNonZero, - #[error("git diff exited with non-zero status")] - GitDiffNonZero, + #[error("git log failed: {stderr}")] + GitLogNonZero { stderr: String }, + #[error("git diff failed: {stderr}")] + GitDiffNonZero { stderr: String }, + #[error("Scan path not found: '{path}'")] + ScanPathMissing { path: String }, + #[error("Cannot read scan path '{path}': {source}")] + ScanPathUnreadable { path: String, source: io::Error }, + #[error("Scan path '{path}' is a symlink; KeyWatch does not follow symlinks")] + ScanPathSymlink { path: String }, + #[error("Scan path '{path}' is not a regular file or directory")] + ScanPathUnsupported { path: String }, #[error("Invalid exclude pattern '{pattern}': {source}")] InvalidExcludePattern { pattern: String, diff --git a/src/scanner/files.rs b/src/scanner/files.rs index 5624d17..47c7927 100644 --- a/src/scanner/files.rs +++ b/src/scanner/files.rs @@ -58,14 +58,18 @@ pub(super) fn baseline_exclusion(args: &ScanArgs) -> Option { /// generated hashes otherwise flood reports and baselines with "Random /// String" findings. Excluded by basename at any depth in every /// filesystem-backed mode, matching gitleaks; `--stdin` is unaffected. -const DEFAULT_EXCLUDED_FILES: [&str; 9] = [ +const DEFAULT_EXCLUDED_FILES: [&str; 13] = [ + "bun.lock", + "bun.lockb", "Cargo.lock", "composer.lock", "Gemfile.lock", "go.sum", + "npm-shrinkwrap.json", "package-lock.json", "packages.lock.json", "Pipfile.lock", + "pnpm-lock.yaml", "poetry.lock", "yarn.lock", ]; @@ -102,27 +106,40 @@ pub(super) struct ScanTarget { pub(super) root: Option, } -pub(super) fn collect_files(dir_path: &str, targets: &mut Vec, root: &str) { - if let Ok(entries) = fs::read_dir(dir_path) { - for entry in entries.flatten() { - let Ok(file_type) = entry.file_type() else { - continue; - }; - if file_type.is_symlink() { - continue; +pub(super) fn collect_files( + dir_path: &str, + targets: &mut Vec, + root: &str, + unlistable_dirs: &mut Vec, +) { + // A directory that cannot be listed hides everything beneath it; record + // it as unscannable instead of silently reporting a clean scan, so + // --fail-on-unscannable catches it. + let entries = match fs::read_dir(dir_path) { + Ok(entries) => entries, + Err(_) => { + unlistable_dirs.push(dir_path.to_string()); + return; + } + }; + for entry in entries.flatten() { + let Ok(file_type) = entry.file_type() else { + continue; + }; + if file_type.is_symlink() { + continue; + } + let path = entry.path(); + if file_type.is_file() { + if let Some(path_str) = path.to_str() { + targets.push(ScanTarget { + path: path_str.to_string(), + root: Some(root.to_string()), + }); } - let path = entry.path(); - if file_type.is_file() { - if let Some(path_str) = path.to_str() { - targets.push(ScanTarget { - path: path_str.to_string(), - root: Some(root.to_string()), - }); - } - } else if file_type.is_dir() && path.file_name().is_none_or(|name| name != ".git") { - if let Some(path_str) = path.to_str() { - collect_files(path_str, targets, root); - } + } else if file_type.is_dir() && path.file_name().is_none_or(|name| name != ".git") { + if let Some(path_str) = path.to_str() { + collect_files(path_str, targets, root, unlistable_dirs); } } } diff --git a/src/scanner/staged.rs b/src/scanner/staged.rs index b085031..acfff15 100644 --- a/src/scanner/staged.rs +++ b/src/scanner/staged.rs @@ -22,16 +22,28 @@ use std::path::{Path, PathBuf}; /// is checked. pub(super) fn scan_git_output( mut command: std::process::Command, - nonzero_status: ScannerError, + nonzero_status: impl FnOnce(String) -> ScannerError, scan: impl FnOnce(BufReader) -> Result, spawn_failed: impl FnOnce(std::io::Error) -> ScannerError, ) -> Result { let mut child = command .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) .spawn() .map_err(spawn_failed)?; let stdout = child.stdout.take().ok_or(ScannerError::CaptureGitStdout)?; + // Drained on its own thread: git can fill the stderr pipe (a full usage + // dump) while this process is still reading stdout, deadlocking both. + let stderr = child.stderr.take(); + let stderr_reader = std::thread::spawn(move || { + use std::io::Read; + let mut buffer = String::new(); + if let Some(mut stderr) = stderr { + let _ = stderr.read_to_string(&mut buffer); + } + buffer + }); let scanned = scan(BufReader::new(stdout)); if scanned.is_err() { let _ = child.kill(); @@ -40,9 +52,30 @@ pub(super) fn scan_git_output( let status = child .wait() .map_err(|source| ScannerError::GitProcess { source })?; + let stderr = stderr_reader.join().unwrap_or_default(); let scanned = scanned?; - status.success().then_some(scanned).ok_or(nonzero_status) + status + .success() + .then_some(scanned) + .ok_or_else(|| nonzero_status(summarize_git_stderr(&stderr))) +} + +/// One line of git's stderr for the error message: the first `fatal:` or +/// `error:` line when present, otherwise the first non-empty line. Keeps a +/// 150-line usage dump out of the report. +fn summarize_git_stderr(stderr: &str) -> String { + let lines = || { + stderr + .lines() + .map(str::trim) + .filter(|line| !line.is_empty()) + }; + lines() + .find(|line| line.starts_with("fatal:") || line.starts_with("error:")) + .or_else(|| lines().next()) + .unwrap_or("exited with non-zero status") + .to_string() } fn parse_hunk_new_start(header: &str) -> usize { let parsed = header diff --git a/tests/baseline_tests.rs b/tests/baseline_tests.rs index 7a240d7..bf480f5 100644 --- a/tests/baseline_tests.rs +++ b/tests/baseline_tests.rs @@ -1,4 +1,4 @@ -use key_watch::baseline::{Baseline, BaselineEntry, BaselineError}; +use key_watch::baseline::{Baseline, BaselineEntry, BaselineError, PathAnchor}; use key_watch::report::{Finding, Severity}; use std::fs; use std::path::Path; @@ -53,7 +53,8 @@ fn test_baseline_filters_known_findings() { "AKIAABCDEFGHIJKLMNOP", "AWSAccessKeyDetector", ); - let baseline = Baseline::from_findings(std::slice::from_ref(&known_finding)); + let baseline = + Baseline::from_findings(std::slice::from_ref(&known_finding), &PathAnchor::default()); let findings = vec![ known_finding, @@ -66,7 +67,7 @@ fn test_baseline_filters_known_findings() { ), ]; - let filtered = baseline.filter_findings(findings); + let filtered = baseline.filter_findings(findings, &PathAnchor::default()); assert_eq!(filtered.len(), 1); assert_eq!(filtered[0].file_path, "other.txt"); } @@ -80,7 +81,8 @@ fn test_baseline_filters_moved_finding_in_same_file() { "AKIAABCDEFGHIJKLMNOP", "AWSAccessKeyDetector", ); - let baseline = Baseline::from_findings(std::slice::from_ref(&known_finding)); + let baseline = + Baseline::from_findings(std::slice::from_ref(&known_finding), &PathAnchor::default()); let findings = vec![make_finding( "test.txt", @@ -90,7 +92,7 @@ fn test_baseline_filters_moved_finding_in_same_file() { "AWSAccessKeyDetector", )]; - let filtered = baseline.filter_findings(findings); + let filtered = baseline.filter_findings(findings, &PathAnchor::default()); assert!(filtered.is_empty()); } @@ -103,7 +105,8 @@ fn test_baseline_keeps_same_finding_in_different_file() { "AKIAABCDEFGHIJKLMNOP", "AWSAccessKeyDetector", ); - let baseline = Baseline::from_findings(std::slice::from_ref(&known_finding)); + let baseline = + Baseline::from_findings(std::slice::from_ref(&known_finding), &PathAnchor::default()); let findings = vec![make_finding( "other.txt", @@ -113,7 +116,7 @@ fn test_baseline_keeps_same_finding_in_different_file() { "AWSAccessKeyDetector", )]; - let filtered = baseline.filter_findings(findings); + let filtered = baseline.filter_findings(findings, &PathAnchor::default()); assert_eq!(filtered.len(), 1); assert_eq!(filtered[0].file_path, "other.txt"); } @@ -128,7 +131,7 @@ fn test_baseline_allows_new_findings() { "sk-abc", "GenericKeyValueDetector", )]; - let filtered = baseline.filter_findings(findings); + let filtered = baseline.filter_findings(findings, &PathAnchor::default()); assert_eq!(filtered.len(), 1); } @@ -184,7 +187,7 @@ fn test_baseline_from_findings() { make_finding("f1.txt", 1, "A", "x", "D1"), make_finding("f2.txt", 2, "B", "y", "D2"), ]; - let baseline = Baseline::from_findings(&findings); + let baseline = Baseline::from_findings(&findings, &PathAnchor::default()); assert_eq!(baseline.entries.len(), 2); } @@ -194,21 +197,24 @@ fn test_baseline_from_findings_deduplicates_and_keeps_first_metadata() { make_finding("f1.txt", 1, "A", "x", "D1"), make_finding("f1.txt", 99, "A", "x", "D1"), ]; - let baseline = Baseline::from_findings(&findings); + let baseline = Baseline::from_findings(&findings, &PathAnchor::default()); assert_eq!(baseline.entries.len(), 1); assert_eq!(baseline.entries[0].line_number, 1); } #[test] fn test_baseline_update_merges_new_findings() { - let mut baseline = Baseline::from_findings(&[make_finding("old.txt", 1, "X", "old", "D")]); + let mut baseline = Baseline::from_findings( + &[make_finding("old.txt", 1, "X", "old", "D")], + &PathAnchor::default(), + ); let new_findings = vec![ make_finding("old.txt", 1, "X", "old", "D"), make_finding("new.txt", 2, "Y", "new", "D2"), make_finding("new.txt", 99, "Y", "new", "D2"), ]; - baseline.update_with_findings(&new_findings); + baseline.update_with_findings(&new_findings, &PathAnchor::default()); assert_eq!(baseline.entries.len(), 2); assert!(baseline.entries.iter().any(|e| e.file_path == "old.txt")); @@ -226,9 +232,11 @@ fn test_baseline_update_merges_new_findings() { #[test] fn test_baseline_update_preserves_existing() { - let mut baseline = - Baseline::from_findings(&[make_finding("existing.txt", 5, "API", "secret", "D")]); - baseline.update_with_findings(&[]); + let mut baseline = Baseline::from_findings( + &[make_finding("existing.txt", 5, "API", "secret", "D")], + &PathAnchor::default(), + ); + baseline.update_with_findings(&[], &PathAnchor::default()); assert_eq!(baseline.entries.len(), 1); assert_eq!(baseline.entries[0].file_path, "existing.txt"); diff --git a/tests/cli_validation_tests.rs b/tests/cli_validation_tests.rs index 9361126..3bea358 100644 --- a/tests/cli_validation_tests.rs +++ b/tests/cli_validation_tests.rs @@ -76,3 +76,35 @@ fn test_staged_allows_zero_or_many_paths() { assert!(options.validate().is_ok(), "staged paths narrow the diff"); } + +#[test] +fn test_rev_range_rejects_flag_shaped_values() { + // The range lands on the git command line; a leading dash would be + // parsed by git as a flag (argument injection). + let options = ScanArgs { + git_history: true, + rev_range: Some("--exec=evil".to_string()), + ..Default::default() + }; + + let error = options + .validate() + .expect_err("flag-shaped rev-range must be rejected"); + assert_eq!( + error, + CliValidationError::RevRangeLooksLikeFlag { + range: "--exec=evil".to_string() + } + ); +} + +#[test] +fn test_rev_range_accepts_sha_ranges() { + let options = ScanArgs { + git_history: true, + rev_range: Some("abc123..def456".to_string()), + ..Default::default() + }; + + assert!(options.validate().is_ok()); +} diff --git a/tests/exit_tests.rs b/tests/exit_tests.rs index a3f2dc1..f56c608 100644 --- a/tests/exit_tests.rs +++ b/tests/exit_tests.rs @@ -477,3 +477,139 @@ fn test_show_secrets_opts_into_raw_matched_content() { let _ = fs::remove_dir_all(&dir); } + +#[test] +fn test_exit_code_2_on_nonexistent_scan_path() { + // A typo'd path in CI must fail the scan, not report a clean pass. + let output = Command::new(env!("CARGO_BIN_EXE_key-watch")) + .args(["scan", "/definitely/not/a/real/path"]) + .env_remove("KEYWATCH_CONFIG_PATH") + .output() + .expect("Run key-watch"); + + assert_eq!(output.status.code(), Some(2)); + assert!( + String::from_utf8_lossy(&output.stderr).contains("Scan path not found"), + "stderr must name the missing path" + ); +} + +#[cfg(unix)] +#[test] +fn test_exit_code_2_on_symlink_scan_operand() { + let test_dir = setup_scan_dir("exit_symlink_operand", false); + let target = test_dir.join("real.txt"); + fs::write(&target, "plain").expect("Write target"); + let link = test_dir.join("link.txt"); + std::os::unix::fs::symlink(&target, &link).expect("Create symlink"); + + let output = Command::new(env!("CARGO_BIN_EXE_key-watch")) + .current_dir(&test_dir) + .args(["scan", "link.txt"]) + .env_remove("KEYWATCH_CONFIG_PATH") + .output() + .expect("Run key-watch"); + + assert_eq!( + output.status.code(), + Some(2), + "an explicitly named symlink must not silently scan zero files" + ); + + fs::remove_dir_all(test_dir).expect("Cleanup"); +} + +#[test] +fn test_same_file_under_two_spellings_reports_once() { + let test_dir = setup_scan_dir("exit_dup_spelling", false); + fs::write(test_dir.join("dup.txt"), "AWS_KEY=AKIAABCDEFGHIJKLMNOP").expect("Write test file"); + + let output = Command::new(env!("CARGO_BIN_EXE_key-watch")) + .current_dir(&test_dir) + .args(["scan", "dup.txt", "./dup.txt", "--verbose"]) + .env_remove("KEYWATCH_CONFIG_PATH") + .output() + .expect("Run key-watch"); + + let stdout = String::from_utf8_lossy(&output.stdout); + assert_eq!( + stdout.matches("AWS Access Key").count(), + 1, + "one file passed twice must yield one finding: {stdout}" + ); + + fs::remove_dir_all(test_dir).expect("Cleanup"); +} + +#[cfg(unix)] +#[test] +fn test_unlistable_directory_is_unscannable_and_fails_with_flag() { + use std::os::unix::fs::PermissionsExt; + + let test_dir = setup_scan_dir("exit_unlistable_dir", false); + let locked = test_dir.join("locked"); + fs::create_dir(&locked).expect("Create locked dir"); + fs::set_permissions(&locked, fs::Permissions::from_mode(0o000)).expect("Lock dir"); + if fs::read_dir(&locked).is_ok() { + // Running as root (e.g. in a container): mode 000 does not make the + // directory unlistable, so the scenario cannot be constructed. + fs::remove_dir_all(&test_dir).expect("Cleanup"); + return; + } + + let output = Command::new(env!("CARGO_BIN_EXE_key-watch")) + .current_dir(&test_dir) + .args(["scan", ".", "--fail-on-unscannable"]) + .env_remove("KEYWATCH_CONFIG_PATH") + .output() + .expect("Run key-watch"); + + fs::set_permissions(&locked, fs::Permissions::from_mode(0o755)).expect("Unlock dir"); + + assert_eq!( + output.status.code(), + Some(1), + "an unlistable directory hides content and must fail --fail-on-unscannable: {}", + String::from_utf8_lossy(&output.stdout) + ); + assert!( + String::from_utf8_lossy(&output.stdout).contains("could not be scanned"), + "the summary must name the unscannable failure, not claim a clean pass" + ); + + fs::remove_dir_all(&test_dir).expect("Cleanup"); +} + +#[cfg(unix)] +#[test] +fn test_exit_code_2_on_unlistable_directory_operand() { + use std::os::unix::fs::PermissionsExt; + + let test_dir = setup_scan_dir("exit_unlistable_operand", false); + let locked = test_dir.join("locked"); + fs::create_dir(&locked).expect("Create locked dir"); + fs::set_permissions(&locked, fs::Permissions::from_mode(0o000)).expect("Lock dir"); + if fs::read_dir(&locked).is_ok() { + // Running as root: mode 000 does not make the directory unlistable. + fs::remove_dir_all(&test_dir).expect("Cleanup"); + return; + } + + let output = Command::new(env!("CARGO_BIN_EXE_key-watch")) + .current_dir(&test_dir) + .args(["scan", "locked"]) + .env_remove("KEYWATCH_CONFIG_PATH") + .output() + .expect("Run key-watch"); + + fs::set_permissions(&locked, fs::Permissions::from_mode(0o755)).expect("Unlock dir"); + + assert_eq!( + output.status.code(), + Some(2), + "an explicitly named directory that cannot be listed must not pass silently: {}", + String::from_utf8_lossy(&output.stdout) + ); + + fs::remove_dir_all(&test_dir).expect("Cleanup"); +} diff --git a/tests/report_tests.rs b/tests/report_tests.rs index d757a04..9f475a1 100644 --- a/tests/report_tests.rs +++ b/tests/report_tests.rs @@ -187,7 +187,9 @@ fn test_create_sarif_report_uses_camel_case_fields_and_hides_matched_content() { result["properties"]["severity"], Severity::Critical.as_str() ); - assert_eq!(result["properties"]["precision"], "very-high"); + // No blanket precision claim: KeyWatch has no per-rule confidence model, + // so asserting "very-high" to SARIF consumers would be false. + assert!(result["properties"].get("precision").is_none()); assert!(!sarif.contains(&secret)); } diff --git a/tests/scanner_tests.rs b/tests/scanner_tests.rs index cabaaad..46e5c91 100644 --- a/tests/scanner_tests.rs +++ b/tests/scanner_tests.rs @@ -553,7 +553,7 @@ fn test_mixed_file_and_directory_paths_are_scanned_once() { } #[test] -fn test_nonexistent_paths_are_ignored_without_counting_as_scanned() { +fn test_nonexistent_operand_fails_the_scan() { let missing_path = temp_dir().join(format!( "keywatch_missing_{}", std::time::SystemTime::now() @@ -568,24 +568,19 @@ fn test_nonexistent_paths_are_ignored_without_counting_as_scanned() { ..Default::default() }; - let (findings, metadata) = run_scan(&options, None).expect("run_scan should succeed"); - assert!( - findings.is_empty(), - "Missing paths should not produce findings" - ); - assert_eq!( - metadata.files_scanned, 0, - "Missing paths should not be counted as scanned" - ); + // A typo'd operand must be a hard error, not a silent clean pass: a CI + // job scanning a wrong path would otherwise report "No secrets found" + // forever. + let error = run_scan(&options, None).expect_err("missing operand must fail the scan"); assert!( - metadata.excluded_files.is_empty(), - "Missing paths should not be marked excluded" + error.to_string().contains("Scan path not found"), + "unexpected error: {error}" ); } #[cfg(unix)] #[test] -fn test_explicit_symlink_path_is_skipped() -> Result<(), String> { +fn test_explicit_symlink_operand_fails_the_scan() -> Result<(), String> { let test_dir = unique_temp_dir("explicit_symlink_skip"); let outside_file = test_dir.join("outside-secret.txt"); let link_path = test_dir.join("linked-secret.txt"); @@ -606,12 +601,12 @@ fn test_explicit_symlink_path_is_skipped() -> Result<(), String> { ..Default::default() }; - let (findings, metadata) = run_scan(&options, None).expect("run_scan should succeed"); - - assert!(findings.is_empty(), "Symlink target should not be scanned"); - assert_eq!( - metadata.files_scanned, 0, - "Symlink should not count as scanned" + // Explicitly naming a symlink must error: KeyWatch never follows + // symlinks, and silently scanning zero files reads as a clean pass. + let error = run_scan(&options, None).expect_err("symlink operand must fail the scan"); + assert!( + error.to_string().contains("is a symlink"), + "unexpected error: {error}" ); fs::remove_dir_all(&test_dir).map_err(|error| format!("cleanup: {error}"))?;