Repository navigation
chore(ci): audit only the dependencies a PR adds - #6337
Conversation
…latest Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe pull request adds a GitHub Actions workflow that audits package versions added in a pull request. It updates CircleCI steps related to the audit and replaces the Cypress server wait command with bounded ChangesPull request dependency audit
Cypress server startup wait
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Other Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant DependencyAuditWorkflow
participant audit.mjs
participant npmBulkAdvisoryAPI
participant GitHubCommitStatus
PullRequest->>DependencyAuditWorkflow: Open, synchronize, or reopen event
DependencyAuditWorkflow->>DependencyAuditWorkflow: Fetch head and merge-base lockfiles and workspace files
DependencyAuditWorkflow->>audit.mjs: Run audit with downloaded inputs
audit.mjs->>npmBulkAdvisoryAPI: Query advisories for added package versions
audit.mjs-->>DependencyAuditWorkflow: Return audit result and new_ignores output
DependencyAuditWorkflow->>GitHubCommitStatus: Post success status when new_ignores is nonzero
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change moves the dependency audit from CircleCI to a GitHub workflow and adds tests for the audit script. No concrete merge-blocking risk was found in the supplied changes. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 64.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 2 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
wayfarer3130
left a comment
wayfarer3130
left a comment
There was a problem hiding this comment.
I reviewed this PR with a focus on security. The design is sound, and the PR improves the security of CI.
Good points
- The workflow uses
pull_request_targetsafely. The workflow does not check out the PR head. PR values go to the shell steps only throughenv:. The permissions are minimal. - I checked the action pins.
df4cb1c…is the commit of the annotated tagactions/checkout@v6.0.3.48b55a0…is the commit ofactions/setup-node@v6.4.0. - The step that parses the PR YAML has no token. The
yamldependency has an integrity pin, andnpm ciruns with--ignore-scripts.yamlv2 does not execute code, andtoJS()limits alias expansion by default. - A PR can add its own ignores.
.github/CODEOWNERSalready requires a code owner review for/pnpm-workspace.yaml, and the new status makes the ignores visible. These two controls together are sufficient. - The removal of the NPM token and of the git identity from
BUILD_PACKAGES_QUICKis a good change. The removal ofnpx wait-on@latestfrom the Cypress job is also good, because that job holds the Cypress record key.
Findings (details in the inline comments)
- Low: text from the PR goes to stdout without escape, so the PR can inject workflow commands. The injection does not bypass the check, but it can hide the annotations for reviewers.
- The check blocks a merge only when branch protection lists "Dependency audit" as a required check. Please confirm this for
masterandrelease/*before the merge. - No job audits the lockfile of
masternow. A new advisory against a version that is already onmastergets no report. - Low: two edge cases pass the check without an audit.
Other review checks
- Reuse of existing methods: pnpm has no audit mode for a lockfile diff, so a custom script is correct. One dependency (
yaml) is a better choice for a security tool than the@pnpm/lockfile-*packages. - Comments: the header comment of the workflow is long. The comment records security decisions, so I think that the length is correct.
- Tests: the script has no automated tests. For a security gate, I recommend 2 or 3 tests of the type "the API must do this": the lockfile diff (with the two-document format), the ignore logic, and the escape of PR text from finding 1.
audit.mjscallsmain()on import, so the tests need exports of the pure functions. - User specification: the viewer behaviour does not change. The contributor rule changes: the check reads the ignores from the PR copy of
pnpm-workspace.yaml. The PR description and the comment inpnpm-workspace.yamlstate this change clearly.
| } | ||
|
|
||
| const markdown = lines.join('\n'); | ||
| console.log(markdown); |
There was a problem hiding this comment.
Security (low): workflow-command injection through stdout.
The Markdown report holds text from the PR without escape:
ignoreGhsasentries that do not matchGHSA_ID(line 173 removes only backticks).- Lockfile keys in the "not audited" list (line 232).
A YAML string such as "x\n::stop-commands::abc" puts a workflow command at the start of a line. The runner then executes the command. With this injection, a PR can:
- hide the later
::warning title=New audit ignoreand::errorannotations, - add a false
::notice, - add
::add-mask::for words in the log.
The exit code and the commit status stay correct. The script writes new_ignores to $GITHUB_OUTPUT after the stdout commands, so the check is not bypassed. The injection only reduces what the reviewers see.
Suggested fix: put the output between ::stop-commands::<random token> and ::<random token>::. Another fix is the validation in the comments on lines 173 and 232. The same text also goes to $GITHUB_STEP_SUMMARY, where the text can add false headings. GitHub sanitizes the HTML of the summary, so that risk is small.
There was a problem hiding this comment.
Good catch, thanks. Fixed in d26655fbed: the report is now printed between ::stop-commands::<random token> and ::<random token>:: (crypto.randomUUID()), so no line of it can act as a workflow command. Our own ::warning and ::error annotations are printed after commands are resumed. The PR text is also cleaned at the source (replies 2 and 3).
| for (const ghsa of newIgnores) { | ||
| const id = GHSA_ID.test(ghsa) | ||
| ? `[${ghsa}](https://github.com/advisories/${ghsa})` | ||
| : `\`${ghsa.replace(/`/g, '')}\``; |
There was a problem hiding this comment.
An ignore that does not match GHSA_ID keeps its newlines and control characters here. I suggest that the script rejects such an entry, or that the script shows the entry as invalid after the removal of all characters outside [A-Za-z0-9-]. In pnpm audit, an entry that is not a GHSA never matches an advisory, so a strict rule here costs nothing. Note that the script itself creates npm-<id> values for advisories without a GHSA. If you want to allow those values in the ignore list, add npm-\d+ to the accepted pattern.
There was a problem hiding this comment.
Done in d26655fbed. An ignore entry is valid only if it is a GHSA ID or npm-<number>. Anything else is listed as "invalid entry", reduced to [A-Za-z0-9-], with a note that it matches no advisory. It does not fail the check, since it cannot silence anything and the "New dependency audit ignores" status already asks reviewers to look.
| `<details><summary>${notAudited.length} added entry(ies) not from the npm registry, not audited</summary>`, | ||
| '', | ||
| // Unvalidated lockfile keys: drop backticks so they stay inside the code span. | ||
| ...notAudited.map(p => `- \`${`${p.name}@${p.version}`.replace(/`/g, '')}\``), |
There was a problem hiding this comment.
These lockfile keys failed the PACKAGE_NAME / REGISTRY_VERSION checks, so the keys can contain any characters, newlines included. Please remove control characters here as well as backticks, for example with .replace(/[\x00-\x1f\x7f]/g, '')`.
There was a problem hiding this comment.
Done in d26655fbed, with your regex: .replace(/[\x00-\x1f\x7f`]/g, '').
| let blocking = []; | ||
| const headText = readText(values.head); | ||
| const baseText = readText(values.base); | ||
| if (headText === null) { |
There was a problem hiding this comment.
Edge case (low): when the base has a pnpm-lock.yaml and the head has no lockfile, the check passes with "nothing to audit". In CI, an install with a frozen lockfile fails in that case, so the risk is small. But a security gate must fail when it cannot do its work. I suggest that the script fails when baseText !== null && headText === null.
There was a problem hiding this comment.
Agreed, done in d26655fbed. If the target branch has pnpm-lock.yaml and the PR does not, the check now fails ("This PR deletes pnpm-lock.yaml, so its dependencies cannot be audited"). If neither side has one (old pre-pnpm branches), it still passes.
| lines.push('`pnpm-lock.yaml` is unchanged; nothing to audit.', ''); | ||
| } else { | ||
| const added = addedPackages(lockfilePackages(baseText ?? ''), lockfilePackages(headText)); | ||
| const auditable = added.filter( |
There was a problem hiding this comment.
Edge case (low): if a future pnpm version changes the format of the packages: keys (for example, back to /name@version), every added entry fails these patterns. All entries then go to the "not audited" list, and the check passes without a report. A small guard prevents that result: fail when the "not audited" entries are more than a small part of the added entries (for example, more than 10% and more than 5 entries).
There was a problem hiding this comment.
Done in d26655fbed, with your numbers: the check fails when more than 5 added entries, and more than 10% of them, are not name@version from the npm registry. The message says the lockfile format may have changed and the script needs an update. Today's lockfile has 0 such entries out of 2,683.
| # a PR to another branch, push a commit (or close and reopen) to rerun. | ||
|
|
||
| on: | ||
| pull_request_target: |
There was a problem hiding this comment.
Two points about the trigger:
- This PR removes the CircleCI audit. After the merge, this check blocks a merge only when branch protection lists "Dependency audit" as a required check. Please add the check to the branch protection of
masterandrelease/*when this PR merges. - This check audits only the versions that a PR adds. No job audits
masternow. When an advisory appears for a version that is already onmaster, no job reports the advisory. I suggest aschedulerun ofpnpm audit --audit-level highonmasterthat does not block, or an explicit decision to rely on Dependabot alerts.
There was a problem hiding this comment.
- Yes. I will add "Dependency audit" as a required check on
masterandrelease/*once this merges. GitHub only offers a check there after it has run at least once. - A daily check of
masteris planned as a separate task. It needs to run privately, since this repo's Actions logs and issues are public, so it will live outside this repo. Dependabot alerts don't work as the source for us: GitHub's dependency graph can't read our pnpm 12 lockfile (two YAML documents), so the alerts only reflectpackage.jsondeclarations, not the installed versions.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@wayfarer3130 On the tests from your review: added in |
Viewers
 |
||||||||||||||||||||||||||||
| Project |
Viewers
|
| Branch Review |
chore/security-check-ng
|
| Run status |
|
| Run duration | 01m 47s |
| Commit |
|
| Committer | Joe Boccanfuso |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
0
|
 |
0
|
 |
0
|
 |
28
|
| View all changes introduced in this branch ↗︎ | |
This branch was successfully deployed
2 participants
Context
The current security audit in CircleCI runs
pnpm auditon the whole lockfile. So any PR that touchespnpm-lock.yamlfails because of problems that are already on master, even when the PR didn't cause them. It also always compares against master, even for PRs intorelease/*.This PR replaces it with a check that only looks at what the PR adds. It also removes some secrets and unpinned downloads from the PR builds.
Changes & Results
pnpm-lock.yamlwith its target branch and checks only the package versions the PR adds.auditConfig.ignoreGhsasinpnpm-workspace.yamlwith a reason. New ignores show up as an extra New dependency audit ignores line in the PR's checks, so reviewers notice them.pull_request_target, so it always uses master's copy of the workflow and script. It only reads the PR's lockfile andpnpm-workspace.yamlas data and never runs PR code.BUILD_PACKAGES_QUICK.BUILD_PACKAGES_QUICK. That job only builds; it never publishes.curlinstead of downloadingwait-on@lateston every run.Note: the new check can't run on this PR.
pull_request_targetuses the workflow from master, so it starts working on PRs opened after this one merges.Testing
Tested on my fork, with this branch set as the fork's default branch:
Also run locally against past PRs (#6327, and fork PR #6286), and against a target with no pnpm lockfile.
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals. (None needed: CI only.)
Tested Environment
ubuntu-latest🤖 Generated with Claude Code
Summary by CodeRabbit