Skip to content

Commit 421d208

Browse files
ralyodioclaude
andauthored
fix(scan): soften findings inside Rust inline #[cfg(test)] blocks (#174)
* fix(scan): soften findings inside Rust inline #[cfg(test)] blocks 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CVMa6BbfoMTWAiGvkFXAdk * fix(scan): stop quoting a fixture credential in the inlineTestLines comment 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CVMa6BbfoMTWAiGvkFXAdk --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 231e57c commit 421d208

3 files changed

Lines changed: 388 additions & 1 deletion

File tree

Lines changed: 195 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,195 @@
1+
/**
2+
* Cause 4: a language whose tests live inside the file they test.
3+
*
4+
* The first three structural causes were all about *where a file sits* —
5+
* `_test.go`, `testutils/`, `docs/`. Rust does not work that way. `cargo test`
6+
* compiles unit tests from a `#[cfg(test)] mod tests` block at the bottom of
7+
* the very file they cover, so the path is `core/src/domain/…/services.rs` for
8+
* the production code and the fixtures alike, and no path rule can separate
9+
* them.
10+
*
11+
* Every Rust line below is copied from ferriskey/ferriskey, an IAM server of
12+
* 689 stars that scored 135 findings and zero true positives at 0.11.8. Twenty
13+
* two of those are this cause.
14+
*
15+
* The stakes on the other side are why this is done with a brace-matched block
16+
* rather than "everything after the first `#[cfg(test)]`": in `services.rs` the
17+
* test module opens at line 2137 of 4042, so a to-end-of-file rule would soften
18+
* production code on almost half a file, and a real hardcoded credential below
19+
* a test module would be reported at `low` for no reason anybody could see.
20+
*/
21+
import { describe, expect, it } from 'vitest';
22+
import { inlineTestLines, scanText } from '../text';
23+
24+
const severityOf = (path: string, source: string, ruleId: string): string | undefined =>
25+
scanText(path, source).find((one) => one.ruleId === ruleId)?.severity;
26+
27+
/**
28+
* Assembled rather than written out, like every other credential fixture here.
29+
*
30+
* The repository's own `gitleaks` job scans all refs, not the diff, so a
31+
* password-shaped literal sitting on this branch reddens pull requests that
32+
* never touched it — see the entries in `.gitleaksignore` for the ones that
33+
* already did. Nothing about the test needs the literal to exist in the tree;
34+
* the scanner reads the string it is handed.
35+
*/
36+
const FIXTURE_PASSWORD = ['Str0ng!', 'P@ssword', '#2024'].join('');
37+
38+
/**
39+
* The shape of `services.rs`: production code, then the test module.
40+
*
41+
* `services.rs:2719` is the sharpest of the twenty two — it reported at `high`,
42+
* not at `low`, because the value is strong enough that none of the existing
43+
* value-side softeners recognise it as a fixture. Being a good fake password is
44+
* what made it look like a real one. Only the block it sits in says otherwise.
45+
*/
46+
const SERVICE = [
47+
'pub async fn reset_password(&self, user: &User, new_password: String) -> Result<(), Error> {',
48+
' self.repository.update_credential(user.id, hash(&new_password)?).await',
49+
'}',
50+
'',
51+
'#[cfg(test)]',
52+
'mod tests {',
53+
' use super::*;',
54+
'',
55+
' #[tokio::test]',
56+
' async fn resets_a_password() {',
57+
' let input = ResetPasswordInput {',
58+
` new_password: "${FIXTURE_PASSWORD}".to_string(),`,
59+
' };',
60+
' assert!(service.reset_password(&user, input.new_password).await.is_ok());',
61+
' }',
62+
'}',
63+
].join('\n');
64+
65+
const SERVICE_PATH = 'core/src/domain/trident/services.rs';
66+
67+
describe('cause 4 — a fixture inside an inline test block', () => {
68+
it('softens a test password in a #[cfg(test)] module', () => {
69+
expect(severityOf(SERVICE_PATH, SERVICE, 'secret-generic-credential')).toBe('low');
70+
});
71+
72+
it('leaves the same line at full severity in the production half of the file', () => {
73+
const production = SERVICE.replace('#[cfg(test)]\nmod tests {', 'mod helpers {').replace('#[tokio::test]\n', '');
74+
expect(severityOf(SERVICE_PATH, production, 'secret-generic-credential')).toBe('high');
75+
});
76+
77+
it('says in the message that the block is what softened it', () => {
78+
const finding = scanText(SERVICE_PATH, SERVICE).find((one) => one.ruleId === 'secret-generic-credential');
79+
expect(finding?.message).toContain('in a test-only block');
80+
});
81+
82+
// The softening is a claim about severity and nothing else — the same line
83+
// `context-severity.test.ts` draws for the path-based rules.
84+
it('keeps the finding in the report rather than dropping it', () => {
85+
const findings = scanText(SERVICE_PATH, SERVICE);
86+
expect(findings.filter((one) => one.ruleId === 'secret-generic-credential')).toHaveLength(1);
87+
});
88+
});
89+
90+
describe('where an inline test block starts and stops', () => {
91+
const linesOf = (source: string): number[] => [...inlineTestLines('a.rs', source.split('\n'))].sort((x, y) => x - y);
92+
93+
it('claims the module and nothing after it', () => {
94+
const source = [
95+
'fn before() {}', // 0
96+
'#[cfg(test)]', // 1
97+
'mod tests {', // 2
98+
' #[test]', // 3
99+
' fn t() {}', // 4
100+
'}', // 5
101+
'fn after() {}', // 6
102+
].join('\n');
103+
expect(linesOf(source)).toEqual([1, 2, 3, 4, 5]);
104+
});
105+
106+
// The reason the block needs a lexer at all. `format!("{}")` is a brace in a
107+
// string on a line that is otherwise ordinary; counted, it closes the module
108+
// early and every fixture below it goes back to reporting at full severity.
109+
it('does not count braces inside strings', () => {
110+
const source = [
111+
'#[cfg(test)]',
112+
'mod tests {',
113+
' fn t() {',
114+
' let s = format!("{} {{}}", x);',
115+
' let n = 1;',
116+
' }',
117+
'}',
118+
'fn after() {}',
119+
].join('\n');
120+
expect(linesOf(source)).toEqual([0, 1, 2, 3, 4, 5, 6]);
121+
});
122+
123+
it('does not count braces inside raw strings or comments', () => {
124+
const source = [
125+
'#[cfg(test)]',
126+
'mod tests {',
127+
' // }',
128+
' /* } /* nested */ } */',
129+
' let q = r#"a "quoted" } brace"#;',
130+
'}',
131+
'fn after() {}',
132+
].join('\n');
133+
expect(linesOf(source)).toEqual([0, 1, 2, 3, 4, 5]);
134+
});
135+
136+
it('reads a lifetime as a lifetime, not as a char literal opening a string', () => {
137+
const source = [
138+
'#[cfg(test)]',
139+
'mod tests {',
140+
" fn t<'a>(s: &'a str) { let c = '}'; }",
141+
'}',
142+
'fn after() {}',
143+
].join('\n');
144+
expect(linesOf(source)).toEqual([0, 1, 2, 3]);
145+
});
146+
147+
it('claims a bare #[test] fn outside any module', () => {
148+
const source = ['fn before() {}', '#[test]', 'fn t() {}', 'fn after() {}'].join('\n');
149+
expect(linesOf(source)).toEqual([1, 2]);
150+
});
151+
152+
it('claims the async and parameterised test attributes too', () => {
153+
for (const attribute of ['#[tokio::test]', '#[rstest]', '#[actix_web::test]']) {
154+
expect(linesOf([attribute, 'fn t() {}', 'fn after() {}'].join('\n'))).toEqual([0, 1]);
155+
}
156+
});
157+
158+
it('claims a declaration that has no block', () => {
159+
expect(linesOf(['#[cfg(test)]', 'mod tests;', 'fn after() {}'].join('\n'))).toEqual([0, 1]);
160+
});
161+
162+
it('claims a gate written with all() or any()', () => {
163+
expect(linesOf(['#[cfg(all(test, unix))]', 'mod tests {', '}', 'fn a() {}'].join('\n'))).toEqual([0, 1, 2]);
164+
});
165+
});
166+
167+
describe('what an inline test block is not', () => {
168+
// `not(test)` is the exact inverse: code compiled when tests are *off*. It is
169+
// production code by definition, and softening it would be backwards.
170+
it('does not claim #[cfg(not(test))]', () => {
171+
const source = ['#[cfg(not(test))]', 'mod real {', ` let p = "${FIXTURE_PASSWORD}";`, '}'].join('\n');
172+
expect(inlineTestLines('a.rs', source.split('\n')).size).toBe(0);
173+
});
174+
175+
// A feature flag whose name merely contains "test". The string blanking is
176+
// what keeps this from reading as a gate.
177+
it('does not claim a feature flag named for testing', () => {
178+
const source = ['#[cfg(feature = "test-util")]', 'mod util {', '}'].join('\n');
179+
expect(inlineTestLines('a.rs', source.split('\n')).size).toBe(0);
180+
});
181+
182+
it('does nothing to a language whose tests live in their own files', () => {
183+
const go = ['// #[cfg(test)]', 'func main() {', ` password := "${FIXTURE_PASSWORD}"`, '}'].join('\n');
184+
expect(inlineTestLines('main.go', go.split('\n')).size).toBe(0);
185+
expect(severityOf('main.go', go, 'secret-generic-credential')).not.toBe('low');
186+
});
187+
188+
// An unbalanced file means the stripper lost its place. Claiming the rest of
189+
// it on a parse that already went wrong is how a scanner goes quiet on real
190+
// findings, so an unclosed block claims only the line it started on.
191+
it('claims only the attribute line when the braces never balance', () => {
192+
const source = ['fn a() {}', '#[cfg(test)]', 'mod tests {', ' fn t() {'].join('\n');
193+
expect([...inlineTestLines('a.rs', source.split('\n'))]).toEqual([1]);
194+
});
195+
});

‎packages/scan/src/index.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ export {
3737
export {
3838
collectSuppressions,
3939
foreignSecurityMark,
40+
inlineTestLines,
4041
isDocPath,
4142
isTestPath,
4243
languageOf,

0 commit comments

Comments
 (0)