Respect taint metavariable unification
ober
355e7285f6ac56db1dd271efaede0bbeee6407e1
--- a/HANDOFF_OPUS_4_8.md +++ b/HANDOFF_OPUS_4_8.md @@ -1,15 +1,15 @@ # Opus 4.8 Handoff: jerboa-semgrep Semgrep Parity -Date: 2026-05-30 05:16 MDT +Date: 2026-05-30 05:46 MDT Workspace: `/Users/user/mine/jerboa-semgrep` Sibling upstream Semgrep checkout: `/Users/user/mine/semgrep` Packaged Semgrep oracle: `/Users/user/.local/bin/semgrep` -Base HEAD before this checkpoint: `4e0eedf Cover JavaScript deep pattern fixtures` +Base HEAD before this checkpoint: `1d6987f Cover JavaScript taint throw reachability` The user wants this project carried forward until the pure Jerboa port reaches -Semgrep parity. Do not treat this handoff as completion. This checkpoint moves -the next parity frontier from Python/JavaScript pattern fixtures into upstream -JavaScript taint rules and fixes one concrete semantic mismatch there. +Semgrep parity. Do not treat this handoff as completion. This checkpoint closes +one more upstream JavaScript taint mismatch by matching Semgrep's +`taint_unify_mvars` behavior after assignment propagation. ## Current Headline @@ -18,20 +18,32 @@ JavaScript taint rules and fixes one concrete semantic mismatch there. - Promoted JavaScript pattern oracle: 91 passed / 0 mismatched. - Promoted Python pattern oracle: 116 passed / 0 mismatched. - Local oracle: 42 passed / 0 failed. -- Smoke suite: 301 tests, 301 passed, 0 failed. +- Smoke suite: 302 tests, 302 passed, 0 failed. - Broad same-basename upstream `tests/rules` sweep: 437 passed / 0 mismatched / 0 Jerboa errors, with 3 packaged-Semgrep current errors. -- New upstream `tests/tainting_rules/js` sweep: 5 passed / 6 mismatched / - 0 Jerboa errors / 0 current errors. This improved from 4 passed / 7 - mismatched by matching Semgrep's handling of unreachable JS taint sinks after - `throw`. +- Upstream `tests/tainting_rules/js` sweep: 6 passed / 5 mismatched / + 0 Jerboa errors / 0 current errors. This improved from 5 passed / 6 + mismatched by closing `metavar_eq_simple`. Semgrep parity is not reached. Python and JavaScript pattern fixtures are clean, but taint/dataflow, parser compatibility, rule validation, CLI behavior, output, autofix, target selection, ignore handling, and broader upstream test corpora remain open. -## What Changed In This Checkpoint +## Checkpoint Chain + +- `4e0eedf Cover JavaScript deep pattern fixtures`: closed the remaining + JavaScript `.sgrep` pattern fixture mismatches. Full JS pattern sweep became + 132 passed / 0 mismatched. +- `1d6987f Cover JavaScript taint throw reachability`: started the upstream JS + taint frontier, normalized `tests/tainting_rules` check-id prefixes, and fixed + unreachable sinks after `throw`. JS tainting-rule sweep became 5 passed / + 6 mismatched. +- This checkpoint: fixes propagated-source handling for + `options: taint_unify_mvars: true`. JS tainting-rule sweep is now 6 passed / + 5 mismatched. + +## Files In This Checkpoint This checkpoint modifies: @@ -40,7 +52,6 @@ HANDOFF_OPUS_4_8.md src/semgrep/scan.ss lib/semgrep/scan.sls src/.jerbuild-hashes -tests/oracle/normalize-findings.ss tests/smoke.ss ``` @@ -48,68 +59,60 @@ tests/smoke.ss `make build`/`make test`, but they are tracked and must stay in the commit with the matching `src/semgrep/scan.ss` changes. -### Oracle Normalization - -`tests/oracle/normalize-findings.ss` now strips check-id prefixes emitted by -packaged Semgrep for rules under `tests/tainting_rules`, including language -subdirectories such as: - -```text -Users.user.mine.semgrep.tests.tainting_rules.js. -tests.tainting_rules.js. -``` +## What Changed -Without this normalization, `tests/tainting_rules/js` appeared to have extra -mismatches where the only difference was a check id like -`Users.user.mine.semgrep.tests.tainting_rules.js.taint-test` versus -`taint-test`. +### Propagated Taint Unification -### JavaScript Taint Reachability +Semgrep's `taint_unify_mvars` keeps source metavariable bindings as constraints +after taint is assigned to another variable. In the upstream case: -`src/semgrep/scan.ss` now has a generic taint reachability wrapper: +```javascript +var source1 = get(A) +var source2 = get(B) +var source3 = get(C) + +sink(A, source1) // report +sink(B, source2) // report +sink(A, source2) // do not report +sink(A, source3) // do not report +sink(B, source3) // do not report +``` -- `javascript-taint-line-opens-unreachable?` -- `javascript-taint-unreachable-finding?` -- `filter-javascript-taint-reachable-findings` -- `filter-taint-reachable-findings` +Jerboa previously overreported the last three sinks. The root cause was not +that source bindings were lost; they were preserved. The problem was that token +reachability treated every source metavariable binding as tainted data. For +example, the state for `source2 = get(B)` contained both the carrier token +`source2` and the source constraint `$X = B`; the old token check let it reach +`sink(B, source3)` just because `B` appeared in the sink's first argument. -The wrapper is applied wherever taint specs are scanned: +The scanner now distinguishes those roles for `taint_unify_mvars`: -- sources and source matches -- sinks -- explicit propagators -- implicit assignment propagators -- sanitizers through `scan-taint-specs` +- `taint-state-mvars-compatible?` checks sink metavariables against both the + current propagated source finding and the original source finding. +- `shared-finding-binding-names` identifies binding names shared by the source + and sink findings. +- `taint-unify-ignored-source-token-names` computes the shared binding names + that should be used only for unification. +- `source-token-in-sink/except?` and + `source-compatible-with-sink/except?` keep the carrier token behavior but + ignore shared unification-only binding names as tainted token carriers. -The JavaScript filter currently treats only a leading `throw ...` line as -opening an unreachable region until the next line that starts with `}`. This is -deliberately narrower than the existing constant-propagation helper. A first -attempt also treated plain `return ...` as unreachable, but that was too coarse: -it suppressed valid taint findings inside multiline return expressions and -returned callbacks. The regression was caught by upstream -`tests/rules/taint_object_destructure`. Future `return` support should be -statement-aware, not line-only. +This keeps `source2` reaching `sink(B, source2)` while preventing `$X = B` from +making unrelated sinks tainted. ### Smoke Coverage `tests/smoke.ss` adds: ```text -scan JavaScript taint ignores unreachable after throw +scan JavaScript taint unifies propagated source metavariables ``` -It checks that: - -```javascript -if (random()) { - sink(source); -} else { - throw "error"; - sink(source); -} -``` +It mirrors the upstream `metavar_eq_simple` shape and guards against treating +source metavariable constraints as carrier tokens. -reports only the reachable sink. +The prior smoke test from `1d6987f`, +`scan JavaScript taint ignores unreachable after throw`, remains in place. ## Verification Run @@ -124,15 +127,15 @@ make test Result: ```text -301 tests, 301 passed, 0 failed +302 tests, 302 passed, 0 failed ``` -Focused upstream JS taint `throw` case: +Focused upstream JS taint `metavar_eq_simple` case: ```sh SEMGREP_CURRENT=/Users/user/.local/bin/semgrep \ UPSTREAM_RULE_DIR=/Users/user/mine/semgrep/tests/tainting_rules/js \ -CASE_REGEX='^(throw)$' LIST_MISMATCHES=1 MAX_DIFFS=80 \ +CASE_REGEX='^(metavar_eq_simple)$' LIST_MISMATCHES=1 MAX_DIFFS=120 \ tests/oracle/upstream-sweep.sh ``` @@ -142,19 +145,19 @@ Result: upstream-sweep: 1 passed, 0 mismatched, 0 jerboa errors, 0 current errors, 1 compared ``` -Focused regression guard for the rejected `return` implementation: +Focused regression guard for broader taint-rule behavior: ```sh SEMGREP_CURRENT=/Users/user/.local/bin/semgrep \ UPSTREAM_RULE_DIR=/Users/user/mine/semgrep/tests/rules \ -CASE_REGEX='^(taint_object_destructure)$' LIST_MISMATCHES=1 MAX_DIFFS=80 \ -tests/oracle/upstream-sweep.sh +CASE_REGEX='^(taint_object_destructure|taint_basic)$' \ +LIST_MISMATCHES=1 MAX_DIFFS=80 tests/oracle/upstream-sweep.sh ``` Result: ```text -upstream-sweep: 1 passed, 0 mismatched, 0 jerboa errors, 0 current errors, 1 compared +upstream-sweep: 2 passed, 0 mismatched, 0 jerboa errors, 0 current errors, 2 compared ``` Full upstream JS tainting-rule subdirectory: @@ -168,7 +171,7 @@ LIST_MISMATCHES=1 MAX_DIFFS=160 tests/oracle/upstream-sweep.sh Result: ```text -upstream-sweep: 5 passed, 6 mismatched, 0 jerboa errors, 0 current errors, 11 compared +upstream-sweep: 6 passed, 5 mismatched, 0 jerboa errors, 0 current errors, 11 compared ``` Local oracle: @@ -203,8 +206,8 @@ mismatches. ## Remaining `tests/tainting_rules/js` Mismatches -The `throw` mismatch is fixed. The six remaining JS tainting-rule mismatches -are real semantic gaps: +The `throw` and `metavar_eq_simple` mismatches are fixed. The five remaining JS +tainting-rule mismatches are real semantic gaps: 1. `await`: Semgrep reports two string-concat SQL sinks under Express-style arrow callbacks; Jerboa reports zero. Likely gap: source matching with @@ -223,18 +226,12 @@ are real semantic gaps: gap: conditional-expression sources need multiple alternative taint states/metavariable environments. -4. `metavar_eq_simple`: The rule sets `options: taint_unify_mvars: true`. - Semgrep reports only matching source/sink metavariable pairs; Jerboa - overreports wrong `$X` combinations and unrelated sources. Likely gap: - source-origin metavariable compatibility is checked too late or with the - wrong finding after assignment propagation. - -5. `sanitized_by_side_effect`: Semgrep still reports `jwt.decode(token, true)` +4. `sanitized_by_side_effect`: Semgrep still reports `jwt.decode(token, true)` when `jwt.verify(token2, key)` sanitized a different token. Jerboa over-sanitizes. Likely gap: by-side-effect sanitizer identity should require compatible token/source binding, not just same function scope and ordering. -6. `simpl_nodejs_eval`: Jerboa misses the direct concat sink +5. `simpl_nodejs_eval`: Jerboa misses the direct concat sink `s.run('lol(' + req.query.userInput + ')', cb)` and has a one-column overlong range on the template-literal sink. Likely gaps: direct string concatenation taint into a focused sink expression, plus template literal @@ -242,34 +239,34 @@ are real semantic gaps: ## Recommended Next Work -Start with one of these focused targets: +The best next target is probably `await` or `sanitized_by_side_effect`. + +For `await`, start with: ```sh SEMGREP_CURRENT=/Users/user/.local/bin/semgrep \ UPSTREAM_RULE_DIR=/Users/user/mine/semgrep/tests/tainting_rules/js \ -CASE_REGEX='^(metavar_eq_simple)$' LIST_MISMATCHES=1 MAX_DIFFS=120 \ +CASE_REGEX='^(await)$' LIST_MISMATCHES=1 MAX_DIFFS=120 \ tests/oracle/upstream-sweep.sh ``` -`metavar_eq_simple` is probably the best next fix because the rule option is -already parsed and helper functions exist: - -- `taint-unify-mvars?` -- `taint-mvars-compatible?` -- `bindings-compatible?` -- `propagated-source` -- `source-state-reaches-sink-spec-shape?` +Inspect how `pattern-inside: function ... ($REQ, $RES) {...}` is matched for +arrow callbacks like `(req, res) => { ... }`. If you touch source/sink token +logic, rerun `metavar_eq_simple` because this checkpoint depends on shared +metavariables not doubling as carrier tokens. -Inspect whether propagated taint states preserve the source metavariable -bindings needed for unification after assignment. Be careful not to break the -existing smoke test `scan JavaScript taint unifies source and sink -metavariables`. +For `sanitized_by_side_effect`, start with: -Alternative focused targets: +```sh +SEMGREP_CURRENT=/Users/user/.local/bin/semgrep \ +UPSTREAM_RULE_DIR=/Users/user/mine/semgrep/tests/tainting_rules/js \ +CASE_REGEX='^(sanitized_by_side_effect)$' LIST_MISMATCHES=1 MAX_DIFFS=120 \ +tests/oracle/upstream-sweep.sh +``` -- `await`, if you want to improve function/arrow `pattern-inside` compatibility. -- `sanitized_by_side_effect`, if you want to improve sanitizer/source identity. -- `simpl_nodejs_eval`, if you want a smaller expression/range fix. +The likely fix is to make by-side-effect sanitizer blocking check the same +token/source identity, so `jwt.verify(token2, key)` does not sanitize +`jwt.decode(token, true)`. After any taint change, run at least: @@ -289,8 +286,8 @@ UPSTREAM_RULE_DIR=/Users/user/mine/semgrep/tests/rules \ LIST_MISMATCHES=1 MAX_DIFFS=40 tests/oracle/upstream-sweep.sh ``` -It is slow and quiet for long periods, but it caught the rejected `return` -reachability implementation. +It is slow and quiet for long periods, but it caught an earlier overbroad +return-reachability implementation. ## Operational Notes @@ -306,8 +303,9 @@ reachability implementation. - The upstream sweep harness compares same-basename rule/target pairs in one directory at a time. Use `UPSTREAM_RULE_DIR=.../tests/tainting_rules/js` for the active JS taint frontier. -- If you broaden check-id normalization again, prefer explicit known prefixes. - Avoid normalizing arbitrary suffixes that could hide real check-id bugs. +- `tests/oracle/normalize-findings.ss` already strips known + `tests.tainting_rules.*` check-id prefixes. If you broaden it again, prefer + explicit known prefixes over arbitrary suffix trimming. Expected post-commit state: --- a/lib/semgrep/scan.sls +++ b/lib/semgrep/scan.sls @@ -30330,12 +30330,22 @@ (string-contains-token? (finding-text sink source) (metavariable-binding-text binding))) - (def (source-token-in-sink? source sink source-text) + (def (source-token-in-sink/except? + source + sink + source-text + ignored-names) (or (finding-text-token-in? source sink source-text) (any? (lambda (entry) - (binding-token-in-finding? (cdr entry) sink source-text)) + (and (not (member (car entry) ignored-names)) + (binding-token-in-finding? + (cdr entry) + sink + source-text))) (finding-metavars source)))) + (def (source-token-in-sink? source sink source-text) + (source-token-in-sink/except? source sink source-text '())) (def (field-source-taints-base-text? source-text base-text) (let ([base-len (string-length base-text)] [source-len (string-length source-text)]) @@ -30408,10 +30418,11 @@ (let ([a-scope (finding-simple-function-scope source a)] [b-scope (finding-simple-function-scope source b)]) (or (equal? a-scope b-scope) (not a-scope)))) - (def (source-compatible-with-sink? + (def (source-compatible-with-sink/except? source-state sink - source-text) + source-text + ignored-names) (let ([source (taint-state-finding source-state)]) (and source (if (taint-state-control? source-state) @@ -30434,10 +30445,11 @@ sink source-text) (and (taint-state-token? source-state) - (source-token-in-sink? + (source-token-in-sink/except? source sink - source-text)) + source-text + ignored-names)) (field-source-taints-base-sink? source sink @@ -30445,6 +30457,15 @@ (and (null? (finding-metavars source)) (null? (finding-metavars sink)) (finding-range-contains? sink source)))))))) + (def (source-compatible-with-sink? + source-state + sink + source-text) + (source-compatible-with-sink/except? + source-state + sink + source-text + '())) (def (taint-assume-safe-booleans? rule) (rule-option-enabled? rule "taint_assume_safe_booleans")) (def (taint-assume-safe-comparisons? rule) @@ -30464,6 +30485,36 @@ (or (not (taint-unify-mvars? rule)) (and (bindings-compatible? source sink) (bindings-compatible? sink source)))) + (def (taint-state-mvars-compatible? rule source-state sink) + (or (not (taint-unify-mvars? rule)) + (let ([source (taint-state-finding source-state)] + [origin (taint-state-origin source-state)]) + (and (or (not source) + (taint-mvars-compatible? rule source sink)) + (or (not origin) + (taint-mvars-compatible? rule origin sink)))))) + (def (shared-finding-binding-names left right) + (if (and left right) + (let loop ([xs (finding-metavars left)] [acc '()]) + (cond + [(null? xs) acc] + [(assoc (caar xs) (finding-metavars right)) + (loop + (cdr xs) + (if (member (caar xs) acc) acc (cons (caar xs) acc)))] + [else (loop (cdr xs) acc)])) + '())) + (def (taint-unify-ignored-source-token-names + rule + source-state + sink) + (if (taint-unify-mvars? rule) + (let ([source (taint-state-finding source-state)] + [origin (taint-state-origin source-state)]) + (append + (shared-finding-binding-names source sink) + (shared-finding-binding-names origin sink))) + '())) (def (taint-only-propagate-through-assignments? rule) (rule-option-enabled? rule @@ -31264,7 +31315,7 @@ (and source sink (same-simple-function-scope? source-text source sink) - (taint-mvars-compatible? rule source sink) + (taint-state-mvars-compatible? rule source-state sink) (or (and non-exact (or (not (taint-state-token? source-state)) (source-state-before-sink? @@ -31323,7 +31374,14 @@ sink sanitizers source-text)) - (source-compatible-with-sink? source-state sink source-text) + (source-compatible-with-sink/except? + source-state + sink + source-text + (taint-unify-ignored-source-token-names + rule + source-state + sink)) (not (any? (lambda (sanitizer-state) (sanitizer-blocks? --- a/src/.jerbuild-hashes +++ b/src/.jerbuild-hashes @@ -3,7 +3,7 @@ ("src/semgrep/output/json.ss" . "293881CFA2ADB7BC") ("src/semgrep/lang.ss" . "6982E07679D20836") ("src/semgrep/parse/parse-target.ss" . "E74854DDDACF6BA") - ("src/semgrep/scan.ss" . "564654C107FA90C0") + ("src/semgrep/scan.ss" . "C428896282E9AB09") ("src/semgrep/fix.ss" . "2E5B65B1FEF3B2B1") ("src/semgrep/output/text.ss" . "BE476CB84B807FBA") ("src/semgrep/rule.ss" . "E12C108153C181FA") --- a/src/semgrep/scan.ss +++ b/src/semgrep/scan.ss @@ -30322,12 +30322,18 @@ (finding-text sink source) (metavariable-binding-text binding))) -(def (source-token-in-sink? source sink source-text) +(def (source-token-in-sink/except? source sink source-text ignored-names) (or (finding-text-token-in? source sink source-text) (any? (lambda (entry) - (binding-token-in-finding? (cdr entry) sink source-text)) + (and (not (member (car entry) ignored-names)) + (binding-token-in-finding? (cdr entry) + sink + source-text))) (finding-metavars source)))) +(def (source-token-in-sink? source sink source-text) + (source-token-in-sink/except? source sink source-text '())) + (def (field-source-taints-base-text? source-text base-text) (let ([base-len (string-length base-text)] [source-len (string-length source-text)]) @@ -30405,7 +30411,7 @@ (or (equal? a-scope b-scope) (not a-scope)))) -(def (source-compatible-with-sink? source-state sink source-text) +(def (source-compatible-with-sink/except? source-state sink source-text ignored-names) (let ([source (taint-state-finding source-state)]) (and source (if (taint-state-control? source-state) @@ -30422,7 +30428,11 @@ sink source-text) (and (taint-state-token? source-state) - (source-token-in-sink? source sink source-text)) + (source-token-in-sink/except? + source + sink + source-text + ignored-names)) (field-source-taints-base-sink? source sink @@ -30431,6 +30441,9 @@ (null? (finding-metavars sink)) (finding-range-contains? sink source)))))))) +(def (source-compatible-with-sink? source-state sink source-text) + (source-compatible-with-sink/except? source-state sink source-text '())) + (def (taint-assume-safe-booleans? rule) (rule-option-enabled? rule "taint_assume_safe_booleans")) @@ -30455,6 +30468,36 @@ (and (bindings-compatible? source sink) (bindings-compatible? sink source)))) +(def (taint-state-mvars-compatible? rule source-state sink) + (or (not (taint-unify-mvars? rule)) + (let ([source (taint-state-finding source-state)] + [origin (taint-state-origin source-state)]) + (and (or (not source) + (taint-mvars-compatible? rule source sink)) + (or (not origin) + (taint-mvars-compatible? rule origin sink)))))) + +(def (shared-finding-binding-names left right) + (if (and left right) + (let loop ([xs (finding-metavars left)] [acc '()]) + (cond + [(null? xs) acc] + [(assoc (caar xs) (finding-metavars right)) + (loop (cdr xs) + (if (member (caar xs) acc) + acc + (cons (caar xs) acc)))] + [else (loop (cdr xs) acc)])) + '())) + +(def (taint-unify-ignored-source-token-names rule source-state sink) + (if (taint-unify-mvars? rule) + (let ([source (taint-state-finding source-state)] + [origin (taint-state-origin source-state)]) + (append (shared-finding-binding-names source sink) + (shared-finding-binding-names origin sink))) + '())) + (def (taint-only-propagate-through-assignments? rule) (rule-option-enabled? rule "taint_only_propagate_through_assignments")) @@ -31184,7 +31227,7 @@ (and source sink (same-simple-function-scope? source-text source sink) - (taint-mvars-compatible? rule source sink) + (taint-state-mvars-compatible? rule source-state sink) (or (and non-exact (or (not (taint-state-token? source-state)) (source-state-before-sink? source-state source sink) @@ -31229,7 +31272,14 @@ sink sanitizers source-text)) - (source-compatible-with-sink? source-state sink source-text) + (source-compatible-with-sink/except? + source-state + sink + source-text + (taint-unify-ignored-source-token-names + rule + source-state + sink)) (not (any? (lambda (sanitizer-state) (sanitizer-blocks? --- a/tests/smoke.ss +++ b/tests/smoke.ss @@ -4455,6 +4455,18 @@ (check (finding-start-line (car findings)) => 3) (check (finding-start-line (cadr findings)) => 9))) +(test-case "scan JavaScript taint unifies propagated source metavariables" + (let* ([taint-config + "rules:\n - id: demo.taint.unify-propagated-mvars\n mode: taint\n languages: [javascript]\n message: matched $X\n severity: WARNING\n options:\n taint_unify_mvars: true\n pattern-sources:\n - pattern: get($X)\n pattern-sinks:\n - pattern: sink($X,...)\n"] + [findings + (scan-config-string + taint-config + "javascript" + "demo.js" + "var source1 = get(A)\nvar source2 = get(B)\nvar source3 = get(C)\nsink(A,source1)\nsink(B,source2)\nsink(A,source2)\nsink(A,source3)\nsink(B,source3)\n")]) + (check (length findings) => 2) + (check (map finding-start-line findings) => '(4 5)))) + (test-case "scan taint labels keep sibling if else assignments separate" (let* ([taint-config "rules:\n - id: demo.taint.labels.branches\n mode: taint\n languages: [python]\n message: labeled branch sink\n severity: WARNING\n pattern-sources:\n - label: TAINTED\n pattern: source(...)\n - label: CLEANED\n pattern: sanitize(...)\n pattern-sinks:\n - requires: TAINTED and not CLEANED\n pattern: sink(...)\n"]