Pular para conteúdo

AUR-462 — the security passage sees Node's exec() and innerHTML shapes

Documento histórico do board. Pode descrever componentes removidos; para uso atual, consulte o guia do produto.

Command

aurumcode review --base <ref> [--fail-on <level>] [--modelo <nome>] [--seguranca]

Same command AUR-435/AUR-442/AUR-461 published. This card does not add a flag or change the output shape; it extends what --seguranca's deterministic pass matches, in internal/review/rules/security.yml.

The measured defect (2026-08-14)

Run against a JavaScript file carrying five planted defects, the pass found the secret, the second secret, and the SQL injection, and missed two: exec("ping -c 1 " + host) and innerHTML = userInput.

Cause 1: security/command-injection's pattern was (?i)\b(system|popen|exec[lv]p?e?|subprocess\.(run|call|Popen))\s*\(.*["']\s*\+. exec[lv]p?e? requires an l or v right after exec, so it matches execl, execv, execve (C, Python) and never the bare exec( of Node's child_process — the canonical Node shape. execSync and spawn(..., {shell: true}) escaped for the same reason.

Cause 2: security/xss was one of the five catalog rules with no pattern at all (a deliberate AUR-442 scope decision), so it never matched anything.

The fix

Both rules keep their id (the AUR-461 mirror in internal/prompt/rulecatalog.go compares ids in both directions against this catalog, so no id was added or renamed):

  • security/command-injection gained two alternatives, additive to the original C/Python/subprocess branch (byte-for-byte unchanged, so AC-003 regresses clean): a bare exec(/execSync( call carrying the same quote-then-+ concatenation shape as every other branch, and a spawn(...) call whose options carry shell: true (flagged on the option alone, since shell: true hands the whole argument list to a shell regardless of whether the command itself is concatenated).
  • security/xss gained a pattern: a bare, unescaped identifier written into innerHTML/outerHTML, into document.write(...), or into React's dangerouslySetInnerHTML's __html key.

AC-002: the false-positive discipline

Every exclusion below is structural (Go's RE2 has no lookahead, so none of this is "match, then check and discard" -- the excluded shapes are simply outside the language the pattern accepts):

  • exec(["ping", host]) (Node's argv form, no shell): has no quote followed by + anywhere in the call, so neither command-injection alternative matches.
  • innerHTML = "<b>Static content</b>" (a literal): the xss identifier class requires the value to start with a lowercase letter, _, or $ ([a-z_$]); a value opening with ", ', or a backtick never matches.
  • // exec is dangerous when combined with string concatenation. (a comment mentioning "exec"): has no exec(/execSync( call shape and no quote-then-+, so it matches nothing.
  • innerHTML = DOMPurify.sanitize(userInput), innerHTML = escapeHtml(x), document.write(escapeHtml(x)), dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(raw) }} (a sanitizer or escaping-helper call): see "Known limitation" below -- found by adversarial review against the first cut of this pattern, and the reason the RHS/argument/__html value is now anchored at BOTH ends, not just the start.
  • innerHTML = TRUSTED_TEMPLATE (an UPPERCASE module constant): see "Known limitation" below for the naming-convention argument.

Known limitation: the xss pattern excludes every call, not just unsafe ones

Adversarial review (2026-08-23) reproduced against the compiled binary, not a fixture: the first cut of the xss pattern anchored only the START of the RHS ([A-Za-z_$][\w$]*), with no requirement on what followed. A MatchString search does not need to consume the whole line, so it matched any call whose callee name begins with an identifier character -- including the canonical XSS mitigation itself:

document.getElementById("out").innerHTML = DOMPurify.sanitize(userInput);
document.getElementById("out").innerHTML = escapeHtml(x);

Both produced [error] Potential XSS vulnerability detected. That is a false positive that fires exactly on the developer who followed best practice -- worse than not having the rule at all, per this card's own principle that a false positive at a merge gate teaches the team to ignore the gate.

The fix, and the trade-off it accepts. The RHS (or document.write's argument, or __html's value) must now be a bare identifier or dotted member-access chain, with NO call anywhere in it, immediately followed by its statement's own closing token (;/end-of-line for the assignment forms, ) for document.write, }} for dangerouslySetInnerHTML) and nothing else. A ( anywhere in that span breaks the match structurally -- there is no other way to exclude a call without a lookahead RE2 does not have. The accepted cost: innerHTML = getUserInput() -- a bare call genuinely returning untrusted data -- is now also excluded, indistinguishable from innerHTML = DOMPurify.sanitize(userInput) by shape alone. This card takes that trade deliberately: excluding every call trades a real but narrower miss (a call result written unescaped, which a reviewer or a future card with real data-flow analysis can still catch) against a false positive on the single most common correct fix for the class of bug this rule exists to catch. Confirmed with a member-access-chain positive control (el.innerHTML = req.body.rawHtml;, no call, still found) so the fix is not simply "identifiers no longer match anything."

The SCREAMING_SNAKE_CASE decision. A bare identifier with no call, like TRUSTED_TEMPLATE, is not excluded by the no-call rule above -- it has no parentheses to begin with. The identifier class was written as [a-z_$] for its first character, not [A-Za-z_$]: an identifier that does not start with a lowercase letter, _, or $ (i.e. starts uppercase -- PascalCase or SCREAMING_SNAKE_CASE) is excluded too. The argument: in conventional JS/TS, an identifier starting uppercase names a class, a component, or a module-level constant -- not a place untrusted runtime input (a request field, a form value, a URL param) is conventionally bound, which is near-universally camelCase or lowercase-led. This is a real, measurable naming convention, not merely a guess: it is the same signal ESLint's constant-casing/naming conventions rely on. The cost is symmetric with the no-call trade-off: a project that assigns user input to an UPPERCASE-named variable (unusual, against convention, but not impossible) would be missed. Given this card's own stated priority -- a false positive at a merge gate is worse than a missed finding -- and that TRUSTED_TEMPLATE-shaped constants are a common, real pattern (compile-time-fixed HTML fragments, i18n templates, static component markup), this card accepts the miss and keeps the exclusion.

AC-003: regression

security/sql-injection, security/hardcoded-secret, and the original C/Python/subprocess branch of security/command-injection are untouched strings; the project's own committed Python fixture (tests/fixtures/review/vuln/repo.git) still reports exactly the same src/db.py:8 security/sql-injection finding it reported before this card.

Why no fixture directory

This card's paths own internal/review/rules and its own unit/integration/e2e/acceptance programs, not a fixtures directory. So no Node source is committed to the repository tree: each test program (tests/unit/AUR-462.go as a synthetic diff; tests/integration/AUR-462.go and tests/e2e/AUR-462.sh as an ephemeral git init/git commit repository; tests/acceptance/AUR-462.sh the same, per case) materializes the identical 27-line Node source at run time instead.

Sibling regressions this card's coverage change caused, and their fix

The coordinator expanded this card's paths to include the two sibling files this coverage change broke (tests/unit/AUR-442.go, tests/acceptance/AUR-442.sh, tests/unit/AUR-450.go, tests/integration/AUR-450.go, tests/e2e/AUR-450.sh, tests/acceptance/AUR-450.sh), so both are fixed here rather than merely disclosed.

AUR-442. testAUR442DeclaredOnlyRulesStayPatternless guards against a rule being declared without a real matcher. Giving security/xss a pattern does not invalidate that guard: xss moved OUT of the declaredOnly list (which still asserts, unchanged, that the remaining four rules — path-traversal, weak-crypto, insecure-random, missing-auth — stay patternless) and into a new positive assertion, right below, proving security/xss now resolves, stays category security, and DOES carry a compiled matcher — the same shape testAUR442HardcodedSecretNowCarriesAMatcher already used for hardcoded-secret. Nothing was deleted; the guard was moved to the list that now applies. bash tests/acceptance/AUR-442.sh AC-001 is green again (see report).

AUR-450. The real count, counted from internal/review/rules/*.yml after this card's change (grep -c '^\s*pattern:'), is 4 — security/command-injection, security/hardcoded-secret, security/sql-injection, security/xss — not 5: xss got a pattern on an already-declared rule, and command-injection gained alternatives on its own already-counted pattern, so the total patterned-rule count grew by exactly one, from 3 to 4, same as security/xss alone predicts. The four files that pinned the live-binary literal "applied 3 of 8 security rules (" (tests/unit/AUR-450.go, tests/integration/AUR-450.go, tests/e2e/AUR-450.sh, tests/acceptance/AUR-450.sh) now pin "applied 4 of 8 security rules (" instead — a fact update, not a loosening: each still requires the tool to declare its own coverage on stderr, print nothing without --seguranca, and stay silent on an explicit --modelo-selected coverage note before the failure. (tests/unit/AUR-455.go's similarly-worded check was confirmed NOT affected: it statically greps the unchanged text of docs/getting-started.md, never runs the live binary, so it was left alone.)

bash tests/acceptance/AUR-450.sh AC-001 still exits 1, but not for the count: it fails at explicit-modelo-must-not-print-security-section, asserting that an explicit --modelo local failure combined with --seguranca must print no security section at all. That contract was overtaken by AUR-458 (cmd/aurumcode/main.go, commit 2920985), which was already an ancestor of this card's base branch before this session's first commit — confirmed with git merge-base --is-ancestor. AUR-458 made the deterministic security pass survive an explicit, failed --modelo: findings still print, only the exit code records that quality review did not happen. That is a real, working, already-integrated behavior change from a different card, unrelated to internal/review/rules/security.yml's patterns or the coverage count — reproduced identically by running the exact two commands explicit-modelo issues against git-demo/repo.git and observing the security section print regardless of the pattern content. cmd/aurumcode is this card's read_paths, not paths: fixing AUR-450's assertion to match AUR-458's already-shipped contract is out of this card's contract, so it is reported, not patched, here.

No third sibling was found: grep -rn "3 of 8\|patternless\|declared.only" tests/ docs/ turns up only the two fixed above (plus tests/unit/AUR-455.go, confirmed unaffected, and tests/e2e/AUR-456.sh/ tests/integration/AUR-456.go, which use "patternless" only to describe the unrelated dead plural id security/hardcoded-secrets). Four documentation files (docs/getting-started.md, docs/specs/AUR-442.md, docs/specs/AUR-450.md, docs/specs/AUR-455.md) still narrate "3 of 8" / "five patternless" prose; none of them gate a test, so they are stale prose, not a broken acceptance, and are left for a documentation follow-up.

Non-goals (unchanged from the card)

  • No AST/semantic analysis: the pass stays a deterministic line matcher.
  • No loosening of existing patterns to chase coverage.
  • No promise of the remaining patternless rules (path-traversal, weak-crypto, insecure-random, missing-auth); the pass still declares how many of the 8 catalog rules it actually applies.