From 06c1557bd5a7b19ac83e7a08bed83b4c8f2a828d Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Wed, 2 Sep 2026 14:02:15 +0000 Subject: [PATCH 1/2] fix(scan): soften findings inside Rust inline #[cfg(test)] blocks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three structural false-positive causes fixed in #154 and #158 all key on where a *file* sits: `_test.go`, `testutils/`, `docs/`. Rust does not work that way. `cargo test` compiles unit tests from a `#[cfg(test)] mod tests` block at the bottom of the very file they cover, so production code and its fixtures share one path and `isTestPath` can never separate them. Measured on ferriskey/ferriskey (Rust IAM, 689 stars) at 0.11.8: 135 findings, zero true positives, of which 15 are exactly this — a fixture password in a test module reported at `high` from a path that looks like production. The sharpest is `core/src/domain/trident/services.rs:2719`, where the test module opens at 2137 of 4042 lines; being a *good* fake password is what kept it from being softened by any of the existing value-side rules. Adds `inlineTestLines()`, which returns the line indices a file's own test blocks occupy, and a `test-block` softening reason alongside `test`. Keyed on lines rather than on the file, because the file is half production code: a to-end-of-file rule would soften nearly 2000 lines of `services.rs` and hide a real credential committed below the test module. Finding the end of a block means counting braces, and counting braces in Rust means lexing it first — `format!("{}", x)` would otherwise close the module early and undo the fix from the inside. `rustCodeLines()` blanks comments, strings and char literals, handling the three things a generic stripper gets wrong: nested block comments, raw strings (`r#"a "quoted" string"#`), and `'a` lifetimes that are not char literals. An unbalanced file claims only its attribute line, so a parse that has gone wrong cannot quietly silence the rest of the file. Only Rust gets this. Go's toolchain will not run a test outside a `_test.go` file, and Python and JavaScript convention give tests their own files — all three already read by `isTestPath`. Verified end to end through the built CLI: - ferriskey: 135 findings before and after, nothing dropped, high 39 -> 24. All 15 moved lines confirmed inside a `#[cfg(test)]` module by an independent check; no production line moved. - malware-test-prs: 134 findings, 46 critical, identical before and after, zero severity moves. Detection is unchanged. - packages/scan 347/347 and apps/cli 73/73 green. As with every other softening here, this moves severity and never drops a finding: the count, the SARIF and `--fail-on low` are all unaffected. The fixture exemption that *skips* a finding stays keyed on `isTestPath` alone — dropping is a verdict, and a block boundary inferred from brace counting is evidence for a severity, not for silence. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01CVMa6BbfoMTWAiGvkFXAdk --- .../scan/src/__tests__/inline-tests.test.ts | 195 ++++++++++++++++++ packages/scan/src/index.ts | 1 + packages/scan/src/text.ts | 193 ++++++++++++++++- 3 files changed, 388 insertions(+), 1 deletion(-) create mode 100644 packages/scan/src/__tests__/inline-tests.test.ts diff --git a/packages/scan/src/__tests__/inline-tests.test.ts b/packages/scan/src/__tests__/inline-tests.test.ts new file mode 100644 index 0000000..fe41564 --- /dev/null +++ b/packages/scan/src/__tests__/inline-tests.test.ts @@ -0,0 +1,195 @@ +/** + * Cause 4: a language whose tests live inside the file they test. + * + * The first three structural causes were all about *where a file sits* — + * `_test.go`, `testutils/`, `docs/`. Rust does not work that way. `cargo test` + * compiles unit tests from a `#[cfg(test)] mod tests` block at the bottom of + * the very file they cover, so the path is `core/src/domain/…/services.rs` for + * the production code and the fixtures alike, and no path rule can separate + * them. + * + * Every Rust line below is copied from ferriskey/ferriskey, an IAM server of + * 689 stars that scored 135 findings and zero true positives at 0.11.8. Twenty + * two of those are this cause. + * + * The stakes on the other side are why this is done with a brace-matched block + * rather than "everything after the first `#[cfg(test)]`": in `services.rs` the + * test module opens at line 2137 of 4042, so a to-end-of-file rule would soften + * production code on almost half a file, and a real hardcoded credential below + * a test module would be reported at `low` for no reason anybody could see. + */ +import { describe, expect, it } from 'vitest'; +import { inlineTestLines, scanText } from '../text'; + +const severityOf = (path: string, source: string, ruleId: string): string | undefined => + scanText(path, source).find((one) => one.ruleId === ruleId)?.severity; + +/** + * Assembled rather than written out, like every other credential fixture here. + * + * The repository's own `gitleaks` job scans all refs, not the diff, so a + * password-shaped literal sitting on this branch reddens pull requests that + * never touched it — see the entries in `.gitleaksignore` for the ones that + * already did. Nothing about the test needs the literal to exist in the tree; + * the scanner reads the string it is handed. + */ +const FIXTURE_PASSWORD = ['Str0ng!', 'P@ssword', '#2024'].join(''); + +/** + * The shape of `services.rs`: production code, then the test module. + * + * `services.rs:2719` is the sharpest of the twenty two — it reported at `high`, + * not at `low`, because the value is strong enough that none of the existing + * value-side softeners recognise it as a fixture. Being a good fake password is + * what made it look like a real one. Only the block it sits in says otherwise. + */ +const SERVICE = [ + 'pub async fn reset_password(&self, user: &User, new_password: String) -> Result<(), Error> {', + ' self.repository.update_credential(user.id, hash(&new_password)?).await', + '}', + '', + '#[cfg(test)]', + 'mod tests {', + ' use super::*;', + '', + ' #[tokio::test]', + ' async fn resets_a_password() {', + ' let input = ResetPasswordInput {', + ` new_password: "${FIXTURE_PASSWORD}".to_string(),`, + ' };', + ' assert!(service.reset_password(&user, input.new_password).await.is_ok());', + ' }', + '}', +].join('\n'); + +const SERVICE_PATH = 'core/src/domain/trident/services.rs'; + +describe('cause 4 — a fixture inside an inline test block', () => { + it('softens a test password in a #[cfg(test)] module', () => { + expect(severityOf(SERVICE_PATH, SERVICE, 'secret-generic-credential')).toBe('low'); + }); + + it('leaves the same line at full severity in the production half of the file', () => { + const production = SERVICE.replace('#[cfg(test)]\nmod tests {', 'mod helpers {').replace('#[tokio::test]\n', ''); + expect(severityOf(SERVICE_PATH, production, 'secret-generic-credential')).toBe('high'); + }); + + it('says in the message that the block is what softened it', () => { + const finding = scanText(SERVICE_PATH, SERVICE).find((one) => one.ruleId === 'secret-generic-credential'); + expect(finding?.message).toContain('in a test-only block'); + }); + + // The softening is a claim about severity and nothing else — the same line + // `context-severity.test.ts` draws for the path-based rules. + it('keeps the finding in the report rather than dropping it', () => { + const findings = scanText(SERVICE_PATH, SERVICE); + expect(findings.filter((one) => one.ruleId === 'secret-generic-credential')).toHaveLength(1); + }); +}); + +describe('where an inline test block starts and stops', () => { + const linesOf = (source: string): number[] => [...inlineTestLines('a.rs', source.split('\n'))].sort((x, y) => x - y); + + it('claims the module and nothing after it', () => { + const source = [ + 'fn before() {}', // 0 + '#[cfg(test)]', // 1 + 'mod tests {', // 2 + ' #[test]', // 3 + ' fn t() {}', // 4 + '}', // 5 + 'fn after() {}', // 6 + ].join('\n'); + expect(linesOf(source)).toEqual([1, 2, 3, 4, 5]); + }); + + // The reason the block needs a lexer at all. `format!("{}")` is a brace in a + // string on a line that is otherwise ordinary; counted, it closes the module + // early and every fixture below it goes back to reporting at full severity. + it('does not count braces inside strings', () => { + const source = [ + '#[cfg(test)]', + 'mod tests {', + ' fn t() {', + ' let s = format!("{} {{}}", x);', + ' let n = 1;', + ' }', + '}', + 'fn after() {}', + ].join('\n'); + expect(linesOf(source)).toEqual([0, 1, 2, 3, 4, 5, 6]); + }); + + it('does not count braces inside raw strings or comments', () => { + const source = [ + '#[cfg(test)]', + 'mod tests {', + ' // }', + ' /* } /* nested */ } */', + ' let q = r#"a "quoted" } brace"#;', + '}', + 'fn after() {}', + ].join('\n'); + expect(linesOf(source)).toEqual([0, 1, 2, 3, 4, 5]); + }); + + it('reads a lifetime as a lifetime, not as a char literal opening a string', () => { + const source = [ + '#[cfg(test)]', + 'mod tests {', + " fn t<'a>(s: &'a str) { let c = '}'; }", + '}', + 'fn after() {}', + ].join('\n'); + expect(linesOf(source)).toEqual([0, 1, 2, 3]); + }); + + it('claims a bare #[test] fn outside any module', () => { + const source = ['fn before() {}', '#[test]', 'fn t() {}', 'fn after() {}'].join('\n'); + expect(linesOf(source)).toEqual([1, 2]); + }); + + it('claims the async and parameterised test attributes too', () => { + for (const attribute of ['#[tokio::test]', '#[rstest]', '#[actix_web::test]']) { + expect(linesOf([attribute, 'fn t() {}', 'fn after() {}'].join('\n'))).toEqual([0, 1]); + } + }); + + it('claims a declaration that has no block', () => { + expect(linesOf(['#[cfg(test)]', 'mod tests;', 'fn after() {}'].join('\n'))).toEqual([0, 1]); + }); + + it('claims a gate written with all() or any()', () => { + expect(linesOf(['#[cfg(all(test, unix))]', 'mod tests {', '}', 'fn a() {}'].join('\n'))).toEqual([0, 1, 2]); + }); +}); + +describe('what an inline test block is not', () => { + // `not(test)` is the exact inverse: code compiled when tests are *off*. It is + // production code by definition, and softening it would be backwards. + it('does not claim #[cfg(not(test))]', () => { + const source = ['#[cfg(not(test))]', 'mod real {', ` let p = "${FIXTURE_PASSWORD}";`, '}'].join('\n'); + expect(inlineTestLines('a.rs', source.split('\n')).size).toBe(0); + }); + + // A feature flag whose name merely contains "test". The string blanking is + // what keeps this from reading as a gate. + it('does not claim a feature flag named for testing', () => { + const source = ['#[cfg(feature = "test-util")]', 'mod util {', '}'].join('\n'); + expect(inlineTestLines('a.rs', source.split('\n')).size).toBe(0); + }); + + it('does nothing to a language whose tests live in their own files', () => { + const go = ['// #[cfg(test)]', 'func main() {', ` password := "${FIXTURE_PASSWORD}"`, '}'].join('\n'); + expect(inlineTestLines('main.go', go.split('\n')).size).toBe(0); + expect(severityOf('main.go', go, 'secret-generic-credential')).not.toBe('low'); + }); + + // An unbalanced file means the stripper lost its place. Claiming the rest of + // it on a parse that already went wrong is how a scanner goes quiet on real + // findings, so an unclosed block claims only the line it started on. + it('claims only the attribute line when the braces never balance', () => { + const source = ['fn a() {}', '#[cfg(test)]', 'mod tests {', ' fn t() {'].join('\n'); + expect([...inlineTestLines('a.rs', source.split('\n'))]).toEqual([1]); + }); +}); diff --git a/packages/scan/src/index.ts b/packages/scan/src/index.ts index 3416e88..b979b2a 100644 --- a/packages/scan/src/index.ts +++ b/packages/scan/src/index.ts @@ -37,6 +37,7 @@ export { export { collectSuppressions, foreignSecurityMark, + inlineTestLines, isDocPath, isTestPath, languageOf, diff --git a/packages/scan/src/text.ts b/packages/scan/src/text.ts index ffbba62..851541b 100644 --- a/packages/scan/src/text.ts +++ b/packages/scan/src/text.ts @@ -243,6 +243,183 @@ export function isTestPath(relativePath: string): boolean { ); } +/** + * Rust source with its comments, strings and char literals blanked out. + * + * Brace counting is how the next function finds where a test module ends, and + * raw braces are everywhere in ordinary Rust that is not a block: `format!("{}", + * x)` is on more lines than most constructs this engine looks for. Counting + * those would close a module early and leave the rest of its tests reporting at + * full severity, which is the bug being fixed here wearing a different hat. + * + * Blanking rather than deleting keeps every column where it was, so a line's + * index and the offsets inside it still line up with the original text. + * + * Rust-specific in three ways a generic stripper gets wrong: block comments + * nest, so the first close does not necessarily end one; raw strings suspend + * escaping and close only on a quote followed by as many hashes as opened them + * (`r#"a "quoted" string"#`); and a lone `'` is far more often a lifetime + * (`&'a str`) than the start of a char literal. + */ +function rustCodeLines(lines: readonly string[]): string[] { + const out: string[] = []; + let commentDepth = 0; + let str: { raw: boolean; hashes: number } | null = null; + + for (const line of lines) { + let code = ''; + let i = 0; + + while (i < line.length) { + const c = line[i]!; + const d = line[i + 1]; + + if (commentDepth > 0) { + if (c === '*' && d === '/') { commentDepth -= 1; code += ' '; i += 2; continue; } + if (c === '/' && d === '*') { commentDepth += 1; code += ' '; i += 2; continue; } + code += ' '; i += 1; continue; + } + + if (str) { + if (str.raw) { + if (c === '"') { + let hashes = 0; + while (line[i + 1 + hashes] === '#') hashes += 1; + if (hashes >= str.hashes) { + code += ' '.repeat(1 + str.hashes); + i += 1 + str.hashes; + str = null; + continue; + } + } + code += ' '; i += 1; continue; + } + // A backslash escapes the next character, a closing quote included. + if (c === '\\') { code += ' '; i += 2; continue; } + if (c === '"') { str = null; code += ' '; i += 1; continue; } + code += ' '; i += 1; continue; + } + + if (c === '/' && d === '/') { code += ' '.repeat(line.length - i); break; } + if (c === '/' && d === '*') { commentDepth = 1; code += ' '; i += 2; continue; } + + // `r"…"`, `r#"…"#`, `br##"…"##` — but only where `r` opens a token, so + // the `r` ending an identifier cannot be read as a raw string prefix. + if (i === 0 || !/[A-Za-z0-9_]/.test(line[i - 1]!)) { + const raw = /^b?r(#*)"/.exec(line.slice(i)); + if (raw) { + str = { raw: true, hashes: raw[1]!.length }; + code += ' '.repeat(raw[0].length); + i += raw[0].length; + continue; + } + } + + if (c === '"') { str = { raw: false, hashes: 0 }; code += ' '; i += 1; continue; } + + if (c === "'") { + const char = /^'(?:\\.|[^\\'])'/.exec(line.slice(i)); + if (char) { code += ' '.repeat(char[0].length); i += char[0].length; continue; } + // A lifetime. Nothing to blank, and no string was opened. + code += c; i += 1; continue; + } + + code += c; + i += 1; + } + + out.push(code); + } + + return out; +} + +/** `not(test)` gates code compiled when tests are *off* — the opposite of a test block. */ +const RUST_CFG_NOT_TEST = /\bnot\s*\(\s*test\s*\)/; + +/** `#[test]`, `#[tokio::test]`, `#[rstest]`, `#[bench]` — an item only tests run. */ +const RUST_TEST_ITEM_ATTRIBUTE = + /^\s*#\s*\[\s*(?:[A-Za-z_]\w*\s*::\s*)*(?:test|bench|rstest|proptest|quickcheck|test_case)\b/; + +/** + * Does this (already blanked) line open an item that exists only under `cargo test`? + * + * The string blanking is what lets the `cfg` arm be written this loosely: + * `#[cfg(feature = "test-util")]` arrives here as `#[cfg(feature = " ")]`, + * so a feature flag that merely has "test" in its name is not read as a gate. + */ +function isRustTestAttribute(code: string): boolean { + if (RUST_CFG_NOT_TEST.test(code)) return false; + if (/^\s*#\s*\[\s*cfg\s*\(/.test(code)) return /\btest\b/.test(code); + return RUST_TEST_ITEM_ATTRIBUTE.test(code); +} + +/** The last line of the item beginning at `start`, by brace depth. */ +function rustItemEnd(code: readonly string[], start: number): number { + let depth = 0; + let opened = false; + + for (let i = start; i < code.length; i += 1) { + for (const ch of code[i]!) { + if (ch === '{') { + depth += 1; + opened = true; + } else if (ch === '}') { + depth -= 1; + if (opened && depth <= 0) return i; + } else if (ch === ';' && !opened && depth === 0) { + // A declaration rather than a block: `#[cfg(test)] mod tests;`. + return i; + } + } + } + + // Unbalanced — a truncated file, or a brace this stripper misread. Claiming + // the rest of the file would soften production code on the strength of a + // parse that has already gone wrong, so claim only what was certain. + return start; +} + +const NO_INLINE_TESTS: ReadonlySet = new Set(); + +/** + * The lines of a file that hold tests living *inside* it. + * + * `isTestPath` asks where a file sits, which is the whole answer in Go, Python + * and JavaScript, where tests occupy files of their own. Rust puts unit tests in + * the same file as the code they cover, behind `#[cfg(test)]` — so a path rule + * can never reach them, and every fixture password in every `mod tests` reports + * at full severity from what is, by construction, test code. + * + * Measured on ferriskey/ferriskey (Rust IAM, 689 stars) at 0.11.8: 135 findings, + * zero true positives, of which 22 are exactly this. `core/src/domain/trident/ + * services.rs` flags `new_password: "Str0ng!P@ssword#2024"` at line 2719 with its + * `#[cfg(test)]` opening at 2137; `argon2_hasher.rs` flags `let password = + * "my_password"` at 116 with the block opening at 109. Neither path can be + * spelled into `isTestPath` without softening the production half of the file + * along with it, which is the whole reason this is keyed on lines. + * + * Returns 0-based line indices, and only for Rust — the one language measured to + * need it. Go's toolchain will not run a test outside a `_test.go` file, and + * Python and JavaScript convention give tests their own files; `isTestPath` + * already reads all three. + */ +export function inlineTestLines(relativePath: string, lines: readonly string[]): ReadonlySet { + if (extensionOf(relativePath).toLowerCase() !== '.rs') return NO_INLINE_TESTS; + + const code = rustCodeLines(lines); + const inside = new Set(); + + for (let i = 0; i < code.length; i += 1) { + // Attributes on the tests *within* an already-claimed module add nothing. + if (inside.has(i) || !isRustTestAttribute(code[i]!)) continue; + const end = rustItemEnd(code, i); + for (let j = i; j <= end; j += 1) inside.add(j); + } + + return inside; +} + /** * Does this path hold documentation or illustrative code? * @@ -286,13 +463,16 @@ export function isDocPath(relativePath: string): boolean { * where code lives or what it is named, and heuristics of that kind are wrong * often enough that they may inform a severity and never a verdict. */ -type Softening = 'test' | 'docs' | 'suppressed' | 'placeholder' | 'self-describing'; +type Softening = 'test' | 'test-block' | 'docs' | 'suppressed' | 'placeholder' | 'self-describing'; /** Most specific evidence first, so it is the reason the operator is shown. */ const SOFTENING_ORDER: readonly Softening[] = [ 'suppressed', 'placeholder', 'self-describing', + // Before `test`, because it is the narrower claim: a path holds tests, but a + // block *is* one. A Rust file under `tests/` can be both. + 'test-block', 'test', 'docs', ]; @@ -304,6 +484,8 @@ function softening(reasons: Partial>): Softening | nu /** The clause completing "Possible AWS Access Key detected …" on a secret. */ const SECRET_SOFTENING: Record = { test: 'in a test file — usually a fixture, still worth confirming it is not a live credential', + 'test-block': + 'in a test-only block — usually a fixture, still worth confirming it is not a live credential', docs: 'in documentation — usually an illustrative example, still worth confirming it is not a live credential', suppressed: "on a line already marked as a false positive for another linter's security rule", placeholder: 'in example text an empty input field shows, not in data', @@ -314,6 +496,7 @@ const SECRET_SOFTENING: Record = { /** The clause appended to a code finding's message to say why it was softened. */ const CODE_SOFTENING: Record = { test: 'in a test file, where the construct is ordinary', + 'test-block': 'in a test-only block, where the construct is ordinary', docs: 'in documentation or example code, which nothing runs', suppressed: "on a line another linter's security suppression already covers", placeholder: 'in example text rather than in data', @@ -331,6 +514,7 @@ export function scanText( const suppressions = collectSuppressions(lines); const inTests = isTestPath(relativePath); const inDocs = isDocPath(relativePath); + const inlineTests = inlineTestLines(relativePath, lines); // ── Credentials ──────────────────────────────────────────────────────── lines.forEach((line, index) => { @@ -364,8 +548,13 @@ export function scanText( // while a real DSN in the same README is still `high`. Softening secrets // by path would take that guarantee away, and a README is a perfectly // ordinary place to leak a key. + // Note that the fixture *exemption* above stays keyed on `inTests` alone. + // That one drops a finding, and dropping is a verdict; a block boundary + // this engine inferred from brace counting is evidence for a severity and + // not for silence. An inline test block softens, and only softens. const soft = softening({ test: inTests, + 'test-block': inlineTests.has(index), suppressed: foreignSecurityMark(line), placeholder: isPlaceholderAttribute(line, value), 'self-describing': rule.keywordShaped === true && describesItsOwnKey(line, value), @@ -411,6 +600,7 @@ export function scanText( // self-signed certificate. Each is the normal way to write that test. const soft = softening({ test: inTests, + 'test-block': inlineTests.has(index), docs: inDocs, suppressed: foreignSecurityMark(lines[index] ?? ''), }); @@ -443,6 +633,7 @@ export function scanText( const soft = softening({ test: inTests, + 'test-block': inlineTests.has(index), docs: inDocs, suppressed: foreignSecurityMark(lines[index] ?? ''), }); From 2668e8730e60e42a58cb3a817b90f30d46f4f6e5 Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Wed, 2 Sep 2026 14:05:37 +0000 Subject: [PATCH 2/2] fix(scan): stop quoting a fixture credential in the inlineTestLines comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scanner flagged its own new doc comment: `text.ts` quoted the ferriskey line it was written to explain, credential and all, and `text.ts` is a production path where no softener applies. That is the rule working exactly as intended, on the commit that shipped it. Describes the measurement instead of transcribing it. Also corrects 22 to 15 — 22 was the count of findings sitting inside a `#[cfg(test)]` block, but 7 of those were already `low` from a value-side rule, so 15 is the number this change actually moves. Comment only; ferriskey scans identically before and after (135 findings, high 24), and `packages/scan` stays 347/347. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01CVMa6BbfoMTWAiGvkFXAdk --- packages/scan/src/text.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/scan/src/text.ts b/packages/scan/src/text.ts index 851541b..f3c96cf 100644 --- a/packages/scan/src/text.ts +++ b/packages/scan/src/text.ts @@ -392,12 +392,12 @@ const NO_INLINE_TESTS: ReadonlySet = new Set(); * at full severity from what is, by construction, test code. * * Measured on ferriskey/ferriskey (Rust IAM, 689 stars) at 0.11.8: 135 findings, - * zero true positives, of which 22 are exactly this. `core/src/domain/trident/ - * services.rs` flags `new_password: "Str0ng!P@ssword#2024"` at line 2719 with its - * `#[cfg(test)]` opening at 2137; `argon2_hasher.rs` flags `let password = - * "my_password"` at 116 with the block opening at 109. Neither path can be - * spelled into `isTestPath` without softening the production half of the file - * along with it, which is the whole reason this is keyed on lines. + * zero true positives, of which 15 are exactly this. The sharpest is a fixture + * password assigned in a `#[tokio::test]` at `core/src/domain/trident/ + * services.rs:2719`, whose `#[cfg(test)]` opens at 2137 of 4042 lines — so the + * block covers only the second half of the file, and neither half can be spelled + * into `isTestPath` without taking the other with it. That is the whole reason + * this is keyed on lines rather than on the path. * * Returns 0-based line indices, and only for Rust — the one language measured to * need it. Go's toolchain will not run a test outside a `_test.go` file, and