chore(security): scope CodeQL to shipped product code - #201
Conversation
CodeQL flagged 459 alerts on main, but triage showed the bulk are in code that never ships in the installer's runtime path: file-not-closed in the omnivoice/eval harnesses, unused-global "FPs" in alembic migration boilerplate (revision/down_revision), bind-all in tests, and path sinks in the legacy Gradio research UI. They drowned out the handful of real findings. Add a CodeQL config (inline, supported because build-mode is `none`/interpreted) that scopes analysis to product code via paths-ignore: omnivoice/eval, research, tests, backend/migrations, and *.test.* files. Queries move into the inline config so security-and-quality stays the single source of truth alongside paths-ignore. Net effect on the next scan: the non-shipped-code alerts auto-resolve, leaving the security tab focused on shipped backend + frontend. No product code changes. Deliberately NOT touched (assessed, left as-is): - Stack-trace-exposure (detail=str(e) in routers) — these are intentional, helpful one-line diagnostics (the error-transparency work in b64f53b). Genericizing them on a loopback/single-user app would regress a product value for ~zero real benefit. - "Critical" command-injection in exports.py and the high path-injections in settings.py / system.py — design-correct false positives: list-form argv (no shell), and the destination/model-dir/ffmpeg paths are arbitrary user-chosen paths by design (containment guards would break the features). Best handled by dismiss-with-justification, not code surgery. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR refines the CodeQL security analysis scope by expanding the workflow configuration to exclude test, evaluation, research, and migration code paths. The init action ChangesCodeQL Security Scanning Configuration
🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| .github/workflows/security.yml | Switches CodeQL from a top-level queries key to an inline config: block that adds paths-ignore to scope analysis to shipped product code; excludes eval harnesses, research, tests, migrations, and *.test.* JS/TS files. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CodeQL Init] --> B{Inline config block}
B --> C[queries: security-and-quality]
B --> D[paths-ignore]
D --> E[omnivoice/eval\noffline eval harnesses]
D --> F[research\nlegacy Gradio / experiments]
D --> G[tests\ntest fixtures]
D --> H[backend/migrations\nAlembic boilerplate]
D --> I["**/*.test.{js,jsx,ts,tsx}"\nfrontend test files]
C --> J[Analyze step]
J --> K[Security tab alerts\nlimited to shipped product code]
Reviews (1): Last reviewed commit: "chore(security): scope CodeQL to shipped..." | Re-trigger Greptile
| - "**/*.test.js" | ||
| - "**/*.test.jsx" | ||
| - "**/*.test.ts" | ||
| - "**/*.test.tsx" |
There was a problem hiding this comment.
.spec.* test files not excluded
The four patterns only cover *.test.{js,jsx,ts,tsx}. If any frontend tests use the .spec. naming convention (common in Vitest/Playwright setups), those files will still be analysed. If they contain the same "bind-all in fixtures" pattern you're eliminating from tests/, you'd still see noise from them. Worth adding **/*.spec.js, **/*.spec.jsx, **/*.spec.ts, **/*.spec.tsx if the project uses that convention, or verifying that no .spec.* files exist outside the already-excluded tests/ directory.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| paths-ignore: | ||
| - omnivoice/eval | ||
| - research | ||
| - tests |
There was a problem hiding this comment.
Python test files outside
tests/ not excluded
The tests entry only covers the top-level tests/ directory. Python projects commonly co-locate test files alongside source as test_*.py or *_test.py (e.g. backend/api/test_routes.py). If any such files exist outside the tests/ tree, they'll remain in scope and can still contribute the bind-all or side-effect-assert findings you're trying to remove. A supplementary **/test_*.py pattern would close this gap.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Scope CodeQL to shipped product code
CodeQL reports 459 open alerts on
main(the Security tab's "100" is just first-page truncation). After triaging every group, the headline (1 critical + 58 high) is dominated by non-shipped code and design-correct false positives — there is no remotely-exploitable issue in default loopback mode.This PR removes the largest, cleanest chunk: alerts in code that never ships in the installer's runtime path.
Change
A single CodeQL config addition (
paths-ignore, valid here becausebuild-mode: none= interpreted analysis), scoping analysis to product code:security-and-qualityqueries move into the inline config so there's one source of truth. No product code changes. On the next scan, the excluded-tree alerts auto-resolve.Deliberately NOT done (assessed against this app's threat model)
detail=str(e)across ~15 routers): these are the intentional, helpful one-line diagnostics from the error-transparency work (b64f53b). On a loopback/single-user app, genericizing them would regress a product value for ~zero benefit.exports.py:73) + high path-injections (settings.py,system.pyFFMPEG_PATH): design-correct FPs — list-form argv (no shell), and the destination / model-dir / ffmpeg paths are arbitrary user-chosen paths by design (the native save/picker dialog). Containment guards would break the features. The right tool for these is dismiss-with-justification, not code surgery.Remaining after this merge
The in-product FPs (10 zustand
superfluous-trailing-argumentsfrom the documented(set, get, api)pattern, plus the design-correct path/cmd sinks above) are best dismissed with justification — happy to do that as a follow-up.Verified:
security.yml+ the embedded CodeQL config both parse as valid YAML.🤖 Generated with Claude Code
Summary by CodeRabbit