fix: harden failure bundle artifact downloads#60
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesArtifact Download Hardening
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/bundle.ts`:
- Around line 737-745: streamUrlToFile currently validates only the initial
artifact URL, but fetchImpl can still follow redirects to unsafe destinations.
Update the fetch path in streamUrlToFile to block redirects, either by passing
redirect: 'error' on the fetchImpl call or by revalidating any Location target
before following it, and keep the existing validateArtifactUrl guard as the
initial check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2437fef4-fb8d-4416-a071-8dcfe4884377
📒 Files selected for processing (2)
src/lib/bundle.test.tssrc/lib/bundle.ts
merlinsantiago982-cmd
requested review from
ruili-testsprite and
zeshi-du
as code owners
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/bundle.ts (1)
866-866: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAvoid putting raw presigned artifact URLs in error details.
These errors can be surfaced or persisted by the bundle failure path, so including the full
urlrisks leaking signed query params. Store a redacted origin/path, or omit the URL entirely.🔒 Proposed redaction
- details: { ...err.details, field: 'artifact-url', url }, + details: { ...err.details, field: 'artifact-url', artifactOrigin: artifactUrlOrigin(url) }, }, }); @@ - details: { url, bytes, maxBytes: STREAM_URL_MAX_BYTES }, + details: { artifactOrigin: artifactUrlOrigin(url), bytes, maxBytes: STREAM_URL_MAX_BYTES }, }, }); } + +function artifactUrlOrigin(url: string): string { + try { + return new URL(url).origin; + } catch { + return '<invalid-url>'; + } +}Also applies to: 913-913
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/bundle.ts` at line 866, The bundle failure details currently include the raw presigned artifact URL in the `details` object, which can leak signed query parameters when surfaced or persisted. Update the error handling in `bundle.ts` around the `err.details` construction (including the other affected location) to avoid storing the full `url`; instead keep only a redacted origin/path or remove the URL field entirely while preserving the `artifact-url` context.
🧹 Nitpick comments (1)
src/lib/bundle.test.ts (1)
856-859: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert malformed evidence kind rejects before any fetch.
This validation test can reuse
throwIfFetched; otherwise a future reorder that downloads before validatingkindis less obvious from the failure.🧪 Proposed test tightening
- const okFetch = (async () => - new Response('planted', { status: 200 })) as unknown as typeof globalThis.fetch; await expect( - writeBundle(forged, { dir, failedOnly: false, fetchImpl: okFetch }), + writeBundle(forged, { dir, failedOnly: false, fetchImpl: throwIfFetched }), ).rejects.toMatchObject({🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/bundle.test.ts` around lines 856 - 859, The validation test in writeBundle should fail before any network call when the evidence kind is malformed. Update the test around writeBundle and throwIfFetched so it uses the fetch-guarding helper instead of okFetch, and assert the rejection happens before any fetch is attempted. Keep the focus on the writeBundle path that validates forged.kind first, so a future reorder that fetches early will break this test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/lib/bundle.ts`:
- Line 866: The bundle failure details currently include the raw presigned
artifact URL in the `details` object, which can leak signed query parameters
when surfaced or persisted. Update the error handling in `bundle.ts` around the
`err.details` construction (including the other affected location) to avoid
storing the full `url`; instead keep only a redacted origin/path or remove the
URL field entirely while preserving the `artifact-url` context.
---
Nitpick comments:
In `@src/lib/bundle.test.ts`:
- Around line 856-859: The validation test in writeBundle should fail before any
network call when the evidence kind is malformed. Update the test around
writeBundle and throwIfFetched so it uses the fetch-guarding helper instead of
okFetch, and assert the rejection happens before any fetch is attempted. Keep
the focus on the writeBundle path that validates forged.kind first, so a future
reorder that fetches early will break this test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 63e2bc74-797b-49a7-a0ec-f3c138e6c738
📒 Files selected for processing (2)
src/lib/bundle.test.tssrc/lib/bundle.ts
merlinsantiago982-cmd
force-pushed
the
fix/failure-bundle-download-safety
branch
from
1f3b126 to
113acfe
Compare
1 participant
Summary
fetchImplreceivesredirect: "error"Testing
Related to #59
Summary by CodeRabbit