fix(review): guard contributor screenshot fetches#4976
Conversation
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
JSONbored
force-pushed
the
codex/propose-fix-for-unsafe-screenshot-url-fetch
branch
from
cb0517a to
11978a2
Compare
|
Rebased onto current `main` to resolve the merge conflict (`worker-configuration.d.ts`'s generated header only; regenerated via `npm run cf-typegen` to confirm), which is what was blocking this PR from ever completing review and had it stuck in the auto-repair loop (`agent.sweep.regate.repair_exhausted`, tracked as #5001). While rebasing, found |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4976 +/- ##
=======================================
Coverage 94.14% 94.14%
=======================================
Files 467 467
Lines 39598 39645 +47
Branches 14444 14457 +13
=======================================
+ Hits 37278 37325 +47
Misses 1664 1664
Partials 656 656
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
gittensory-orb
Bot
added
the
gittensor:bug
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-12 01:09:44 UTC
⏸️ Suggested Action - Manual Review
Review summary Concerns raised — review before merging
Review context
Contributor next steps
Signal definitions
[BETA] Chat with GittensoryAsk Gittensory a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://gittensory.aethereal.dev/docs/gittensory-commands 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.
|
…nt-type fetchExternalScreenshotContentBlock (added by the preceding commit) validates the response Content-Type before accepting an image -- a real hardening step, since a contributor-controlled URL could otherwise return anything. screenshot-table-vision-wiring.test.ts's fake image responses never set that header, so every fetch was silently rejected and every finding-producing test regressed to an empty findings array. Add the header to each fake response, and add a regression test for a Location header the URL parser itself rejects (distinct from an absent one), covering the one branch this left uncovered.
JSONbored
force-pushed
the
codex/propose-fix-for-unsafe-screenshot-url-fetch
branch
from
11978a2 to
a9fb634
Compare
…rnalScreenshotContentBlock
The existing "rejects non-image" test always supplied a content-type header
(just the wrong MIME type), so headers.get("content-type") returning null --
and the ?? "" fallback actually firing -- was never exercised. A string-body
Response auto-gets a "text/plain" content-type from the Fetch API itself, so
the new test uses a binary body (Uint8Array) instead, which has no such
auto-assignment.
1 participant
Motivation
fetchShotContentBlock), which followed redirects and buffered entire responses without timeout/size/type checks, enabling SSRF and resource-consumption risks.Description
fetchExternalScreenshotContentBlock(url)insrc/review/visual/capture.tsthat revalidates every redirect hop withisSafeHttpUrl, uses manual redirects, enforces a fetch timeout, enforces a bounded redirect budget, caps buffered response bytes, and validatesContent-Typeagainst an allowlist before producing anAiContentBlock(PNG/JPEG/WebP/GIF). PNG bytes are downscaled for vision like bot-produced shots.fetchExternalScreenshotContentBlockinstead of the trustedfetchShotContentBlockinsrc/queue/processors.tsto ensure untrusted URLs get the stricter guards.test/unit/visual-capture.test.tsthat cover successful public image responses, unsafe redirect targets, oversized responses, non-image responses, safe redirect handling with MIME preservation, inputs that cause early rejection, bounded redirect budget, and fetch failures.worker-configuration.d.ts) to satisfy the repo checks.Testing
npx vitest run test/unit/visual-capture.test.ts -t "fetchExternalScreenshotContentBlock"— passed.npx vitest run test/unit/screenshot-table-vision-wiring.test.ts test/unit/visual-capture.test.ts -t "screenshot-table|fetchExternalScreenshotContentBlock|fetchShotContentBlock"— passed.npm run typecheck -- --pretty false— passed.npm run test:ciwas started but could not complete reliably in this environment (long-suite timeouts and external network/audit failures);npm audit --audit-level=moderatereturned403 Forbiddenfrom the registry so the audit step could not complete here.Codex Task