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

[thypon]

Reference: brave/brave-core#37366 (comment)

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

[thypon]

Cost analysis: running opengrep/reviewdog twice + deduplicating by content

Timing from PR #37366 (run 28038508722, job security)

Step	Duration
Checkout (brave-core)	3m22s
brave/security-action/actions/main@main (opengrep + reviewdog)	2m50s
Overhead (post-run, complete)	~2s
Total wall	6m17s

Doubling strategy

Only main@main doubles — checkout reused within same job. Two scans:

• Run 1: current approach (PR head scan → reviewdog posts findings)
• Run 2: baseline scan against base ref (origin/$GITHUB_BASE_REF) → produces "before" finding set → dedupe by content-hash against run 1 → only genuinely new findings posted

	Current	Doubled
Checkout	3m22s	3m22s (reused)
main@main	2m50s	5m40s (×2)
Overhead	~2s	~2s
Total	~6m17s	~9m02s
Delta	—	+2m50s (+45%)

Implementation path

The opengrepCompare.js module in security-action already implements base-vs-head delta logic for rule-change testing. It keys findings by path:line. To make it content-aware:

• Key by path:content-hash (hash of matched source lines, or metavariable-capture content)
• Findings with same content-hash on both base and head are suppressed (moved code, not new)

This would fix both the line-shift baseline artifact AND the reviewdog context-line false positive in one pass, without needing to change end.line → start.line or reviewdog filter-mode.