brave-execute-script: false positives on unchanged diff context lines (reviewdog filter + baseline artifact)
- Dominant language
- JavaScript
- Stars
- 22
- Forks
- 8
- Avg merge
- 2d 6h
- Merged PRs (30d)
- 40
Description
Reference: https://github.com/brave/brave-core/pull/37366#discussion_r3460966675
## Summary
The `brave-execute-script` rule (`assets/opengrep_rules/client/brave-execute-script.yaml`) posted a false positive on brave/brave-core#37366 at `youtube_script_injector_tab_helper.cc:321`. The `script_injector_remote_->RequestAsyncExecuteScript(...)` call site is **unchanged** in the diff — it appears as context lines only. Yet it was flagged.
Cc @brave/sec-team
## Root Cause: Two reinforcing factors
### 1. reviewdog `added` filter mode + `end.line` emission
In `assets/reviewdog/reviewdog.yml` line 18, opengrep output is piped through:
```
jq -r '.results[] | "\(.extra.severity[0:1]):\(.path):\(.end.line) \(.extra.message ...)"'
```
The **end line** of the AST match is emitted. For this call expression, `end.line` = 321.
In `assets/reviewdog.sh` line 24, the second reviewdog invocation posts to `-reporter=github-pr-review` with **no `-filter-mode` flag**, so reviewdog defaults to `filter-mode=added`.
reviewdog's `added` filter keeps findings whose reported line number falls within the diff's added/modified region. Line 321 sits inside a hunk that has `+` additions *above* it (function rename, new locals), so it passes the filter. The call itself is unchanged context.
Fixes to consider:
- Emit `start.line` instead of `end.line` (or both for range-based filtering)
- Post-process to intersect findings with actual `+` lines from the diff
- Switch `filter-mode` (but `diff_context` is too noisy, `file` too aggressive)
### 2. opengrep `--baseline-commit` line-keyed baseline
`reviewdog.yml` line 16:
```
--baseline-commit origin/${GITHUB_BASE_REF:-main}
```
Opengrep baseline suppresses findings matching `(path, line)` keys from the baseline scan. Because surrounding edits shifted the function down (rename + restructuring), the semantically-identical call moved from e.g. line ~313 to line 321. Mismatch key → baseline miss → emitted as "\new\". Harder to fix without content-aware baseline diffing.
## Secondary concern: Rule design gap (Brian Johnson)
The rule flags call-site presence only (pattern: `$OBJ.$FUNC(...)` with regex `^(.*ExecuteScript.*|ExecuteMethodAndReturnValue|CallFunctionEvenIfScriptDisabled|ExecuteJavaScript)$`). It does **not** flag changes to the injected script *content* (e.g. `kYoutubeFullscreen` JS constant). If a developer changes the injected JS string without touching the call site, the rule never fires. This is a coverage hole in the rule design itself, not a filtering artifact.
## Proposed Solution
- [ ] Fork and improve the rule
- Consider `focus-metavariable` / `metavariable-pattern` scoping to injected-script arguments so matches are tighter and potentially less susceptible to context-line drift
- Consider also matching references to known injected-script constants (e.g. `kYoutubeFullscreen`, other JS string constants) so content changes trigger the rule
- [ ] Consider emitting `start.line` instead of `end.line` for reviewdog compatibility (or emit both) to reduce context-line false positives
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.