fix: Audit/supply chain issues#126
Conversation
There was a problem hiding this comment.
Solid supply-chain hardening overall. Verified locally: bun install --frozen-lockfile, scripts/check-pinned-versions.sh, and scripts/ci-supply-chain.sh all pass.
Blocking
1. The workspace:* migration breaks the Docker image build (both Dockerfile and arm-Dockerfile).
Reproduced locally with bun run docker:build:arm:
npm error code EUNSUPPORTEDPROTOCOL
npm error Unsupported URL Type "workspace:": workspace:*
Cause: this PR changed packages/static-wado-webserver/package.json deps from ">=1.7.0" to "workspace:*". The Dockerfile packs each workspace package and installs the tarballs with npm install ./*.tgz. The pack:js script uses npm pack, and npm does not rewrite the workspace: protocol on pack (unlike bun/yarn/pnpm), so workspace:* ends up verbatim inside the packed package.json and npm install rejects it.
Fix that worked (verified — image builds and the server boots and listens): change pack:js from npm pack to bun pm pack in the workspace packages, since bun pm pack resolves workspace:* to the concrete version (e.g. 1.7.6) in the tarball, which npm installs cleanly. This also aligns with the bun migration in this PR. Affected packages:
packages/create-dicomweb/package.json
packages/cs3d/package.json
packages/static-wado-creator/package.json
packages/static-wado-util/package.json
packages/static-wado-webserver/package.json
each: "pack:js": "npm pack --" -> "pack:js": "bun pm pack"
After this change bun run docker:build:arm completes and the container starts (Listening on port 5000, responds to HTTP). This needs to be applied for the PR to be mergeable.
2. Vendored dicer license. LICENSE.dicer.md must include the full upstream MIT license text + copyright notice, not just a pointer to the upstream repo.
3. HeaderParser silently truncates headers at 16 KiB (legacy dicer allowed 80 KiB) with no error emitted on overflow. Emit an error on overflow and add tests for the vendored parser (there are currently none).
Nice-to-have
- Pin GitHub Actions to commit SHAs (
actions/checkout@v4,oven-sh/setup-bun@v2) to match the dependency-pinning theme. - Split the CRLF→LF normalization into its own PR — it inflates this diff to ~6.5k/~6k lines and buries the ~800-line substantive change.
TFRadicalImaging
left a comment
There was a problem hiding this comment.
Re-reviewed after addressing comments. Verified locally (build + full test + Docker).
Blocking items:
- ✅
workspace:*Docker breakage — resolved by reverting workspace deps to>=1.7.6(noworkspace:protocol remains). Verified:bun run docker:build:armbuilds clean with noEUNSUPPORTEDPROTOCOL; container boots (Listening on port 5000) and/dicomweb/studiesreturns 200. - ✅ Vendored dicer license — full upstream MIT text + copyright notice now present in
lib/vendor/dicer/LICENSE, referenced fromLICENSE.dicer.mdwith scope clarification. - ✅
HeaderParseroverflow — now emits anerroron oversized headers (limit 16 KiB → 64 KiB) instead of silently truncating. Dedicated parser tests waived for this PR by reviewer decision.
Verified green: bun install --frozen-lockfile, bun run build (10/10 packages), bun run test (all packages), check-pinned-versions.sh, ci-supply-chain.sh.
Note (non-blocking, out of scope): the non-arm Dockerfile fails locally on canvas native compilation (missing pangocairo dev libs) — this is pre-existing on master (Dockerfile is byte-identical and canvas is already a master dep), not a regression from this PR. Worth tracking separately.
Nice-to-haves not addressed (non-blocking): GH Actions SHA pinning; splitting the CRLF→LF normalization into its own PR.
Approving.
Use pinned versions and run an audit for high/critical issues on commits that change lock files.