Repository navigation
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe audit command now distinguishes ranked, ChangesAudit Severity Handling
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
|
Updated 7:58 AM PT - Oct 20th, 2026
✅ @robobun, your commit e9e02d0010d30066fdedbb2ad79afb064b6d7d86 passed in 🧪 To try this PR locally: bunx bun-pr 45668That installs a local version of the PR into your bun-45668 --bun |
|
Status: ready for review. CI is green on this head (build 129421, 181 of 181 jobs passed). How it was reproduced. A loopback registry answers the bulk advisory endpoint with one advisory for an installed package. When the advisory has The eight new tests and the changed escaping test in |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked two things and ruled them out: the --json and bun audit fix paths both go through the same keep_vulnerability in src/runtime/cli/audit_command.rs, so there is no second classification site left on the old "moderate" default; and the unranked word is passed to pretty! as a runtime argument (after escape_control_chars), so registry-supplied <red>-style markup is printed literally, not interpreted as a color tag. The [u32; 4] summary index matches the four repr(u8) discriminants of AuditLevel.
Extended reasoning...
The change replaces the raw severity bytes in bun audit with a three-way enum (info, ranked, unranked) across src/runtime/cli/audit_command.rs and src/install/PackageManager/CommandLineArguments.rs, with docs and tests updated. It touches how untrusted registry strings reach the terminal and how the exit code is decided, and it intentionally changes exit codes for missing or non-npm severity words. One confirmed finding about unsuppressible id-less unranked advisories is posted inline, so a human should weigh that and the deliberate stricter-than-npm behavior change; the sibling sites and output-injection concerns were checked and are not affected.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/runtime/cli/audit_command.rs— An advisory with noid, nourland an unrankable or missingseveritynow fails everybun auditandbun audit fixat every--audit-level, and nothing the docs offer can suppress it. src/runtime/cli/audit_command.rs:878 keeps every unranked advisory, and the--ignorematch at audit_command.rs:886-889 compares againstidandurlonly, both empty here. On the base branch such an advisory ranked as moderate and--audit-level=highdropped it. docs/pm/cli/audit.mdx says to pass--ignore <id>to accept it, which has nothing to match. Fix: give every kept advisory an ignore handle (for example fall back to matching the title, or print a note that the advisory has no id and cannot be ignored) so the documented escape hatch covers the unranked population it was added for. [also at: src/runtime/cli/audit_command.rs:1067 - Users at--audit-level=high|criticalwhose registry sends an unranked advisory with an emptytitlenow get exit 1 and a summary that says1 unknown severitywith no advisory line to read or ignore. src/runtime/cli/audit_command.rs:1065-1067 skips printing any advisory whose title is empty, but the counting loop at audit_command.rs:1036-1042 and the exit code at audit_command.rs:1118 still include it.]Why this was flagged
If any registry sends an advisory object with only
title, parse_vulnerability at src/runtime/cli/audit_command.rs:721-760 leavesidandurlas empty boxes andseverityas Unranked. keep_vulnerability at audit_command.rs:875-879 never marks it below a level. The ignore check at audit_command.rs:886-889 isstrings::eql(&vulnerability.id, ignored_cve) || strings::index_of(&vulnerability.url, ignored_cve).is_some(); with an empty id and empty url,strings::index_ofreturns None for an empty haystack (src/bun_core/string/immutable.rs:598), so no--ignorevalue matches. The result is exit 1 from every audit and the fixer at audit_command.rs:604 acting on it, with no remedy short of removing the dependency. The base branch gave this advisory the moderate rank and--audit-level=high|criticalfiltered it out. The new docs paragraph in docs/pm/cli/audit.mdx promises--ignore <id>as the way to accept an unranked advisory, which is false for this shape. Remedy: ensure every advisory the new rule keeps has an ignore handle, or fail loudly that it has none.Verification: A registry returns an advisory object with no
id, nourl, and a missing or unrankableseverity.keep_vulnerabilityat src/runtime/cli/audit_command.rs:878:Severity::Unranked(_) => false, so it survives--audit-level=high|critical. Ignore check lines 886-889 matchesidorurl; both empty, so no--ignore <id>value matches and the command exits 1. On the base branch the same advisory was dropped at--audit-level=high|criticaland the command exited 0.
|
The finding is correct about the code. Both parts are the behaviour of main, and this PR widens where they apply.
I did not change either in this PR. No registry is known to send an advisory without an The follow-up that matches each advisory range against each installed copy changes the same report loop. It will print every advisory that the summary counts. |
aa369b2 to
8315679
Compare
|
The last push also closes the second part of the finding. An advisory whose The first part is unchanged: an advisory with no |
8315679 to
c5eb28d
Compare
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline nit, I also checked for sibling sites that still classify raw severity bytes: --json and bun audit fix both go through collect_vulnerabilities -> keep_vulnerability, so the single Severity match is the only filter, and the derived Ord on AuditLevel follows the repr(u8) declaration order (low < moderate < high < critical), matching the old as u8 comparison.
Extended reasoning...
The change replaces raw-byte severity handling in bun audit with a Severity enum resolved once at parse time, changing exit codes for unknown, missing, and info severities under --audit-level. It touches no auth or crypto surface, but it alters a security tool's fail-open/fail-closed behavior, which the author explicitly asks a maintainer to sign off on. I confirmed every consumer routes through the one keep_vulnerability predicate and that the ordinal comparison preserves the previous ranking.
c5eb28d to
5e1d229
Compare
An advisory whose severity is not low, moderate, high or critical had the rank moderate, so --audit-level=high and --audit-level=critical dropped it and the audit passed. The severity is now read once in parse_vulnerability: info, one of the four levels, or the word the registry sent. info is below every --audit-level, as in npm, pnpm and yarn. An advisory without a rank is never below --audit-level, in the report, with --json, and in bun audit fix.
5e1d229 to
e9e02d0
Compare
Problem
bun audit --audit-level=highexits 0 for an advisory whoseseverityisHIGH, another unknown word, or missing. It printsNo vulnerabilities found (checked 1 package, 1 below --audit-level=high).--jsonandbun audit fixagree.AuditLevel::should_include_severityranked every unknown word asmoderate(src/install/PackageManager/CommandLineArguments.rs:867). Three more sites defaulted tomoderate.Fix
parse_vulnerabilityreads the severity once:Info,Ranked(AuditLevel)orUnranked(word).unknown severity (HIGH), and--ignore <id>accepts it.inforanks belowlow, as in npm, pnpm and yarn.test/cli/install/bun-audit.test.ts. Eight new tests and one changed test fail on main.--audit-levelhelp line is unchanged (Notes).Background
--audit-levelfilters the report and sets the exit code.severityis free text: registry.npmjs.org sends the four levels, and ProGet can sendinfo([BUG] handle-info-severity npm/npm-audit-report#118).Downsides
--audit-level=high|critical.infoprints asinfo, notmoderate, and exits 0, not 1, at--audit-level=low|moderate.high. Found by review: no user report, no registry known to send such a severity.Notes
Cells. One package, loopback registry, npm 11.16.0 with a cold cache for each run, main at 21620e2.
HIGH--audit-level=critical--json: exit 0HIGHandlow--audit-level=criticalHIGHandlow--audit-level=moderatemoderate--audit-level=highhigh--audit-level=criticalinfo--json: exit 0moderateinfoinfoandlow--audit-level=moderateinfo--audit-level=critical--json: exit 0@npmcli/arborist/lib/vuln.js), and its exit code skips a word that it does not know (npm-audit-report/lib/exit-code.js). The TypeError is in the text reporter, when a package has no ranked advisory.low. Bun has no default, so an audit with onlyinfoadvisories and no flag exits 1, as on main.--audit-level=HIGHis still rejected.Prior art. Tools that have an explicit unknown severity and a threshold are split. NuGet audit skips a vulnerability below the audit level unless its severity is
Unknown(AuditUtility.cs). The rustsec query behindcargo auditsays: "Vulnerabilities without associated CVSS information will always match regardless of what this is set to." Grype and Trivy order unknown as the lowest severity.Questions for a maintainer.
highas npm reads it. It is one arm inparse_vulnerability.--audit-level=infois accepted. This PR does not add it. It also needs the help text and the three completion files.The test that this PR replaces.
an unknown severity is counted and filtered as moderatecame with #38333. Its fixture isinfo. Its exit 0 at--audit-level=highstays. Its exit 1 at--audit-level=moderateis now 0.Help text.
bun audit --helpstill saysOnly print advisories with severity greater than or equal to <level>. An unranked advisory has no severity to compare. The docs have one new paragraph. A new wording belongs with question 2, which edits the same lines.Malloc calls. Debug builds of main 21620e2 and of this PR, one registry package, counted by ASAN (
print_stats=1:atexit=1), the same count in every run of a build.bun audit --json, 15 advisoriesbun audit, 15 advisoriesbun audit, no advisoryEach advisory is parsed twice (
collect_vulnerabilitiesvalidates the response, then reads it for the report), and each parse made 2 allocations for the severity. The 2 calls of the last row are not from this change: an earlier build of the same source made 257.VulnerabilityInfostays 96 bytes.Limit.
--ignorematches theidor the URL of an advisory, as on main. An advisory that has neither cannot be ignored. With this PR that also applies at--audit-level=high|critical, when the advisory has no rank.Empty title. An advisory whose
titleis an empty string now prints asVulnerability found, like an advisory with notitle. Before, the report counted it and did not print it (#19855), so an audit could fail with no advisory line.History. The
moderatedefault came with the first implementation (#19855) and the level filter (#21833). Also considered: refusing the whole answer when one severity has no rank. One unusual advisory then hides all the others and stopsbun audit fix.Related. #38557 is closed: #44968 included it. A second PR records the version of tarball and git packages in the lockfile so that
bun auditsends them. It touches other regions ofaudit_command.rsand merges cleanly with this PR in either order. The report still lists every installed version of a package under an advisory. A change that matches the range per installed copy follows separately.