fix(nextly): authorize the draft a preview token hands out - #680
Conversation
Minting read the entry back with an ordinary by-id read, which answers a different question than the token. The token is consumed with draft true and overrideAccess true, so it exposes the working draft; the probe only established the caller could see the published row. Where a collection allows broad reads and restricts updates per row, an editor could mint a working credential for another author's unpublished edits. Enabling draft on that read would not have caught it: the overlay falls back to the published row for a caller who cannot edit rather than denying, so the read succeeds either way. Both questions now go through the handler that serves collection reads and writes, so the verdict is the one the real operation would reach. The read also passes status all, without which a status-enabled collection filters to published only and an entry that has never been published reports as missing, which is the entry an editor most wants to share.
|
@codex please review this PR |
|
Warning Review limit reached
Next review available in: 21 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
@nextlyhq/adapter-drizzle
@nextlyhq/adapter-mysql
@nextlyhq/adapter-postgres
@nextlyhq/adapter-sqlite
@nextlyhq/admin
@nextlyhq/admin-css
@nextlyhq/blocks-engine
@nextlyhq/blocks-react
@nextlyhq/builder
create-nextly-app
nextly
@nextlyhq/plugin-form-builder
@nextlyhq/plugin-page-builder
@nextlyhq/plugin-sdk
@nextlyhq/plugin-seo
@nextlyhq/storage-s3
@nextlyhq/storage-uploadthing
@nextlyhq/storage-vercel-blob
@nextlyhq/ui
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2de31866ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| overrideAccess: false, | ||
| user, | ||
| authenticatedScope: actor, | ||
| status: "all", |
There was a problem hiding this comment.
For a never-published entry, status: "all" is what now lets minting proceed, but a successful getEntry only proves that some redacted representation is readable: the caller may update the row while one or more field-level access.read rules hide sensitive fields. The preview consumer subsequently calls findByID with overrideAccess: true, which bypasses applyFieldReadAccess, so the bearer receives fields the minting caller could not read; the parent implementation returned 404 for these unpublished rows, making this exposure newly reachable. Preserve the caller's field-level projection during preview consumption or refuse to mint unless the full preview payload is authorized.
AGENTS.md reference: packages/nextly/AGENTS.md:L23-L24
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed as a real defect, and deliberately not fixed in this PR — left unresolved so it stays visible.
This is #601's finding 4, which merged unfixed. Your framing is sharper than the original: a successful getEntry proves only that some redacted representation is readable. The bearer's read then runs with overrideAccess: true, which bypasses applyFieldReadAccess, so the token can deliver fields the minting caller could not see. status: "all" widening the lifecycle filter genuinely enlarges the population this applies to, so this PR makes the finding more reachable even though it does not cause it.
Why it is not fixed here. The founder decided the shape: the preview read enforces the minter's field access, rather than refusing to mint. That is not a change to this gate — it is a change to how the bearer's read is performed, in runtime/routing/**, and it must cover four entry points: ContentPage, generateMetadata, generateStaticParams (which calls nextly.find directly and does NOT go through resolveContent), and the working-draft overlay's by-id re-read inside resolveContent.
That last one is not pedantry. #650 shipped a fix whose two tests both asserted through ContentPage, so the static path stayed open and the bug survived a green review. The verification bar for this work is to mutate the fix and confirm four failures, not one.
Landing that inside a PR whose subject is the mint gate would mix a route-layer redaction change into an access-gate fix and make both harder to review. It needs its own PR, and its tests belong beside the routing entry points rather than at the mint.
Recorded with the four-entry-point gate and the #650 precedent in tasks/left-tasks/192-*.md. Also flagged there: this PR's helper inherits the same getEntry limitation behind finding 5 — the by-id path discards a stored custom rule's query constraint while listEntries, countEntries and the relationship service all apply it. Three of four read paths agree; getEntry is the odd one, and that is a query-service fix, not a preview one.
A beforeOperation hook may rewrite the id a read uses, and getEntry resolves it before fetching while canUpdateEntry was handed the raw request id. An editable row mapped to a readable-but-uneditable one passed the read on the second and the edit gate on the first, and the token delivers the second. The edit gate now names the id the read returned, which is the row the bearer's own read will reach through the same hook. routeAuthorized is now true. The mint route already ran the coarse update gate for this collection and that flag skips only that check; the stored owner-only, role-based and custom rules still evaluate against the loaded document, which is what decides the row-level question.
|
@codex please review this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b26249deb6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const read = await collections.getEntry({ | ||
| collectionName: collection, | ||
| entryId, | ||
| depth: 0, | ||
| overrideAccess: false, | ||
| user, |
There was a problem hiding this comment.
When a read beforeOperation hook branches on user, this probe runs the hook as the minting caller and may authorize row B, but the token still signs the original entryId; the public preview path later calls findByID with user undefined and overrideAccess: true, so the same hook can redirect that ID to row C and expose C without its update check ever running. Fresh evidence beyond the earlier fix is that the bearer read does not share the minting user context, so extracting B from read.data.id does not ensure both reads resolve the same row; bind the credential to an authoritative row identity that consumption cannot re-resolve differently.
AGENTS.md reference: AGENTS.md:L201-L204
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and this defeats the fix I just made — recording that plainly rather than claiming partial credit.
b26249deb made the edit gate authorize the id the READ returned, on the reasoning that the bearer's read would run the same hook and reach the same row. Your evidence is that the premise is false: the bearer read runs with user undefined and overrideAccess: true, so a beforeOperation hook branching on user resolves in a different context. The minter's context yields B; the bearer's can yield C. Binding B therefore binds the wrong row, and no amount of care at the mint gate fixes it, because the mint cannot observe the context the bearer will read in.
That means my fix narrowed one gap (raw id vs read id, same context) and left the real one open (minter context vs bearer context). It is strictly better than before and it is not sufficient, and it would be wrong to let the resolved thread above imply otherwise.
This is #601's open finding "Keep hook resolution identical when consuming preview links", which merged unfixed. It belongs there, not here, because the fix is not at the mint at all — it is making the bearer's read resolve hooks the way the authorized read did, which means the preview consumption path carries the minter's identity rather than reading as an anonymous trusted caller.
Leaving unresolved. Recorded in tasks/left-tasks/192-*.md with your evidence, and specifically the part I had wrong: a hook that branches on user makes "the same id" mean different rows in different contexts, so an id is not a safe unit of authorization across a trust boundary. The unit has to be the row, resolved in the context that will actually be served.
One consequence worth flagging for whoever takes it: the same reasoning applies to status and to field redaction, because all three are decided by a context the bearer read does not share. That is three findings with one root — the preview consumption path discards the minter's identity — and fixing them one at a time will keep producing this shape of finding.
| const read = await collections.getEntry({ | ||
| collectionName: collection, | ||
| entryId, | ||
| depth: 0, |
There was a problem hiding this comment.
When the entry contains a relationship to a row the caller cannot read, this depth-0 probe authorizes only the root document, but the standard preview resolver defaults to depth 1 and consumes the token through findByID with overrideAccess: true. Relationship expansion therefore skips the target collection's row-level read rules and can return the related row—including an unpublished one—even though the minting caller would receive only its reference ID; either authorize the same populated graph the bearer receives or prevent trusted relationship expansion for token-backed reads.
AGENTS.md reference: packages/nextly/AGENTS.md:L23-L24
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and deliberately not fixed here — this is #601's finding 6, owned by task 186 and the read-path lane's #671.
Your description matches the finding as filed: the probe reads at depth: 0 so relations stay as reference ids, while the preview resolver defaults to depth 1 and consumes the token through a trusted read, so expansion skips the target collection's row-level rules.
Why it is not fixed in this PR, and it is not scope avoidance. #650 already merged a bound on relationship expansion. Another lane has #671 open specifically to make that bound complete, with the required-field change measured at 35 construction sites across five files. Shipping a third independently-derived bound from a preview PR is how two rules end up disagreeing about the same behaviour — which is the failure #650 itself demonstrates.
Deferred to task 186 by explicit agreement with that lane, and named in #601's thread rather than closed, so it stays visible if 186 stalls.
One thing from this PR that the owner should have. The four-entry-point gate on any such fix is a hard requirement: ContentPage, generateMetadata, generateStaticParams (which calls nextly.find directly and does NOT go through resolveContent), and the working-draft overlay's by-id re-read. #650's two tests both asserted through ContentPage, which is exactly why the static path survived it. Mutate the fix and confirm four failures, not one.
Recorded in tasks/left-tasks/192-*.md alongside the observation that this, the hook-context finding above, and field-level redaction share one root: the preview consumption path discards the minter's identity and reads as an anonymous trusted caller. Each is a different consequence of that single fact, which is worth knowing before anyone fixes them individually.
|
@codex please review this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bff1e2cd21
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The edit gate takes its subject from the returned document, but that document is presentation data: afterRead may reshape the row and remove id. Falling back to the requested id authorized a row that was never read, so a beforeOperation hook mapping A to B plus an afterRead hook dropping id gave read(B) with update(A) while the token delivers B. An unidentifiable row now yields no link. That is the only answer available here that cannot be wrong, since the service does not expose which row it fetched.
|
@codex please review this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04b8aab6c1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const mayEdit = await collections.canUpdateEntry({ | ||
| collectionName: collection, | ||
| entryId: resolvedId, | ||
| user, | ||
| routeAuthorized: true, |
There was a problem hiding this comment.
When a stored custom update rule throws or its metadata lookup fails, CollectionAccessService.checkCollectionAccess returns a 500 failure envelope, but canUpdateEntry collapses every non-null result to false, so this new route reports the outage as a 403 denial. Fresh evidence beyond the routeAuthorized: true fix is that this flag skips only the coarse RBAC check; stored-rule evaluation still runs and can return that 500. Use a probe that preserves the service failure status instead of reducing every outcome to a boolean.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and not fixable at this boundary — leaving unresolved rather than closing it with a workaround.
Verified: canUpdateEntry returns Promise<boolean>, so a 500 envelope from a stored custom update rule and an honest denial arrive here as the same false. This route cannot tell them apart, and any attempt to (a timeout, a probe, a heuristic) would be guessing at a distinction the service already has and discards.
That makes it the same defect class as the read side, which I DID fix on this PR — readVerdict now rebuilds the service's error through errorFromServiceEnvelope so a 429 keeps its status. The asymmetry is not a design choice: getEntry returns an envelope carrying statusCode and code, and canUpdateEntry returns a bare boolean. The read side could preserve failure semantics because the service exposed them; the update side cannot because it does not.
So the fix belongs in collections-handler.ts / collection-mutation-service.ts: canUpdateEntry should return the verdict AND the failure, the way the read path does, rather than collapsing every non-null result to false. That is a service-signature change affecting every caller of that method, not a preview concern, and it should not ride a PR about the mint gate.
Recorded in tasks/left-tasks/192-*.md with the observation that matters for whoever takes it: "denied" and "could not ask" are different answers, and a boolean return type makes it impossible for any caller to keep them apart. The type is the defect, not the call site — which is why fixing it here would only move the guess.
Worth noting the consequence you named is real and user-visible: an outage in a custom update rule currently reports to the operator as a permission denial, which sends them looking at roles rather than at the rule that threw.
| overrideAccess: false, | ||
| user, | ||
| authenticatedScope: actor, | ||
| status: "all", |
There was a problem hiding this comment.
For a status-enabled, never-published entry hidden by a custom read rule that returns a query constraint, status: "all" now makes the row reachable, but CollectionQueryService.getEntry treats that constraint as an allowed verdict and only folds owner-only constraints into its by-ID query. If the caller has broad update access, this probe succeeds and the trusted bearer read exposes the hidden draft; the parent implementation's published-only read returned 404 for the same row. Apply the custom constraint to the selected row, as the list path does, before minting.
AGENTS.md reference: AGENTS.md:L201-L204
Useful? React with 👍 / 👎.
The edit gate took its subject from the document the read returned, first with a fallback to the requested id and then refusing when the id was absent. Both were unsound for the same reason: afterRead may remove id or rewrite it to another row's, so nothing in that document identifies what was fetched, and a rewritten id authorized an editable row while the token still delivers the read one. The subject is the requested id again, which is what the token signs. The gap that remains, a beforeOperation hook resolving that id differently in the bearer's context where user is undefined, cannot be observed at this boundary and is recorded as a known limitation rather than guessed at.
|
@codex please review this PR |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Recovers work that was finished, reviewed and stub-verified on #601 but did not land: #601 merged at
264bda2ebfrom headcacef1816, three commits behind what had been pushed.Verified by content rather than ancestry, per
.claude/rules/verifying-merged-work.md, with a positive control so an unfetched object and a genuine absence could not look alike:mainassertEntryPreviewable@nextlyhq/builderin the changesetIndependently reproduced by the blocks-engine lane.
1. The preview token authorizes a view it never checked
mintPreviewLinkread the entry back with an ordinary by-id read. The token it hands out is consumed withdraft: trueandoverrideAccess: true, so it exposes the working draft — while the probe only established that the caller could see the published row. Where a collection allows broad reads but restricts updates per row, an editor could mint a working bearer credential for another author's unpublished edits.The obvious fix does not work, and that is the substance. Enabling
drafton that read runs the update-capability probe, butFindByIDArgs.draftis documented as "gated by an update-capability probe: a caller who cannot edit the document still gets the published row" — it falls back, it does not deny. The read succeeds either way and the mint proceeds. A test asserting refused-vs-succeeded passes identically with and without that change, so the fix and the defect are indistinguishable at the only granularity such a test observes.So the gate asks the capability question directly instead of inferring it from what a read returned.
2. A never-published entry could not be previewed
The probe had no way to widen the lifecycle filter, so a status-enabled collection filtered to published only and an entry that had never been published reported as missing — precisely the entry an editor wants to share for review.
Architecture
New
api/preview-access.ts, built on the patternapi/versions-access.tsalready establishes: resolvecollectionsHandlerfrom the container, ask it with explicit flags, interpret the result.Two alternatives were considered and rejected. A new Direct API capability method — the Direct API exposes zero capability-query methods today, so this would add a concept to a published package's public surface for a need already solved internally. A container registration for
CollectionAccessService— it has none and a five-argument constructor, and reaching it from a route is the layering leakversions-access.tsexists to avoid.canUpdateEntryalready existed on the handler: "Whether this user may update the entry, without performing the update", documented as routed through the handler "because this is the instance that actually serves collection writes, so a decision taken here is the decision the write would take."routeAuthorized: falseis deliberate and is the flag that fails open if wrong. The mint route authorizedupdateon the COLLECTION, one granularity coarser than this question, so the gate runs in full rather than being told it already ran.versions-access.tspassestruefor its read — correct there, wrong here, because that flag attests what the route actually checked.Read and edit verdicts collapse to one
403. A row hidden by a rule, an id matching nothing, and a readable-but-uneditable entry are different reasons and one answer on purpose: answering them differently would let an unauthorized caller enumerate which entry ids exist.3. The changeset covered 22 of 23 lockstep packages
@nextlyhq/builderjoined the fixed group in #664, seven minutes after #601 merged. Regenerated from.changeset/config.jsonrather than hand-edited.🔴 Nothing else would have caught this.
scripts/release/check-changesets.mjsis deliberately scoped to changesets a branch ADDS or EDITS and never scans the backlog, so a deficient file already onmainis never re-examined. Without this PR,@nextlyhq/builderis version-bumped with no changelog entry on the next release train.Verification
pnpm run check-typesexits 0 on both tsc configs; 19/19 inpreview-links.test.ts.echo <path> | node scripts/release/check-changesets.mjs→ "Checked 1 changeset(s): all cover the group." Run with a positive control: deleting@nextlyhq/builderagain makes it fail naming that exact package. (Invoked bare it reports "no changesets" and exits 0 — it takes paths, it does not discover them.)canUpdateEntrygate fails 4 of 19; flippingrouteAuthorizedtotruefails exactly 1; removingstatus: "all"fails exactly 1, and it is the other fix's test — so the two mechanisms are independent rather than one masking the other. The flag stub was re-run on this branch after the cherry-pick, since the surrounding tree differs.Not included
#601's remaining findings — field-level read restrictions, custom read constraints on the by-id path, and hook resolution parity — are unfixed on
mainand not addressed here. Relationship expansion stays with task 186 / #671.