diff --git a/.changeset/single-write-response-related-access.md b/.changeset/single-write-response-related-access.md new file mode 100644 index 000000000..6e1cdf0b5 --- /dev/null +++ b/.changeset/single-write-response-related-access.md @@ -0,0 +1,35 @@ +--- +"nextly": patch +"create-nextly-app": patch +"@nextlyhq/admin": patch +"@nextlyhq/admin-css": patch +"@nextlyhq/blocks-engine": patch +"@nextlyhq/ui": patch +"@nextlyhq/adapter-drizzle": patch +"@nextlyhq/adapter-postgres": patch +"@nextlyhq/adapter-mysql": patch +"@nextlyhq/adapter-sqlite": patch +"@nextlyhq/storage-s3": patch +"@nextlyhq/storage-uploadthing": patch +"@nextlyhq/storage-vercel-blob": patch +"@nextlyhq/plugin-form-builder": patch +"@nextlyhq/plugin-page-builder": patch +"@nextlyhq/plugin-seo": patch +"@nextlyhq/plugin-sdk": patch +"@nextlyhq/eslint-config": patch +"@nextlyhq/prettier-config": patch +"@nextlyhq/telemetry": patch +"@nextlyhq/tsconfig": patch +--- + +Updating a Single no longer returns related fields the writer is not allowed to read. + +The response expands relationships, and those rows belong to another collection carrying its own field-level `access.read` rules. Read paths have evaluated them since the field-access work landed; this path forwarded no caller, so it returned every related field intact — including ones the same caller's `GET` would withhold. That made the write path a way around the rule: write anything, read the response back. + +A writer supplied a relationship id, not the related row's protected fields, so "they supplied the data" does not cover them. The rule that applies is the target collection's own, and it is now evaluated against the caller that made the write. + +This reaches every hop the response expands, not only the first: a related row's own relationships carry the rules of the collection at the far end, and those are evaluated too. + +Every caller the access gate applies to is judged, including one with no identity — an anonymous write permitted by a public update rule gets the same answer its read would give. Only a trusted write bypasses this, through `overrideAccess` rather than through an absent user. + +One consequence worth knowing if you write `afterUpdate` hooks: they receive the response as the caller will see it, so a related field that caller may not read is already gone. That matches how reads behave — related-row rules are applied while relationships expand, before `afterRead` hooks run — and it is why the two paths now agree. The Single's own fields are still redacted after your hooks, unchanged. diff --git a/packages/nextly/src/domains/singles/__tests__/write-response-related-access.integration.test.ts b/packages/nextly/src/domains/singles/__tests__/write-response-related-access.integration.test.ts new file mode 100644 index 000000000..a5bf21fb2 --- /dev/null +++ b/packages/nextly/src/domains/singles/__tests__/write-response-related-access.integration.test.ts @@ -0,0 +1,213 @@ +/** + * A write response is a read of the rows it expands. + * + * Updating a Single returns the document, relationships expanded. Those related + * rows belong to another collection and carry that collection's own field-level + * `access.read` rules. A read evaluates them; this response did not, because it + * forwarded no caller for them to be evaluated against. + * + * The writer supplied a relationship id, not the related row's protected + * fields, so "they just wrote it" does not cover them. Leaving the response + * unredacted makes the write path a way around the rule: write anything, read + * the response, see what a GET would refuse. + */ + +import { afterEach, describe, expect, it } from "vitest"; + +import { + checkbox, + defineCollection, + defineSingle, + relationship, + text, +} from "../../../config"; +import { + createTestNextly, + type TestNextly, +} from "../../../plugins/test-nextly"; +import type { CollectionsHandler } from "../../../services/collections-handler"; +import type { SingleEntryService } from "../services/single-entry-service"; + +let current: TestNextly | undefined; +afterEach(async () => { + await current?.destroy(); + current = undefined; +}); + +/** A Single pointing at a collection that hides one of its fields. */ +async function boot(): Promise<{ + entry: SingleEntryService; + authorId: string; +}> { + current = await createTestNextly({ + collections: [ + defineCollection({ + slug: "authors", + fields: [ + text({ name: "name" }), + checkbox({ name: "suspended", access: { read: () => false } }), + ], + }), + ], + singles: [ + defineSingle({ + slug: "branding", + fields: [ + text({ name: "siteName" }), + relationship({ name: "author", relationTo: "authors" }), + ], + }), + ], + }); + + const handler = current.getService("collectionsHandler"); + const author = await handler.createEntry( + { collectionName: "authors", overrideAccess: true }, + { name: "Ada", suspended: true } + ); + return { + entry: current.getService("singleEntryService"), + authorId: (author.data as { id: string }).id, + }; +} + +describe("single write response — related-row field access (integration)", () => { + it("withholds a related field the writer may not read", async () => { + const { entry, authorId } = await boot(); + + const result = await entry.update( + "branding", + { siteName: "Acme", author: authorId }, + { user: { id: "writer-1" }, routeAuthorized: true } + ); + + expect(result.success).toBe(true); + const author = (result.data as { author?: Record }) + ?.author; + // The relationship is still expanded — only the protected field is gone. + expect(author).toMatchObject({ name: "Ada" }); + expect(author).not.toHaveProperty("suspended"); + }); + + it("withholds a protected field a hop further out", async () => { + // Nested expansion reaches the related row's own relationships, so the + // rules of the collection at the far end apply too. Threading the caller + // into the first hop is only half the answer if it stops there. + current = await createTestNextly({ + collections: [ + defineCollection({ + slug: "orgs", + fields: [ + text({ name: "name" }), + checkbox({ name: "blocked", access: { read: () => false } }), + ], + }), + defineCollection({ + slug: "authors", + fields: [ + text({ name: "name" }), + relationship({ name: "org", relationTo: "orgs" }), + ], + }), + ], + singles: [ + defineSingle({ + slug: "branding", + fields: [ + text({ name: "siteName" }), + relationship({ name: "author", relationTo: "authors" }), + ], + }), + ], + }); + const handler = + current.getService("collectionsHandler"); + const org = await handler.createEntry( + { collectionName: "orgs", overrideAccess: true }, + { name: "Acme", blocked: true } + ); + const author = await handler.createEntry( + { collectionName: "authors", overrideAccess: true }, + { name: "Ada", org: (org.data as { id: string }).id } + ); + const entry = current.getService("singleEntryService"); + + const result = await entry.update( + "branding", + { siteName: "Acme", author: (author.data as { id: string }).id }, + { user: { id: "writer-1" }, routeAuthorized: true } + ); + + expect(result.success).toBe(true); + const nested = ( + result.data as { author?: { org?: Record } } + )?.author?.org; + expect(nested).toMatchObject({ name: "Acme" }); + expect(nested).not.toHaveProperty("blocked"); + }); + + it("withholds the field from a caller with no identity too", async () => { + // A Single whose update rule admits anyone still returns related rows, and + // an anonymous caller's read strips the protected field — so the write + // response cannot be the one place it survives. Absence of a user is not + // trust; `overrideAccess` is. + current = await createTestNextly({ + collections: [ + defineCollection({ + slug: "authors", + fields: [ + text({ name: "name" }), + checkbox({ name: "suspended", access: { read: () => false } }), + ], + }), + ], + singles: [ + defineSingle({ + slug: "branding", + access: { update: () => true }, + fields: [ + text({ name: "siteName" }), + relationship({ name: "author", relationTo: "authors" }), + ], + }), + ], + }); + const handler = + current.getService("collectionsHandler"); + const author = await handler.createEntry( + { collectionName: "authors", overrideAccess: true }, + { name: "Ada", suspended: true } + ); + const entry = current.getService("singleEntryService"); + + const result = await entry.update( + "branding", + { siteName: "Acme", author: (author.data as { id: string }).id }, + { routeAuthorized: true } + ); + + expect(result.success).toBe(true); + const related = (result.data as { author?: Record }) + ?.author; + expect(related).toMatchObject({ name: "Ada" }); + expect(related).not.toHaveProperty("suspended"); + }); + + it("leaves a trusted write's response whole", async () => { + // The mirror, and the reason enforcement is not simply switched on + // everywhere: a trusted write supplies no caller to judge, and stripping + // there would hide fields from an internal writer that nothing denied. + const { entry, authorId } = await boot(); + + const result = await entry.update( + "branding", + { siteName: "Acme", author: authorId }, + { overrideAccess: true } + ); + + expect(result.success).toBe(true); + const author = (result.data as { author?: Record }) + ?.author; + expect(author).toMatchObject({ name: "Ada", suspended: true }); + }); +}); diff --git a/packages/nextly/src/domains/singles/services/single-mutation-service.ts b/packages/nextly/src/domains/singles/services/single-mutation-service.ts index c48dd7941..4141d06df 100644 --- a/packages/nextly/src/domains/singles/services/single-mutation-service.ts +++ b/packages/nextly/src/domains/singles/services/single-mutation-service.ts @@ -1883,10 +1883,29 @@ export class SingleMutationService extends BaseService { fieldConfigs ); - // 10.6. Expand relationship fields with full related entry data + // 10.6. Expand relationship fields with full related entry data. + // + // The rows this pulls in belong to another collection and carry that + // collection's field rules. A writer supplied a relationship id, not the + // related row's protected fields, so returning them here would answer a + // question the same caller's GET refuses — the write path is not a way + // around the rule. + // + // Enforced for every caller the access gate applies to, which is what the + // read path does. A caller with no identity is judged as one — the same + // answer their read would get — and only a trusted write bypasses it, + // through `overrideAccess` rather than through an absent user. updatedDoc = await this.queryService.expandRelationshipFields( updatedDoc, - fieldConfigs + fieldConfigs, + // The write response has no depth option of its own; expansion applies + // its own default. + undefined, + { + enforceFieldAccess: true, + user: options.user, + overrideAccess: options.overrideAccess, + } ); // 11. Execute afterChange hooks (afterUpdate equivalent for Singles)