ci(security): add dependency review and bun audit checks#858
Conversation
Two dependency vulnerability checks per the approved plan: - dependency-review: fails PRs introducing dependencies with high+ advisories. Covers what GitHub's dependency graph sees (direct deps and Actions; bun.lock transitives are out of its reach). - bun audit: audits the full lockfile. Report-only and green on pull_request/push/workflow_dispatch (severity counts in the job summary, raw JSON as artifact); scheduled runs fail on high/critical findings to notify maintainers. The workflow is check/report only: it never modifies package.json or bun.lock and opens no upgrade PRs. Actions are pinned to release commit SHAs; Bun version matches ci.yml to avoid result drift. Closes Nagi-ovo#850 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PVZLWprb3HLfiDvc6YuHNw
|
Validation evidence (runs on my fork):
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 609e27be6b
ℹ️ 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".
| bun audit --json > audit.json || true | ||
| # An invalid file means the audit itself broke (network, registry) — | ||
| # that must fail on every event, unlike advisory findings below. | ||
| jq empty audit.json |
There was a problem hiding this comment.
Fail closed when audit output is empty
Because line 70 masks every bun audit nonzero exit with || true, a registry/network failure or unsupported audit command can leave audit.json as a zero-byte file; jq empty audit.json exits successfully on empty input, so the severity variables become empty and the scheduled high/critical gate evaluates as zero. In that scenario the security workflow goes green without actually auditing dependencies, so this validation needs to require real JSON content before treating the audit as report-only.
Useful? React with 👍 / 👎.
anontokyo-dev
left a comment
There was a problem hiding this comment.
Review result: changes requested
I verified the approved #850 scope and the supplied fork evidence.
Verified
- ✅ The PR only adds
.github/workflows/security.yml; it does not add Dependabot or modifypackage.json/bun.lock. - ✅ Permissions are limited to
contents: read, and checkout credential persistence is disabled. - ✅ Every Action is pinned to a full SHA that resolves to a stable release:
actions/checkout→v4.3.1actions/dependency-review-action→v4.9.0oven-sh/setup-bun→v2.2.0actions/upload-artifact→v7.0.1
- ✅ Bun
1.3.12matches the existing CI workflow. - ✅ Fork evidence is reproducible: source-only PR skips Security; dependency-manifest PR runs both checks green; dispatch and push are report-only green.
- ✅ I downloaded the dispatch artifact and confirmed valid JSON with 99 advisories, including 3 critical and 40 high.
- ✅ No UI is changed, so screenshots are not required; the linked workflow runs are the appropriate evidence.
Blocking P1
The latest Codex finding is valid:
bun audit --json > audit.json || true masks every non-zero exit. A zero-byte file then passes jq empty audit.json with exit code 0; the count variables become empty, Bash treats the scheduled high/critical total as zero, and the workflow can report success without performing an audit.
Please fail closed before counting advisories. At minimum:
- Capture the audit exit status instead of discarding it blindly.
- Require
audit.jsonto be non-empty. - Validate the expected JSON structure, not only JSON syntax.
- Continue treating a valid advisory result as report-only for PR/push/dispatch, while actual command/network/output failures fail every event.
Please add validation evidence showing that:
- normal audit JSON still produces the expected 99 / 3 critical / 40 high summary and remains green for dispatch;
- an empty or malformed audit output fails the job;
- the scheduled high/critical enforcement path still exits non-zero.
All current CI checks are green, but this P1 must be fixed before approval. This PR changes the sensitive .github/workflows/** path, so even after technical approval it still requires Nagi's explicit pre-merge confirmation.
Capture the audit exit code instead of masking it, reject empty output and JSON that is not an advisory map, and treat a nonzero exit with zero parsed advisories as a failed audit. Findings remain report-only on pull_request/push/workflow_dispatch; infrastructure failures now fail on every event. Addresses the P1 in the Nagi-ovo#858 review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SAMGouFiJj4UQfDAJvJkZ4
|
P1 addressed in ffff4fa — the audit step now fails closed:
Requested evidence
Local 8-path matrix against the extracted step logic |
Re-review status for
|
Operational failures (missing, empty, or malformed audit output) fail on every event; the header comment now states this instead of calling pull_request/push/dispatch always green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SAMGouFiJj4UQfDAJvJkZ4
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 392754e6e8
ℹ️ 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".
| persist-credentials: false | ||
| - uses: actions/dependency-review-action@2031cfc080254a8a887f58cffee85186f0e49e48 # v4 | ||
| with: | ||
| fail-on-severity: high |
There was a problem hiding this comment.
Add dev scopes to dependency review
For PRs that add or upgrade a vulnerable devDependency, this check remains green: the dependency-review-action docs list fail-on-scopes with a default of runtime, and the Bun audit job is report-only on PRs, pushes, and manual runs. Since this repo relies heavily on dev tooling for building and shipping the extension, a high/critical advisory in a newly introduced build dependency would not be blocked until the weekly scheduled audit; add an explicit fail-on-scopes: development, runtime, unknown (or the intended scope list) alongside the severity gate.
Useful? React with 👍 / 👎.
fail-on-scopes defaults to runtime only, which would let a PR introduce a vulnerable devDependency unblocked until the weekly audit. Build and release tooling is part of the extension's supply chain, so block development and unknown scopes as well. Addresses the P2 in the Nagi-ovo#858 review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SAMGouFiJj4UQfDAJvJkZ4
|
P2 addressed in 66249b4 — Evidence (HavenXia/voyager#4, fault-injection PR adding
|
|
@anontokyo-dev Please review. LGTM |
Final re-review status for
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
anontokyo-dev
left a comment
There was a problem hiding this comment.
Approved at 66249b4
- ✅ The fail-closed audit handling is verified.
- ✅ Runtime, development, and unknown dependency scopes are enforced.
- ✅ The fault-injection evidence covers affected behavior and failure paths.
- ✅ All required CI checks pass, including Security, quality checks, tests, i18n, four browser builds, and Native Swift/Xcode.
- ✅ The repository Codex review now covers the exact current head and reports no major issues.
- ✅ No UI is changed, so screenshots are not applicable.
Technical review is complete. This PR changes the sensitive .github/workflows/** path, so merge still requires the final maintainer authorization.
|
Thanks so much!!! |
|
You are welcome! The technical approval is recorded on the current head, and the PR is ready for the final merge authorization. |
|
Merged successfully as |
Closes #850
Adds the two dependency vulnerability checks approved in #850:
fail-on-severity: high. Covers what GitHub's dependency graph sees (direct deps + Actions);bun.locktransitives are handled by the audit job below.pull_request/push/workflow_dispatch(severity counts + Bun version in the job summary, raw JSON as artifact);scheduleruns fail on high/critical findings to notify maintainers. Operational failures fail on every event: the step fails closed on missing, empty, or structurally invalid audit output instead of treating it as "no findings". The per-event behavior is documented at the top of the workflow file.Per the maintainer constraints: no
dependabot.yml, the workflow never modifiespackage.json/bun.lock, actions are pinned to release commit SHAs, and the Bun version matchesci.yml(1.3.12) to avoid result drift.Validation
mainreproduces the triage baseline exactly: 99 advisories (3 critical / 40 high / 48 moderate / 8 low); simulatedpull_requestexits 0,scheduleexits 1.workflow_dispatchwith summary, fail-closed fault injection) are linked in the comments below.GITHUB_TOKEN; the workflow needs no secrets.🤖 Generated with Claude Code
https://claude.ai/code/session_01SAMGouFiJj4UQfDAJvJkZ4