fix(send): remove GET /api/download/:id and FileStore.get (#1144) - #1162
Merged
aaspinwall merged 3 commits intoAug 26, 2026
Merged
Conversation
The read-side twin of the WebSocket upload path, and the same shape of
problem. The route streamed any object out of storage by id, with no auth
and no ownership check -- its own comment deferred that ("Security for this
route will be addressed in ticket #101"). Its only caller was the filesystem
download branch in the browser, which goes with it here.
FileStore.get() follows: routes/download.ts was its only caller, which is
the other half of #1144. With set() already gone, the untested pair that
issue is about no longer exists to test.
Also removed: the non-bucket branches of getBlob and downloadMultipart, the
by-id form of _download, and the isBucketStorage argument threaded through
Downloader -> getBlob. The flag itself now has no readers anywhere in the
frontend; retiring it is the next change.
Signed downloads are untouched. GET /api/download/:id/signed still returns a
presigned URL and the browser still fetches the bytes itself -- that is the
path every deployment already took.
A route test replaces the route: /api/download/:id 404s, and storage.get is
asserted never to be called. The status alone does not hold the line -- a
reinstated route whose storage.get returns nothing 404s too -- so the suite
would have stayed green with the route back. Verified by putting it back.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 24, 2026
radishmouse
approved these changes
Aug 25, 2026
radishmouse
left a comment
radishmouse
left a comment
Collaborator
There was a problem hiding this comment.
Farewell to another golden oldie
Nothing is left that could use it. The WebSocket upload path and GET /api/download/:id were its only two data paths and both are gone, along with FileStore.set() and .get(); every deployment has been on b2 throughout; and the dev stack and CI moved to MinIO earlier in this series. An unrecognised STORAGE_BACKEND now throws at boot rather than quietly constructing a local store. That is the point of the change: a store with no bucket cannot serve a single upload, because an upload is a presigned PUT from the browser. Failing at startup, naming the variable and the value it got, beats finding out on the first file. Also removed: - IS_USING_BUCKET_STORAGE, and the getStorageType tRPC procedure that published it. ApiConnection no longer fires a query from its constructor to learn something that was constant. - @tweedegolf/sab-adapter-local. Adapters resolve lazily by type key and nothing constructs a LOCAL store now. del() and sizeOf() still go through the abstraction, so the remaining @tweedegolf packages stay -- dropping those is #1140's job, and most of what it was blocked on is now clear. - FS_LOCAL_DIR / FS_LOCAL_BUCKET from .env.sample and from the ci, stage and prod pulumi configs, where FS_LOCAL_BUCKET was set on backends that have read b2 the whole time. This will break existing local checkouts on first boot: a developer .env predating this series says STORAGE_BACKEND=fs, which is now a startup error. The message names the fix and `pnpm setup:local` resolves it. Those same checkouts will trip compare_envs on the removed FS_LOCAL_* keys. Tests cover the four ways to get it wrong -- unset, empty, 'fs', and a typo -- and assert the reported value, so the message stays useful. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ve-download-by-id
aaspinwall
merged commit Aug 26, 2026
b3e6a0b
into
security/44-remove-ws-upload-path
16 checks passed
aaspinwall
added a commit
that referenced
this pull request
Aug 26, 2026
) * fix(send-backend): refuse /api/ws wherever storage is a bucket (#44) Requiring a session closed the anonymous half of private-issue-tracking#44. This closes the rest of it for every deployment. The handler behind /api/ws streams into storage on the server's own credentials under a key it invents. Unlike every route in routes/uploads.ts it runs no checkStorageLimit, and it writes no database row -- that is the separate POST /api/uploads step, which a caller is under no obligation to make. So a session alone still bought unbounded, unaccounted objects in the bucket, invisible to quota accounting and to the UI, and never collected. No client reaches the path where a bucket exists: the browser only uploads over the socket when the backend has none, and pulumi/config.{prod,stage,ci} .yaml all set STORAGE_BACKEND=b2. IS_USING_BUCKET_STORAGE also defaults to true when the variable is unset, so this fails closed. A filesystem dev stack still uploads here and is unaffected. Removing the path and that backend outright is tracked separately; this is the part that does not have to wait for it. 404 rather than 403, because that is the answer the path gives once it is gone -- the new assertion is then unchanged by the removal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(send-frontend): delete the unreachable chat app (#832) apps/chat/ is 13 components and views, plus the two modules that served only it -- lib/messageBus.js and lib/messageSocket.ts. Nothing outside the set imports any of it: no route, no Vite entrypoint, no other component. `vite build` produces the same app without it. It had already rotted where nobody could see. Receive.vue imports `download` from lib/filesync and Send.vue imports `upload`; neither symbol exists there. That went unnoticed because the tree is invisible to the checks: env.d.ts declares '*.vue' as an opaque DefineComponent, typecheck is `tsc --noEmit` rather than vue-tsc, and lint:all is scoped to src/**/*.ts. So the .vue files are neither type-checked nor linted. messageSocket.ts is the exception -- a .ts file, and the last importer of `connectToWebSocketServer` in lib/utils.ts. Removing it here is what lets that helper go with the /api/ws upload path later. The backend halves are already gone: /api/messagebus and wsMsgHandler.ts went in #1153. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci(send): run the dev stack and the default e2e lane against MinIO Both ran on the filesystem storage backend. That backend is on its way out, and nothing else uses it: every deployment sets STORAGE_BACKEND=b2, so the one path CI exercised by default was the one production never takes. This moves dev and the default e2e lane onto a bucket -- MinIO, which needs no account and no credentials, so it still runs on a fork. Purely enabling. The filesystem backend still works and is still selectable; it is simply no longer the default in .env.sample. Removing it, and the code that only it reaches, is separate work that this unblocks. One new setting made it possible. A presigned URL is only valid for the host it was signed for, and the browser is what has to reach that host -- but the backend knows the bucket as `minio:9000`, which means nothing outside the container network. S3_PUBLIC_ENDPOINT names the address to sign with, and FileStore keeps a second client for that purpose. Production leaves it unset and signs exactly as it does today. S3_FORCE_PATH_STYLE rides along, since MinIO at a bare host:port cannot put the bucket in a subdomain. compose.yml gains minio (no longer behind the `test` profile) and a minio-init that creates the bucket and exits, reading the same env_file as the backend so the two cannot name different buckets. For CI the bucket is an overlay, compose.ci.minio.yml, rather than part of compose.ci.yml: the Backblaze lane brings the same base file up and has no use for MinIO. scripts/e2e.sh picks the overlay off STORAGE_BACKEND in the backend .env, so the stack and the configuration cannot disagree. Verified: compose.yml and the CI overlay both resolve; minio comes up healthy without the profile and with CORS set; the minio-init entrypoint creates the bucket, is idempotent on a second run, and exits non-zero when S3_BUCKET_NAME is unset, so the backend's `condition: service_completed_successfully` fails closed. The presigned round-trip suite passes against the running MinIO, and the new signing test fails when the second client is removed. Note for anyone with an existing checkout: .env gains S3_PUBLIC_ENDPOINT and S3_FORCE_PATH_STYLE, so the compare_envs pre-commit hook will ask for them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(send): remove the /api/ws upload path and FileStore.set (#44) The second write path, gone rather than gated. #1153 required a session for the upgrade and the change before this one refused it wherever storage is a bucket; both were holding a door shut on a room nothing uses. Nothing reached it. The browser only took this path when the backend had no bucket, and it no longer has code that can: sendBlob is a presigned PUT and nothing else. Deleted with it: - backend: wsUploadHandler.ts, the wsUploadServer instance in index.ts, and the upgrade branch in ws/setup.ts. FileStore.set() goes too -- wsUploadHandler.ts was its only caller, which is half of #1144. The untested production path is no longer a path. - frontend: _upload in lib/helpers.ts and the three WebSocket helpers in lib/utils.ts that existed only for it, calculateEncryptedSize (only _upload sized anything), the Canceler type, and the non-bucket branch of sendBlob. The last importer of connectToWebSocketServer went with the chat app in the previous change. - nginx: the Upgrade/Connection mapping under /api/ in the SPA image config. Nothing under that prefix is a WebSocket now, and forwarding the upgrade dance would quietly proxy a re-added one. /trpc keeps it, because /trpc/ws needs it. proxy_buffering stays off, for a reason that is now simply "the right default" rather than "download streams a body through here". The pin that replaces the gate: /api/ws is refused with a 404 even with a valid session cookie, asserting on the handler as well as the status so a reinstated route cannot pass by 404ing after the fact. Transitional note: STORAGE_BACKEND=fs is selectable until the change that removes it, and between here and there it cannot serve an upload -- the browser asks for a presigned URL and a filesystem store has no bucket to sign for. No deployment selects it, and the default moved to a bucket in the previous change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(send): address review nits on #1161 - ws/setup.ts: correct the in-code comment to name #1158 (bucket refusal), not the stack neighbor; reflow to <=80 cols - helpers.ts: remove double blank line left where calculateEncryptedSize was - presigned-roundtrip.test.ts: drop stale FileStore.set() reference (removed in this PR); .get() still exists and is kept * fix(send): remove GET /api/download/:id and FileStore.get (#1144) (#1162) * fix(send): remove GET /api/download/:id and FileStore.get (#1144) The read-side twin of the WebSocket upload path, and the same shape of problem. The route streamed any object out of storage by id, with no auth and no ownership check -- its own comment deferred that ("Security for this route will be addressed in ticket #101"). Its only caller was the filesystem download branch in the browser, which goes with it here. FileStore.get() follows: routes/download.ts was its only caller, which is the other half of #1144. With set() already gone, the untested pair that issue is about no longer exists to test. Also removed: the non-bucket branches of getBlob and downloadMultipart, the by-id form of _download, and the isBucketStorage argument threaded through Downloader -> getBlob. The flag itself now has no readers anywhere in the frontend; retiring it is the next change. Signed downloads are untouched. GET /api/download/:id/signed still returns a presigned URL and the browser still fetches the bytes itself -- that is the path every deployment already took. A route test replaces the route: /api/download/:id 404s, and storage.get is asserted never to be called. The status alone does not hold the line -- a reinstated route whose storage.get returns nothing 404s too -- so the suite would have stayed green with the route back. Verified by putting it back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(send): remove the filesystem storage backend (#1163) Nothing is left that could use it. The WebSocket upload path and GET /api/download/:id were its only two data paths and both are gone, along with FileStore.set() and .get(); every deployment has been on b2 throughout; and the dev stack and CI moved to MinIO earlier in this series. An unrecognised STORAGE_BACKEND now throws at boot rather than quietly constructing a local store. That is the point of the change: a store with no bucket cannot serve a single upload, because an upload is a presigned PUT from the browser. Failing at startup, naming the variable and the value it got, beats finding out on the first file. Also removed: - IS_USING_BUCKET_STORAGE, and the getStorageType tRPC procedure that published it. ApiConnection no longer fires a query from its constructor to learn something that was constant. - @tweedegolf/sab-adapter-local. Adapters resolve lazily by type key and nothing constructs a LOCAL store now. del() and sizeOf() still go through the abstraction, so the remaining @tweedegolf packages stay -- dropping those is #1140's job, and most of what it was blocked on is now clear. - FS_LOCAL_DIR / FS_LOCAL_BUCKET from .env.sample and from the ci, stage and prod pulumi configs, where FS_LOCAL_BUCKET was set on backends that have read b2 the whole time. This will break existing local checkouts on first boot: a developer .env predating this series says STORAGE_BACKEND=fs, which is now a startup error. The message names the fix and `pnpm setup:local` resolves it. Those same checkouts will trip compare_envs on the removed FS_LOCAL_* keys. Tests cover the four ways to get it wrong -- unset, empty, 'fs', and a typo -- and assert the reported value, so the message stays useful. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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.
What changed?
Removed
GET /api/download/:idandFileStore.get()— the read-side twin of the WebSocket upload path, and the same shape of problem.The route streamed any object out of storage by id, with no auth and no ownership check. Its own comment deferred that: "Security for this route will be addressed in ticket #101." Its only caller was the filesystem download branch in the browser, which goes with it here.
FileStore.get()follows, becauseroutes/download.tswas its only caller — the other half of #1144. Withset()already gone in #1161, the untested pair that issue is about no longer exists to test.Also removed: the non-bucket branches of
getBlobanddownloadMultipart, the by-id form of_download, and theisBucketStorageargument threaded throughDownloader→getBlob. The flag now has no readers anywhere in the frontend; retiring it is the next PR.Signed downloads are untouched.
GET /api/download/:id/signedstill returns a presigned URL and the browser still fetches the bytes itself — the path every deployment already took.AI disclosure. Written with Claude Code. I set the scope; the agent traced the callers, made the deletions and wrote the route test. I reviewed it.
On the test
A route test replaces the route:
/api/download/:id404s, andstorage.getis asserted never to be called. The status alone does not hold that line — a reinstated route whosestorage.getreturns nothing also answers 404, so the suite would have stayed green with the route back. Verified by putting the route back and watching the assertion fail.Why?
Closes #1144, and removes most of what #1140 is blocked on — though dropping the
@tweedegolfpackages is still that issue's job, sincedel()andsizeOf()still go through the adapter.The route is also the read-side half of the same class of defect as
thunderbird/private-issue-tracking#44: an unauthenticated path into object storage, reachable by anyone who could reach the host, unused by the product.Limitations and Notes
filesync.ts,helpers.tsandstorage/index.ts, so the ordering here is only to keep the stack linear.Applicable Issues
Closes #1144