Preserve conditional taint metavariables
ober
9e696c729da7974000ed4afb9ce28e72c70ef802
--- a/HANDOFF_OPUS_4_8.md +++ b/HANDOFF_OPUS_4_8.md @@ -1,25 +1,25 @@ # Opus 4.8 Handoff: jerboa-semgrep Semgrep Parity -Date: 2026-05-30 08:39 MDT +Date: 2026-05-30 09:54 MDT Workspace: `/Users/user/mine/jerboa-semgrep` Sibling upstream Semgrep checkout: `/Users/user/mine/semgrep` Packaged Semgrep oracle: `/Users/user/.local/bin/semgrep` Branch: `main` Base HEAD before this checkpoint: -`3213521 Trim JavaScript template metavariable ranges` +`3955970 Respect JavaScript branch assignment taint` The user wants the pure Jerboa Semgrep port carried forward until it reaches Semgrep parity. Do not treat this handoff as completion. This checkpoint closes -the upstream JavaScript taint `eslint_obj_inj` mismatch by preventing a clean -assignment in a mutually exclusive JavaScript `if`/`else` branch from killing a -tainted assignment made in its sibling branch. +the last known upstream JavaScript taint mismatch, `metavar_eq_conditional`, by +preserving alternative source metavariable environments through assignment +propagation and rendering taint sink messages from the reaching source state. ## Immediate State -The worktree was clean at `3213521` before this checkpoint. The implementation +The worktree was clean at `3955970` before this checkpoint. The implementation change is in `src/semgrep/scan.ss`; `lib/semgrep/scan.sls` and `src/.jerbuild-hashes` were regenerated by `make test`. A focused smoke test -was added in `tests/smoke.ss`. +and small test helper were added in `tests/smoke.ss`. Current headline: @@ -28,14 +28,15 @@ Current headline: - Promoted JavaScript pattern oracle: 91 passed / 0 mismatched. - Promoted Python pattern oracle: 116 passed / 0 mismatched. - Local oracle: 42 passed / 0 failed. -- Smoke suite: 306 tests / 306 passed. +- Smoke suite: 307 tests / 307 passed. - Broad same-basename upstream `tests/rules` sweep: 437 passed / 0 mismatched / 0 Jerboa errors, with 3 packaged-Semgrep current errors. -- Upstream `tests/tainting_rules/js` sweep: 10 passed / 1 mismatched / +- Upstream `tests/tainting_rules/js` sweep: 11 passed / 0 mismatched / 0 Jerboa errors / 0 current errors. -Semgrep parity is not reached. The cleanest active frontier is JavaScript taint -semantics under upstream `tests/tainting_rules/js`. +Semgrep parity is not reached. The JavaScript tainting-rule subdirectory is +clean; the active frontier has moved to other upstream `tests/tainting_rules` +subdirectories, especially Python, Go, PHP, Dart, and Ruby. ## Repository Context @@ -157,6 +158,21 @@ This checkpoint: - Closes upstream JS tainting-rule case `eslint_obj_inj`. - JS tainting-rule sweep is now 10 passed / 1 mismatched. +This checkpoint: + +- Makes taint-state deduplication distinguish states by metavariable binding + values, so `source` can retain separate `$X = A` and `$X = B` alternatives. +- Renders ordinary taint sink findings once per reaching source state, with the + reaching source bindings taking precedence over same-named sink bindings for + message rendering. +- Preserves the sink finding's existing extra fields while replacing metavars, + so taint sinks do not accidentally gain source-rendered `fix` or `fix-regex` + output. +- Adds smoke coverage: + `scan JavaScript taint preserves conditional source metavariables`. +- Closes upstream JS tainting-rule case `metavar_eq_conditional`. +- JS tainting-rule sweep is now 11 passed / 0 mismatched. + This checkpoint modifies: ```text @@ -169,6 +185,70 @@ tests/smoke.ss ## Latest Code Change Details +### Conditional Source Metavariable Environments + +The upstream `metavar_eq_conditional` taint case is: + +```javascript +var source = cond ? get(A) : get(B) + +sink(A, source) +sink(B, source) +``` + +with: + +```yaml +pattern-sources: + - pattern: get($X) +pattern-sinks: + - pattern: sink($X,...) +message: Matched on $X! +``` + +Semgrep reports both `Matched on A!` and `Matched on B!` at both sink calls, +because `source` may carry either source metavariable environment. Jerboa +previously collapsed the two propagated states for token `source` as duplicate +taint states, and ordinary taint sink output always returned the sink finding's +own metavariables. + +This checkpoint changes two pieces: + +- `taint-state-already-present?` now compares metavariable binding values on + the taint state's finding. Same carrier range plus different bindings is no + longer treated as a duplicate state. +- `sink-output-findings` now renders non-`taint_focus_on: source` sink output + per reaching source state, with source bindings taking precedence over + same-named sink bindings. This lets line 9 render both `A` and `B` messages. + +Important implementation details: + +- `finding-with-message-and-extra` creates a finding with a re-rendered message + while preserving the existing sink extra fields. +- `extra-with-metavars` replaces only the displayed metavars. It intentionally + does not call `finding-extra-for-match`, because that would generate + source-rendered `fix`/`fix-regex` output on taint sinks. The broad + `taint_param_default` case caught this during validation. +- `binding-list-with-source-precedence` keeps sink-only bindings available but + lets source bindings win when a name appears in both places. + +Relevant code locations in [src/semgrep/scan.ss](/Users/user/mine/jerboa-semgrep/src/semgrep/scan.ss): + +- `finding-with-message-and-extra`: near the finding helpers around line 530. +- `binding-list-same-values?` and `finding-bindings-same-values?`: near + `taint-state-already-present?`. +- `taint-output-finding-for-source`: near `sink-output-findings`. + +Smoke coverage added: + +```scheme +(test-case "scan JavaScript taint preserves conditional source metavariables" + ...) +``` + +The smoke asserts four findings: `matched A` and `matched B` at both +`sink(A, source)` and `sink(B, source)`. + ### JavaScript Mutually Exclusive Branch Assignment Kills The upstream `eslint_obj_inj` rule treats function parameters and calls as @@ -460,7 +540,37 @@ make test Result: ```text -306 tests, 306 passed, 0 failed +307 tests, 307 passed, 0 failed +``` + +Focused upstream JS taint `metavar_eq_conditional`: + +```sh +SEMGREP_CURRENT=/Users/user/.local/bin/semgrep \ +UPSTREAM_RULE_DIR=/Users/user/mine/semgrep/tests/tainting_rules/js \ +CASE_REGEX='^(metavar_eq_conditional)$' LIST_MISMATCHES=1 MAX_DIFFS=180 \ +tests/oracle/upstream-sweep.sh +``` + +Result: + +```text +upstream-sweep: 1 passed, 0 mismatched, 0 jerboa errors, 0 current errors, 1 compared +``` + +Focused regression guard for taint fix rendering: + +```sh +SEMGREP_CURRENT=/Users/user/.local/bin/semgrep \ +UPSTREAM_RULE_DIR=/Users/user/mine/semgrep/tests/rules \ +CASE_REGEX='^(taint_param_default)$' LIST_MISMATCHES=1 MAX_DIFFS=180 \ +tests/oracle/upstream-sweep.sh +``` + +Result: + +```text +upstream-sweep: 1 passed, 0 mismatched, 0 jerboa errors, 0 current errors, 1 compared ``` Focused upstream JS taint `eslint_obj_inj`: @@ -594,31 +704,33 @@ LIST_MISMATCHES=1 MAX_DIFFS=240 tests/oracle/upstream-sweep.sh Result: ```text -upstream-sweep: 10 passed, 1 mismatched, 0 jerboa errors, 0 current errors, 11 compared +upstream-sweep: 11 passed, 0 mismatched, 0 jerboa errors, 0 current errors, 11 compared ``` -## Remaining JS Taint Mismatch +## Cross-Language Tainting-Rule Frontier -The 1 remaining mismatch under `/Users/user/mine/semgrep/tests/tainting_rules/js` -is: +The upstream `tests/tainting_rules/js` directory is clean as of this checkpoint. +Additional subdirectory sweeps found: -1. `metavar_eq_conditional` +- `java`: 2 passed / 0 mismatched / 0 Jerboa errors. +- `scala`: 1 passed / 0 mismatched / 0 Jerboa errors. +- `ts`: 1 passed / 0 mismatched / 0 Jerboa errors. +- `dart`: 0 passed / 2 mismatched. +- `go`: 4 passed / 5 mismatched. +- `php`: 2 passed / 4 mismatched. +- `python`: 5 passed / 6 mismatched / 1 Jerboa parse error. +- `ruby`: 0 passed / 1 mismatched. -Semgrep keeps both possible taint bindings from `cond ? get(A) : get(B)` and -reports both `A` and `B` messages at both sinks. Jerboa keeps only the locally -matching binding at each sink. Likely gap: conditional-expression sources need -multiple alternative taint states/metavariable environments. +The Python parse error is `source_param`, where Jerboa reports: -Current diff shape: - -```diff -@@ -1,4 +1,2 @@ --(finding "test-metavar-eq" ".../metavar_eq_conditional.js" 12 1 209 12 15 223 "WARNING" "Matched on A!" "") - (finding "test-metavar-eq" ".../metavar_eq_conditional.js" 12 1 209 12 15 223 "WARNING" "Matched on B!" "") - (finding "test-metavar-eq" ".../metavar_eq_conditional.js" 9 1 167 9 15 181 "WARNING" "Matched on A!" "") --(finding "test-metavar-eq" ".../metavar_eq_conditional.js" 9 1 167 9 15 181 "WARNING" "Matched on B!" "") +```text +Exception in parse-config-string: patterns must be a nonempty list ``` +The cleanest next parser/config target is probably `python/source_param`. +The cleanest next taint-control-flow target is probably `python/break`, +because it is a single overreported sink after a `break`. + ## Completed Target: `sanitized_by_side_effect` Verification command: @@ -1026,10 +1138,9 @@ Actual improvement: - Broad `tests/rules`: still 0 mismatches and 0 Jerboa errors, with the same 3 current-side packaged-Semgrep errors. -## Recommended Next Target +## Completed Target: `metavar_eq_conditional` -The next target is `metavar_eq_conditional`, the last currently known mismatch -in upstream `tests/tainting_rules/js`: +Verification command: ```sh SEMGREP_CURRENT=/Users/user/.local/bin/semgrep \ @@ -1038,27 +1149,83 @@ CASE_REGEX='^(metavar_eq_conditional)$' LIST_MISMATCHES=1 MAX_DIFFS=180 \ tests/oracle/upstream-sweep.sh ``` -Semgrep reports both `Matched on A!` and `Matched on B!` at both sink calls -after: +Result: -```javascript -var source = cond ? get(A) : get(B) +```text +upstream-sweep: 1 passed, 0 mismatched, 0 jerboa errors, 0 current errors, 1 compared +``` + +Old diff before this checkpoint: + +```diff +@@ -1,4 +1,2 @@ +-(finding "test-metavar-eq" ".../metavar_eq_conditional.js" 12 1 209 12 15 223 "WARNING" "Matched on A!" "") + (finding "test-metavar-eq" ".../metavar_eq_conditional.js" 12 1 209 12 15 223 "WARNING" "Matched on B!" "") + (finding "test-metavar-eq" ".../metavar_eq_conditional.js" 9 1 167 9 15 181 "WARNING" "Matched on A!" "") +-(finding "test-metavar-eq" ".../metavar_eq_conditional.js" 9 1 167 9 15 181 "WARNING" "Matched on B!" "") +``` + +Validation run for this patch: + +```sh +make test +SEMGREP_CURRENT=/Users/user/.local/bin/semgrep \ +UPSTREAM_RULE_DIR=/Users/user/mine/semgrep/tests/tainting_rules/js \ +CASE_REGEX='^(metavar_eq_conditional)$' LIST_MISMATCHES=1 MAX_DIFFS=180 \ +tests/oracle/upstream-sweep.sh +SEMGREP_CURRENT=/Users/user/.local/bin/semgrep \ +UPSTREAM_RULE_DIR=/Users/user/mine/semgrep/tests/tainting_rules/js \ +LIST_MISMATCHES=1 MAX_DIFFS=240 tests/oracle/upstream-sweep.sh +SEMGREP_CURRENT=/Users/user/.local/bin/semgrep \ +UPSTREAM_RULE_DIR=/Users/user/mine/semgrep/tests/rules \ +CASE_REGEX='^(taint_param_default)$' LIST_MISMATCHES=1 MAX_DIFFS=180 \ +tests/oracle/upstream-sweep.sh +SEMGREP_CURRENT=/Users/user/.local/bin/semgrep make oracle +SEMGREP_CURRENT=/Users/user/.local/bin/semgrep \ +UPSTREAM_RULE_DIR=/Users/user/mine/semgrep/tests/rules \ +LIST_MISMATCHES=1 MAX_DIFFS=160 tests/oracle/upstream-sweep.sh +git diff --check +``` + +Actual improvement: + +- Focused `metavar_eq_conditional`: 1 passed / 0 mismatched. +- Full JS tainting rules: 11 passed / 0 mismatched. +- Focused `taint_param_default` regression guard: 1 passed / 0 mismatched. +- Smoke: one more test, all passing. +- Local oracle: 42 passed / 0 failed. +- Broad `tests/rules`: still 0 mismatches and 0 Jerboa errors, with the same + 3 current-side packaged-Semgrep errors. + +## Recommended Next Target + +The next target is outside JavaScript taint. A practical parser/config target +is the Python `source_param` Jerboa error: + +```sh +SEMGREP_CURRENT=/Users/user/.local/bin/semgrep \ +UPSTREAM_RULE_DIR=/Users/user/mine/semgrep/tests/tainting_rules/python \ +CASE_REGEX='^(source_param)$' LIST_MISMATCHES=1 MAX_DIFFS=180 \ +tests/oracle/upstream-sweep.sh ``` -Jerboa currently reports only the locally matching message for each sink. The -likely fix is to represent conditional-expression sources as multiple -alternative taint states with distinct metavariable environments, then preserve -both through assignment propagation to `source`. Do not solve this by loosening -`taint_unify_mvars` compatibility globally; that would risk regressing -`metavar_eq_simple`, which is already fixed. +Current Jerboa error: -If touching conditional-source logic, keep `metavar_eq_simple` and -`metavar_eq_conditional` separate in your head: +```text +Exception in parse-config-string: patterns must be a nonempty list +``` + +If preferring a pure taint-control-flow target instead, use Python `break`: + +```sh +SEMGREP_CURRENT=/Users/user/.local/bin/semgrep \ +UPSTREAM_RULE_DIR=/Users/user/mine/semgrep/tests/tainting_rules/python \ +CASE_REGEX='^(break)$' LIST_MISMATCHES=1 MAX_DIFFS=180 \ +tests/oracle/upstream-sweep.sh +``` -- `metavar_eq_simple` is now fixed by not treating source unification bindings - as tainted carrier tokens. -- `metavar_eq_conditional` still needs multiple alternative source binding - states from a conditional expression. +That case currently overreports one sink after a `break`, so the likely gap is +Python loop/`break` reachability in taint mode. ## Commit Hygiene --- a/lib/semgrep/scan.sls +++ b/lib/semgrep/scan.sls @@ -20869,6 +20869,12 @@ (finding-end-line finding) (finding-end-col finding) (finding-start-offset finding) (finding-end-offset finding) (finding-message finding) (finding-severity finding) extra)) + (def (finding-with-message-and-extra finding message extra) + (make-finding (finding-rule-id finding) (finding-path finding) + (finding-start-line finding) (finding-start-col finding) + (finding-end-line finding) (finding-end-col finding) + (finding-start-offset finding) (finding-end-offset finding) + message (finding-severity finding) extra)) (def (finding-with-range finding source start end) (let-values ([(start-line start-col) (offset->line-col source start)] @@ -31942,6 +31948,36 @@ (requires-satisfied? labels requires)))) (def (sink-finding sink-spec) (alist-ref/default sink-spec 'finding #f)) + (def (binding-list-with-source-precedence + sink-bindings + source-bindings) + (append + source-bindings + (sg-filter + (lambda (entry) (not (assoc (car entry) source-bindings))) + sink-bindings))) + (def (extra-with-metavars extra bindings) + (let* ([visible-bindings (public-bindings bindings)] + [extra-without-metavars (remove-extra-key 'metavars extra)]) + (if (null? visible-bindings) + extra-without-metavars + (cons + (cons 'metavars visible-bindings) + extra-without-metavars)))) + (def (taint-output-finding-for-source + rule + sink + source-state + source-text) + (let* ([source-finding (taint-state-finding source-state)] + [bindings (binding-list-with-source-precedence + (finding-metavars sink) + (if source-finding + (finding-metavars source-finding) + '()))] + [message (render-fix-template (rule-message rule) bindings)] + [extra (extra-with-metavars (finding-extra sink) bindings)]) + (finding-with-message-and-extra sink message extra))) (def (sink-output-findings rule sink-spec sources sanitizers assignment-kills source-text) (let* ([requires (alist-ref/default sink-spec 'requires #f)] @@ -31954,7 +31990,13 @@ (sg-filter (lambda (finding) finding) (map taint-state-origin reaching)) - (list (sink-finding sink-spec))) + (map (lambda (source-state) + (taint-output-finding-for-source + rule + (sink-finding sink-spec) + source-state + source-text)) + reaching)) '()))) (def (finding-text finding source) (source-slice @@ -32116,12 +32158,42 @@ (or (null? xs) (and (member (car xs) b) (loop (cdr xs))))) (let loop ([xs b]) (or (null? xs) (and (member (car xs) a) (loop (cdr xs))))))) + (def (metavariable-binding-same-value? left right) + (and left + right + (string=? + (metavariable-binding-text left) + (metavariable-binding-text right)))) + (def (binding-list-same-values? left right) + (and (let loop ([xs left]) + (or (null? xs) + (let ([other (assoc (caar xs) right)]) + (and other + (metavariable-binding-same-value? + (cdar xs) + (cdr other)) + (loop (cdr xs)))))) + (let loop ([xs right]) + (or (null? xs) + (let ([other (assoc (caar xs) left)]) + (and other + (metavariable-binding-same-value? + (cdar xs) + (cdr other)) + (loop (cdr xs)))))))) + (def (finding-bindings-same-values? left right) + (binding-list-same-values? + (finding-metavars left) + (finding-metavars right))) (def (taint-state-already-present? state states) (any? (lambda (existing) (and (finding-same-identity? (taint-state-finding state) (taint-state-finding existing)) + (finding-bindings-same-values? + (taint-state-finding state) + (taint-state-finding existing)) (eq? (taint-state-exact? state) (taint-state-exact? existing)) (eq? (taint-state-token? state) --- 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" . "C3882074EBC22BFC") + ("src/semgrep/scan.ss" . "AF0C0A1AAA7621F8") ("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 @@ -20914,6 +20914,20 @@ (finding-severity finding) extra)) +(def (finding-with-message-and-extra finding message extra) + (make-finding + (finding-rule-id finding) + (finding-path finding) + (finding-start-line finding) + (finding-start-col finding) + (finding-end-line finding) + (finding-end-col finding) + (finding-start-offset finding) + (finding-end-offset finding) + message + (finding-severity finding) + extra)) + (def (finding-with-range finding source start end) (let-values ([(start-line start-col) (offset->line-col source start)] [(end-line end-col) (offset->line-col source end)]) @@ -31891,6 +31905,32 @@ (def (sink-finding sink-spec) (alist-ref/default sink-spec 'finding #f)) +(def (binding-list-with-source-precedence sink-bindings source-bindings) + (append source-bindings + (sg-filter + (lambda (entry) + (not (assoc (car entry) source-bindings))) + sink-bindings))) + +(def (extra-with-metavars extra bindings) + (let* ([visible-bindings (public-bindings bindings)] + [extra-without-metavars (remove-extra-key 'metavars extra)]) + (if (null? visible-bindings) + extra-without-metavars + (cons (cons 'metavars visible-bindings) + extra-without-metavars)))) + +(def (taint-output-finding-for-source rule sink source-state source-text) + (let* ([source-finding (taint-state-finding source-state)] + [bindings (binding-list-with-source-precedence + (finding-metavars sink) + (if source-finding + (finding-metavars source-finding) + '()))] + [message (render-fix-template (rule-message rule) bindings)] + [extra (extra-with-metavars (finding-extra sink) bindings)]) + (finding-with-message-and-extra sink message extra))) + (def (sink-output-findings rule sink-spec @@ -31914,7 +31954,13 @@ (sg-filter (lambda (finding) finding) (map taint-state-origin reaching)) - (list (sink-finding sink-spec))) + (map (lambda (source-state) + (taint-output-finding-for-source + rule + (sink-finding sink-spec) + source-state + source-text)) + reaching)) '()))) (def (finding-text finding source) @@ -32091,11 +32137,40 @@ (and (member (car xs) a) (loop (cdr xs))))))) +(def (metavariable-binding-same-value? left right) + (and left + right + (string=? (metavariable-binding-text left) + (metavariable-binding-text right)))) + +(def (binding-list-same-values? left right) + (and (let loop ([xs left]) + (or (null? xs) + (let ([other (assoc (caar xs) right)]) + (and other + (metavariable-binding-same-value? (cdar xs) + (cdr other)) + (loop (cdr xs)))))) + (let loop ([xs right]) + (or (null? xs) + (let ([other (assoc (caar xs) left)]) + (and other + (metavariable-binding-same-value? (cdar xs) + (cdr other)) + (loop (cdr xs)))))))) + +(def (finding-bindings-same-values? left right) + (binding-list-same-values? (finding-metavars left) + (finding-metavars right))) + (def (taint-state-already-present? state states) (any? (lambda (existing) (and (finding-same-identity? (taint-state-finding state) (taint-state-finding existing)) + (finding-bindings-same-values? + (taint-state-finding state) + (taint-state-finding existing)) (eq? (taint-state-exact? state) (taint-state-exact? existing)) (eq? (taint-state-token? state) --- a/tests/smoke.ss +++ b/tests/smoke.ss @@ -41,6 +41,13 @@ (format "~a: predicate failed for ~s" current-test got))))])) +(define (count-findings pred findings) + (let loop ([xs findings] [count 0]) + (cond + [(null? xs) count] + [(pred (car xs)) (loop (cdr xs) (+ count 1))] + [else (loop (cdr xs) count)]))) + (define config "rules:\n - id: demo.eval\n languages: [python]\n message: avoid eval\n severity: WARNING\n pattern-regex: \"eval\\\\s*\\\\(\"\n") @@ -4528,6 +4535,41 @@ (check (length findings) => 2) (check (map finding-start-line findings) => '(4 5)))) +(test-case "scan JavaScript taint preserves conditional source metavariables" + (let* ([taint-config + "rules:\n - id: demo.taint.conditional-source-mvars\n mode: taint\n languages: [javascript]\n message: matched $X\n severity: WARNING\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 source = cond ? get(A) : get(B)\nsink(A,source)\nsink(B,source)\n")]) + (check (length findings) => 4) + (check (count-findings + (lambda (finding) + (and (= (finding-start-line finding) 2) + (string=? (finding-message finding) "matched A"))) + findings) + => 1) + (check (count-findings + (lambda (finding) + (and (= (finding-start-line finding) 2) + (string=? (finding-message finding) "matched B"))) + findings) + => 1) + (check (count-findings + (lambda (finding) + (and (= (finding-start-line finding) 3) + (string=? (finding-message finding) "matched A"))) + findings) + => 1) + (check (count-findings + (lambda (finding) + (and (= (finding-start-line finding) 3) + (string=? (finding-message finding) "matched B"))) + findings) + => 1))) + (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"]