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..f3c96cf 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 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 + * 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] ?? ''), });