Repository navigation
🐛 Only read known query results after 'query' middleware - #760
Draft
alecgibson wants to merge 2 commits into
Draft
alecgibson wants to merge 2 commits into
alecgibson wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Destroying an emitter during a failed re-poll still allows _finishPoll() to schedule continued polling.
1 open finding
What changed in this PR
Ensures query middleware authorizes resubscriptions before known results are read, while documenting ShareDB read-access controls.
Changes:
- Defers known-result reads and stream opening until query authorization succeeds.
- Adds reconnect, failure cleanup, and buffered-op tests.
- Documents required
readSnapshots,op, andquerymiddleware.
| File | Description |
|---|---|
lib/agent.js |
Reorders query subscription setup and cleanup. |
lib/query-emitter.js |
Documents stream buffering before opening. |
test/agent.js |
Tests authorization, cleanup, and buffered ops. |
test/client/query-subscribe.js |
Tests reconnect op catch-up. |
docs/middleware/access-control.md |
Adds access-control guidance. |
docs/middleware/actions.md |
Clarifies read middleware responsibilities. |
docs/middleware/index.md |
Links access-control documentation. |
docs/projections.md |
Clarifies projection security behavior. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
alecgibson
force-pushed
the
query-resubscribe-after-middleware
branch
from
October 13, 2026 16:34
a2d53dc to
69ca2f4
Compare
|
coverage: 97.953% (+0.04%) from 97.916% — query-resubscribe-after-middleware into main |
Fixes #737 At the moment, a query resubscribe (`qs` with `r`) starts reading the known results' snapshots and ops before `'query'` middleware has run, so a rejected query still sends its ops to the client: 1. Reject all queries in `'query'` middleware 2. Send a `qs` with `r: [['fido', 0]]` 3. The client receives `fido`'s ops, then the query error This change only reads the known results once `querySubscribe()` succeeds, before opening the emitter's streams. The streams buffer until then, so an op committed during the reads arrives as a duplicate rather than going missing, as in `_subscribe()`. A failed read or re-poll now also destroys the emitter, which used to stay subscribed after the client had dropped the query. That includes a rejected known snapshot, which now fails the resubscribe with `ERR_SNAPSHOT_READS_REJECTED`, as a fresh subscribe does, instead of replying with an `{error}` entry the client can't ingest. Other teardown races are left for #755. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Refs #737 At the moment, nothing in the docs says that `'readSnapshots'` only sees snapshots. Ops go through `'op'` middleware instead, so an app that only checks reads in `'readSnapshots'` still lets clients fetch a doc's full history, including its `create` data. This change adds an "Access control" middleware page listing which middleware sees which reads, with an example, and links it from the actions, middleware and projections pages. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
alecgibson
force-pushed
the
query-resubscribe-after-middleware
branch
from
October 13, 2026 18:23
69ca2f4 to
e44b624
Compare
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.
Fixes #737
At the moment, a query resubscribe (
qswithr) starts reading the known results' snapshots and ops before'query'middleware has run, so a rejected query still sends its ops to the client:'query'middlewareqswithr: [['fido', 0]]fido's ops, then the query errorThis change only reads the known results once
querySubscribe()succeeds, before opening the emitter's streams. The streams buffer until then, so an op committed during the reads arrives as a duplicate rather than going missing, as in_subscribe().A failed read or re-poll now also destroys the emitter, which used to stay subscribed after the client had dropped the query. That includes a rejected known snapshot, which now fails the resubscribe with
ERR_SNAPSHOT_READS_REJECTED, as a fresh subscribe does, instead of replying with an{error}entry the client can't ingest. Other teardown races are left for #755.🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com