brave / brave/security-action

brave-execute-script: false positives on unchanged diff context lines (reviewdog filter + baseline artifact)

Open
#933 1 comment 0 reactions 2 assignees Claimed by @thypon View on GitHub
bug
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.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.