From 30be11dc32639938276dd47c928a653c5365f2ea Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 21:51:28 +0500 Subject: [PATCH 1/7] fix(nextly): authorize the draft a preview token hands out 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. --- packages/nextly/src/api/preview-access.ts | 124 ++++++++++++++ packages/nextly/src/api/preview-links.test.ts | 152 ++++++++++++------ packages/nextly/src/api/preview-links.ts | 84 +++------- 3 files changed, 245 insertions(+), 115 deletions(-) create mode 100644 packages/nextly/src/api/preview-access.ts diff --git a/packages/nextly/src/api/preview-access.ts b/packages/nextly/src/api/preview-access.ts new file mode 100644 index 0000000000..0caabba669 --- /dev/null +++ b/packages/nextly/src/api/preview-access.ts @@ -0,0 +1,124 @@ +/** + * Access gate for minting a preview link. + * + * A preview token is a bearer credential: whoever holds it reads the entry it + * names, with no session of their own, and the runtime consumes it with + * `draft: true` and `overrideAccess: true`. So minting must authorize the view + * the token will actually hand out, not a weaker one that happens to be easier + * to ask for. + * + * Two questions, because the token's view needs both and they are decided by + * different rules: + * + * 1. **May this caller read this entry at all** — including one that has never + * been published, which is the case previews exist for. + * 2. **May this caller edit this entry** — which is what the draft overlay + * itself requires before surfacing a working draft. + * + * Both are asked through `collectionsHandler`, the instance that actually + * serves collection reads and writes, so a verdict here is the verdict the real + * operation would reach. This mirrors `api/versions-access.ts`, which gates + * version history the same way and for the same reason. + * + * @module api/preview-access + */ + +import type { AuthenticatedScope } from "../auth/authenticated-scope"; +import { getService } from "../di"; +import type { UserContext } from "../domains/singles/types"; +import { NextlyError } from "../errors/nextly-error"; + +/** + * Turn a read outcome into a verdict, keeping "denied" and "could not ask" + * apart. + * + * A 403 or 404 is an answer: this caller does not get this row, and the two are + * deliberately collapsed because telling them apart would reveal which entry ids + * exist. Any other failure is the read itself breaking, and reporting that as an + * ordinary denial would hide an outage behind a permission error. + */ +function readVerdict(success: boolean, statusCode: number): boolean { + if (success) return true; + if (statusCode === 403 || statusCode === 404) return false; + throw NextlyError.internal({ + logContext: { + reason: "preview-mint-probe-failed", + statusCode, + }, + }); +} + +/** + * Confirm the caller may be handed a preview link for this entry. + * + * Throws `forbidden` when they may not, with the same answer for a row hidden by + * a rule, an id that matches nothing, and an entry the caller may read but not + * edit. Those are different reasons and one answer on purpose: a caller who is + * refused a link learns only that they are refused. + * + * @throws {NextlyError} `forbidden` when no link may be minted. + */ +export async function assertEntryPreviewable( + collection: string, + entryId: string, + user: UserContext, + actor?: AuthenticatedScope +): Promise { + const collections = getService("collectionsHandler"); + + // Enforced and as the caller, which is the same evaluation the bearer's read + // will face. `status: "all"` is the part a plain by-id read cannot express: a + // status-enabled collection otherwise filters to published only, so an entry + // that has never been published reports as missing — exactly the entry an + // editor most wants to share for review. + const read = await collections.getEntry({ + collectionName: collection, + entryId, + depth: 0, + overrideAccess: false, + user, + authenticatedScope: actor, + status: "all", + }); + + if (!readVerdict(read.success, read.statusCode)) { + throw NextlyError.forbidden({ + logContext: { + reason: "preview-link-entry-not-visible", + collection, + entryId, + }, + }); + } + + // The token's view is the DRAFT, and the draft overlay surfaces a pending + // working draft only to a caller trusted to edit the document. Reading the + // published row proves nothing about that: where a collection allows broad + // reads but restricts updates per row, a caller can read another author's + // published entry and would otherwise mint a credential exposing that + // author's unpublished edits. + // + // `routeAuthorized: false` deliberately. The mint route authorized `update` + // on the COLLECTION, which is one granularity coarser than this question, so + // the gate is asked to run in full rather than told it already ran. The + // caller's identity and key scope are supplied, so a scoped API key is judged + // on its own grant here as well and re-running costs a verdict, not a + // rejection. + const mayEdit = await collections.canUpdateEntry({ + collectionName: collection, + entryId, + user, + routeAuthorized: false, + authenticatedScope: actor, + }); + + if (!mayEdit) { + throw NextlyError.forbidden({ + logContext: { + reason: "preview-link-draft-not-editable", + collection, + entryId, + }, + }); + } +} diff --git a/packages/nextly/src/api/preview-links.test.ts b/packages/nextly/src/api/preview-links.test.ts index 77e621ee67..f702a4c4b3 100644 --- a/packages/nextly/src/api/preview-links.test.ts +++ b/packages/nextly/src/api/preview-links.test.ts @@ -7,17 +7,18 @@ */ import { beforeEach, describe, expect, it, vi } from "vitest"; -import { NextlyError } from "../errors/nextly-error"; - vi.mock("./route-auth", () => ({ requireRouteCollectionAccess: vi.fn(), requireRoutePermission: vi.fn(), })); -const { findByID } = vi.hoisted(() => ({ findByID: vi.fn() })); +const { getEntry, canUpdateEntry } = vi.hoisted(() => ({ + getEntry: vi.fn(), + canUpdateEntry: vi.fn(), +})); vi.mock("../init", () => ({ - getCachedNextly: vi.fn().mockResolvedValue({ findByID }), + getCachedNextly: vi.fn().mockResolvedValue({}), })); vi.mock("../services/lib/permissions", () => ({ @@ -26,6 +27,7 @@ vi.mock("../services/lib/permissions", () => ({ vi.mock("../di", () => ({ container: { get: vi.fn(), has: vi.fn().mockReturnValue(false) }, + getService: vi.fn(() => ({ getEntry, canUpdateEntry })), })); vi.mock("../lib/env", () => ({ @@ -65,9 +67,15 @@ async function json(response: Response): Promise> { beforeEach(() => { vi.clearAllMocks(); - // The default: the caller can see the entry they named. Tests about the - // entry gate override this. - findByID.mockResolvedValue({ id: "7" }); + // The default: the caller can see the entry they named AND may edit it, so + // the draft the token hands out is one they could already open. Tests about + // the entry gate override whichever half they are about. + getEntry.mockResolvedValue({ + success: true, + statusCode: 200, + data: { id: "7" }, + }); + canUpdateEntry.mockResolvedValue(true); (requireRouteCollectionAccess as ReturnType).mockResolvedValue({ userId: "u1", permissions: [], @@ -87,61 +95,63 @@ beforeEach(() => { }); describe("mintPreviewLink", () => { - it("refuses an entry the caller cannot read when the read THROWS rather than returns null", async () => { - // How the production read actually reports an unreadable row. `findByID` - // returns null only under `disableErrors`; otherwise it throws NOT_FOUND, - // and a row hidden by a row-level rule is reported the same way as an id - // that matches nothing. A mock that resolves null exercises a path the - // caller never takes, and would certify this gate while the throw sailed - // past it into a 404. - findByID.mockRejectedValue( - NextlyError.notFound({ logContext: { collection: "pages" } }) - ); + it("refuses an entry the caller cannot read, even inside a collection it may edit", async () => { + // The collection gate answers a coarser question than the token asks. A + // caller bounded by a row-level rule to their own documents passes it, so + // without a check on the ENTRY they could mint a working credential for + // someone else's draft — a read they cannot perform themselves. + getEntry.mockResolvedValue({ success: false, statusCode: 403 }); const response = await mintPreviewLink( post({ collection: "pages", entryId: "someone-elses-draft" }) ); - // 403, the same answer a visible-but-forbidden row gets. Answering 404 here - // would tell an unauthorized caller which entry ids exist. expect(response.status).toBe(403); + // No token is minted for a refused entry. + const body = await json(response); + expect(body.item).toBeUndefined(); }); - it("lets a genuine failure keep its own status instead of reading as a denial", async () => { - // The neighbouring case, and the reason only NOT_FOUND is translated. - // Collapsing every failure into "not visible" would report a broken - // database as an ordinary permission denial and hide the outage. - findByID.mockRejectedValue(new Error("connection reset")); + it("answers a missing entry the same way as a hidden one", async () => { + // 403 for both, deliberately. Answering 404 for an id that matches nothing + // would let an unauthorized caller enumerate which entry ids exist by + // reading the status code. + getEntry.mockResolvedValue({ success: false, statusCode: 404 }); const response = await mintPreviewLink( - post({ collection: "pages", entryId: "7" }) + post({ collection: "pages", entryId: "no-such-entry" }) ); - // 500, not the 403 an unreadable row gets. The route wraps a thrown error - // rather than rejecting, so the only thing separating an outage from a - // permission denial is the status it lands on. - expect(response.status).toBe(500); + expect(response.status).toBe(403); }); - it("refuses an entry the caller cannot read, even inside a collection it may edit", async () => { - // The collection gate answers a coarser question than the token asks. A - // caller bounded by a row-level rule to their own documents passes it, so - // without a check on the ENTRY they could mint a working credential for - // someone else's draft — a read they cannot perform themselves. - findByID.mockResolvedValue(null); + it("lets a genuine failure keep its own status instead of reading as a denial", async () => { + // The neighbouring case, and the reason only 403/404 are treated as answers. + // Collapsing every failure into "not visible" would report a broken database + // as an ordinary permission denial and hide the outage. + getEntry.mockResolvedValue({ success: false, statusCode: 500 }); const response = await mintPreviewLink( - post({ collection: "pages", entryId: "someone-elses-draft" }) + post({ collection: "pages", entryId: "7" }) ); - expect(response.status).toBe(403); - // And the entry was judged as the CALLER, not as a trusted reader: an - // `overrideAccess: true` probe here would answer the wrong question. - expect(findByID).toHaveBeenCalledWith( + expect(response.status).toBe(500); + }); + + it("judges the entry as the CALLER and keeps a never-published one visible", async () => { + await mintPreviewLink(post({ collection: "pages", entryId: "7" })); + + expect(getEntry).toHaveBeenCalledWith( expect.objectContaining({ - collection: "pages", - id: "someone-elses-draft", + collectionName: "pages", + entryId: "7", + // Enforced, not a trusted read: an `overrideAccess: true` probe would + // answer a different question than the bearer's read will face. overrideAccess: false, + // Without this a status-enabled collection filters to published only, + // so an entry that has never been published reports as missing — which + // is exactly the entry an editor wants to share for review. + status: "all", // The whole identity, not an id: `roles` drives role-based rules and // the claims drive `custom` ones. A probe missing either decides a // different question than the read it stands in for — an @@ -155,23 +165,55 @@ describe("mintPreviewLink", () => { }), }) ); - // No token is minted for a refused entry. + }); + + it("refuses a readable entry the caller may NOT edit", async () => { + // The defect this gate exists for. Where a collection allows broad reads but + // restricts updates per row, the caller passes both the collection gate and + // the read — and the token they would receive is consumed with `draft: true` + // and `overrideAccess: true`, exposing another author's unpublished edits. + // + // 🔴 Enabling `draft` on the read instead would NOT catch this: the overlay + // falls back to the published row for a caller who cannot edit rather than + // denying, so the read succeeds either way and the mint proceeds. + canUpdateEntry.mockResolvedValue(false); + + const response = await mintPreviewLink( + post({ collection: "pages", entryId: "someone-elses-draft" }) + ); + + expect(response.status).toBe(403); const body = await json(response); expect(body.item).toBeUndefined(); }); + it("asks the edit question about the ROW, without claiming the route already did", async () => { + await mintPreviewLink(post({ collection: "pages", entryId: "7" })); + + expect(canUpdateEntry).toHaveBeenCalledWith( + expect.objectContaining({ + collectionName: "pages", + entryId: "7", + // The mint route authorized `update` on the COLLECTION, which is one + // granularity coarser than this question. Passing `true` would tell the + // gate a row-level check had already run when only the coarse one had. + routeAuthorized: false, + }) + ); + }); + it("judges an API key on the key's own grants, not its owner's", async () => { // The leak direction, which is the one a naive test gets backwards. Asserting // that a key is DENIED something it should not have passes against the broken // code too, because the OWNER's grants happen to allow it. What is wrong is - // that the key is still GRANTED something only the owner had — so the probe - // has to carry the key's own scope for the service to judge it on. + // that the key is still GRANTED something only the owner had — so both gates + // have to carry the key's own scope for the services to judge it on. ( requireRouteCollectionAccess as ReturnType ).mockResolvedValue({ userId: "owner-who-can-read", // Update but NOT read. The owner is a super-admin who can read everything; - // without the scope below the probe resolves the OWNER's RBAC and mints. + // without the scope below the gates resolve the OWNER's RBAC and mint. permissions: ["update-pages"], roles: [], authMethod: "api-key", @@ -181,20 +223,24 @@ describe("mintPreviewLink", () => { await mintPreviewLink(post({ collection: "pages", entryId: "7" })); - expect(findByID).toHaveBeenCalledWith( - expect.objectContaining({ - actor: { actorType: "apiKey", permissions: ["update-pages"] }, - }) + const scope = { actorType: "apiKey", permissions: ["update-pages"] }; + expect(getEntry).toHaveBeenCalledWith( + expect.objectContaining({ authenticatedScope: scope }) + ); + // Both gates, not just the read. A scope carried into one and dropped from + // the other judges a single request as two different callers. + expect(canUpdateEntry).toHaveBeenCalledWith( + expect.objectContaining({ authenticatedScope: scope }) ); }); - it("sends no actor for a session caller, so it resolves grants the normal way", async () => { + it("sends no scope for a session caller, so it resolves grants the normal way", async () => { // Not merely absent: an empty scope would read as an API key holding nothing // and deny a legitimate session caller everything. await mintPreviewLink(post({ collection: "pages", entryId: "7" })); - const [call] = findByID.mock.calls; - expect(call[0].actor).toBeUndefined(); + expect(getEntry.mock.calls[0][0].authenticatedScope).toBeUndefined(); + expect(canUpdateEntry.mock.calls[0][0].authenticatedScope).toBeUndefined(); }); it("gates on update for the collection that was named", async () => { diff --git a/packages/nextly/src/api/preview-links.ts b/packages/nextly/src/api/preview-links.ts index c7a401db0b..848f8e56a9 100644 --- a/packages/nextly/src/api/preview-links.ts +++ b/packages/nextly/src/api/preview-links.ts @@ -29,6 +29,7 @@ import { env } from "../lib/env"; import type { GeneralSettingsService } from "../services/general-settings/general-settings-service"; import { resolveRoleSlugs } from "../services/lib/permissions"; +import { assertEntryPreviewable } from "./preview-access"; import { respondMutation } from "./response-shapes"; import { requireRouteCollectionAccess, @@ -60,27 +61,6 @@ async function settingsService(): Promise { return container.get("generalSettingsService"); } -/** - * Run an access-enforced read and report an unreadable entry as `null`. - * - * Two outcomes are deliberately collapsed: a row a row-level rule hides, and an - * id that matches nothing. Both mean the caller gets no link, and answering them - * differently would tell an unauthorized caller which entries exist. - * - * Only a not-found is translated. Any other failure keeps its own error, so a - * broken database is not reported as an ordinary denial. - */ -async function readEntryAsCaller( - read: () => Promise -): Promise { - try { - return await read(); - } catch (error) { - if (NextlyError.isNotFound(error)) return null; - throw error; - } -} - /** * The key preview tokens are signed with. * @@ -124,12 +104,10 @@ export const mintPreviewLink = withErrorHandler(async (req: Request) => { // two diverge, and the coarse answer alone would let a caller bounded to // their own documents mint a working credential for someone else's. // - // So the entry is authorized here too, by reading it back as the caller: - // enforced (`overrideAccess: false`) and with their identity, which is the - // same evaluation the bearer's own read will face. A row this caller cannot - // see yields no link, and an entry that does not exist yields no link - // either, rather than a token for nothing. - const nextly = await getCachedNextly(); + // So the entry is authorized here too, against the gate that serves the real + // read. Booted first because that gate resolves services from the container, + // and on a cold process the permission lookup itself needs them registered. + await getCachedNextly(); const roles = await resolveRoleSlugs(auth); // An API key is authorized on the grants stamped on the KEY, never on its @@ -141,42 +119,24 @@ export const mintPreviewLink = withErrorHandler(async (req: Request) => { auth.authMethod === "api-key" ? { actorType: "apiKey", permissions: auth.permissions } : undefined; - // `findByID` reports an unreadable row by THROWING `NOT_FOUND`, not by - // returning null: null comes back only under `disableErrors`, which would also - // swallow a genuine internal failure and report it here as an ordinary denial. - // So the not-found case is translated and everything else keeps its own status - // — otherwise the throw skips the check below and the caller is told the entry - // does not exist, which is both the wrong answer and a different one from the - // answer a hidden row gets. - const visible = await readEntryAsCaller(() => - nextly.findByID({ - collection, - id: entryId, - depth: 0, - overrideAccess: false, - ...(actor ? { actor } : {}), - // Built the one way a caller is built, so this probe reaches the verdict - // the caller's own read would. Claims matter here specifically: a stored - // `custom` rule that decides on one is absence-tolerant, so a probe that - // dropped them would admit exactly the caller the rule refuses. - user: buildUserContext({ - claims: auth.claims, - id: auth.userId, - name: auth.userName, - email: auth.userEmail, - roles, - }), - }) + // Authorized through the gate that serves collection reads and writes, so the + // verdict here is the verdict the bearer's own read will reach. It asks two + // questions the previous by-id probe could not express: whether the entry is + // visible at all INCLUDING one never published, and whether this caller may + // edit it — which is what the draft overlay requires before surfacing the + // working draft the token hands out. + await assertEntryPreviewable( + collection, + entryId, + buildUserContext({ + claims: auth.claims, + id: auth.userId, + name: auth.userName, + email: auth.userEmail, + roles, + }), + actor ); - if (!visible) { - throw NextlyError.forbidden({ - logContext: { - reason: "preview-link-entry-not-visible", - collection, - entryId, - }, - }); - } const generation = await ( await settingsService() From 89fa7bc667a0efa120bca9d65019fb089634e278 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 21:54:51 +0500 Subject: [PATCH 2/7] chore(release): regenerate the changeset from the fixed group --- .changeset/preview-mint-entry-gate.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.changeset/preview-mint-entry-gate.md b/.changeset/preview-mint-entry-gate.md index 7038b2f020..caec7221c5 100644 --- a/.changeset/preview-mint-entry-gate.md +++ b/.changeset/preview-mint-entry-gate.md @@ -21,6 +21,7 @@ "@nextlyhq/prettier-config": patch "@nextlyhq/telemetry": patch "@nextlyhq/tsconfig": patch +"@nextlyhq/builder": patch --- Minting a preview link now authorizes the entry it names, not just the collection: a caller bounded by a row-level rule can no longer mint a working link for a document they cannot read themselves. From 2de31866ab541ffa3894b721cdc47ce740138118 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 22:02:37 +0500 Subject: [PATCH 3/7] test(nextly): keep the error import the merged tests still use --- packages/nextly/src/api/preview-links.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/nextly/src/api/preview-links.test.ts b/packages/nextly/src/api/preview-links.test.ts index f702a4c4b3..b9bb40ac5c 100644 --- a/packages/nextly/src/api/preview-links.test.ts +++ b/packages/nextly/src/api/preview-links.test.ts @@ -7,6 +7,8 @@ */ import { beforeEach, describe, expect, it, vi } from "vitest"; +import { NextlyError } from "../errors/nextly-error"; + vi.mock("./route-auth", () => ({ requireRouteCollectionAccess: vi.fn(), requireRoutePermission: vi.fn(), From b26249deb60835926923f60f662b3d491813a3b2 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 22:23:14 +0500 Subject: [PATCH 4/7] fix(nextly): authorize the row the preview read settled on 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. --- packages/nextly/src/api/preview-access.ts | 32 ++++++++++++++----- packages/nextly/src/api/preview-links.test.ts | 25 +++++++++++---- 2 files changed, 43 insertions(+), 14 deletions(-) diff --git a/packages/nextly/src/api/preview-access.ts b/packages/nextly/src/api/preview-access.ts index 0caabba669..d48dee669a 100644 --- a/packages/nextly/src/api/preview-access.ts +++ b/packages/nextly/src/api/preview-access.ts @@ -98,17 +98,33 @@ export async function assertEntryPreviewable( // published entry and would otherwise mint a credential exposing that // author's unpublished edits. // - // `routeAuthorized: false` deliberately. The mint route authorized `update` - // on the COLLECTION, which is one granularity coarser than this question, so - // the gate is asked to run in full rather than told it already ran. The - // caller's identity and key scope are supplied, so a scoped API key is judged - // on its own grant here as well and re-running costs a verdict, not a - // rejection. + // The id the READ settled on, not the one the request named. A collection's + // `beforeOperation` hook may rewrite the id, and the read resolves it before + // fetching; the bearer's own read will run that hook and land on the same row. + // Authorizing the raw id would judge a different row than the token delivers — + // editable row A mapped to readable-but-uneditable row B passes read(B) plus + // update(A) while the token hands out B. + const readRow: unknown = read.data; + const resolvedId = + readRow !== null && + typeof readRow === "object" && + "id" in readRow && + (typeof readRow.id === "string" || typeof readRow.id === "number") + ? String(readRow.id) + : entryId; + + // `routeAuthorized: true`: the mint route already ran + // `requireRouteCollectionAccess(req, "update", collection)`, and this flag + // skips ONLY that coarse RBAC/code-access gate. The stored owner-only, + // role-based and custom rules still evaluate against the loaded document with + // the real user, which is the part that answers this question. Passing `false` + // would repeat a lookup the route just performed and, for a scoped key, resolve + // it a second way for no gain. const mayEdit = await collections.canUpdateEntry({ collectionName: collection, - entryId, + entryId: resolvedId, user, - routeAuthorized: false, + routeAuthorized: true, authenticatedScope: actor, }); diff --git a/packages/nextly/src/api/preview-links.test.ts b/packages/nextly/src/api/preview-links.test.ts index b9bb40ac5c..5975fb1ce9 100644 --- a/packages/nextly/src/api/preview-links.test.ts +++ b/packages/nextly/src/api/preview-links.test.ts @@ -189,17 +189,30 @@ describe("mintPreviewLink", () => { expect(body.item).toBeUndefined(); }); - it("asks the edit question about the ROW, without claiming the route already did", async () => { + it("asks the edit question about the row the READ settled on", async () => { + // A `beforeOperation` hook may rewrite the id, and the read resolves it + // before fetching. The bearer's own read runs that same hook and lands on + // the same row, so authorizing the id the request NAMED would judge a + // different row than the token delivers: editable row A mapped to + // readable-but-uneditable row B passes read(B) plus update(A), while the + // token hands out B. + getEntry.mockResolvedValue({ + success: true, + statusCode: 200, + data: { id: "rewritten-by-hook" }, + }); + await mintPreviewLink(post({ collection: "pages", entryId: "7" })); expect(canUpdateEntry).toHaveBeenCalledWith( expect.objectContaining({ collectionName: "pages", - entryId: "7", - // The mint route authorized `update` on the COLLECTION, which is one - // granularity coarser than this question. Passing `true` would tell the - // gate a row-level check had already run when only the coarse one had. - routeAuthorized: false, + entryId: "rewritten-by-hook", + // The route already ran the coarse `update` gate for this collection, + // and this flag skips ONLY that. The stored owner-only/role/custom rules + // still evaluate against the loaded document, which is what decides the + // row-level question. + routeAuthorized: true, }) ); }); From bff1e2cd21264a09a49bcc80e940f7c89ee1f38d Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 22:37:40 +0500 Subject: [PATCH 5/7] fix(nextly): keep the probe read's own failure status --- packages/nextly/src/api/preview-access.ts | 36 ++++++++++++------- packages/nextly/src/api/preview-links.test.ts | 18 +++++++--- 2 files changed, 37 insertions(+), 17 deletions(-) diff --git a/packages/nextly/src/api/preview-access.ts b/packages/nextly/src/api/preview-access.ts index d48dee669a..c17d288ce3 100644 --- a/packages/nextly/src/api/preview-access.ts +++ b/packages/nextly/src/api/preview-access.ts @@ -26,6 +26,7 @@ import type { AuthenticatedScope } from "../auth/authenticated-scope"; import { getService } from "../di"; import type { UserContext } from "../domains/singles/types"; +import { errorFromServiceEnvelope } from "../errors/from-service-envelope"; import { NextlyError } from "../errors/nextly-error"; /** @@ -34,18 +35,29 @@ import { NextlyError } from "../errors/nextly-error"; * * A 403 or 404 is an answer: this caller does not get this row, and the two are * deliberately collapsed because telling them apart would reveal which entry ids - * exist. Any other failure is the read itself breaking, and reporting that as an - * ordinary denial would hide an outage behind a permission error. + * exist. + * + * Anything else is the read failing rather than refusing, and it keeps the + * status the service gave it. Flattening every other outcome to 500 would erase + * what the caller needs to act on — a rate limit's 429 and its retry interval + * become an opaque server error, and a validation failure loses its field + * detail. The shared converter rebuilds the service's own error, which is what + * the Direct API boundary does with the same envelope. */ -function readVerdict(success: boolean, statusCode: number): boolean { - if (success) return true; - if (statusCode === 403 || statusCode === 404) return false; - throw NextlyError.internal({ - logContext: { - reason: "preview-mint-probe-failed", - statusCode, - }, - }); +function readVerdict(read: { + success: boolean; + statusCode: number; + code?: string; + message?: string; + messageKey?: string; + publicData?: unknown; +}): boolean { + if (read.success) return true; + if (read.statusCode === 403 || read.statusCode === 404) return false; + throw errorFromServiceEnvelope( + { ...read, message: read.message ?? "Preview authorization read failed" }, + { reason: "preview-mint-probe-failed" } + ); } /** @@ -81,7 +93,7 @@ export async function assertEntryPreviewable( status: "all", }); - if (!readVerdict(read.success, read.statusCode)) { + if (!readVerdict(read)) { throw NextlyError.forbidden({ logContext: { reason: "preview-link-entry-not-visible", diff --git a/packages/nextly/src/api/preview-links.test.ts b/packages/nextly/src/api/preview-links.test.ts index 5975fb1ce9..5843a17ca4 100644 --- a/packages/nextly/src/api/preview-links.test.ts +++ b/packages/nextly/src/api/preview-links.test.ts @@ -128,16 +128,24 @@ describe("mintPreviewLink", () => { }); it("lets a genuine failure keep its own status instead of reading as a denial", async () => { - // The neighbouring case, and the reason only 403/404 are treated as answers. - // Collapsing every failure into "not visible" would report a broken database - // as an ordinary permission denial and hide the outage. - getEntry.mockResolvedValue({ success: false, statusCode: 500 }); + // 429, deliberately NOT 500. A test using 500 as both the input and the + // expected output cannot tell "the status was preserved" from "every + // failure is flattened to 500" — it passes under both, which is how the + // flattening survived the first version of this test. + getEntry.mockResolvedValue({ + success: false, + statusCode: 429, + code: "RATE_LIMITED", + message: "Too many requests", + }); const response = await mintPreviewLink( post({ collection: "pages", entryId: "7" }) ); - expect(response.status).toBe(500); + // Not 403 (an unreadable row) and not 500 (a flattened one): the caller + // needs the retry semantics the service actually reported. + expect(response.status).toBe(429); }); it("judges the entry as the CALLER and keeps a never-published one visible", async () => { From 04b8aab6c1be806325c6139fb42210cf1ee47807 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 22:52:48 +0500 Subject: [PATCH 6/7] fix(nextly): refuse a preview when the row read cannot be identified 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. --- packages/nextly/src/api/preview-access.ts | 38 ++++++++++++------- packages/nextly/src/api/preview-links.test.ts | 24 ++++++++++++ 2 files changed, 48 insertions(+), 14 deletions(-) diff --git a/packages/nextly/src/api/preview-access.ts b/packages/nextly/src/api/preview-access.ts index c17d288ce3..122567a9db 100644 --- a/packages/nextly/src/api/preview-access.ts +++ b/packages/nextly/src/api/preview-access.ts @@ -110,12 +110,19 @@ export async function assertEntryPreviewable( // published entry and would otherwise mint a credential exposing that // author's unpublished edits. // - // The id the READ settled on, not the one the request named. A collection's - // `beforeOperation` hook may rewrite the id, and the read resolves it before - // fetching; the bearer's own read will run that hook and land on the same row. - // Authorizing the raw id would judge a different row than the token delivers — - // editable row A mapped to readable-but-uneditable row B passes read(B) plus - // update(A) while the token hands out B. + // Which row was actually read, taken from the returned document — and refused + // when it cannot be established. + // + // `read.data` is PRESENTATION data: it has been through `afterRead`, which the + // service explicitly allows to reshape the row, `id` included. So a missing or + // non-scalar `id` does not mean "the request id was used" — it means the row + // that was read is unknown here. + // + // Falling back to the requested id would authorize a row that was never read: + // a `beforeOperation` hook mapping A to B, plus an `afterRead` hook dropping + // `id`, yields read(B) with update(A) while the token delivers B. Refusing is + // the only answer available that cannot be wrong, so an unidentifiable row + // yields no link rather than a link checked against the wrong row. const readRow: unknown = read.data; const resolvedId = readRow !== null && @@ -123,15 +130,18 @@ export async function assertEntryPreviewable( "id" in readRow && (typeof readRow.id === "string" || typeof readRow.id === "number") ? String(readRow.id) - : entryId; + : null; + + if (resolvedId === null) { + throw NextlyError.forbidden({ + logContext: { + reason: "preview-link-row-unidentifiable", + collection, + entryId, + }, + }); + } - // `routeAuthorized: true`: the mint route already ran - // `requireRouteCollectionAccess(req, "update", collection)`, and this flag - // skips ONLY that coarse RBAC/code-access gate. The stored owner-only, - // role-based and custom rules still evaluate against the loaded document with - // the real user, which is the part that answers this question. Passing `false` - // would repeat a lookup the route just performed and, for a scoped key, resolve - // it a second way for no gain. const mayEdit = await collections.canUpdateEntry({ collectionName: collection, entryId: resolvedId, diff --git a/packages/nextly/src/api/preview-links.test.ts b/packages/nextly/src/api/preview-links.test.ts index 5843a17ca4..a1c0cc2f55 100644 --- a/packages/nextly/src/api/preview-links.test.ts +++ b/packages/nextly/src/api/preview-links.test.ts @@ -225,6 +225,30 @@ describe("mintPreviewLink", () => { ); }); + it("refuses when the row that was read cannot be identified", async () => { + // `read.data` is presentation data — it has been through `afterRead`, which + // the service allows to reshape the row, `id` included. A missing `id` does + // NOT mean the request id was used; it means the row that was read is + // unknown here. + // + // Falling back to the requested id would authorize a row that was never + // read: a `beforeOperation` hook mapping A to B plus an `afterRead` hook + // dropping `id` yields read(B) with update(A), while the token delivers B. + getEntry.mockResolvedValue({ + success: true, + statusCode: 200, + data: { title: "afterRead stripped the id" }, + }); + + const response = await mintPreviewLink( + post({ collection: "pages", entryId: "7" }) + ); + + expect(response.status).toBe(403); + // And crucially the edit gate was never asked about the WRONG row. + expect(canUpdateEntry).not.toHaveBeenCalled(); + }); + it("judges an API key on the key's own grants, not its owner's", async () => { // The leak direction, which is the one a naive test gets backwards. Asserting // that a key is DENIED something it should not have passes against the broken From 557239cc329b76df235f2ed3504fcec7c4686ddc Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 23:09:27 +0500 Subject: [PATCH 7/7] fix(nextly): stop deriving the gate's subject from presentation data 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. --- packages/nextly/src/api/preview-access.ts | 47 ++++++---------- packages/nextly/src/api/preview-links.test.ts | 55 ++++++------------- 2 files changed, 35 insertions(+), 67 deletions(-) diff --git a/packages/nextly/src/api/preview-access.ts b/packages/nextly/src/api/preview-access.ts index 122567a9db..f3a10e7c04 100644 --- a/packages/nextly/src/api/preview-access.ts +++ b/packages/nextly/src/api/preview-access.ts @@ -110,41 +110,28 @@ export async function assertEntryPreviewable( // published entry and would otherwise mint a credential exposing that // author's unpublished edits. // - // Which row was actually read, taken from the returned document — and refused - // when it cannot be established. + // The subject is the REQUESTED id, deliberately — not one derived from the + // returned document. `read.data` is presentation data: `afterRead` may remove + // `id` or rewrite it to another row's, so no value in it can be trusted as the + // identity of what was fetched, and deriving the gate's subject from it + // authorized a row the bearer will not receive whenever a hook reshaped that + // field. // - // `read.data` is PRESENTATION data: it has been through `afterRead`, which the - // service explicitly allows to reshape the row, `id` included. So a missing or - // non-scalar `id` does not mean "the request id was used" — it means the row - // that was read is unknown here. + // The token signs the requested id, so that is the id whose editability is + // asserted here. Where a `beforeOperation` hook maps ids, the bearer's read + // resolves in ITS OWN context — `user` undefined, `overrideAccess: true` — + // which this boundary cannot observe. Closing that needs the consumption path + // to carry the minter's identity rather than a better guess here. // - // Falling back to the requested id would authorize a row that was never read: - // a `beforeOperation` hook mapping A to B, plus an `afterRead` hook dropping - // `id`, yields read(B) with update(A) while the token delivers B. Refusing is - // the only answer available that cannot be wrong, so an unidentifiable row - // yields no link rather than a link checked against the wrong row. - const readRow: unknown = read.data; - const resolvedId = - readRow !== null && - typeof readRow === "object" && - "id" in readRow && - (typeof readRow.id === "string" || typeof readRow.id === "number") - ? String(readRow.id) - : null; - - if (resolvedId === null) { - throw NextlyError.forbidden({ - logContext: { - reason: "preview-link-row-unidentifiable", - collection, - entryId, - }, - }); - } + // `routeAuthorized: true`: the mint route already ran + // `requireRouteCollectionAccess(req, "update", collection)`, and this flag + // skips ONLY that coarse RBAC/code-access gate. The stored owner-only, + // role-based and custom rules still evaluate against the loaded document with + // the real user, which is the part that answers this question. const mayEdit = await collections.canUpdateEntry({ collectionName: collection, - entryId: resolvedId, + entryId, user, routeAuthorized: true, authenticatedScope: actor, diff --git a/packages/nextly/src/api/preview-links.test.ts b/packages/nextly/src/api/preview-links.test.ts index a1c0cc2f55..9a5b95f8c0 100644 --- a/packages/nextly/src/api/preview-links.test.ts +++ b/packages/nextly/src/api/preview-links.test.ts @@ -197,17 +197,22 @@ describe("mintPreviewLink", () => { expect(body.item).toBeUndefined(); }); - it("asks the edit question about the row the READ settled on", async () => { - // A `beforeOperation` hook may rewrite the id, and the read resolves it - // before fetching. The bearer's own read runs that same hook and lands on - // the same row, so authorizing the id the request NAMED would judge a - // different row than the token delivers: editable row A mapped to - // readable-but-uneditable row B passes read(B) plus update(A), while the - // token hands out B. + it("asks the edit question about the id the token will sign", async () => { + // Two earlier tests here pinned a derived subject — the id read back from + // the returned document, and a refusal when that id was absent. Both were + // removed deliberately, not lost: `read.data` is presentation data and + // `afterRead` may remove `id` OR rewrite it to another row's, so no value in + // it identifies what was fetched. Deriving the subject from it authorized a + // row the bearer would not receive. + // + // The token signs the requested id, so that is what is asserted editable. + // The remaining gap — a `beforeOperation` hook resolving that id differently + // in the bearer's context, where `user` is undefined — is not closeable at + // this boundary and is tracked as a known limitation. getEntry.mockResolvedValue({ success: true, statusCode: 200, - data: { id: "rewritten-by-hook" }, + data: { id: "reshaped-by-afterRead" }, }); await mintPreviewLink(post({ collection: "pages", entryId: "7" })); @@ -215,40 +220,16 @@ describe("mintPreviewLink", () => { expect(canUpdateEntry).toHaveBeenCalledWith( expect.objectContaining({ collectionName: "pages", - entryId: "rewritten-by-hook", - // The route already ran the coarse `update` gate for this collection, - // and this flag skips ONLY that. The stored owner-only/role/custom rules - // still evaluate against the loaded document, which is what decides the - // row-level question. + // The requested id, NOT the one the returned document carries. + entryId: "7", + // The route already ran the coarse `update` gate for this collection and + // this flag skips only that; stored owner-only/role/custom rules still + // evaluate against the loaded document. routeAuthorized: true, }) ); }); - it("refuses when the row that was read cannot be identified", async () => { - // `read.data` is presentation data — it has been through `afterRead`, which - // the service allows to reshape the row, `id` included. A missing `id` does - // NOT mean the request id was used; it means the row that was read is - // unknown here. - // - // Falling back to the requested id would authorize a row that was never - // read: a `beforeOperation` hook mapping A to B plus an `afterRead` hook - // dropping `id` yields read(B) with update(A), while the token delivers B. - getEntry.mockResolvedValue({ - success: true, - statusCode: 200, - data: { title: "afterRead stripped the id" }, - }); - - const response = await mintPreviewLink( - post({ collection: "pages", entryId: "7" }) - ); - - expect(response.status).toBe(403); - // And crucially the edit gate was never asked about the WRONG row. - expect(canUpdateEntry).not.toHaveBeenCalled(); - }); - it("judges an API key on the key's own grants, not its owner's", async () => { // The leak direction, which is the one a naive test gets backwards. Asserting // that a key is DENIED something it should not have passes against the broken