ci(security): add advanced CodeQL setup with path-scoped query filters#860
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
miguel-heygen
left a comment
There was a problem hiding this comment.
Clean, well-scoped security policy change. Approved.
Verified:
actions/checkoutSHA (34e114876b…) matches all 9 existing workflows in the repo- All 4 excluded source files exist on main:
capture/index.ts,whisper/normalize.ts,lint/utils.ts,htmlCompiler.ts - Test fixture globs match real paths (
packages/producer/tests/**/output/compiled.html,skills/**/test-corpus/**) - No existing CodeQL workflow or config in
.github/— this is additive, no conflict query-filtersuse per-ruleexclude+ exact file paths (not directory globs) for source files, so new files in the same directories still get scanned
Design:
The key insight — silencing specific rules on specific paths rather than paths-ignore on whole directories — is the right tradeoff. A malicious execSync(user_input) in a test fixture still trips js/command-line-injection because only the false-positive rules are filtered out. Good separation of the config file from the workflow file too (policy changes show up in codeql-config.yml diffs).
One note for the maintainer toggle: the PR body documents this well, but worth double-checking that the schedule trigger (cron: "39 14 * * 1") fires correctly after the default→advanced switch — sometimes GitHub requires a manual workflow dispatch or a push to main before scheduled runs start.
vanceingalls
left a comment
There was a problem hiding this comment.
Clean security-policy change with the right tradeoff (per-rule path filters, not blanket paths-ignore). Magi already covered the structural review (SHA pin matches the rest of the repo, all 4 source files + fixture globs exist on main, additive — no conflict with existing .github/codeql/ setup, and the design rationale). Adding a few gap-fill findings rather than restating his points.
Strengths
.github/workflows/codeql.yml:18-22— least-privilegepermissions:block (onlysecurity-events: write; everything elseread). This is the right shape for code-scanning workflows and matches what GitHub's hardening guide recommends; easy to get wrong on hand-written advanced setup..github/codeql/codeql-config.yml:22-28and:46-50— scoping the two cleanup-regex rule exclusions to exact file paths rather than directory globs is the load-bearing detail. A new file underpackages/cli/src/that looks like a sanitizer will still trip CodeQL, so the policy doesn't silently widen as the codebase grows.- Stacking on #856 (the actual code fixes) is the right ordering — #856 is already merged at
06:50:25Z, so the 3 alerts referenced as "close automatically when #856 merges" will resolve on the first advanced-setup scan rather than being open noise at switchover.
important
- Independent SHA verification of
github/codeql-action@7fd177fa680c9881b53cdab4d346d32574c9f7f4— confirmed againstgh api repos/github/codeql-action/git/refs/tags/v3: thev3annotated tag dereferences to exactly that commit (committed 2026-05-08 onbackport-v3.35.4). So the pin is live-correct, not just internally consistent. Worth recording because the next timev3rolls, this pin needs a follow-up. Consider adding a comment in the workflow naming the v3 sub-version (v3.35.4per the merge commit message) so the bump cadence is grep-able. - Unchecked test-plan items #3 + #4 in the PR body are the actual gate. "CodeQL workflow run on this PR confirms the config parses and the matrix executes" — that's still a
[ ]. Item #4 ("re-scan should produce only 13 open alerts") is post-merge so it can stay unchecked, but item #3 needs to land green before merge — otherwise a malformedquery-filters:key (e.g. a typo'd rule id, or a path glob the parser rejects) sails through. Easy mitigation: a dry-run on this PR's commit before flipping the default→advanced toggle, or pre-flighting the YAML against CodeQL's schema. Today'smergeable_stateisblockedbecause required CI checks are still in progress (Typecheck,Test,CLI smoke, Windows render) — none failed — but none of them exercise this workflow file.
nit
- The
skills/**/test-corpus/**glob (without theassets/infix) in thejs/functionality-from-untrusted-sourceexclusion currently matches no path onmain— the only test-corpus that exists isskills/remotion-to-hyperframes/assets/test-corpus/(matched by the next line). Harmless and forward-looking, but the PR-body table presents both as active surface; either one is fine and could be dropped, or both kept for the explicit "we cover both layouts" intent. Cite if the second variant is intentionally pre-provisioned for an upcoming skill.
notes (not actionable on this PR)
- Rule 7 scan: the local composite at
.github/actions/install-ffmpeg-windows/action.ymldoesInvoke-WebRequeston a BtbN/FFmpeg-Builds zip without SHA256 verification on the downloaded artifact. Pre-existing, not introduced here — out of scope for this PR — but worth a follow-up ticket on the same security-hardening theme. A pinned URL alone doesn't prevent a tag/asset rebuild on the upstream side. - The CodeQL workflow's
schedule: cron: "39 14 * * 1"does not fire on forks/branches — it only runs on the default branch. Magi's note about possibly needing a manualworkflow_dispatchafter the default→advanced toggle is right; consider addingworkflow_dispatch:to theon:block so the toggle can be tested without waiting for Monday or amainpush.
Verdict: APPROVE
Reasoning: Policy file is well-scoped, SHA pins are independently verified, design tradeoff (per-rule path filter vs. paths-ignore) is the right one. The two important items are operational follow-ups, not code defects; the nit is forward-looking. No blockers.
Review by Vai

What
Switch CodeQL from default setup to advanced setup, with two new files:
.github/workflows/codeql.yml— workflow scanningjavascript-typescript,python, andactions(mirroring what default setup was configured with)..github/codeql/codeql-config.yml— per-rule path filters that silence the known-noisy rules on the file shapes where they're always false positives, without excluding those paths from analysis entirely.Stacked on top of #856 (the actual code fixes for the critical + 2 medium findings).
Why
The CodeQL dashboard hit 170 open alerts, and ~157 of them were duplicates of the same 4 patterns across either generated test fixtures or functional regex that CodeQL misreads as sanitizers. That noise level made it nearly impossible to spot a real finding land — which is exactly the regression mode we don't want for an OSS repo that takes external contributions.
Naive fix:
paths-ignoreforpackages/producer/tests/. That would also blind CodeQL to a malicious contributor smuggling a realexecSync(user_input)into a "test fixture." Wrong tradeoff.Better fix: silence specific rules on specific paths, scoped narrowly enough that anything outside the pattern still trips the alarms.
How
The config has four rule-scoped exclusions, with comments explaining why each path-shape is genuinely a false positive for that one rule:
js/incomplete-sanitizationpackages/producer/tests/**/output/compiled.html,packages/producer/tests/**/failures/*.htmljs/functionality-from-untrusted-sourcepackages/producer/tests/**,skills/**/test-corpus/**,skills/**/assets/test-corpus/**js/bad-tag-filterpackages/cli/src/capture/index.ts,packages/cli/src/whisper/normalize.ts,packages/core/src/lint/utils.ts,packages/producer/src/services/htmlCompiler.tsjs/incomplete-multi-character-sanitizationpackages/cli/src/capture/index.ts,packages/cli/src/whisper/normalize.tsKey property: rules stay on for everything else.
js/command-line-injection,js/code-injection,js/xss-through-dom, etc. all still scanpackages/producer/tests/— so a real exploit slipped into a test fixture would still get caught.The two cleanup-regex files are named individually, not by glob, so any new file in
cli/,core/, orproducer/that looks like a sanitizer still trips the rules. Loosening a path means touching this file — which shows up in PR review the same way a CODEOWNERS change does.Required action before merge
This repo is currently on CodeQL default setup. Both default and advanced setup can't run simultaneously — a maintainer needs to disable default setup before this workflow can take effect:
If we merge before doing the toggle, the workflow file is harmless (it'll just sit unrun) — but the path-filtered scan won't actually start until default setup is off.
Companion: bulk dismissal of existing noise
In tandem with this PR, 157 of the 170 existing alerts were dismissed via the GitHub Code Scanning API with per-category reason strings:
used in tests(test fixtures, golden baselines,*.test.tsfiles, perf test scenarios)false positive(functional regex, scheme check that already filtersdata:/javascript:)Each dismissal carries a one-line audit comment visible on the Security tab. Open count: 170 → 13. The 13 that remain:
js/polynomial-redos(real but lower-risk — build-time regex on developer-authored input, exceptstudio-api/routes/preview.tswhich is server-facing and worth a closer look)js/incomplete-sanitizationinstudio/src/captions/parser.ts:283(real but low-risk single-quote → JSON normalization for caption pastes)These were intentionally left open as a small to-do list visible on the dashboard rather than buried in a comment thread.
Test plan
bunx oxfmt --checkclean on both new files.gh api repos/{actions/checkout,github/codeql-action}/git/refs/tags/{v4,v3}— using same checkout SHA the rest of the repo pins (34e114876b…) and the resolved tip ofcodeql-action@v3(7fd177fa68…).What this PR is not doing
security-extendedwas already enabled by default setup).paths-ignore— every path is still analyzed, only specific rules are silenced where they're known false-positive.🤖 Generated with Claude Code