Parse empty inline YAML mapping comments
ober
ff250a766b593c754bd9014765f77b9c82882e60
--- a/HANDOFF_OPUS_4_8.md +++ b/HANDOFF_OPUS_4_8.md @@ -1,25 +1,28 @@ # Opus 4.8 Handoff: jerboa-semgrep Semgrep Parity -Date: 2026-05-30 09:54 MDT +Date: 2026-05-30 10:04 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: -`3955970 Respect JavaScript branch assignment taint` +`9e696c7 Preserve conditional taint metavariables` 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 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. +Semgrep parity. Do not treat this handoff as completion. The latest checkpoint +closes the upstream Python taint parser error `source_param` by normalizing +YAML lines shaped like `patterns: # comment` before the bundled YAML reader +sees them. ## Immediate State -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 -and small test helper were added in `tests/smoke.ss`. +The worktree was clean at `9e696c7` before this checkpoint. The latest +implementation change is in +[src/semgrep/rule/parse-rule.ss](/Users/user/mine/jerboa-semgrep/src/semgrep/rule/parse-rule.ss); +[lib/semgrep/rule/parse-rule.sls](/Users/user/mine/jerboa-semgrep/lib/semgrep/rule/parse-rule.sls) +and [src/.jerbuild-hashes](/Users/user/mine/jerboa-semgrep/src/.jerbuild-hashes) +were regenerated by `make test`. Focused parser and scan smoke coverage was +added in [tests/smoke.ss](/Users/user/mine/jerboa-semgrep/tests/smoke.ss). Current headline: @@ -28,15 +31,19 @@ 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: 307 tests / 307 passed. +- Smoke suite: 309 tests / 309 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: 11 passed / 0 mismatched / 0 Jerboa errors / 0 current errors. +- Upstream `tests/tainting_rules/python` sweep: 6 passed / 6 mismatched / + 0 Jerboa errors / 0 current errors. The prior `source_param` parser error is + gone. 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. +clean; the active frontier is now semantic taint-control-flow parity in other +upstream `tests/tainting_rules` subdirectories, especially Python, Go, PHP, +Dart, and Ruby. ## Repository Context @@ -173,18 +180,115 @@ This checkpoint: - Closes upstream JS tainting-rule case `metavar_eq_conditional`. - JS tainting-rule sweep is now 11 passed / 0 mismatched. -This checkpoint modifies: +Latest checkpoint: + +- Adds a parser normalization pass for YAML lines where an inline comment is + the only mapping value after a colon, for example `patterns: # comment`. +- Keeps that normalization out of block scalar bodies so Semgrep pattern text + is not rewritten. +- Closes upstream Python tainting-rule case `source_param`. +- Moves the Python tainting-rule sweep from `5 passed / 6 mismatched / + 1 Jerboa parse error` to `6 passed / 6 mismatched / 0 Jerboa errors`. +- Adds smoke coverage: + `parse YAML empty mapping value with inline comment`. +- Adds smoke coverage: + `scan Python taint source parameter under inline-comment nested patterns`. + +Latest checkpoint modifies: ```text HANDOFF_OPUS_4_8.md -src/semgrep/scan.ss -lib/semgrep/scan.sls +src/semgrep/rule/parse-rule.ss +lib/semgrep/rule/parse-rule.sls src/.jerbuild-hashes tests/smoke.ss ``` ## Latest Code Change Details +### YAML Empty Mapping Inline Comments + +The upstream Python taint case `source_param` uses this source formula: + +```yaml +pattern-sources: + - patterns: + - pattern-inside: | + source1($SRC, ...) + ... + - patterns: # HACK to disable `pattern: $X` optim + - pattern: $SRC +``` + +The YAML is valid: the second `patterns:` key has no scalar value, and the +text after `#` is a comment. Jerboa's bundled `(std text yaml)` reader does +not implement that inline-comment behavior. A direct probe showed: + +```scheme +(yaml-load-string "patterns: # HACK\n - pattern: $SRC\n") +``` + +returning this shape: + +```scheme +((("patterns" . "# HACK")) ((("pattern" . "$SRC")))) +``` + +`parse-positive-pattern` then saw the `patterns` value as the scalar +`"# HACK"` instead of the nested list, and failed with: + +```text +Exception in parse-config-string: patterns must be a nonempty list +``` + +The patch adds `normalize-yaml-empty-value-comments` before the existing plain +scalar and sequence-indentation normalizers. It rewrites only lines where the +first non-space character after a mapping colon is `#`; for example: + +```yaml +patterns: # comment +``` + +becomes: + +```yaml +patterns: +``` + +The helper tracks simple block-scalar contexts so pattern bodies under `|` or +`>` are not altered if they contain text shaped like `foo: # bar`. This is +important because Semgrep configs frequently embed target-language code in +block scalars, and that code can contain comments or YAML-looking text that is +not YAML syntax. + +Relevant code locations in +[src/semgrep/rule/parse-rule.ss](/Users/user/mine/jerboa-semgrep/src/semgrep/rule/parse-rule.ss): + +- `block-scalar-indicator-tail?` +- `line-starts-block-scalar?` +- `empty-mapping-value-comment-index` +- `strip-empty-mapping-value-comment` +- `normalize-yaml-empty-value-comments` +- `parse-config-string`, where the new normalizer is composed before + `normalize-yaml-plain-scalar-continuations` and + `normalize-yaml-sequence-indentation`. + +Smoke coverage added: + +```scheme +(test-case "parse YAML empty mapping value with inline comment" + ...) + +(test-case "scan Python taint source parameter under inline-comment nested patterns" + ...) +``` + +Focused upstream verification: + +```text +upstream-sweep: 1 passed, 0 mismatched, 0 jerboa errors, 0 current errors, 1 compared +``` + ### Conditional Source Metavariable Environments The upstream `metavar_eq_conditional` taint case is: @@ -540,7 +644,22 @@ make test Result: ```text -307 tests, 307 passed, 0 failed +309 tests, 309 passed, 0 failed +``` + +Focused upstream Python taint `source_param`: + +```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=220 \ +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 `metavar_eq_conditional`: @@ -707,6 +826,29 @@ Result: upstream-sweep: 11 passed, 0 mismatched, 0 jerboa errors, 0 current errors, 11 compared ``` +Full Python tainting-rule subdirectory: + +```sh +SEMGREP_CURRENT=/Users/user/.local/bin/semgrep \ +UPSTREAM_RULE_DIR=/Users/user/mine/semgrep/tests/tainting_rules/python \ +LIST_MISMATCHES=1 MAX_DIFFS=240 tests/oracle/upstream-sweep.sh +``` + +Result: + +```text +upstream-sweep: 6 passed, 6 mismatched, 0 jerboa errors, 0 current errors, 12 compared +``` + +Remaining Python mismatches: + +- `break`: Jerboa overreports line 9 after a `break`. +- `raise_from`: Jerboa overreports line 5. +- `raise_no_args`: Jerboa overreports line 5. +- `simpl_django_redirect`: Jerboa overreports line 43. +- `try_finally1`: Jerboa overreports line 9. +- `try_raise`: Jerboa overreports line 5. + ## Cross-Language Tainting-Rule Frontier The upstream `tests/tainting_rules/js` directory is clean as of this checkpoint. @@ -718,16 +860,9 @@ Additional subdirectory sweeps found: - `dart`: 0 passed / 2 mismatched. - `go`: 4 passed / 5 mismatched. - `php`: 2 passed / 4 mismatched. -- `python`: 5 passed / 6 mismatched / 1 Jerboa parse error. +- `python`: 6 passed / 6 mismatched / 0 Jerboa errors. - `ruby`: 0 passed / 1 mismatched. -The Python parse error is `source_param`, where Jerboa reports: - -```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`. @@ -1199,33 +1334,41 @@ Actual improvement: ## Recommended Next Target -The next target is outside JavaScript taint. A practical parser/config target -is the Python `source_param` Jerboa error: +The next target is outside JavaScript taint and outside parser normalization. +`source_param` is now fixed. A practical next target is Python `break`: ```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 \ +CASE_REGEX='^(break)$' LIST_MISMATCHES=1 MAX_DIFFS=180 \ tests/oracle/upstream-sweep.sh ``` -Current Jerboa error: +Current diff: -```text -Exception in parse-config-string: patterns must be a nonempty list +```diff +@@ -1 +1,2 @@ + (finding "test-break" ".../break.py" 6 13 101 6 25 113 "WARNING" "Match Found!" "") ++(finding "test-break" ".../break.py" 9 13 161 9 25 173 "WARNING" "Match Found!" "") ``` -If preferring a pure taint-control-flow target instead, use Python `break`: +The likely gap is Python loop/`break` reachability in taint mode: a source or +propagated state from before or inside an abruptly exited loop is reaching a +sink Semgrep treats as unreachable or infeasible. + +After `break`, the next Python taint-control-flow cluster is probably the +raise/try family: ```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 \ +CASE_REGEX='^(raise_from|raise_no_args|try_raise|try_finally1)$' \ +LIST_MISMATCHES=1 MAX_DIFFS=220 \ tests/oracle/upstream-sweep.sh ``` -That case currently overreports one sink after a `break`, so the likely gap is -Python loop/`break` reachability in taint mode. +Those cases all overreport early sinks in exception paths, so a shared +Python control-flow refinement may close several of them. ## Commit Hygiene @@ -1235,12 +1378,15 @@ For a code checkpoint: 2. Make source edits in `src/**/*.ss` with `apply_patch`. 3. Run `make test`; let generated `lib/**/*.sls` and `.jerbuild-hashes` update. 4. Run focused upstream oracle for the touched case. -5. Run full JS taint sweep and local oracle. -6. Run broad `tests/rules` sweep for changes to pattern-inside, formula merge, +5. Run the relevant subdirectory sweep, for example Python tainting rules when + touching Python taint semantics. +6. Run full JS taint sweep and local oracle if taint reachability changes in + shared code. +7. Run broad `tests/rules` sweep for changes to pattern-inside, formula merge, or taint reachability. -7. Run `git diff --check`. -8. Update this handoff with new baseline and remaining mismatches. -9. Commit all tracked changes together. +8. Run `git diff --check`. +9. Update this handoff with new baseline and remaining mismatches. +10. Commit all tracked changes together. Do not mark the broader Semgrep parity goal complete until the project has real Semgrep-compatible behavior across parser coverage, rule parsing, search mode, --- a/lib/semgrep/rule/parse-rule.sls +++ b/lib/semgrep/rule/parse-rule.sls @@ -127,6 +127,59 @@ (let ([after (+ colon 1)]) (or (= after (string-length line)) (char-whitespace? (string-ref line after))))))) + (def (block-scalar-indicator-tail? tail) + (and (> (string-length tail) 0) + (let ([first (string-ref tail 0)]) + (or (char=? first #\|) (char=? first #\>))))) + (def (line-starts-block-scalar? line) + (let ([colon (mapping-colon-index line)]) + (and colon + (let ([tail (string-trim-local + (substring + line + (+ colon 1) + (string-length line)))]) + (block-scalar-indicator-tail? tail))))) + (def (empty-mapping-value-comment-index line) + (let ([colon (mapping-colon-index line)]) + (and colon + (let ([len (string-length line)]) + (let loop ([i (+ colon 1)]) + (cond + [(>= i len) #f] + [(char-whitespace? (string-ref line i)) (loop (+ i 1))] + [(char=? (string-ref line i) #\#) i] + [else #f])))))) + (def (strip-empty-mapping-value-comment line) + (let ([comment (empty-mapping-value-comment-index line)]) + (if comment + (substring line 0 (+ (mapping-colon-index line) 1)) + line))) + (def (normalize-yaml-empty-value-comments source) + (join-lines + (reverse + (let loop ([lines (split-lines source)] + [block-indent #f] + [acc '()]) + (cond + [(null? lines) acc] + [else + (let* ([line (car lines)] + [indent (line-indent line)] + [in-block? (and block-indent + (or (blank-line? line) + (> indent block-indent)))]) + (if in-block? + (loop (cdr lines) block-indent (cons line acc)) + (let* ([stripped (strip-empty-mapping-value-comment + line)] + [next-block-indent (and (line-starts-block-scalar? + stripped) + indent)]) + (loop + (cdr lines) + next-block-indent + (cons stripped acc)))))]))))) (def (plain-scalar-continuation? line scalar-indent) (and scalar-indent (not (blank-line? line)) @@ -926,7 +979,8 @@ (parse-config-object (yaml-load-string (normalize-yaml-sequence-indentation - (normalize-yaml-plain-scalar-continuations source))))) + (normalize-yaml-plain-scalar-continuations + (normalize-yaml-empty-value-comments source)))))) (def (parse-config-file path) (if (file-directory? path) (apply --- a/src/.jerbuild-hashes +++ b/src/.jerbuild-hashes @@ -1,13 +1,13 @@ -(("src/semgrep/output/sarif.ss" . "E935456E4B1921FB") ("src/semgrep/rule/parse-rule.ss" . "EC5BDBE8CB185021") +(("src/semgrep/output/sarif.ss" . "E935456E4B1921FB") ("src/semgrep/rule/parse-rule.ss" . "EFA6D401699CEDEF") ("src/semgrep/result.ss" . "22D23E40B49BA529") ("src/semgrep/output/json.ss" . "293881CFA2ADB7BC") ("src/semgrep/lang.ss" . "6982E07679D20836") ("src/semgrep/parse/parse-target.ss" . "E74854DDDACF6BA") ("src/semgrep/scan.ss" . "AF0C0A1AAA7621F8") - ("src/semgrep/fix.ss" . "2E5B65B1FEF3B2B1") - ("src/semgrep/output/text.ss" . "BE476CB84B807FBA") ("src/semgrep/rule.ss" . "E12C108153C181FA") ("src/semgrep/schema/lang.ss" . "CAE2CA859C9A9FD0") + ("src/semgrep/output/text.ss" . "BE476CB84B807FBA") + ("src/semgrep/fix.ss" . "2E5B65B1FEF3B2B1") ("src/semgrep/match/structural.ss" . "6FE77014EE9FDCE4") ("src/semgrep/main.ss" . "A4EC9E7F2A09D25E") ("src/semgrep/cli.ss" . "EBDC4B1DAD3F13CC")) --- a/src/semgrep/rule/parse-rule.ss +++ b/src/semgrep/rule/parse-rule.ss @@ -120,6 +120,65 @@ (or (= after (string-length line)) (char-whitespace? (string-ref line after))))))) +(def (block-scalar-indicator-tail? tail) + (and (> (string-length tail) 0) + (let ([first (string-ref tail 0)]) + (or (char=? first #\|) + (char=? first #\>))))) + +(def (line-starts-block-scalar? line) + (let ([colon (mapping-colon-index line)]) + (and colon + (let ([tail (string-trim-local + (substring line (+ colon 1) (string-length line)))]) + (block-scalar-indicator-tail? tail))))) + +(def (empty-mapping-value-comment-index line) + (let ([colon (mapping-colon-index line)]) + (and colon + (let ([len (string-length line)]) + (let loop ([i (+ colon 1)]) + (cond + [(>= i len) #f] + [(char-whitespace? (string-ref line i)) (loop (+ i 1))] + [(char=? (string-ref line i) #\#) i] + [else #f])))))) + +(def (strip-empty-mapping-value-comment line) + (let ([comment (empty-mapping-value-comment-index line)]) + (if comment + (substring line 0 + (+ (mapping-colon-index line) 1)) + line))) + +(def (normalize-yaml-empty-value-comments source) + ;; The bundled YAML reader treats `key: # comment` as a scalar value instead + ;; of an empty mapping value with an inline comment. Strip only that shape, + ;; and avoid changing Semgrep pattern text inside block scalars. + (join-lines + (reverse + (let loop ([lines (split-lines source)] + [block-indent #f] + [acc '()]) + (cond + [(null? lines) acc] + [else + (let* ([line (car lines)] + [indent (line-indent line)] + [in-block? + (and block-indent + (or (blank-line? line) + (> indent block-indent)))]) + (if in-block? + (loop (cdr lines) block-indent (cons line acc)) + (let* ([stripped (strip-empty-mapping-value-comment line)] + [next-block-indent + (and (line-starts-block-scalar? stripped) + indent)]) + (loop (cdr lines) + next-block-indent + (cons stripped acc)))))]))))) + (def (plain-scalar-continuation? line scalar-indent) (and scalar-indent (not (blank-line? line)) @@ -833,7 +892,8 @@ (parse-config-object (yaml-load-string (normalize-yaml-sequence-indentation - (normalize-yaml-plain-scalar-continuations source))))) + (normalize-yaml-plain-scalar-continuations + (normalize-yaml-empty-value-comments source)))))) (def (parse-config-file path) (if (file-directory? path) --- a/tests/smoke.ss +++ b/tests/smoke.ss @@ -156,6 +156,15 @@ (check (rule-id rule) => "demo.yaml.aligned") (check (rule-pattern-kind rule) => 'taint))) +(test-case "parse YAML empty mapping value with inline comment" + (let* ([comment-config + "rules:\n - id: demo.yaml.inline-comment\n languages: [python]\n message: inline comment\n severity: WARNING\n mode: taint\n pattern-sources:\n - patterns:\n - pattern-inside: |\n source($X, ...)\n ...\n - patterns: # comment after empty mapping value\n - pattern: $X\n pattern-sinks:\n - pattern: sink(...)\n"] + [rules (parse-config-string comment-config)] + [rule (car rules)]) + (check (length rules) => 1) + (check (rule-id rule) => "demo.yaml.inline-comment") + (check (rule-pattern-kind rule) => 'taint))) + (test-case "parse top-level aligned YAML sequence" (let* ([aligned-config "rules:\n- id: demo.yaml.top-aligned\n languages: [python]\n message: aligned top yaml\n severity: WARNING\n pattern: foo($X)\n"] @@ -4642,6 +4651,18 @@ (check (finding-start-line (car findings)) => 5) (check (finding-start-line (cadr findings)) => 9))) +(test-case "scan Python taint source parameter under inline-comment nested patterns" + (let* ([taint-config + "rules:\n - id: demo.taint.source-param\n mode: taint\n languages: [python]\n message: source param\n severity: WARNING\n pattern-sources:\n - patterns:\n - pattern-inside: |\n source1($SRC, ...)\n ...\n - patterns: # disable bare-metavariable shortcut\n - pattern: $SRC\n pattern-sinks:\n - pattern: sink1(...)\n"] + [findings + (scan-config-string + taint-config + "python" + "demo.py" + "def foo():\n source1(a, b, c)\n # ruleid: demo.taint.source-param\n sink1(a)\n # ok\n sink1(b)\n # ok\n sink1(c)\n")]) + (check (length findings) => 1) + (check (finding-start-line (car findings)) => 4))) + (test-case "scan taint propagator label does not use source call argument" (let* ([taint-config "rules:\n - id: demo.taint.propagator.label\n mode: taint\n languages: [python]\n message: labeled propagated sink\n severity: WARNING\n pattern-sources:\n - label: INPUT\n pattern: source($A)\n - pattern: any($A)\n pattern-propagators:\n - pattern: $S.add($A)\n from: $A\n to: $S\n requires: INPUT\n label: CONTAINER\n pattern-sinks:\n - requires: CONTAINER\n pattern: sink($S)\n"]