chore: security audit cleanup — CodeQL, workflow permissions, dependency overrides#2884
Conversation
Exclude test fixtures (pre-built webpack bundles) and spec/dummy apps from CodeQL scanning. These generate false positive alerts for code that is not production source. Resolves 14 code scanning alerts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add explicit permissions: { contents: read } to every workflow that
lacked a top-level permissions block. Follows GitHub's principle of
least privilege for GITHUB_TOKEN.
Resolves 22 code scanning alerts.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Generated via `pnpm audit --fix`. Forces non-vulnerable versions of 28 transitive dependencies that cannot be fixed by bumping direct deps. Reduces npm audit vulnerabilities from 38 to 8. Remaining 8 are either unfixable (elliptic has no patched version) or edge cases with pnpm's range-based override resolution for deeply nested deps. These overrides should be removed when upstream packages update.
Addresses Dependabot advisory for improper detection of disallowed URIs via `allowed_uri?` in loofah 2.25.0. react_on_rails/spec/dummy/Gemfile.lock not updated due to a local bootsnap compilation issue — CI can handle this lockfile update.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds CodeQL advanced setup with path exclusions for test fixtures and spec/dummy apps, introduces a new CodeQL workflow, adds workflow-level GitHub Actions permissions ( Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
size-limit report 📦
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f42b61ba61
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Greptile SummaryThis PR is a thorough security audit cleanup that addresses 65 open GitHub security alerts through four coordinated changes: switching CodeQL from default to advanced setup with path exclusions, adding Key changes:
Confidence Score: 5/5Safe to merge — all findings are P2 suggestions with no runtime or security impact. All three comments are P2: the missing .github/workflows/codeql.yml and .github/codeql/codeql-config.yml deserve a second look for the two P2 suggestions, but neither blocks merging. Important Files Changed
Reviews (1): Last reviewed commit: "chore: update loofah to 2.25.1 for URI d..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
package.json (1)
129-157: Security overrides look reasonable; consider consolidating redundant entries.The security-focused version overrides address the transitive dev-dependency vulnerabilities as intended. However, there are several overlapping rules that could be consolidated for maintainability:
Package Redundant entries Could consolidate to minimatch<3.1.3,<3.1.4<3.1.4→>=3.1.4minimatch<9.0.6,<9.0.7<9.0.7→>=9.0.7serialize-javascript<=7.0.2,<7.0.5<7.0.5→>=7.0.5flatted<3.4.0,<=3.4.1<=3.4.1→>=3.4.2handlebars<=4.7.8,<4.7.9<4.7.9→>=4.7.9This is likely because
pnpm audit --fixwas run multiple times against different advisory states. The current entries are functionally correct—pnpm will resolve them appropriately—but fewer entries would be easier to maintain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 129 - 157, Consolidate redundant pnpm overrides in package.json by removing duplicate rules and keeping the stricter single override per package: for "minimatch" remove the `<3.1.3` and `<9.0.6` entries and keep the `>=3.1.4` and `>=9.0.7` overrides respectively; for "serialize-javascript" remove `<=7.0.2` and keep `>=7.0.5`; for "flatted" remove `<3.4.0` and `<=3.4.1` and keep `>=3.4.2`; for "handlebars" remove `<=4.7.8` and `<4.7.9` and keep `>=4.7.9`; update the override keys (the string entries like "minimatch@<3.1.3") accordingly so each package appears once with the consolidated constraint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@package.json`:
- Around line 129-157: Consolidate redundant pnpm overrides in package.json by
removing duplicate rules and keeping the stricter single override per package:
for "minimatch" remove the `<3.1.3` and `<9.0.6` entries and keep the `>=3.1.4`
and `>=9.0.7` overrides respectively; for "serialize-javascript" remove
`<=7.0.2` and keep `>=7.0.5`; for "flatted" remove `<3.4.0` and `<=3.4.1` and
keep `>=3.4.2`; for "handlebars" remove `<=4.7.8` and `<4.7.9` and keep
`>=4.7.9`; update the override keys (the string entries like "minimatch@<3.1.3")
accordingly so each package appears once with the consolidated constraint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0dce4e64-f208-4ba0-82ce-f7c46a69498e
⛔ Files ignored due to path filters (5)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlreact_on_rails/Gemfile.lockis excluded by!**/*.lockreact_on_rails_pro/Gemfile.lockis excluded by!**/*.lockreact_on_rails_pro/spec/dummy/Gemfile.lockis excluded by!**/*.lockreact_on_rails_pro/spec/execjs-compatible-dummy/Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.github/codeql/codeql-config.yml.github/workflows/actionlint.yml.github/workflows/bundle-size.yml.github/workflows/check-markdown-links.yml.github/workflows/codeql.yml.github/workflows/examples.yml.github/workflows/gem-tests.yml.github/workflows/integration-tests.yml.github/workflows/lint-js-and-ruby.yml.github/workflows/package-js-tests.yml.github/workflows/playwright.yml.github/workflows/precompile-check.yml.github/workflows/pro-integration-tests.yml.github/workflows/pro-lint.yml.github/workflows/pro-test-package-and-gem.yml.github/workflows/trigger-docs-site.ymlpackage.json
Review: chore: security audit cleanupOverall this is a well-structured security sweep. The four change categories (CodeQL advanced setup, workflow permissions, pnpm overrides, loofah/nokogiri bumps) are all sound. A few things worth flagging. Concerns1. yaml major version bump (v1 to v2) via transitive override The override 2. Redundant/overlapping pnpm overrides
These are harmless auto-generated artefacts, but cleaning them up would reduce noise in future 3. Mentioned in the commit message (bootsnap compilation issue). This leaves a stale lockfile in the PR. It would be cleaner to update it here; leaving it as-is means the security fix for that lockfile depends on a follow-up run that may never be triggered. 4. The workflow dispatches to a third-party repo using a GitHub App token ( Looks good
|
The ajv overrides (v6→v6.14+ and v7→v8.18+) cause incompatibility with ajv-keywords used by schema-utils in terser-webpack-plugin. ajv v8 removed APIs that webpack's toolchain depends on. These two moderate-severity dev-only alerts are accepted as tolerable risk since fixing them requires a breaking API change.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Code ReviewOverall this is a solid security hardening PR. The approach of using CodeQL advanced setup with path exclusions, adding least-privilege permissions to CI workflows, and using pnpm overrides to pin vulnerable transitive dependencies is well-reasoned. The changes are largely additive and non-breaking. Three issues worth addressing before merge: 1. The new 2.
3. Redundant pnpm overrides in The |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37354a9a69
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c5017aec9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Review: Security Audit CleanupOverall this is a solid, well-scoped security hardening PR. The workflow permission changes and CodeQL advanced setup are both correct and follow GitHub's least-privilege recommendations. A few issues worth addressing: Redundant dependency overrides (see inline comments) The Open-ended override ranges can resolve to new major versions Several overrides use open-ended ranges like Post-merge manual step is high-risk to forget The PR description includes a post-merge admin action: disabling the Default setup CodeQL scanner. If this is missed, both scanners run in parallel and the default one re-raises the false-positive alerts this PR was designed to suppress. Consider adding a comment to CodeQL paths-ignore coverage looks correct The three excluded paths match the actual directory structure in this repo — no gaps found. Workflow permissions All 14 workflows correctly receive Generated with Claude Code |
Review: chore — security audit cleanupOverall this is a well-scoped cleanup: the workflow permission hardening is correct and consistent, the CodeQL advanced setup is the right approach for suppressing false positives, and the Ruby gem bumps (loofah, nokogiri) are straightforward. A few issues need attention before merge. Blocking1. minimatch upgrades from 9.x → 10.x (see inline comment on 2. brace-expansion resolves to 5.0.5 (see inline comment on Non-blocking3. Redundant duplicate overrides (see inline comment on 4. Action refs not SHA-pinned in |
- Consolidate redundant pnpm overrides (remove 5 subsumed entries that were auto-generated by pnpm audit --fix for different CVEs covering the same version ranges) - Cap minimatch 9.x override at <10 to prevent major version jump - Add category parameter to CodeQL analyze step (matches official GitHub template, provides cleaner labels in Security tab) - Add actions: read permission to CodeQL workflow (required if repo ever becomes private, harmless on public repos) - Add execjs-compatible-dummy to CodeQL paths-ignore exclusions
|
Review: Security Audit Cleanup Good overall direction. Three issues to address before merging. BUG (must fix): bundle-size.yml PR commenting is broken. The new top-level 'permissions: contents: read' silently caps the job-level 'pull-requests: write' permission the check-bundle-size job needs to post size comparison comments. This won't be caught by CI on this PR since the workflow only triggers for PRs touching bundle-related files. ISSUE: brace-expansion resolves to v5, not v2.x. The '>=2.0.3' range has no upper bound, so pnpm resolved it to 5.0.5 (three major versions ahead). Inconsistent with the stated goal of 'manual caps to prevent major version jumps'. The v5 series also requires node 18/20/22+. Suggest adding <3 as a cap. NOTE: @tootallnate/once forces v1 and v2 consumers to v3 — a cross-major jump, not a patch bump. Low risk since these are dev-only proxy utilities, but the comment should reflect this. What looks good:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78759c3e8c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Review: Security Audit CleanupOverall this is a well-scoped and carefully documented security PR. The incremental commit history shows good discipline. A few items worth addressing before merge: Several pnpm overrides are missing upper-bound major-version caps (inline comments already posted). The later commits wisely capped Missing
Seven workflows were not touched and still lack a top-level permissions block: Informational: CodeQL action |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4df7e19e7d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
pnpm overrides replace the parent's constraint entirely, so an uncapped ">=4.17.23" would resolve to 5.x if published. Cap every override at the next major for consistency and to prevent future surprises. Most of these are currently harmless (no new major exists) but this matches the intent documented in the comment block.
Review: Security Audit CleanupGood, well-scoped PR. Adding least-privilege permissions to all workflows, switching to CodeQL advanced setup with path exclusions to kill false positives, and patching transitive deps via pnpm overrides are all sound approaches. A few things worth checking: Potential blocker: codeql-action@v4 Missing top-level permissions block in codeql.yml trigger-docs-site.yml: verify no functional regression Overrides maintenance note |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3d3be4c69
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Express 4.22.1 (pulled by webpack-dev-server) declares path-to-regexp ~0.1.12. The previous cap of <1 allowed resolution to 0.2.5 which is outside Express 4's router contract.
Review: chore/security-audit-cleanupOverall this is a well-scoped and clearly reasoned security hardening PR. The approach — adding least-privilege workflow permissions, switching to advanced CodeQL setup with path exclusions, and using pnpm audit generated overrides — is solid. Confirmed correct
Minor issues (see inline comments)
Out-of-band dismissalsThe PR description mentions 13 alerts were dismissed via API with reasons documented in issue 2883. Since those dismissals are not in the diff, reviewers cannot audit them here — recommend verifying 2883 covers each one before merging. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d551e929d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ncy overrides (#2884) ## Summary Addresses all 65 open GitHub security alerts (49 code scanning + 16 Dependabot) identified during a full security audit. None represent real risk to end users — see #2883 for the complete investigation. - **Switch CodeQL to advanced setup** with path exclusions for test fixtures and spec/dummy apps (eliminates 14 false-positive code scanning alerts from build artifacts and test infrastructure) - **Add `permissions: { contents: read }`** to all 14 CI workflows that lacked a top-level permissions block (resolves 22 code scanning alerts, follows GitHub's principle of least privilege) - **Generate pnpm overrides** via `pnpm audit --fix` for 28 transitive dependency vulnerabilities (all dev-only, reduces npm audit from 38 to 8 — remaining are unfixable or pnpm edge cases) - **Update loofah** 2.25.0 → 2.25.1 across 4 Gemfile.lock files (URI detection fix) Additionally, 13 code scanning false positives were dismissed via API with documented reasons (by-design VM execution, internal service architecture, non-vulnerable regex patterns). See #2883 for each dismissal rationale. ## Post-merge admin action After merging, disable the "Default setup" CodeQL scanner in Settings > Code security > Code scanning. The new advanced setup workflow replaces it — if both run, the default scanner ignores path exclusions. ## Test plan - [ ] CI passes on all existing workflows (the permissions change is additive and non-breaking) - [ ] CodeQL workflow runs successfully on this PR (new workflow) - [ ] After merge + default CodeQL disabled: code scanning alerts drop to ~0 - [ ] After merge: Dependabot detects lockfile changes and auto-closes resolved alerts Closes #2883 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** - Restrict CI workflow permissions to read-only access for repository contents. - Add a scheduled automated CodeQL security scan covering JavaScript/TypeScript and Ruby. - Exclude test fixture and dummy application directories from automated security analysis. - Expand package manager override rules to pin/remap many transitive dependency versions for more consistent installs. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Summary
Addresses all 65 open GitHub security alerts (49 code scanning + 16 Dependabot) identified during a full security audit. None represent real risk to end users — see #2883 for the complete investigation.
permissions: { contents: read }to all 14 CI workflows that lacked a top-level permissions block (resolves 22 code scanning alerts, follows GitHub's principle of least privilege)pnpm audit --fixfor 28 transitive dependency vulnerabilities (all dev-only, reduces npm audit from 38 to 8 — remaining are unfixable or pnpm edge cases)Additionally, 13 code scanning false positives were dismissed via API with documented reasons (by-design VM execution, internal service architecture, non-vulnerable regex patterns). See #2883 for each dismissal rationale.
Post-merge admin action
After merging, disable the "Default setup" CodeQL scanner in Settings > Code security > Code scanning. The new advanced setup workflow replaces it — if both run, the default scanner ignores path exclusions.
Test plan
Closes #2883
🤖 Generated with Claude Code
Summary by CodeRabbit