Repository navigation
fix: harden sync, storage and pagination against untrusted client input - #47
Open
MoatazNoaman2001 wants to merge 4 commits into
Open
MoatazNoaman2001 wants to merge 4 commits into
MoatazNoaman2001 wants to merge 4 commits into
Conversation
parseClientMessage was a bare JSON.parse, called inside the async
handleMessage with no try/catch. Every Node/Bun transport invokes
handleMessage fire-and-forget (`void ...`), so one invalid frame from
any unauthenticated peer (e.g. "x", or {"type":"ModifyQuerySet"})
became an unhandled rejection and exited the whole process.
- parseClientMessage now shape-checks every client message and throws
ProtocolError for anything malformed.
- handleMessage answers a ProtocolError with FatalError and closes only
that session.
- The concile dev/serve (node ws + Bun) and Vite embed transports catch
any rejection from handleMessage and close the offending connection.
handleServe echoed the uploader-supplied content type back with no nosniff and no Content-Disposition. With the default FS blobstore the bytes are streamed from the app's own origin, so an upload labelled text/html or image/svg+xml ran script with the app's origin (and, in dev, the dashboard's). Streamed files now always carry X-Content-Type-Options: nosniff, and any type outside an inline-safe allowlist (raster images, audio, video, text/plain, PDF) is served as an attachment.
The cursor is raw index-key bytes returned by the client, and paginate used it as the new scan start (asc) or end (desc) without checking it against the query's own interval. A forged cursor could therefore read rows outside an .eq() prefix, e.g. other owners' rows on a by_owner index. The cursor may now only narrow the interval; an out-of-range cursor yields the first page or an empty one.
|
@MoatazNoaman2001 is attempting to deploy a commit to the Dibyajyoti's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
This branch has not been deployed
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.
Summary
Three fixes for paths where untrusted client input reaches the server unchecked. Each one is a separate commit with its own changeset and regression tests.
1. A malformed WebSocket frame crashes the server (
@concile/sync,@concile/cli,@concile/vite)parseClientMessagewas a bareJSON.parse, called inside the asynchandleMessage. The Nodewsand Bun transports inconcile dev/serveand the Vite embed all callhandleMessagefire-and-forget (void ...). A single invalid frame from any unauthenticated peer therefore became an unhandled rejection, which exits a Node process. Examples:"x", or{"type":"ModifyQuerySet"}, which throws onfor (const q of msg.add).parseClientMessagenow shape-checks every client message. It only checks fields a handler dereferences unconditionally;argsandeventpayloads stay opaque. Anything malformed throwsProtocolError.handleMessageanswers aProtocolErrorwithFatalErrorand closes only that session. An unknown session still rejects, as before.handleMessageand closes the offending connection. A future handler bug therefore costs one socket, not the process.2. Stored XSS via uploaded files (
@concile/storage)handleServeechoed the uploader-supplied content type back with nonosniffand noContent-Disposition. With the default FS blobstore,publicUrlandsignGetUrlboth return null, so the bytes are streamed from the app's own origin. A file uploaded astext/htmlorimage/svg+xmlthen ran script with the app's origin, and in dev with the dashboard's origin too.Streamed files now always carry
X-Content-Type-Options: nosniff. Any type outside an inline-safe allowlist (raster images, audio/*, video/*,text/plain, PDF) is served withContent-Disposition: attachment, and so is a missing type.fetch(),<img>and<video>ignoreContent-Disposition, so programmatic use is unchanged.3. A forged pagination cursor reads outside the query's range (
@concile/query-engine)The cursor is raw index-key bytes handed back by the client.
paginateused it directly as the scan start (asc) or end (desc), without checking it against the query's own interval. A client could therefore send a cursor below or above an.eq()prefix. For example,.withIndex("by_owner", q => q.eq("owner", me)).paginate(opts)with cursorAA==returned rows belonging to other owners.The cursor may now only narrow the planned interval. An out-of-range cursor yields the first page or an empty page, and genuine cursors behave exactly as before.
Tests
packages/sync/test/malformed-frame.test.ts: 21 malformed frames each produceFatalErrorand a close for that session only, the other sessions keep working, and every well-formed client shape still parses.packages/storage/test/http.test.ts: active types (text/html, SVG, XHTML, case and parameter variants), a missing type, inline-safe types, and Range responses.packages/query-engine/test/paginate-cursor.test.ts: forged cursors before and after the range in both orders, a cross-query cursor, and normal paging in both orders.Each new test fails on
mainand passes with its fix. The fullbun run build,bun run typecheckandbun run testpass locally, with one exception:client/test/outbox-fs.test.ts › unopenable journalfails identically on unmodifiedmain. It is a Windows-onlyEISDIRin a filesystem test and unrelated to these changes.After you open it, post this comment on the PR to sign the CLA (required on a first PR):
I have read the CLA Document and I hereby sign the CLA