Repository navigation
feat(storage): signed and proxied screenshot URLs - #9
Merged
Merged
Conversation
scrape-dna.test.ts pointed at https://www.theguardian.com/europe, so commit 09e1ded dropped it from test:snips:fork when the fork snips moved onto the local test site. It has not run in CI since, and it was the only fork feature with no runtime guard. Add apps/test-site/public/dna.html: a static page built to feed every DNA module -- 20 :root tokens, a strict 8px spacing grid, h1..h6 plus code/small/ figcaption, two @Keyframes, four @media breakpoints, hover rules, buttons and form controls, twelve uniquely-labelled nav links, and @font-face pointing at the .woff files already in public/fonts so the fonts module sees real loaded faces. Because we own the page, the assertions now pin values (detectedBase is 8, breakpoints are exactly [480,768,1024,1280], --color-primary resolves to #1f6feb) instead of guessing at thresholds a news site happened to clear. Two problems surfaced while wiring this up: - The test asserted dna.url / dna.timestamp / dna.viewport. dedupAndTrim() nests those under _meta, so they were always undefined. The old `if (!shouldRun()) return` guard turned a failed scrape into a silent pass, which is why an assertion that could never hold looked green. The guard is gone: a failed scrape now throws with the status and body. - The suite is gated with describeIf so it skips cleanly without playwright, rather than passing vacuously. Locally: 41 passed across all six fork snips. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Screenshot URLs assumed a world-readable bucket. That assumption came from the
Supabase implementation upstream deleted, which returned
`/storage/v1/object/public/...` from a bucket configured public; when the fork
replaced it with pluggable S3/local storage the assumption came along but the
public bucket did not. On our deployment every screenshot uploaded fine and
every fetch of the returned URL got 403.
SCREENSHOT_STORAGE_URL_MODE now selects how the URL is produced:
public bare object URL (default -- unchanged behaviour)
signed presigned S3 GET; bucket stays private, link expires
(SCREENSHOT_STORAGE_S3_SIGNED_URL_TTL, default 1h, clamped to the
SigV4 maximum of 7 days)
proxy GET /v2/screenshot/:token streams the object back through the API;
bucket stays private and the link does not expire
The proxy token is an HMAC over the object key using the same construction as
upstream's parse-upload refs (base64url payload + signature, timingSafeEqual),
and the route is unauthenticated for the same reason /parse/upload/:uploadId
is: the signature is the credential. It authorises exactly one object.
This is the fork's first deviation from "no new public routes", so it is
documented in CLAUDE.md and pinned by three new invariant checks -- a merge
that rewrites routes/v2.ts would otherwise silently drop it and send us back
to 403s.
Proxy mode applies only to the env-configured provider. A per-request
`storage` block keeps its own URL, because the route resolves storage from env
and would otherwise sign a key for one bucket and read it from another.
Testing: the existing storage snip asserted the URL *looked* right and never
fetched it, which is precisely why this shipped. Both it and the new
scrape-screenshot-url snip now fetch the URL and assert image bytes come back;
the new one also checks that tampered and malformed tokens 404. Fork E2E runs
with env-level MinIO storage in proxy mode, and its bucket now gets
`mc anonymous set download` to match docker-compose.selfhost.yaml.
Locally: 45 passed across all seven fork snips; 51/51 invariants.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The first cut handed out permanent, unauthenticated proxy tokens. Both signing
schemes we already have disagree with that: upstream's ParseUploadRefPayload
carries expiresAt, and our watchword proxy signer carries exp and refuses aged
links. A link that never dies is readable forever once it leaks, and screenshot
URLs travel through logs, webhooks and workflow tools.
The token payload is now {v, key, exp}, with the lifetime from
SCREENSHOT_PROXY_URL_TTL (default 7 days; 0 opts into permanent links
deliberately). An aged-out link gets 410 Gone rather than 404, so a caller can
tell "ask for a fresh scrape" from "this was never valid".
Signature is verified BEFORE expiry -- the reverse of watchword's order, on
purpose. The payload is caller-supplied, so checking expiry first lets anyone
get an "expired" answer out of a token they invented; this way only a genuinely
signed token can reach that branch, which is also why returning 410 leaks
nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed mode shipped with no test at all. Fork E2E only ever set SCREENSHOT_STORAGE_URL_MODE=proxy, so the presigning path -- the one a deployment would pick if it does not want to route image traffic through the API -- was never executed anywhere. Allow `signedUrls` (plus `signedUrlTtlSeconds` and `signingEndpoint`) in the per-request storage.s3 block. That is useful on its own for callers bringing their own bucket, and it lets one server cover both modes: the global env knob and the per-request one hit the same provider code. The tests run against a second MinIO bucket that is deliberately NOT public, because the existing `screenshots` bucket has anonymous download enabled -- against that one a completely broken signer would still return 200 and the test would prove nothing. A control assertion checks the unsigned URL into the private bucket is refused, so the bucket cannot quietly become public and hollow out the suite. Locally: 50 passed across seven fork snips (screenshot-url is now 9). Still untested, and called out rather than papered over: the local provider's fetch() path, which only matters for proxy mode with SCREENSHOT_STORAGE_PROVIDER=local. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts: # CLAUDE.md # apps/api/package.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
Screenshot uploads succeed and the API returns a URL — fetching that URL returns 403.
The code assumes a world-readable bucket. That assumption is inherited: upstream's original implementation uploaded to Supabase and returned
.../storage/v1/object/public/media/...from a bucket configured public. Upstream deleted Supabase; the fork replaced it with pluggable S3/local storage and carried the assumption over, but not the public bucket.Worth stating plainly: there was never a proxy here to lose. The design was always "hand back a public object URL".
What changed
SCREENSHOT_STORAGE_URL_MODEselects how the URL is produced:public(default)signedSCREENSHOT_STORAGE_S3_SIGNED_URL_TTL, default 1h, clamped to the SigV4 max of 7 daysproxyGET /v2/screenshot/:tokenDefault is unchanged, so existing public-bucket setups keep permanent URLs.
The proxy token is an HMAC over the object key using the same construction as upstream's parse-upload refs (base64url payload + signature, compared with
timingSafeEqual), and the route is unauthenticated for the same reason/parse/upload/:uploadIdis: the signature is the credential. It authorises exactly one object and carries no team identity — the same exposure as a presigned URL, minus the expiry.Two things worth reviewing
This is the fork's first new public route.
CLAUDE.mdstates our features ride existing upstream endpoints. Signed mode needs no route; proxy mode does. The deviation is now documented there and pinned by three new invariant checks — otherwise a merge that rewritesroutes/v2.tsdrops it silently and we are back to 403s.Proxy mode applies only to the env-configured provider. The route resolves storage from env, so a per-request
storageblock would have its key signed against one bucket and read from another — a guaranteed 404. Per-request storage keeps its own URL.Testing
The existing storage snip asserted the URL looked right (
startsWith("http"), contains the prefix) and never fetched it. That is exactly why this shipped. Now:scrape-storage.test.tsfetches the per-request MinIO URL and asserts image bytes come back.scrape-screenshot-url.test.ts(new) fetches the proxy URL and asserts image bytes, plus tampered payload, tampered signature and malformed token all return 404.mc anonymous set download, matchingdocker-compose.selfhost.yaml.Locally: 45 passed across all seven fork snips. 51/51 invariants.
Note on the type cast
getSignedUrltakes the client through a cast.@aws-sdk/client-s3and@aws-sdk/s3-request-presignership from the same release (3.1097.0) but resolve to different@smithy/corecopies under pnpm, so theirClientclasses are structurally distinct to tsc. The runtime contract is identical; deduping would mean lockfile-wide@smithyoverrides for a cosmetic clash. Reasoning is in the comment at the call site.🤖 Generated with Claude Code