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. diff --git a/packages/nextly/src/api/preview-access.ts b/packages/nextly/src/api/preview-access.ts new file mode 100644 index 0000000000..f3a10e7c04 --- /dev/null +++ b/packages/nextly/src/api/preview-access.ts @@ -0,0 +1,149 @@ +/** + * 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 { errorFromServiceEnvelope } from "../errors/from-service-envelope"; +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. + * + * 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(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" } + ); +} + +/** + * 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)) { + 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. + // + // 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. + // + // 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. + // + // `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, + user, + routeAuthorized: true, + 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..9a5b95f8c0 100644 --- a/packages/nextly/src/api/preview-links.test.ts +++ b/packages/nextly/src/api/preview-links.test.ts @@ -14,10 +14,13 @@ vi.mock("./route-auth", () => ({ 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 +29,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 +69,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 +97,71 @@ 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 () => { + // 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: "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( + // 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 () => { + 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 +175,73 @@ 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 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: "reshaped-by-afterRead" }, + }); + + await mintPreviewLink(post({ collection: "pages", entryId: "7" })); + + expect(canUpdateEntry).toHaveBeenCalledWith( + expect.objectContaining({ + collectionName: "pages", + // 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("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 +251,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()