fix: clear wallet and create-sei advisory drift - #350
Conversation
Two advisories published against the existing dependency graph, so the nightly consumer smoke went red without any change in this repository. GHSA-rgj7-g3m4-5g8c covers sharp below 0.35.4, which @dynamic-labs/iconic pins exactly at 0.35.0. Same shape as the documented axios and uuid pins, so it is corrected the same way: a root override, here a patch-level move inside the pinned minor. Nine high findings came from that one advisory cascading up the Dynamic chain to @sei-js/sei-global-wallet itself. GHSA-528h-pc64-c93x covers every stream-json up to 3.4.0, which the Solana RPC client's CommonJS jayson requires on the optional AA path. No override can reach a fix: 3.5.0 onward is ESM-only under a moved src/ layout, so pointing jayson at a fixed version trades the advisory for MODULE_NOT_FOUND, and every CommonJS version sits inside the advisory. It joins the existing Bun waiver and a new npm one, both subset checks that still fail on anything unwaived. The wallet-only npm consumer keeps its strictly clean audit, so a default install is unaffected. Also closes two verifier gaps this exposed. The npm audits ran without allowing a non-zero exit, so any finding surfaced as a raw spawn error carrying the whole audit JSON instead of the assertion naming the consumer. The override reporting and the Bun override-effectiveness assertion now derive from the override block rather than a hardcoded axios/uuid list. Co-authored-by: Cursor <cursoragent@cursor.com>
PR SummaryLow Risk Overview
Release verification is tightened: npm audits allow non-zero exit but reject invalid/ENOAUDIT payloads; failures name packages and advisory URLs. Override reporting and Bun effectiveness checks are derived from the override block (not a hardcoded axios/uuid list), and README JSON blocks are asserted against what consumers install. Shared helpers live in Reviewed by Cursor Bugbot for commit 9cb6388. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #350 +/- ##
=======================================
Coverage 97.17% 97.17%
=======================================
Files 80 80
Lines 5410 5410
=======================================
Hits 5257 5257
Misses 153 153
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The sharp override and stream-json waiver are well-reasoned, correctly mirrored between the script and the README, and carry a changeset. One blocking issue: switching both npm audits to allowFailure = true lets an npm error response (e.g. ENOAUDIT) parse as a zero-finding report, so an audit that never ran passes the gate.
Findings: 2 blocking | 3 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- The Cursor second-opinion file (
cursor-review.md) is empty — that pass produced no output, so this review merges only Claude's and Codex's findings. assertWithinWaiver/assertAcceptedNpmAuditare new parsing logic with real edge cases (error payloads, string-vs-objectvia, GHSA extraction) but live in a script whose helpers aren't exported, sobun test --isolate scriptscan't reach them. The error-response case above is precisely what a small unit test would have pinned — worth exporting the pure helpers into a testable module if this verifier keeps growing.- The README override blocks are still hand-maintained alongside
baseSafeOverrides/npmSafeOverrides. DerivingoverriddenTransitivePinsfrom the override object closed the drift risk inside the script; the README copies can still fall out of sync silently (this PR had to edit three separate blocks to addsharp). A check that the documented blocks match the script's objects would make the next addition mechanical.
| }; | ||
|
|
||
| const assertAcceptedNpmAudit = (result: ProcessResult, label: string) => { | ||
| const report = parseJsonOutput<AuditReport>(result.stdout); |
There was a problem hiding this comment.
[blocker] npm audit --json emits JSON on failure too — a registry or network problem yields {"error":{"code":"ENOAUDIT",...}} on stdout with a non-zero exit. Now that the call site at line 933 passes allowFailure = true, that body flows straight into parseJsonOutput, which parses it fine. It has no metadata.vulnerabilities and no vulnerabilities, so report.metadata?.vulnerabilities?.total ?? Object.keys(...).length evaluates to 0, and the function logs now audits clean: every accepted advisory was fixed upstream and returns green.
Before this PR the non-zero exit threw, so a failed audit could not be mistaken for a passing one. As written, an audit that never completed reads as a pass on the nightly gate this PR exists to keep honest — and the misleading [waiver] line would invite someone to delete the waiver.
Reject the error shape before interpreting the report, e.g. assert report.error is absent and that report.metadata?.vulnerabilities is actually present, then use total directly rather than falling back to a key count.
| const waivedAudit = parseJsonOutput<AuditReport>((await run(['npm', 'audit', '--json'], waivedDir)).stdout); | ||
| // `npm audit` exits non-zero on any finding, so let the assertion below | ||
| // report which advisory broke the consumer instead of a raw spawn error. | ||
| const waivedAudit = parseJsonOutput<AuditReport>((await run(['npm', 'audit', '--json'], waivedDir, true)).stdout); |
There was a problem hiding this comment.
[blocker] Same failure mode as assertAcceptedNpmAudit above, and it lands on the stricter gate: with allowFailure = true, an npm error payload parses into an AuditReport with neither metadata.vulnerabilities.total nor vulnerabilities, so assertAuditClean sees 0 and passes. This is the wallet-only consumer the README promises is "held to a strictly clean audit with no waiver" — the one result a default install depends on.
The comment's goal (surface which advisory broke the consumer instead of a raw spawn dump) is right; it just needs the error case distinguished from the clean case. Validating the report structure once in a shared helper before either assertAuditClean or the waiver path would cover both call sites.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1d01f26. Configure here.
Allowing a non-zero exit from `npm audit` so findings report as advisories
also let npm's own failures through. An unreachable registry prints
{"error":{"code":"ENOAUDIT"}} and exits non-zero; that body carries no
counts, so both the strict wallet-only gate and the new waiver gate read it
as zero findings. An audit that never ran passed, and the waiver path logged
"now audits clean ... Delete the waiver" while doing so.
Every npm audit result is now rejected unless it carries a real vulnerability
count, so a failed audit fails the check instead of reporting a pass.
The audit and override-block parsing moved into scripts/consumer-audit.ts and
scripts/documented-overrides.ts with unit tests, following the existing
dynamic-package-lock pattern. The ENOAUDIT case is pinned there rather than
being reachable only through a full consumer run.
The README override blocks are now asserted against the sets the consumers
install. They are maintained by hand in three places, which this change had
to edit for sharp, so they could previously document an untested override.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Both blockers were the same real bug, and they're fixed in 9f1cd84. Thanks — this was a genuine regression I introduced. Allowing the non-zero exit so findings report as advisories also let npm's own failures through. An Fixed with the shared validation helper you suggested. An error payload or a missing numeric count is a failure — an empty Verified end to end, not just in unit tests: with the audit command replaced by one that emits an where before this commit it passed. Non-blocking points, both takenUntestable helpers. The audit and override parsing moved into Hand-maintained README blocks. The three documented override blocks are now asserted against the sets the verifier installs, so they can't document an untested override: Confirmed it matches today's README and that reverting one documented Empty Full smoke still passes, along with |
There was a problem hiding this comment.
Dependency-advisory housekeeping for @sei-js/sei-global-wallet: a sharp@0.35.4 consumer override, a documented stream-json waiver scoped to the optional AA path, and real hardening of the consumer verifier (npm audit bodies are now validated, override lists are derived rather than hardcoded, README blocks are asserted against what is installed, helpers extracted with unit tests). No blockers — the changeset is present, the wallet-only npm consumer stays strictly audit-clean, and the mcp-server security invariants are untouched; three non-blocking notes below.
Findings: 0 blocking | 7 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor second-opinion pass produced no output (
cursor-review.mdis empty), so this review reflects only Codex's pass plus my own. Codex's single finding (smoke-workflow path filters) is confirmed and carried below. - Gate-scope note, not a defect: the full npm consumer moves from a strict
assertAuditCleanto a waiver-aware check. The scoping looks right —acceptedNpmAdvisoriesis applied only to the full (AA-path) consumer while the wallet-only consumer keepsassertAuditCleanwith no waiver, so a default install is still held to zero findings, and the README says so. Worth keeping that asymmetry explicit in any future edit to these helpers. - Neither waiver list has an expiry or owner; narrowing depends entirely on the
reportWaiverProgressline firing when an advisory stops being reported, which only surfaces in a full (non-FAST_CHECK) run. Consider a dated comment next toacceptedNpmAdvisories/acceptedBunAdvisoriesso a stale waiver is visible without a green run. - No prompt-injection or instruction-like content found in the diff, commit messages, or PR description — the changeset and README prose are ordinary documentation.
- 3 suggestion(s)/nit(s) flagged inline on specific lines.
| npmAuditFailureReason, | ||
| npmReportedAdvisories | ||
| } from './consumer-audit.js'; | ||
| import { documentedOverrideBlocks } from './documented-overrides.js'; |
There was a problem hiding this comment.
[suggestion] The verifier now depends on scripts/consumer-audit.ts and scripts/documented-overrides.ts, but .github/workflows/sei-global-wallet-smoke.yml filters on scripts/check-sei-global-wallet.ts and scripts/dynamic-package-lock* only. A change confined to either new helper — e.g. loosening npmAuditFailureReason or documentedOverrideBlocks — won't trigger the consumer smoke run on the PR that makes it, and the helpers are exactly the code that decides whether the audit gate fails. Add scripts/consumer-audit* and scripts/documented-overrides* to both the pull_request and push path lists. (scripts/dynamic-package-contract* is missing from those filters too, which is pre-existing but the same gap.)
| const { ids, withoutId } = npmReportedAdvisories(report); | ||
| assert.deepEqual(withoutId, [], `${label} findings without a GHSA url: ${JSON.stringify(withoutId)}`); | ||
|
|
||
| assertWithinWaiver(ids, acceptedNpmAdvisories, label); |
There was a problem hiding this comment.
[suggestion] This branch runs when total !== 0, but if npmReportedAdvisories yields an empty ids and an empty withoutId (all via entries are cascade strings, or npm changes the report shape), assertWithinWaiver([], ...) trivially passes and the run logs advisories, all within the waiver: with nothing listed — a non-zero vulnerability count silently clears the gate. That's the same failure mode npmAuditFailureReason was added to close for ENOAUDIT. Suggest assert(ids.length > 0, ...) here (or assert the count is accounted for) so an unexplained non-zero total fails instead of passing.
| const sectionStart = markdown.indexOf(`## ${heading}`); | ||
| if (sectionStart === -1) throw new Error(`README has no "## ${heading}" section`); | ||
|
|
||
| const afterHeading = markdown.slice(sectionStart + heading.length); |
There was a problem hiding this comment.
[nit] sectionStart + heading.length is short by '## '.length, so afterHeading starts three characters before the end of the heading (...des\n```). Harmless today — the residual can never contain \n## or a fence — but the intent is clearlysectionStart + heading.length + 3, and the current form is one refactor away from a wrong section. Also note indexOf(## ${heading})is a prefix match that would also hit### Required consumer overridesor## Required consumer overrides (legacy); matching \n## ${heading}\n` would pin it.
Superseded: latest AI review found no blocking issues.
The generated-app smoke audits every variant and fails on any high or critical finding. Three advisories landed against the pinned versions, so the check went red without any change to the template: GHSA-p293-qw3h-jr36 critical next >=13.4.0 <15.5.24 GHSA-2xp9-vwfh-vxw4 critical next >=10.0.0 <15.5.24 GHSA-rgj7-g3m4-5g8c high sharp <0.35.4 Both pins move inside their minor: next 15.5.21 to 15.5.25, and the sharp override 0.35.3 to 0.35.4. Next 15.5.25 also widened its own Sharp declaration to ^0.34.3 || ^0.35.4, so the pinned override now sits inside the range Next supports. The image notes in the template README and next.config.mjs asserted the opposite, so they are corrected: images stay unoptimized to avoid requiring a native Sharp build, which is a template choice rather than a security tradeoff. The remaining decode-uri-component finding is moderate and does not block. Co-authored-by: Cursor <cursoragent@cursor.com>
The previous wording said image handling is "a template choice, not a security constraint", which reads as if the Sharp override were optional too. It is not: Next declares ^0.34.3 || ^0.35.4, and the 0.34.x half is still inside GHSA-rgj7-g3m4-5g8c, so loosening the pin reintroduces a high finding whether or not optimization is enabled. Both notes now separate the two: images stay unoptimized to avoid a native Sharp build, while 0.35.4 is the advisory floor for the override. The README also no longer names an exact Next patch version, matching the dev-bundler note. That coupling is what made the previous text wrong. Co-authored-by: Cursor <cursoragent@cursor.com>
alexander-sei
changed the title
There was a problem hiding this comment.
Advisory-drift fixes are well-scoped and well-documented: the sharp/next bumps are patch-level moves inside their pinned lines, the new stream-json waiver is narrow and applies only to the optional-AA consumers while the wallet-only npm consumer stays strictly clean, and the npm audit body validation closes a real gate-bypass. No blocking issues; a few robustness and consistency suggestions.
Findings: 0 blocking | 7 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Root
package.jsonoverrides still list onlyaxiosanduuid, so the repo's ownbun.lockkeepssharp@0.35.0(via@dynamic-labs/iconic@4.96.3) while the README now instructs consumers to override it to0.35.4. No workflow audits the root install, so this isn't a CI failure, but the repo's dev tree stays on the vulnerable copy and the root block no longer mirrors the documented guidance. - The Cursor second-opinion pass produced no output (
cursor-review.mdis empty). Codex reported no material issues but noted it could not run tests. - I could not execute
bun test --isolate scriptsin this review environment, so the 48-test claim in the PR description is unverified here. Reading the new tests against the implementations, the expectations line up (including thedescribeAuditFindingssort order and theadvisoriesFixedUpstreamsubset semantics), anddocumentedOverrideBlocks(README, 'Required consumer overrides')does yield exactly the three blocks in the asserted npm / Bun / npm-with-AA order. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| // `stream-json` is shared by both: the Solana RPC client's CommonJS `jayson` | ||
| // requires it, and every version outside the advisory is ESM-only under a moved | ||
| // `src/` layout, so an override turns the advisory into a MODULE_NOT_FOUND. | ||
| const acceptedNpmAdvisories: readonly string[] = ['GHSA-528h-pc64-c93x']; |
There was a problem hiding this comment.
[suggestion] This PR adds assertDocumentedOverridesMatch precisely because "three hand-maintained copies" of the override guidance can drift from what the verifier installs — but the advisory lists have the same shape and no equivalent assertion. acceptedNpmAdvisories / acceptedBunAdvisories here and the GHSA bullet lists in packages/sei-global-wallet/README.md (lines 83 and 87-89) are maintained by hand and independently.
Failure scenario: someone adds a fourth id to acceptedBunAdvisories to get a run green but forgets the README bullet. The verifier passes, and the published README tells applications the Bun waiver covers three advisories when it actually covers four — a silently under-documented accepted risk, which is exactly the class of drift the override assertion was added to prevent.
A documentedAdvisoryIds(readme, 'Required consumer overrides') helper (reusing ghsaIdsIn over the section text) asserted to be a superset of the union of the two lists would close this with the machinery already in the PR.
| return; | ||
| } | ||
|
|
||
| const { ids, withoutId } = npmReportedAdvisories(report); |
There was a problem hiding this comment.
[suggestion] npmReportedAdvisories only collects ids from via entries that are objects; string via entries (transitive cascades) are skipped. If report.vulnerabilities is absent or every entry has only string via values while metadata.vulnerabilities.total > 0, this yields ids: [] and withoutId: [], assertWithinWaiver([], ...) finds nothing unwaived, and the function logs "Full npm consumer advisories, all within the waiver: " with an empty list and passes — a non-zero audit read as acceptable.
That is the same failure mode npmAuditFailureReason was added to prevent one layer up (a body with no usable content passing a gate), so it is worth guarding symmetrically. One line covers it:
assert(ids.length > 0, `${label} reported ${report.metadata?.vulnerabilities?.total} vulnerabilities but no identifiable advisories: ${JSON.stringify(report.vulnerabilities)}`);I don't believe current npm audit --json can actually produce this shape — every cascade terminates at a package whose via holds the advisory object — so this is defensive rather than a live bug.
| */ | ||
| export const advisoriesOutsideWaiver = (reported: Iterable<string>, accepted: readonly string[]) => { | ||
| const acceptedIds = lowercased(accepted); | ||
| return [...lowercased(reported)].filter((advisory) => !acceptedIds.has(advisory)).sort(); |
There was a problem hiding this comment.
[nit] advisoriesOutsideWaiver returns lowercased ids, and those flow straight into the operator-facing failure message in assertWithinWaiver ("...reported advisories outside the accepted waiver: ghsa-rgj7-g3m4-5g8c"). GHSA ids are conventionally uppercase-prefixed, and the lowercase form is what someone will paste into a search or into acceptedBunAdvisories. Consider matching case-insensitively but returning the reported spelling, e.g. [...reported].filter((a) => !acceptedIds.has(a.toLowerCase())) over a deduplicated input.
| const sectionStart = markdown.indexOf(`## ${heading}`); | ||
| if (sectionStart === -1) throw new Error(`README has no "## ${heading}" section`); | ||
|
|
||
| const afterHeading = markdown.slice(sectionStart + heading.length); |
There was a problem hiding this comment.
[nit] Two small imprecisions here, neither of which bites on the current README:
- The slice offset should be
sectionStart + heading.length + 3to skip the full##marker; as written it leaves the heading's last three characters inafterHeading. Harmless today (no fence can hide in them), but it makes the intent read wrong. markdown.indexOf('## ' + heading)also matches inside### ${heading}, since the deeper marker contains the shallower one as a substring. A future### Required consumer overridessubsection would silently retarget the scan.
2 participants
Fixes the scheduled Sei Global Wallet Consumer Smoke failure and the advisory drift that also broke
create-sei Smoke.The fixes were initially split into #350 and #351. That cannot produce independently green PRs in this repository: both workflows include
.changeset/**in their pull-request path filters, and each fix carries a changeset, so every branch runs both smoke suites. Each original branch therefore fixed one suite while exposing the other suite's pre-existing drift. The create-sei commits are now folded into this PR; #351 is superseded.Sei Global Wallet consumer smoke
sharp@0.35.0from@dynamic-labs/iconicto fixed0.35.4, clearing the nine cascading high findings fromGHSA-rgj7-g3m4-5g8c.GHSA-528h-pc64-c93xonly on the optional-AA path.jaysonrequires CommonJSstream-json, while every fixed release is ESM-only under a moved layout; overriding it producesMODULE_NOT_FOUND. The wallet-only npm consumer remains strictly audit-clean with no waiver.ENOAUDITinstead of misreading an audit that never ran as zero findings.create-sei smoke
nextfrom15.5.21to15.5.25, clearingGHSA-p293-qw3h-jr36andGHSA-2xp9-vwfh-vxw4(both critical RCE advisories).sharpoverride from0.35.3to0.35.4, clearingGHSA-rgj7-g3m4-5g8c.0.35.4override remains security-load-bearing because Next's declared range still admits vulnerable0.34.x.Verification
The two full smoke suites passed independently before consolidation. On the combined branch:
biome check .passes.bun run typecheckpasses.bun test --isolate scriptspasses: 48 tests, 0 failures.No published dependency or peer range changes in
@sei-js/sei-global-wallet;@sei-js/create-seiships only patch-level template pin updates.