From a0af12749c294907c14321c6c6941ec34cdaf08c Mon Sep 17 00:00:00 2001 From: Zack Whitson Date: Sat, 8 Aug 2026 05:29:07 -0500 Subject: [PATCH] Release CodeTruss CLI v0.2.50 Syncs 0.2.50 from the private monorepo, and with it the source of 0.2.47, 0.2.48 and 0.2.49, which were merged there but never tagged. The website manifest already advertises 0.2.50 and a releaseUrl that 404s until this is tagged, so shipping it is what makes our own published verify instructions work again. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 206 +++++++++++++++++- packages/analyzer-engine/src/comment-slop.ts | 6 +- packages/analyzer-engine/src/complexity.ts | 6 +- packages/analyzer-engine/src/dead-code.ts | 153 +++++++++++-- packages/analyzer-engine/src/duplication.ts | 6 +- .../analyzer-engine/src/overengineering.ts | 6 +- packages/analyzer-engine/src/scoring.ts | Bin 5166 -> 8676 bytes packages/analyzer-engine/src/secrets.ts | 192 ++++++++++++---- .../analyzer-engine/src/security/engine.ts | 45 +++- .../analyzer-engine/src/security/normalize.ts | 58 ++++- .../analyzer-engine/src/security/taint.ts | 90 +++++++- packages/analyzer-engine/src/todos.ts | 41 ++-- packages/analyzer-engine/src/types.ts | 19 ++ .../analyzer-engine/src/vulnerabilities.ts | 83 ++++++- packages/cli/CHANGELOG.md | 204 +++++++++++++++++ packages/cli/package.json | 2 +- .../codetruss-cli-0.2.50.sbom.cdx.json | 170 +++++++++++++++ public/downloads/codetruss-cli-0.2.50.tgz | Bin 0 -> 925789 bytes .../downloads/codetruss-cli-0.2.50.tgz.sha256 | 1 + public/downloads/codetruss-cli-latest.json | 14 +- .../codetruss-cli-latest.sbom.cdx.json | 10 +- public/downloads/codetruss-cli-latest.tgz | Bin 906328 -> 925789 bytes .../downloads/codetruss-cli-latest.tgz.sha256 | 2 +- release-reference.json | 10 +- 24 files changed, 1186 insertions(+), 138 deletions(-) create mode 100644 public/downloads/codetruss-cli-0.2.50.sbom.cdx.json create mode 100644 public/downloads/codetruss-cli-0.2.50.tgz create mode 100644 public/downloads/codetruss-cli-0.2.50.tgz.sha256 diff --git a/CHANGELOG.md b/CHANGELOG.md index 21e2e1e..3dfb867 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,7 @@ CodeTruss CLI follows semantic versioning. Release artifacts and their SHA-256 checksums are published at . -The current public release is [v0.2.46 on GitHub](https://github.com/CodeTruss/codetruss-cli/releases/tag/v0.2.46), +The current public release is [v0.2.50 on GitHub](https://github.com/CodeTruss/codetruss-cli/releases/tag/v0.2.50), distributed from . The npm `latest` tag is still [`@codetruss/cli@0.2.41`](https://www.npmjs.com/package/@codetruss/cli/v/0.2.41): @@ -16,6 +16,210 @@ were superseded before distribution. No unreleased changes. +## 0.2.50 — 2026-08-08 + +- **`dead-code` spent 26 of this analysis's 27 seconds and bought nothing with + them.** The pass concatenated every indexed JS/TS file into a single string — + 18.2 MB on `calcom/cal.com` — and then ran one regular expression per + candidate module against the whole of it. That is O(candidates × corpus + bytes): 1,500 sweeps of an 18 MB string, roughly 64 GB of scanning, to decide + 1,500 yes/no questions. And it bought nothing, because the pass has been + saturated at its 20-finding output cap the entire time — more scanning changed + only *which* candidates were examined, never how many findings came out. It + now reads the corpus twice, building one index of the filename stems the + repository actually references, and answers each candidate with a set lookup. + On `calcom/cal.com@b2c28a23` (7,691 files, 517,420 LOC) `dead-code` drops from + 25.8s to 0.03s and the whole deterministic analyzer phase from 27.3s to 1.5s. + + **The findings are unchanged, and that was checked rather than assumed.** The + old expression matched a filename stem wherever it appeared — inside strings, + inside comments, inside unrelated tokens — and a tidier index that quietly + stopped doing so would start reporting live modules as dead. So the + replacement reproduces that looseness exactly. On four pinned repositories + (cal.com, astro, TanStack/query, hono) the reported findings, the findings + withheld behind the cap, the completeness flags and the pass metrics are + byte-identical before and after, and a differential run against the original + expression agrees on all 60,600 generated cases. A stem containing whitespace + or a quote character can still straddle the delimiters the index keys on, so + those candidates keep the original whole-corpus test; no real filename needs + it, and the fallback exists so the rewrite cannot narrow the rule by accident. + +## 0.2.49 — 2026-08-08 + +- **A redirect to a path this codebase wrote itself was reported as an open + redirect, and on one real repository seventeen of nineteen such reports were + false.** 0.2.45 widened the open-redirect sink to JSX `href`/`action`, + `location.assign|replace|href` and `router.push|replace`, and that widening + was narrowed against this repository alone. An open redirect requires the + attacker to control the ORIGIN — the host, or the leading `//` or scheme that + decides it — and two shapes cannot reach it. `new URL(reference, base)` + resolves by WHATWG rules, so any reference that is not itself absolute takes + the base's origin and the origin question collapses onto the first argument; + the head-position analysis that already answers exactly that question for a + template or a concatenation now re-asks itself there instead of treating the + construction as opaque. `new URLSearchParams({ … })` is a query-string + builder whose serialization is `application/x-www-form-urlencoded`, so `/`, + `:`, `<`, `&`, `=`, `?` and `#` all come back percent-encoded and a value put + in this way can only ever emerge as an encoded query value. On `calcom/cal.com` + the open-redirect count falls from 19 to 12, on `vercel/commerce` from 1 to 0, + on `shadcn-ui/taxonomy` from 1 to 0, and six further SaaS repositories produce + byte-identical findings before and after. + + What still fires, deliberately: `new URL(req.query.next, req.url)`, because a + bare reference may be absolute and override the base; a protocol-relative + reference, which discards the base origin; `new URL(x)` with one argument, + where `x` is the whole URL; and `new URLSearchParams(location.search)`, which + PARSES rather than builds, so reading a value back out of it keeps its taint. + Writing a tainted value into a query string is safe; reading one out and using + it as the target is the vulnerability, and that direction is unchanged. + + Recall cost, stated plainly: a redirect whose reference begins with a literal + `/` is no longer reported however tainted the rest of it is, which takes + `new URL('/fixed', attackerControlledBase)` host injection with it — the same + trade-off `evalOrigins` already documented for the constant case. Neither + suppression can be reached through a local variable, so + ``const u = `/x/${t}`; res.redirect(u)`` is still reported. + +## 0.2.48 — 2026-08-07 + +- **Ten bounded passes said "truncated" without ever learning what they were + truncated out of, so the largest repositories got no score at all.** A + required pass that stopped early is not authoritative, and the only way back + is to show it still covered enough of its input. An output cap could never + show that: it stops at N and never counts past N, so no denominator exists and + the gate has to assume the worst. On a 7,690-file, 517k-LOC repository ten + required passes tripped a cap — acquisition, the symbol graph, the knowledge + graph, and the duplication, secrets, dead-code, complexity, vulnerabilities, + comment-slop and speculative-structure analyzers — and all five score axes + were withheld. Nobody decided that; it fell out of the arithmetic, and it got + more certain the bigger the repository was. + + Every one of those caps now reports what it cost, in the unit it bounds. + Candidate-file bounds (duplication, dead-code, complexity, comment-slop, + speculative structure) divide the files they examined by the files the filter + produced — both numbers already existed, one slice apart. The secret scan and + the TODO scan keep counting after they stop collecting, so they report matches + shown over matches found. The vulnerability pass reports manifests read over + manifests present, and package versions checked over package versions + declared, whichever is worse. Acquisition divides the archive entries it wrote + by the entries it saw. The symbol graph reports files parsed over candidate + files and call sites kept over call sites found; the knowledge graph counts + the distinct nodes and edges its caps refuse, so those have denominators too. + + What this is not: a way to publish a score that should be withheld. The + threshold is unchanged at 95%, no pass gained a path around it, and a cap that + still cannot name its denominator still claims nothing and still voids the + score. The change is that a pass which CAN name it now does, so the existing + gate can tell one oversized archive entry in 7,690 apart from an analyzer that + lost half its findings. A repository whose secret scan reported 500 of 5,000 + matches is withheld exactly as it was before — and now says so in those words. + +- **`vulnerabilities` withheld every score with a reason that said nothing had + been lost.** The pass truncated on the dependency-manifest bound and then + reported package-version coverage, which was complete: "covered 181 of 181 + package versions", printed as the justification for publishing no score at + all. Two independent bounds share that pass, and only one of them was ever + described. Each bound that bites is now named in its own unit — "read 5 of 92 + dependency manifests", "checked 200 of 400 declared package versions" — and + the coverage claimed is the worst of them. + +- The secret scan's reported coverage is the fraction of MATCHES shown rather + than the fraction of files read, superseding the file-fraction measure added + in 0.2.47. Both answer "how much did the cap cost", but only the match count + answers it in findings: the file measure reads 100% whenever the cap is + reached on the last file scanned, and 2% when it is reached on the first, + neither of which is the number of credentials a reader cannot see. The sweep + no longer stops at the cap, which costs one more pass over a tree the scanner + already sweeps in full whenever the cap is not reached. + +- The TODO pass counted markers only up to its 500-marker retention bound, so + its headline finding — a count — reported the bound instead of the codebase on + any repository above it. The count is now exact; the bound governs only which + markers get individual findings. + +## 0.2.47 — 2026-08-07 + +- **A loop variable named `event` was treated as an HTTP request.** The taint + engine decided an expression was untrusted input by looking at the NAME of the + root identifier: `event`, `args`, `context`, `params` and `ctx` were on the + request-root list alongside `req` and the PHP superglobals, and nothing + checked that a parameter had actually bound the name. In a 517k-LOC codebase + we scanned, `for (const event of salesforceEvents)` made every + `event.` a source, and the three CRITICAL SQL-injection reports that + came out of that one loop were 39% of the repository's entire security + deduction. The rows were already inside the database they were said to be + injecting into. Those five ambiguous roots now count as a request only when + the enclosing function's signature bound them, positionally or by + destructuring (`({ ctx, input })` is how most handlers are written, so the + gate sees through patterns). The unambiguous roots are untouched: nobody names + a loop variable `req` or `$_GET`. + + The narrowing is real and we are not hiding it. A handler that lands its + request in a LOCAL named `event` — `const event = JSON.parse(body)` — or a + serverless adapter that binds it through a module-level variable now loses the + source, and every finding that depended on it. The one shape that mattered in + practice, `const params = useSearchParams()`, is covered again by treating + `useSearchParams()` and `useParams()` as sources in their own right: the call + is evidence, the variable's name never was. + +- **Placeholder detection could bless a real key, and did it silently.** Two + compounding defects. The words that mark a value as fake were matched anywhere + inside it, so `xxx` occurring by chance inside a random credential silenced + the line — measured against 400 real `generateKeyPairSync` RSA-2048 keys, 18 + of them (4.5%) contain a matching run. And the check ran against the whole + regex match rather than the value, so `examplePassword = ""` was + dismissed because of the identifier next to it. Every alternative is now + anchored to a token boundary, the pre-existing ones included, and the subject + is the extracted quoted value. Private key blocks are excluded from + value-level matching entirely — a base64 PEM body is a high-entropy blob, not + a string that can announce itself as fake — so the 4.5% collision cannot reach + the "No action needed." path by another route. The same 400 keys now produce + zero matches. `mock`, `stub`, `test-key`/`test-secret` and `not-a-real` join + the list as genuine placeholder forms. + + Consequence worth knowing before you upgrade: a credential-shaped string that + merely CONTAINS a word like `fake` inside its random body is no longer + downgraded to INFO. `sk_live_51QxR8fake2eKjL9…` is indistinguishable from a + live key by any rule that does not simply trust a substring, so it is reported + rather than blessed. Announce a placeholder with delimiters + (`sk_live_test-key-…`, `not-a-real-password`) and it is recognised as before. + +- **A placeholder declared one line up was invisible.** The placeholder check + read a single line, so a Swagger `@ApiProperty({ example: { … } })` two lines + above a documentation sample did not cover it and the sample was reported as a + committed leak. A placeholder marker that opens a block now covers that block, + which ends at the first non-blank line indented no further than the opener. + +- **Findings were double-charged, and repeats were charged in full.** Scoring + summed severity weights flat. Eight firings of one rule against one mock + string in one spec file cost eight findings — 14% of one repository's security + deduction for a single review decision — and a hard-coded credential found by + both the secrets analyzer and the SAST rule was charged twice for one line. + Repeat hits of the same rule in the same file now decay to `1 + ln(n)`, with + the worst finding in the group still charged in full, and `(filePath, line)` + is deduped across analyzers. The finding LIST is unchanged — both entries + still appear, with their own evidence and their own fix; only the arithmetic + collapses them. + + The cost lands on concentration: a file with twelve DISTINCT injection sinks + now prices close to a file with one, and two genuinely different defects on + one line price as one. Concentration is legitimate signal and most of it is + lost from the score. It remains visible in the findings. + +- **The secret scan stopped at 50 matches and took every score down with it.** + 50 was reached exactly on a 517k-LOC repository, which made every count anyone + quoted a truncated prefix — and because a truncated required pass is not + authoritative, a benign cap withheld all five score axes. The cap is now 500, + and the pass reports the fraction of its eligible files it actually read, so + an immaterial cap no longer voids the scores while a real coverage loss still + does. + +- Known, unchanged: the `Private key block` pattern matches only the PEM header, + so anything that inspects the matched text sees `-----BEGIN PRIVATE KEY-----` + and never the body. That is why the exclusion above is written by credential + type rather than by trusting the value check to hold over 1,700 random + characters. + ## 0.2.46 — 2026-08-07 - **A repository's own scope globs could crash the review that reads them.** diff --git a/packages/analyzer-engine/src/comment-slop.ts b/packages/analyzer-engine/src/comment-slop.ts index ad28b2e..2089f2b 100644 --- a/packages/analyzer-engine/src/comment-slop.ts +++ b/packages/analyzer-engine/src/comment-slop.ts @@ -1,6 +1,7 @@ import { annotatedAnalyzerOutput, incompleteAnalyzerOutput, + measuredCoverage, type Analyzer, type AnalyzerFinding, } from './types' @@ -430,7 +431,10 @@ export const commentSlopAnalyzer: Analyzer = { if (candidates.length > candidateLimit) { return incompleteAnalyzerOutput(findings, { truncated: true, - detail: `Comment analysis hit a candidate bound (${candidates.length} candidate files).`, + // The filter ran over the whole tree before the slice, so the files the + // bound cut are counted, not merely unknown. + coverageRatio: measuredCoverage(candidateLimit, candidates.length), + detail: `Comment analysis measured ${candidateLimit} of ${candidates.length} candidate files.`, metrics: { ...metrics, candidates: candidates.length, candidateLimit }, }, withheld) } diff --git a/packages/analyzer-engine/src/complexity.ts b/packages/analyzer-engine/src/complexity.ts index f919fe3..9a33f5a 100644 --- a/packages/analyzer-engine/src/complexity.ts +++ b/packages/analyzer-engine/src/complexity.ts @@ -1,6 +1,7 @@ import { annotatedAnalyzerOutput, incompleteAnalyzerOutput, + measuredCoverage, type Analyzer, type AnalyzerFinding, } from './types' @@ -164,7 +165,10 @@ export const complexityAnalyzer: Analyzer = { if (candidates.length > candidateLimit) { return incompleteAnalyzerOutput(output, { truncated: true, - detail: `Complexity analysis hit a candidate bound (${candidates.length} candidate files, ${findings.length} matches).`, + // The filter ran over the whole tree before the slice, so the files the + // bound cut are counted, not merely unknown. + coverageRatio: measuredCoverage(candidateLimit, candidates.length), + detail: `Complexity analysis measured ${candidateLimit} of ${candidates.length} candidate files (${findings.length} matches).`, metrics: { candidates: candidates.length, candidateLimit, matches: findings.length, findingLimit }, }, withheld) } diff --git a/packages/analyzer-engine/src/dead-code.ts b/packages/analyzer-engine/src/dead-code.ts index fc3482d..0ac314b 100644 --- a/packages/analyzer-engine/src/dead-code.ts +++ b/packages/analyzer-engine/src/dead-code.ts @@ -1,6 +1,7 @@ import { annotatedAnalyzerOutput, incompleteAnalyzerOutput, + measuredCoverage, type Analyzer, type AnalyzerFinding, } from './types' @@ -17,6 +18,101 @@ import { */ export const CONVENTION_FILENAME = /(page|layout|route|loading|error|not-found|template|default|middleware|proxy|instrumentation|opengraph-image|twitter-image|icon|apple-icon|sitemap|robots|manifest|index|main|app|server|config|next-env|globals)\.[jt]sx?$|^instrumentation[-.][a-z]+\.[jt]sx?$|\.(stories|story)\.[jt]sx?$|\.(d|config|test|spec)\.[cm]?[jt]s$/ +/** + * The characters that end a filename stem in every reference shape the needle + * below recognises: the three quote styles it looks between, plus whitespace + * and the path separator. + */ +const STEM_BOUNDARY = /[\s/'"`]/ + +/** Module extensions the needle accepts after a stem. Mirrors `\.[cm]?[jt]sx?`. */ +const REFERENCE_EXTENSION = /\.[cm]?[jt]sx?(?=[\s'"`])/g + +/** `"…/stem"` — an extensionless import specifier closing on its quote. */ +const QUOTED_PATH_TAIL = /\/([^\s/'"`]+)['"`]/g + +function isQuote(char: string | undefined): boolean { + return char === '"' || char === "'" || char === '`' +} + +/** + * Imported anywhere? look for `/stem'`, `/stem"`, or `from './stem`-style + * refs — or a bare quoted `stem.ext` (a spawn-by-string worker path built + * from segments, e.g. join(cwd, 'src', 'lib', 'batch-process.ts')). + * The final alternative catches a path embedded in a longer command + * string — `"sync-stripe": "node --env-file .env sync-stripe.js"` — where + * the quote does not directly abut the path. + */ +function referenceNeedle(stem: string): RegExp { + return new RegExp( + `['"\`](?:[^'"\`]*/${escapeRegExp(stem)}(\\.[cm]?[jt]sx?)?|${escapeRegExp(stem)}\\.[cm]?[jt]sx?)['"\`]` + + `|[\\s/]${escapeRegExp(stem)}\\.[cm]?[jt]sx?(?=[\\s'"\`])`, + ) +} + +/** + * Which of `stems` the concatenated corpus refers to, decided in TWO passes over + * the corpus instead of one whole-corpus regex per candidate. + * + * Running `referenceNeedle` per candidate is O(candidates x corpus bytes): on a + * 500k-LOC monorepo that is 1,500 sweeps of an 18 MB string, roughly 64 GB of + * scanning, and it dominated the entire analysis. The membership question + * answered here is deliberately the SAME one — matches inside strings, comments + * and unrelated tokens included. This is a cost fix, not a precision change; a + * stricter index would silently change which modules are called dead. + * + * The needle's three branches, rewritten as (character before stem, stem, + * character after the stem's extension): + * + * A. `['"`][^'"`]*\/stem['"`]` — `/` before, quote after, NO extension + * B. `['"`]stem.ext['"`]` — quote before, quote after + * C. `[\s/]stem.ext(?=[\s'"`])` — whitespace or `/` before, whitespace + * or quote after + * + * B and C are found together by anchoring on the extension (rare in source text) + * and reading backwards to the delimiter that opens the stem; the extension form + * of A — `['"`][^'"`]*\/stem.ext['"`]` — is a strict subset of C, because C + * already admits a `/` before and a quote after and asks for no opening quote. + * A's extensionless form has no extension to anchor on, so it gets its own scan. + * + * Correct only for stems that contain no boundary character, which is why the + * caller keeps the original needle for the ones that do. + */ +function referencedStems(corpus: string, stems: ReadonlySet): Set { + const referenced = new Set() + if (stems.size === 0) return referenced + const note = (stem: string) => { + if (stems.has(stem)) referenced.add(stem) + } + + // B and C. A stem with no boundary character is exactly the run of characters + // between the extension and the delimiter that precedes it. + for (const match of corpus.matchAll(REFERENCE_EXTENSION)) { + const dot = match.index + let start = dot + while (start > 0 && !STEM_BOUNDARY.test(corpus[start - 1])) start -= 1 + // No opening delimiter at all, or nothing between it and the extension: + // every branch needs one character of each. + if (start === 0 || start === dot) continue + // A quoted `"stem.ext"` (B) must close on its quote. The unquoted form (C) + // may close on whitespace but can only OPEN on whitespace or `/`, so a + // stem that opens on a quote and closes on whitespace matches neither. + if (isQuote(corpus[start - 1]) && !isQuote(corpus[dot + match[0].length])) continue + note(corpus.slice(start, dot)) + } + + // A, extensionless. `[^'"`]*` spans anything quote-free, so the opening quote + // is satisfied by the nearest quote before the slash — that is, by any quote + // occurring earlier in the corpus at all. + const firstQuote = corpus.search(/['"`]/) + if (firstQuote !== -1) { + for (const match of corpus.matchAll(QUOTED_PATH_TAIL)) { + if (match.index > firstQuote) note(match[1]) + } + } + return referenced +} + /** * Dead-code candidates: JS/TS modules that are never imported anywhere. * Heuristic (static string matching), so results are labeled candidates. @@ -61,6 +157,10 @@ export const deadCodeAnalyzer: Analyzer = { generatedDirs.some((dir) => (dir === '' ? !path.includes('/') : path.startsWith(dir))) const candidateLimit = 1500 + // Select every candidate before testing any of them, so the reference index + // is built once for exactly the stems that will be asked about. The order is + // the order of `jsFiles`, which is the order the findings keep below. + const candidates: Array<{ path: string; stem: string }> = [] for (const file of jsFiles.slice(0, candidateLimit)) { if (nearGeneratedOutput(file.path)) continue // CLI/tooling entry points are invoked by runners, not imports @@ -74,28 +174,32 @@ export const deadCodeAnalyzer: Analyzer = { if (base.startsWith('.')) continue const stem = base.replace(/\.[cm]?[jt]sx?$/, '') if (stem.length < 3) continue // too ambiguous to match safely - // Imported anywhere? look for `/stem'`, `/stem"`, or `from './stem`-style - // refs — or a bare quoted `stem.ext` (a spawn-by-string worker path built - // from segments, e.g. join(cwd, 'src', 'lib', 'batch-process.ts')). - // The final alternative catches a path embedded in a longer command - // string — `"sync-stripe": "node --env-file .env sync-stripe.js"` — where - // the quote does not directly abut the path. - const needle = new RegExp( - `['"\`](?:[^'"\`]*/${escapeRegExp(stem)}(\\.[cm]?[jt]sx?)?|${escapeRegExp(stem)}\\.[cm]?[jt]sx?)['"\`]` - + `|[\\s/]${escapeRegExp(stem)}\\.[cm]?[jt]sx?(?=[\\s'"\`])`, - ) - if (!needle.test(allContent)) { - findings.push({ - category: 'DEAD_CODE', - severity: 'LOW', - title: `Possibly unused module: ${file.path}`, - description: `No other file appears to import "${stem}". If it is not loaded by convention or tooling, it is dead code.`, - filePath: file.path, - suggestion: 'Verify with your bundler or `knip`/`ts-prune`, then delete if truly unused.', - impactScore: 35, - effort: 'low', - }) - } + candidates.push({ path: file.path, stem }) + } + + // A stem holding whitespace or a quote can straddle the delimiters the index + // keys on, so those candidates keep the original whole-corpus needle. No + // real filename does this; the fallback exists so the rewrite cannot narrow + // the rule by accident on one that does. + const indexable = new Set( + candidates.map((c) => c.stem).filter((stem) => !STEM_BOUNDARY.test(stem)), + ) + const referenced = referencedStems(allContent, indexable) + const isReferenced = (stem: string) => + indexable.has(stem) ? referenced.has(stem) : referenceNeedle(stem).test(allContent) + + for (const { path, stem } of candidates) { + if (isReferenced(stem)) continue + findings.push({ + category: 'DEAD_CODE', + severity: 'LOW', + title: `Possibly unused module: ${path}`, + description: `No other file appears to import "${stem}". If it is not loaded by convention or tooling, it is dead code.`, + filePath: path, + suggestion: 'Verify with your bundler or `knip`/`ts-prune`, then delete if truly unused.', + impactScore: 35, + effort: 'low', + }) } const findingLimit = 20 // Only the candidate-file cap is real coverage loss; the finding cap just @@ -109,7 +213,10 @@ export const deadCodeAnalyzer: Analyzer = { if (truncated) { return incompleteAnalyzerOutput(output, { truncated: true, - detail: `Dead-code analysis hit a bound (${jsFiles.length} candidate files, ${findings.length} matches).`, + // The candidate list was built from the whole tree before the slice, so + // the files the bound cut are counted, not merely unknown. + coverageRatio: measuredCoverage(candidateLimit, jsFiles.length), + detail: `Dead-code analysis examined ${candidateLimit} of ${jsFiles.length} candidate files (${findings.length} matches).`, metrics: { candidates: jsFiles.length, candidateLimit, matches: findings.length, findingLimit }, }, withheld) } diff --git a/packages/analyzer-engine/src/duplication.ts b/packages/analyzer-engine/src/duplication.ts index 6804e56..c9ce2ed 100644 --- a/packages/analyzer-engine/src/duplication.ts +++ b/packages/analyzer-engine/src/duplication.ts @@ -2,6 +2,7 @@ import { createHash } from 'crypto' import { annotatedAnalyzerOutput, incompleteAnalyzerOutput, + measuredCoverage, type Analyzer, type AnalyzerFinding, } from './types' @@ -78,7 +79,10 @@ export const duplicationAnalyzer: Analyzer = { if (candidates.length > candidateLimit) { return incompleteAnalyzerOutput(output, { truncated: true, - detail: `Duplication analysis hit a candidate bound (${candidates.length} candidate files, ${findings.length} matches).`, + // The filter ran over the whole tree before the slice, so the files the + // bound cut are counted, not merely unknown. + coverageRatio: measuredCoverage(candidateLimit, candidates.length), + detail: `Duplication analysis compared ${candidateLimit} of ${candidates.length} candidate files (${findings.length} matches).`, metrics: { candidates: candidates.length, candidateLimit, matches: findings.length, findingLimit }, }, withheld) } diff --git a/packages/analyzer-engine/src/overengineering.ts b/packages/analyzer-engine/src/overengineering.ts index 42713da..c6c8371 100644 --- a/packages/analyzer-engine/src/overengineering.ts +++ b/packages/analyzer-engine/src/overengineering.ts @@ -1,6 +1,7 @@ import { annotatedAnalyzerOutput, incompleteAnalyzerOutput, + measuredCoverage, type Analyzer, type AnalyzerFinding, type RepoIndex, @@ -313,7 +314,10 @@ export const overengineeringAnalyzer: Analyzer = { if (production.length > candidateLimit) { return incompleteAnalyzerOutput(findings, { truncated: true, - detail: `Speculative-structure analysis hit a candidate bound (${production.length} candidate files).`, + // The production-file list was built from the whole tree before the + // slice, so the files the bound cut are counted, not merely unknown. + coverageRatio: measuredCoverage(candidateLimit, production.length), + detail: `Speculative-structure analysis examined ${candidateLimit} of ${production.length} candidate files.`, metrics, }, withheld) } diff --git a/packages/analyzer-engine/src/scoring.ts b/packages/analyzer-engine/src/scoring.ts index eb4f186dd3adc644a4b81250741cd9ec1c83849a..57d2c15811d3144d7916ed31fddc5c01aca9697b 100644 GIT binary patch delta 3572 zcma)8O>Z1U5Jid)U=D~g3M!-!&xFl55fMlZc4UkLRyGNWEex_Owmnm`+jzET)IH

reLaUnKdbAq$t-W(`J~^?&j_urOH~(4(|uDa3>p(G8xq)CdaAe+EJw(zDv9T zUSw=)t%|*A#e=)VCv2zn2=Ryu$D|$Uj9V zAMiBF*J#tIV)`xH`=Ro94GCCD`Pnj2S{jElxK^}Ic_I&)!avH<6HZH;eH0e=s4>T; zoR|%Ij>n9FBWLpf9_4lXOQ@g)IA!jG-b(_kw7$MhD&c4Nd+wITxabX3-(g!Y6Q$U$nfe4=?m8K>H5jE)dM-gAb+uu|sHbB6jmpv*CId_IWssRO9rR`|Wt4!xPTPYZafI9$F!Yf*Q$o12^60S5wC{bBVHU?1(T1BBM2c%K^ zxI=lXboH8V!;npbUSVUVU9B`*6<~*2P^_%6_yOnvbBSi}^BCAEe-T`wmgSC0$Ar^wqb)j2M zg!7WBaib^??DTn{Qw{VFf2aO8oajO^o~qpe?dh5@S55#+Wk*;$;vz%R0fA)5btJpP z5k5y*u*hPz&dt{K=-;wgU8M(@aX%G@lhVqtDcX9nv$wOg*{5*>!8D@$n0cAIs!(Dr zAgH4Ld zeB$H?T~80=uFOm63wASk6O;iA1}8d5l4Mryi7Fa)QG6Jm)3+yFG3Iw9#)J;ntrwMt z+rfvRQ$mK_O}y|f1XY0l6y-3mQm3jYrh>=NmQCe(o0CCT9O4ab(|c5y$i#zAGoTFO zSf=CdwjPdZ!2y);3Bas6=~F|c#f{|6xJ-{bpF&dd%2k|(q$VKb2Y2M~3Hx{=(6{fr zD+MWf|JJSJoBhXIUSKg6rJb%g5h@L~!qpo=)zeMnpfz6JW%89ooN@J%D0VuC_EF?# zBJL^-+V0^HMV-m*Oj+`#D7-d67DtkhV6U=FR3X!2ZFKAW^#R&tAseRdkt;iRW!P(A zB4+KAn-3wx4uJ#eh*+>fVqTcKB^}@*mO-8%KHhayA^r2+#T^1-T~D|;VfqB>zVm2n z@0?NsjAOZSOo9@6*-wiS%2!L0U5t_RC!sA&p!mUD`3fHDT8|Y(%IP88*F!NzM$#!= z3*IeqLPSg?7!zPdgp8q(4tF0sbj~6J9>Rl3_c!~y+fH}?)1K{QpEqYCd#`Mydq#gSF!c#TYh%b>+A zEhL@E4_9bdPS+8cYFZ5AsHSFUWa2rd2qWIrfqu*Tu63SInC)de6^SK2!_W z@YC)7+i4CC&wEW{{65b9$#KrEF0HiZ>|`O>^R55aJdSya^xz$Gq4O8tSgv;qtGZwx zfvxKzV*RFa-arsx;U9iM97OIy3jUh!zd9&%9bs3G4suAWnAzE1S8hGPn4#-I2awZC zuHf(=-_fpzXy!?&VQ3pOn++i*boR^A)z?6hu>9=qQoo&e_wZ>rh%smTK?2`fC;23% zaM{J@V8*koqP&MmxVSPv0$R?B>&&C;iGQ8A7FX6NQn>HMlI~^aI$GEk-F)GzT6_&q jhoH!J8}b-QT7EVqa==esUEsm%9TDO<{^_@+{WtywY7(Up delta 67 zcmaFjyiQ|-JM-i&zM{#z{N|IV3m8mZ!LKseQP6qvA5Lj0E(IV+%_&Y*C{9kyNlgh) U%}mcIQP8$k07-A|6$]+>|\$\{)/i +/** + * A line that announces itself as documentation or a template. Matching here + * skips the line in silence. + * + * The word markers are TOKEN-ANCHORED. Unanchored they matched inside any + * alphanumeric run, and `xxx` is three characters: a random 2048-bit credential + * contains one about 4% of the time, so one line in twenty-five carrying a real + * key was dropped without a finding of any kind. The structural markers + * (``, `${VAR}`) are punctuation-delimited already and stay as + * they are, and `your-`/`your_` anchors on its left only because the token + * continues past it (`your_api_key`). + */ +const PLACEHOLDER = new RegExp( + [ + '<[^>]+>', + '\\$\\{', + '(?"` — so judging it let an identifier + * bless the credential sitting beside it. Judge the quoted literal when the + * pattern captured one; patterns that match a bare credential token (an AWS key + * in YAML) carry no quotes and are judged whole. */ -const FAKE_LITERAL_VALUE = - /(example|sample|dummy|fake|placeholder|changeme|your[-_]?(key|token|secret)|abc123|xxx+)/i +function placeholderSubject(credentialType: string, matched: string): string | null { + // A private key's "value" is a base64 PEM body: a high-entropy blob, not a + // string that can announce itself as fake. Excluded by construction rather + // than by trusting the anchoring above to hold for 1,700 random characters. + if (credentialType === 'Private key block') return null + return /['"`]([^'"`]*)['"`]/.exec(matched)?.[1] ?? matched +} /** SCREAMING_SNAKE values are constant identifiers, not credentials — * `UPDATE_PASSWORD = 'UPDATE_PASSWORD'` is an enum member. */ @@ -96,7 +160,34 @@ export const secretsAnalyzer: Analyzer = { description: 'Detects credentials committed to the repository (defensive; values are never reported).', async run(index) { const findings: AnalyzerFinding[] = [] - const findingLimit = 50 + // 50 was hit exactly on a 300k-LOC repository, which made every count + // anyone quoted a truncated prefix AND — because a truncated required pass + // is not authoritative — voided all five score axes over a benign cap. + // 500 is above any real repository's genuine count while still bounding a + // pathological tree. + const findingLimit = 500 + /** + * Every credential-shaped line this pass matched, including the ones the cap + * left no room to report. Counting past the cap is the whole point: 500 + * reported findings could mean 500 secrets or 5,000, and a consumer that + * cannot tell them apart has to treat both as a total loss. + * + * This is why the sweep no longer stops at the cap. Reading on costs one + * more regex pass over a tree the analyzer already sweeps whenever the cap + * is NOT hit, and it buys the difference between "we showed you 500 of 512" + * and "we showed you 500 of 9,000" — which is the difference between an + * immaterial cap and a security score nobody should publish. + */ + let matches = 0 + /** + * Building the finding is deferred because a reported secret carries a + * generated fix diff, and generating one for a finding the cap will discard + * is pure waste. + */ + const report = (build: () => AnalyzerFinding): void => { + matches++ + if (findings.length < findingLimit) findings.push(build()) + } // Where a move-to-env fix appends its variable. Undefined means the file // does not exist, which the diff renders as a new-file hunk. const envExample = index.files.find((file) => file.path === '.env.example')?.content @@ -117,15 +208,30 @@ export const secretsAnalyzer: Analyzer = { const isTestContext = TEST_PATH_RE.test(file.path) const isSeedScript = SEED_PATH_RE.test(file.path) const lines = content.split('\n') - for (let i = 0; i < lines.length && findings.length < findingLimit; i++) { + /** Indent column of the open placeholder block, or -1 when none is open. */ + let placeholderBlock = -1 + for (let i = 0; i < lines.length; i++) { const line = lines[i] - if (PLACEHOLDER.test(line)) continue + const indent = line.length - line.trimStart().length + const blank = line.trim() === '' + // A blank line ends nothing; the first non-blank line at or left of the + // opener's indent does. + if (placeholderBlock >= 0 && !blank && indent <= placeholderBlock) placeholderBlock = -1 + const declaresPlaceholder = PLACEHOLDER.test(line) + const inPlaceholderBlock = placeholderBlock >= 0 + // Only the OUTERMOST block is tracked; anything nested inside it is + // more deeply indented and therefore already covered. + if (placeholderBlock < 0 && declaresPlaceholder && PLACEHOLDER_BLOCK_OPEN.test(line)) { + placeholderBlock = indent + } + if (declaresPlaceholder || inPlaceholderBlock) continue for (const { name, re } of SECRET_PATTERNS) { const match = line.match(re) if (!match) continue if (RUNTIME_CREDENTIAL_REFERENCE.test(match[0])) continue - if (FAKE_LITERAL_VALUE.test(match[0])) { - findings.push({ + const subject = placeholderSubject(name, match[0]) + if (subject !== null && FAKE_LITERAL_VALUE.test(subject)) { + report(() => ({ category: 'SECURITY_HYGIENE', severity: 'INFO', title: `Credential-shaped placeholder ignored in ${file.path.split('/').pop()}`, @@ -136,7 +242,7 @@ export const secretsAnalyzer: Analyzer = { impactScore: 5, effort: 'low', metadata: { credentialType: name, placeholder: true }, - }) + })) break } // A credential committed to source is a single token. Internal @@ -161,7 +267,7 @@ export const secretsAnalyzer: Analyzer = { const isEnvFile = /(^|\/)\.env/.test(file.path) const baseName = file.path.split('/').pop() if (!isEnvFile && !messageString && isSeedScript && TEST_DOWNGRADEABLE.has(name)) { - findings.push({ + report(() => ({ category: 'SECURITY_HYGIENE', severity: 'MEDIUM', title: `Seed-script credential in ${baseName}`, @@ -172,11 +278,11 @@ export const secretsAnalyzer: Analyzer = { impactScore: 45, effort: 'low', metadata: { credentialType: name, seedScript: true }, - }) + })) break } if (!isEnvFile && (messageString || (isTestContext && TEST_DOWNGRADEABLE.has(name)))) { - findings.push({ + report(() => ({ category: 'SECURITY_HYGIENE', severity: 'LOW', title: messageString @@ -193,7 +299,7 @@ export const secretsAnalyzer: Analyzer = { impactScore: messageString ? 10 : 25, effort: 'low', metadata: { credentialType: name, testContext: isTestContext, messageString }, - }) + })) } else { // A concrete fix only where the evidence determines one: a tracked // .env is untracked wholesale, and a source assignment becomes an @@ -205,40 +311,46 @@ export const secretsAnalyzer: Analyzer = { // but its LINE is not the place to fix them. The next generation // overwrites any edit; the credential has to leave the generator's // input. So the leak is reported and the diff is withheld. - const fix = isEnvFile - ? untrackEnvFileFix(file.path) - : isGeneratedFile - ? undefined - : moveSecretToEnvFix({ - filePath: file.path, - line: i + 1, - lineText: line, - credentialType: name, - envExampleLines, - }) - findings.push({ - category: 'SECURITY_HYGIENE', - severity: isEnvFile ? 'CRITICAL' : 'HIGH', - title: `Possible ${name} committed in ${file.path.split('/').pop()}`, - description: `Line ${i + 1} of ${file.path} appears to contain a ${name}. Committed credentials should be treated as compromised.`, - filePath: file.path, - line: i + 1, - suggestion: 'Rotate this credential immediately, move it to environment configuration, and add the file to .gitignore. Consider a pre-commit secret scanner.', - ...(fix ? { fix } : {}), - impactScore: 95, - effort: 'low', - metadata: { credentialType: name }, + report(() => { + const fix = isEnvFile + ? untrackEnvFileFix(file.path) + : isGeneratedFile + ? undefined + : moveSecretToEnvFix({ + filePath: file.path, + line: i + 1, + lineText: line, + credentialType: name, + envExampleLines, + }) + return { + category: 'SECURITY_HYGIENE', + severity: isEnvFile ? 'CRITICAL' : 'HIGH', + title: `Possible ${name} committed in ${file.path.split('/').pop()}`, + description: `Line ${i + 1} of ${file.path} appears to contain a ${name}. Committed credentials should be treated as compromised.`, + filePath: file.path, + line: i + 1, + suggestion: 'Rotate this credential immediately, move it to environment configuration, and add the file to .gitignore. Consider a pre-commit secret scanner.', + ...(fix ? { fix } : {}), + impactScore: 95, + effort: 'low', + metadata: { credentialType: name }, + } }) } break // one finding per line } } } - return findings.length >= findingLimit + // Reaching the cap exactly is not a loss — every match was reported — so + // only an overflow truncates. What it reports is measurable: the share of + // this repository's real matches the reader can actually see. + return matches > findingLimit ? incompleteAnalyzerOutput(findings, { truncated: true, - detail: `Secret scanning stopped after ${findingLimit} matches.`, - metrics: { matches: findings.length, findingLimit }, + coverageRatio: measuredCoverage(findings.length, matches), + detail: `Secret scanning reported ${findings.length} of ${matches} credential matches; the ${findingLimit}-finding cap dropped the rest.`, + metrics: { matches, reported: findings.length, findingLimit }, }) : findings }, diff --git a/packages/analyzer-engine/src/security/engine.ts b/packages/analyzer-engine/src/security/engine.ts index 4cef718..4d3f453 100644 --- a/packages/analyzer-engine/src/security/engine.ts +++ b/packages/analyzer-engine/src/security/engine.ts @@ -560,16 +560,43 @@ const PATH_ANCHOR = /^(\/(?!\/)|[?#])/ const AUTHORITY_OPENER = /^([a-z][a-z0-9+.-]*:)?\/\//i /** - * Position-aware suppression for head-position sinks (SSRF). A URL built as a - * template/concat is only attacker-steerable when taint can reach its scheme/ - * authority: suppress when a constant head pins the authority (or is a - * single-slash relative path), or when the head expression is untainted (e.g. - * a `${BASE}` config var) and every tainted part lands after a path-anchoring - * constant fragment. Deliberately conservative — an empty, protocol-relative - * ('//') or incomplete-host ('http://', 'http://a') prefix, or a tainted head, - * still flags, and a non-template/concat argument is never suppressed. + * The relative reference of `new URL(reference, base)`, or null when the shape + * does not apply. + * + * WHATWG resolution makes the base supply the origin for every reference that + * is not itself absolute, so the origin-position question collapses onto the + * FIRST argument and the base drops out: `new URL('/booking/' + uid, WEBAPP_URL)` + * lands on WEBAPP_URL's origin no matter what `uid` holds. A single argument is + * not this shape — there is no base, so that argument is the whole URL. + * + * The base is not re-examined, matching the constant-first-argument carve-out + * in `evalOrigins`: `new URL('/login', req.url)` is the Next.js middleware + * idiom, and the same trade-off (a host injection through an attacker-chosen + * base is suppressed too) is accepted here for the same reason. + */ +function urlBaseRelativeReference(arg: SyntaxNode, lang: SastLanguage): SyntaxNode | null { + const call = asCall(arg, lang) + if (!call || !call.isConstruct || call.fullName.toLowerCase() !== 'url') return null + return call.args.length >= 2 ? call.args[0] ?? null : null +} + +/** + * Position-aware suppression for head-position sinks (SSRF, open redirect). A + * URL built as a template/concat is only attacker-steerable when taint can + * reach its scheme/authority: suppress when a constant head pins the authority + * (or is a single-slash relative path), or when the head expression is + * untainted (e.g. a `${BASE}` config var) and every tainted part lands after a + * path-anchoring constant fragment. Deliberately conservative — an empty, + * protocol-relative ('//') or incomplete-host ('http://', 'http://a') prefix, + * or a tainted head, still flags, and a non-template/concat argument is never + * suppressed. */ -function headTaintSuppressed(arg: SyntaxNode, ft: FunctionTaint, lang: SastLanguage): boolean { +function headTaintSuppressed(arg: SyntaxNode, ft: FunctionTaint, lang: SastLanguage, depth = 0): boolean { + // `new URL(ref, base)` — ask the same question of `ref`, which is what decides + // whether the base's origin survives. A reference that opens its own authority + // ('//host', 'https://host') fails the checks below exactly as it should. + const reference = depth < 4 ? urlBaseRelativeReference(arg, lang) : null + if (reference) return headTaintSuppressed(reference, ft, lang, depth + 1) const { constantPrefix, headExpr, parts } = urlHeadOf(arg, lang) if (!parts) return false // opaque expression — the whole argument is the URL if (constantPrefix !== null && (AUTHORITY_PINNED_PREFIX.test(constantPrefix) || SINGLE_SLASH_PATH.test(constantPrefix))) { diff --git a/packages/analyzer-engine/src/security/normalize.ts b/packages/analyzer-engine/src/security/normalize.ts index 0809340..191ea75 100644 --- a/packages/analyzer-engine/src/security/normalize.ts +++ b/packages/analyzer-engine/src/security/normalize.ts @@ -61,22 +61,60 @@ export function asFunction(node: SyntaxNode, lang: SastLanguage): NFunc | null { return { name, params: paramNames(node), body, node, line: node.startPosition.row + 1 } } -function paramNames(node: SyntaxNode): string[] { - // The parameter container differs per grammar; find it, then pull leaf names. +/** The parameter container for a function node; grammars name it differently. */ +function paramContainer(node: SyntaxNode): SyntaxNode | null { const containerTypes = new Set([ 'formal_parameters', 'parameters', 'parameter_list', 'method_parameters', 'formal_parameter_list', 'block_parameters', 'closure_parameters', ]) - let container: SyntaxNode | null = field(node, 'parameters') - if (!container) { - for (let i = 0; i < node.childCount; i++) { - const c = node.child(i) - if (c && containerTypes.has(c.type)) { - container = c - break - } + const container = field(node, 'parameters') + if (container) return container + for (let i = 0; i < node.childCount; i++) { + const c = node.child(i) + if (c && containerTypes.has(c.type)) return c + } + return null +} + +/** + * EVERY name a function's signature binds, including the leaves of a + * destructuring pattern (`({ ctx, input })`, `({ params }: Props)`). + * + * Distinct from {@link paramNames}, which returns POSITIONAL parameters and + * feeds the interprocedural summary — a destructured leaf has no argument + * position, so it cannot appear there. This set answers a different question: + * "did this function's signature introduce this identifier?" Destructuring is + * how request-handling code normally binds its request, so a gate that only + * saw positional names would miss the majority of real handlers. + * + * Over-approximates slightly: an identifier inside a default-value expression + * (`function f(a = fallback)`) is collected too. That only ever makes the gate + * more permissive, which is the safe direction for a precision narrowing. + */ +export function paramBoundNames(node: SyntaxNode): Set { + const names = new Set() + const add = (n: SyntaxNode | null): void => { + if (!n) return + if (n.type === 'identifier' || n.type === 'shorthand_property_identifier_pattern') { + names.add(n.text) + return + } + if (n.type === 'variable_name') { + names.add(n.text.replace(/^\$/, '')) + return } + // Type annotations contribute `type_identifier`, not `identifier`, so a + // blanket descent does not pull type names in. + for (const c of n.namedChildren) add(c) } + const container = paramContainer(node) + if (container) for (const c of container.namedChildren) add(c) + else add(field(node, 'parameter')) + return names +} + +function paramNames(node: SyntaxNode): string[] { + const container = paramContainer(node) // JS arrow with a single bare identifier param: `x => ...` if (!container) { const p = field(node, 'parameter') diff --git a/packages/analyzer-engine/src/security/taint.ts b/packages/analyzer-engine/src/security/taint.ts index 53e6d1c..fb7fcea 100644 --- a/packages/analyzer-engine/src/security/taint.ts +++ b/packages/analyzer-engine/src/security/taint.ts @@ -7,6 +7,7 @@ import { dottedName, identifierName, interpolationExprs, + paramBoundNames, stringLiteralValue, subscriptBase, unwrap, @@ -69,6 +70,47 @@ const REQUEST_ROOTS = new Set([ 'req', 'request', 'ctx', 'context', 'httprequest', 'httpcontext', 'event', 'args', '$_get', '$_post', '$_request', '$_cookie', '$_server', 'params', 'searchparams', ]) +/** + * Request roots whose NAME alone establishes nothing. + * + * `event`, `args`, `context`, `params` and `ctx` are ordinary local and loop + * names in ordinary code. `for (const event of salesforceEvents)` made every + * `event.` an untrusted source, so rows already read back from a CRM + * were treated as attacker input — three CRITICAL injection reports out of one + * loop variable. So these count as a request object only when the enclosing + * function BOUND them, positionally or by destructuring. + * + * The unambiguous roots are deliberately left ungated: nobody names a loop + * variable `req`, `httpRequest` or `$_GET`, and a framework that hands a + * request to a non-parameter binding still reaches those. + * + * Recall cost, stated rather than hidden: a handler that destructures its + * request into a LOCAL named `event` — `const event = JSON.parse(body)` — or a + * serverless adapter that binds the request through a module-level variable + * loses the source, and every finding that depended on it. That is a real + * narrowing, accepted because the name alone was never evidence. + */ +const PARAM_BOUND_REQUEST_ROOTS = new Set(['event', 'args', 'context', 'params', 'ctx']) + +/** Names the enclosing function's signature bound, for the ambiguous-root gate. */ +export interface TaintScope { + paramNames: ReadonlySet +} + +/** + * Whether `name` denotes an untrusted request object in the given scope. + * + * `scope` is undefined for callers that recognize request SHAPES without a + * function context (the navigation-target predicate in rules.ts). Those keep + * the pre-gate answer: they consult this only to decide whether an already- + * tainted value IS the request rather than something built from it, so + * narrowing them would change a measured predicate for no stated defect. + */ +function isRequestRoot(name: string, scope?: TaintScope): boolean { + if (!REQUEST_ROOTS.has(name)) return false + if (!PARAM_BOUND_REQUEST_ROOTS.has(name)) return true + return scope ? scope.paramNames.has(name) : true +} /** Member properties on a request object that expose untrusted data. */ const REQUEST_PROPS = new Set([ 'query', 'body', 'params', 'cookies', 'headers', 'form', 'get', 'post', 'data', @@ -139,7 +181,7 @@ const READER_SOURCE_METHODS = new Set([ const READER_RECEIVER = /reader|stream|buffer|^(br|rdr)$/ /** If the expression is an untrusted source, return its kind label. */ -export function sourceKindOf(node: SyntaxNode, lang: SastLanguage): string | null { +export function sourceKindOf(node: SyntaxNode, lang: SastLanguage, scope?: TaintScope): string | null { // `await params` / `await searchParams` — Next 15+ async route/page props. // Checked on the RAW node because unwrap() strips the await. Awaiting a bare // identifier with exactly these names is a strong framework signal; ordinary @@ -166,23 +208,28 @@ export function sourceKindOf(node: SyntaxNode, lang: SastLanguage): string | nul if ( (method === 'json' || method === 'text' || method === 'formdata') && call.receiverName && - REQUEST_ROOTS.has(call.receiverName.toLowerCase()) + isRequestRoot(call.receiverName.toLowerCase(), scope) ) { return `${call.receiverName}.${method}()` } // next/headers cookies() — every cookie value is attacker-controlled. + // useSearchParams()/useParams() — the client-side equivalents: both read + // straight off the URL. These are named here rather than inferred from the + // variable they land in, which is what makes the parameter-binding gate on + // `params` affordable: `const params = useSearchParams()` is a local, so + // the name proves nothing, but the CALL proves everything. if ( - method === 'cookies' && + (method === 'cookies' || method === 'usesearchparams' || method === 'useparams') && !call.receiverName && !call.isConstruct && (lang === 'javascript' || lang === 'typescript' || lang === 'tsx') ) { - return 'cookies()' + return method === 'cookies' ? 'cookies()' : `${call.method}()` } // Getter calls on a request-shaped root: searchParams.get('q'), // params.get(…). The member branch below already treats `.anything` // as a source, so this adds no exposure the member form doesn't have. - if (method === 'get' && call.receiverName && REQUEST_ROOTS.has(call.receiverName.toLowerCase())) { + if (method === 'get' && call.receiverName && isRequestRoot(call.receiverName.toLowerCase(), scope)) { return `${call.receiverName}.get` } // bare input() in python @@ -207,8 +254,8 @@ export function sourceKindOf(node: SyntaxNode, lang: SastLanguage): string | nul if (m) { const rootName = rootIdentifier(n, lang)?.toLowerCase() const prop = m.property.toLowerCase() - if (rootName && REQUEST_ROOTS.has(rootName) && REQUEST_PROPS.has(prop)) return `${rootName}.${prop}` - if (rootName && REQUEST_ROOTS.has(rootName)) return rootName // req..x + if (rootName && isRequestRoot(rootName, scope) && REQUEST_PROPS.has(prop)) return `${rootName}.${prop}` + if (rootName && isRequestRoot(rootName, scope)) return rootName // req..x // Go: r.URL / r.Header / r.Body etc. — request fields, gated on a // request-shaped root so a *sql.Rows named `r` is not caught. if (lang === 'go' && rootName && GO_REQUEST_ROOTS.has(rootName) && GO_REQUEST_MEMBERS.has(prop)) { @@ -274,6 +321,8 @@ function isSanitizer(call: NCall): boolean { interface Env { lang: SastLanguage + /** Names this function's signature bound — gates the ambiguous request roots. */ + scope: TaintScope taint: Map /** Remaining node-visit budget for the current phase (fixpoint solve, then * sink queries). Each evalOrigins visit accesses web-tree-sitter node @@ -304,7 +353,7 @@ function evalOrigins(node: SyntaxNode, env: Env, depth = 0): Origins { // 1. direct source? (raw node — sourceKindOf needs to see `await params` // before unwrap strips the await; it unwraps internally for everything else) - const src = sourceKindOf(node, lang) + const src = sourceKindOf(node, lang, env.scope) if (src) return [{ kind: 'source', sourceKind: src, node: n }] // 2. calls @@ -318,6 +367,24 @@ function evalOrigins(node: SyntaxNode, env: Env, depth = 0): Origins { // req.url))` idiom, not an open redirect or SSRF. Trade-off: the rare // `new URL('/fixed', attackerControlledBase)` host injection is suppressed too. if (call.fullName.toLowerCase() === 'url' && stringLiteralValue(call.args[0], lang) !== null) return [] + // `new URLSearchParams({ … })` is a query-string BUILDER, and its + // serialization is application/x-www-form-urlencoded: every character that + // could carry structure — '/', ':', '<', '&', '=', '?', '#' — comes back + // percent-encoded. A value put in this way can only ever emerge as an + // encoded query VALUE, so it can reach neither an origin nor a markup + // context. Gated on the object-literal argument form, which is the builder: + // `new URLSearchParams(location.search)` PARSES instead, and reading a + // value back out of it is the DOM-XSS shape, so that form keeps its taint. + // Residual unsoundness: `.get()` on a builder returns the value decoded + // again. That round trip is a no-op nobody writes. + if ( + call.isConstruct && + call.fullName.toLowerCase() === 'urlsearchparams' && + call.args[0] && + unwrap(call.args[0], lang).type === 'object' + ) { + return [] + } let acc: Origins = [] if (call.receiver) acc = mergeOrigins(acc, evalOrigins(call.receiver, env, depth + 1)) for (const a of call.args) acc = mergeOrigins(acc, evalOrigins(a, env, depth + 1)) @@ -393,7 +460,12 @@ export interface FunctionTaint { */ export function analyzeFunction(fn: NFunc, lang: SastLanguage): FunctionTaint { const taint = new Map() - const env: Env = { lang, taint, budget: TAINT_VISIT_BUDGET, exhausted: false } + // Lower-cased because every root lookup in sourceKindOf is lower-cased — + // `searchParams` must still match the `searchparams` root. + const paramNames = new Set( + [...paramBoundNames(fn.node)].map((p) => p.toLowerCase()), + ) + const env: Env = { lang, scope: { paramNames }, taint, budget: TAINT_VISIT_BUDGET, exhausted: false } // Seed parameters so we can learn which ones reach sinks. fn.params.forEach((p, i) => { diff --git a/packages/analyzer-engine/src/todos.ts b/packages/analyzer-engine/src/todos.ts index 19277ca..553f20c 100644 --- a/packages/analyzer-engine/src/todos.ts +++ b/packages/analyzer-engine/src/todos.ts @@ -1,4 +1,4 @@ -import { incompleteAnalyzerOutput, type Analyzer, type AnalyzerFinding } from './types' +import { incompleteAnalyzerOutput, measuredCoverage, type Analyzer, type AnalyzerFinding } from './types' const MARKER = /(?:\/\/|#|\/\*|