fix(db-sync): websocket auth errors, semantic API 400s, and sync server hardening by yuxi-liu-wired · Pull Request #13781 · logseq/logseq · GitHub
Skip to content

fix(db-sync): websocket auth errors, semantic API 400s, and sync server hardening - #13781

Open
yuxi-liu-wired wants to merge 9 commits into
logseq:masterfrom
yuxi-liu-wired:fix/db-sync-combined
Open

yuxi-liu-wired wants to merge 9 commits into
logseq:masterfrom
yuxi-liu-wired:fix/db-sync-combined

Conversation

@yuxi-liu-wired

Copy link
Copy Markdown
Contributor

This PR combines #13692, #13772, #13780, which change the same files, into one branch on current master so they no longer conflict with each other. Each part is unchanged except where two of them touched the same lines.


#13692: fix(db-sync): the node server answers a websocket upgrade whose auth fails

A websocket upgrade to the Node sync server whose authentication fails with an error (a malformed bearer token such as x.y.z, or a JWKS fetch failure) is never answered: the socket is neither upgraded, refused nor closed, and stays open until the client gives up. The same failure on an HTTP request gets a 500 (node-server-returns-500-when-auth-claims-rejects-test).

Cause: the server's "upgrade" handler runs access-allowed? (which calls auth/auth-claims) and the readiness check in a promise chain with no catch. A token read as no user is dropped (allowed? is false), but a rejected auth-claims skips every branch, so nothing is written to the socket. Anyone can hold sockets open on the server this way without valid credentials; no graph data is reached.

Fix: the chain gets a catch that logs :db-sync/node-ws-upgrade-failed and answers the upgrade with a 500 (then closes the socket), unless the socket is already gone. reject-ws-upgrade! now writes the reason phrase for its status (it always wrote Conflict, which was right only for its one caller, the 409).

Test: logseq.db-sync.node-server-test/node-server-answers-ws-upgrade-when-auth-claims-rejects-test: with auth-claims rejecting (as the existing HTTP test does), a websocket upgrade to /sync/graph-x gets an HTTP 500. On master it gets no answer and the test fails; with the fix it passes, along with the rest of logseq.db-sync.node-server-test and logseq.db-sync.worker-handler-ws-test (12 tests, 0 failures). Lint as in CI for deps/db-sync and the app.

Found by mutation testing (triage of node/server.cljs).


#13772: fix(db-sync): the semantic API answers 400 to a write the graph does not allow

The semantic API answers a write the graph does not allow with 500 "server error" and a debug stack. Examples: POST /semantic/pages with title a/b ("Page name can't include "/""), POST /semantic/tags with #tag, PATCH or DELETE of a built-in page or block ("Built-in nodes can't be modified", "Built-in nodes can't be deleted").

The outliner rejects such writes by throwing an error meant to notify the user (ex-data :type :notification, the message in :payload). The semantic handlers check some inputs themselves and answer 400, but these errors come from inside the outliner call and reach handle-http's catch-all, which answers 500.

semantic/handle now answers 400 with the error's message for an error of :type :notification, synchronous or from a promise. Every other error still reaches the 500 path.

The routes that insert a block tree (/semantic/capture, /semantic/blocks/:block-id/children, /semantic/block-trees) checked that blocks is a non-empty list but not its blocks: a block whose title is not a string ({"blocks": [{"title": 1}]}, or a child {}) failed an assert, (string? title), answered 500. They now check the whole tree (every block a map with a string title and optional children of the same shape) and answer 400 as for a missing list.

The property routes resolved :property-id to the ident of any entity it names: PUT /semantic/blocks/:block-id/properties/logseq.class%2FTask (a class, not a property) set it as a property and failed with "No validation for property type nil", answered 500. resolve-property-ident now returns an ident only for a property, so such a request answers 400 "property not found" as for a missing one.

PATCH /semantic/properties/:property-id took any title: {"title": {}} failed with "str.indexOf is not a function", answered 500. A title there must now be a non-blank string, as on the other write routes.

A value for a ref property (status, priority, tags, node) that is a number went to d/entity as an entity id. Datascript throws on a number that is not one (-1, 1.5: "Expected number or lookup ref for entity id"), answered 500, on PUT /semantic/blocks/:block-id/properties/:property-id and POST /semantic/block-properties/batch-set. Only a positive integer is now looked up; another number resolves to nothing and answers 400 as for a missing entity.

Test: semantic-write-the-graph-does-not-allow-answers-400-test (a page titled a/b, a tag titled #tag, a capture of a block titled 1, a capture of a child without a title, a class named as the property, a property update with an object as title, a status value of -1 and of 1.5).

Found by a fuzzer that sends random semantic API requests to a server on a full graph.


#13780: fix(db-sync): confine third-party OAuth tokens and harden the sync server

fix(db-sync): confine third-party OAuth tokens and harden the sync server

Security review follow-ups (H1, H3, M1-M4, L1, L2, L4, L5):

  • Third-party OAuth tokens (logseq/* scopes or additional Cognito client
    ids) are rejected on the native surface: /sync, /assets, /graphs, /e2ee,
    the PAT endpoint and the node WebSocket upgrade. The MCP code-mode guard
    checks the resolved URL origin and pathname.
  • Graph deletion requires a manager; /sync//admin/reset is 404 without
    the admin token; /internal/* tails are blocked in both dispatchers.
  • DB_SYNC_ALLOW_UNVERIFIED_JWT_CLAIMS falls back only on the JWKS-
    unreachable error; every other verification failure is a plain 401.
  • The x-graph-id header is gone; the sync path is authoritative.
  • Member removal closes the member's sockets on a new DO-internal route;
    expired tokens close their socket on the next message.
  • The DO routes the path exactly as received (no /sync prefix stripping),
    the node adapter blocks /semantic tails, and graph-e2ee is always set
    explicitly by dispatch.
  • Error responses carry no internal details (debug-*, error-detail).
  • Non-image asset types are served as attachment with nosniff; SVG gets a
    CSP sandbox. Plain-graph asset PUTs verify the declared checksum.
  • The node adapter no longer sends self-hosted users' tokens to
    api.logseq.com unless the issuer is Logseq's own pool.

Note: the auth-claims catch handler returns false instead of nil and
normalizes it, working around a promesa 11 bug: every nil resolution is
one shared promise, and a task that throws during its drain rejects the
other tasks of that drain.

Copy link
Copy Markdown
Contributor

@yuxi-liu-wired

Copy link
Copy Markdown
Contributor Author

@yuxi-liu-wired
yuxi-liu-wired force-pushed the fix/db-sync-combined branch 3 times, most recently from a549b97 to 8bc75ee Compare October 21, 2026 18:05
…rver

Security review follow-ups (H1, H3, M1-M4, L1, L2, L4, L5):

- Third-party OAuth tokens (logseq/* scopes or additional Cognito client
  ids) are rejected on the native surface: /sync, /assets, /graphs, /e2ee,
  the PAT endpoint and the node WebSocket upgrade. The MCP code-mode guard
  checks the resolved URL origin and pathname.
- Graph deletion requires a manager; /sync/<g>/admin/reset is 404 without
  the admin token; /internal/* tails are blocked in both dispatchers.
- DB_SYNC_ALLOW_UNVERIFIED_JWT_CLAIMS falls back only on the JWKS-
  unreachable error; every other verification failure is a plain 401.
- The x-graph-id header is gone; the sync path is authoritative.
- Member removal closes the member's sockets on a new DO-internal route;
  expired tokens close their socket on the next message.
- The DO routes the path exactly as received (no /sync prefix stripping),
  the node adapter blocks /semantic tails, and graph-e2ee is always set
  explicitly by dispatch.
- Error responses carry no internal details (debug-*, error-detail).
- Non-image asset types are served as attachment with nosniff; SVG gets a
  CSP sandbox. Plain-graph asset PUTs verify the declared checksum.
- The node adapter no longer sends self-hosted users' tokens to
  api.logseq.com unless the issuer is Logseq's own pool.

Note: the auth-claims catch handler returns false instead of nil and
normalizes it, working around a promesa 11 bug: every nil resolution is
one shared promise, and a task that throws during its drain rejects the
other tasks of that drain.
… routes wait for the user upsert

Removing a member fetched /internal/close-sockets without a graph id, and
the node server's Durable Object stand-in found the graph only by the
graph-id query parameter, so the removed member's socket stayed open
there. The request now carries graph-id, and the stand-in falls back to
the id from idFromName (the graph id). Index routes again wait for the
user upsert (skipped for personal access tokens, as before), so a new
user is in D1 before invite-by-email looks them up.
…bject at its internal paths

The Durable Object routes the path dispatch forwards to it, unprefixed;
the test now does as the others: /pull and /snapshot/download, and it
checks the download hands the client the public /sync/<graph> stream URL.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants