diff --git a/.changeset/public-form-empty-declaration-refusal.md b/.changeset/public-form-empty-declaration-refusal.md new file mode 100644 index 0000000000..12886b3703 --- /dev/null +++ b/.changeset/public-form-empty-declaration-refusal.md @@ -0,0 +1,55 @@ +--- +"@objectstack/rest": minor +--- + +fix(rest): a public form that declares no fields now REFUSES the submit instead of accepting every key the caller sent (#6920) + +`POST /api/v1/forms/:slug/submit` narrows a visitor's suppliable keys to the +fields the matched FormView's `sections` declare — and the filter read +`allowedFields.size === 0 || allowedFields.has(k)`. For a form with no declared +fields that limb degenerated, and not into "every field of the object": it +accepted **every key the caller sent**, minus the `#3022` server-managed anchors +and the three prototype keys. Measured on the real registered handler, +anonymously, against a `sections: []` form: + + submit accepted = ["email","internal_margin","internal_tier", + "not_even_a_field","status","subject"] + +`not_even_a_field` is not declared on the target object at all. So an anonymous +visitor could set `status`, a workflow stage, an internal tier — anything, on +the one object the form targets. `publicFormGrant` (ADR-0056) keeps the insert +scoped to that object, so this was never a cross-object hole; it was an +unbounded **column** surface on one. The way in is an ordinary authoring +mid-state: the author creates the public form and wires its sections later. + +**What changes.** A form whose sections declare no fields now answers +`400 VALIDATION_ERROR` and inserts nothing. The message names the empty +declaration and gives the author's fix ("wire the fields it collects into the +form's sections"); it names no object, field or slug, because this reply is +readable by anyone on the internet. The three authoring shapes that reach it — +`sections: []`, sections present but declaring no fields, and `sections` omitted +— are treated identically, and the refusal keys off the **declaration**, not the +body, so an empty POST is refused too rather than inserting a blank row. + +`VALIDATION_ERROR` is the standard ADR-0112 catalog's generic validation +failure, and what `HttpStatusErrorCodeMap[400]` already names a bare 400. It is +deliberately not a newly minted `FORM_*` synonym of a condition the catalog +already covers. + +**Why a refusal and not a silent drop.** Dropping the keys would have kept the +`201` and changed no wire status, but it would swallow data the caller believes +it wrote — a visitor is told their support ticket was filed and an empty row is +stored. Loud is also the only answer that reaches the author, who is the one who +can fix it. + +**This is a behaviour change on a shipped success path.** A deployment that +today collects submissions through a section-less public form starts getting +`400`s. That form's read side already publishes nothing (`fields: {}`) since +`#6601`, so it cannot render either — the two planes now enforce the same rule, +"the form declares what it collects", on both. **Fix: declare the fields in the +form's `sections`.** Forms that already declare sections are entirely +unaffected — that path never consulted the removed limb. + +`#3022`'s anchor guarantee is preserved unchanged: `owner_id`, +`organization_id`, `id` and the audit columns remain unsuppliable on this +surface, including when a FormView mis-declares one in a section. diff --git a/content/docs/ui/forms.mdx b/content/docs/ui/forms.mdx index a5da15de35..b6f529c2d9 100644 --- a/content/docs/ui/forms.mdx +++ b/content/docs/ui/forms.mdx @@ -96,6 +96,7 @@ export default defineView({ > Rules: > - The slug in `publicLink` (`contact-us`) becomes the `:slug` segment in the REST URL. > - Anything not in the `sections[].fields[]` whitelist is silently stripped at submit time. Treat the whitelist as the form's authoritative "what the public is allowed to set" list. +> - A form whose sections declare **no** fields collects nothing, so the submit is **refused** (`400 VALIDATION_ERROR`) rather than accepting whatever the caller sent (#6920). Its `GET /forms/:slug` publishes no schema either (#6601) — declare the fields and both planes come alive together. > - Multiple form views per object are fine — only the one(s) with `sharing.allowAnonymous === true` are exposed. ## 2. (Optional) Create the `guest_portal` permission set @@ -223,6 +224,7 @@ Errors: | Status | Code | When | |---|---|---| | `400 INVALID_REQUEST` | missing / blank slug (an empty body is coerced to `{}` and surfaces as `VALIDATION_FAILED` below, not here) | +| `400 VALIDATION_ERROR` | the form's sections declare **no** fields, so it collects nothing — wire the fields and resubmit (#6920) | | `400 VALIDATION_FAILED` | object schema validators fail (`required`, `format`, `length`, …) | | `403 PERMISSION_DENIED` | the resolved profile does not allow create on the target object | | `404 FORM_NOT_FOUND` | slug not registered on any `sharing.allowAnonymous: true` view | diff --git a/packages/rest/src/public-form-routes.test.ts b/packages/rest/src/public-form-routes.test.ts index 2c21590ec8..4356a0f8af 100644 --- a/packages/rest/src/public-form-routes.test.ts +++ b/packages/rest/src/public-form-routes.test.ts @@ -2,11 +2,16 @@ // // [#3022] Anonymous public-form routes (ADR-0056 Option A) — server-managed // anchor enforcement. The submit route must never accept `owner_id` / -// `organization_id` / audit columns from a visitor: not via an explicit -// section declaration, and not via the zero-declared-sections fallback that -// previously merged the raw body wholesale (the insert-forge of #3004, but -// with no credentials at all). The resolve/lookup routes must agree with the -// submit boundary so a form never collects what the submit refuses. +// `organization_id` / audit columns from a visitor, not even via an explicit +// section declaration (the insert-forge of #3004, but with no credentials at +// all). The resolve/lookup routes must agree with the submit boundary so a +// form never collects what the submit refuses. +// +// [#6601 / #6920] And a form that declares NO fields publishes nothing and +// accepts nothing — one rule on both planes. #3022 originally pinned the +// zero-declared-sections FALL-THROUGH as intentional alongside its anchor +// property; maintainer ruling 5229989845 re-judged that half. See the #6920 +// block for what replaced it and what was kept. import { describe, it, expect, vi } from 'vitest'; import { RestServer } from './rest-server'; @@ -121,22 +126,44 @@ describe('POST /forms/:slug/submit — server-managed anchors (#3022)', () => { expect(createData.mock.calls[0][0].data).toEqual({ subject: 'Help', email: 'a@b.c' }); }); - it('zero declared sections: business fields fall through, anchors do NOT', async () => { - // The all-fields fallback previously merged the raw body wholesale — - // the unauthenticated insert-forge of the issue. Business fields keep - // the documented fall-through; every server-managed anchor is excluded. - const { submit, createData } = buildServer([]); + // [#6920] The case that used to live here — 'zero declared sections: + // business fields fall through, anchors do NOT' — asserted the + // `allowedFields.size === 0 ||` fall-through as INTENDED, and showed an + // undeclared `status` being accepted to prove it. Maintainer ruling + // 5229989845 (2026-08-09) re-judged that pin: the fall-through half was a + // wrong invariant (an anonymous visitor could mass-assign the whole object), + // and the section-less form now REFUSES — pinned in the #6920 block below. + // + // #3022's ANCHOR half is untouched and stays pinned, in the stronger of the + // two spellings it ever had: 'declared-field whitelist' above supplies the + // full `FORGED` anchor set to a form that even MIS-DECLARES `owner_id`, and + // the anchors are still dropped. That is the property #3022 bought. Its + // zero-sections spelling is now vacuous — a refused submit inserts nothing, + // so "anchors are not among what was inserted" is true of a set that does + // not exist — which is why it is replaced rather than re-spelled, and why + // the #6920 block pins `createData` was never called at all. The anchor + // property is additionally re-asserted on the accepting path below, where it + // still does work. + + it('anchors alone on a DECLARED form: nothing forgeable survives (#3022, on the accepting path)', async () => { + // The anchor half of #3022, standing on its own: every server-managed key + // and nothing else. The form accepts (it declares `subject`), so this + // exercises the real filter rather than a refusal that trivially inserts + // nothing. + const { submit, createData } = buildServer([{ fields: ['subject'] }]); const res = mockRes(); - await submit.handler( - { params: { slug: 'test' }, body: { subject: 'Help', status: 'open', ...FORGED } } as any, - res, - ); + await submit.handler({ params: { slug: 'test' }, body: { ...FORGED } } as any, res); expect(res.statusCode).toBe(201); - expect(createData.mock.calls[0][0].data).toEqual({ subject: 'Help', status: 'open' }); + expect(createData.mock.calls[0][0].data).toEqual({}); }); it('a __proto__ body key cannot smuggle inherited anchors past the filter', async () => { - const { submit, createData } = buildServer([]); + // [#6920] Re-spelled from `buildServer([])` to a form that DECLARES + // `subject`. The prototype-pollution property was never about zero + // sections — it rode the fall-through only because that was the shortest + // way to get a body through the filter. On the declared path it is a + // stronger pin, because the assignment loop it guards actually runs. + const { submit, createData } = buildServer([{ fields: ['subject'] }]); const res = mockRes(); // JSON.parse produces `__proto__` as an OWN key; a naive `obj[k] = v` // assignment would replace the payload's prototype with this object and @@ -159,6 +186,130 @@ describe('POST /forms/:slug/submit — server-managed anchors (#3022)', () => { }); }); +describe('POST /forms/:slug/submit — a form that declares nothing accepts nothing (#6920)', () => { + // The write-side twin of #6601, ruled by maintainer comment 5229989845. + // + // Before this pin the filter read `allowedFields.size === 0 || allowedFields.has(k)`. + // A form with no declared fields therefore did NOT accept "every field of the + // object" — it accepted every KEY THE CALLER SENT, minus the #3022 anchors and + // the three prototype keys. Measured on the real registered handler at + // `origin/main`, anonymously, with a `sections: []` form: + // + // submit accepted = ["email","internal_margin","internal_tier", + // "not_even_a_field","status","subject"] + // + // `not_even_a_field` is not declared on `ticket` at all, which is what makes + // "unbounded" literal rather than rhetorical: an anonymous visitor could set + // `status`, an internal tier, a formula column — anything, on the one object + // the form targets. A form created before its sections are wired is an + // ordinary authoring mid-state, so nothing exotic was needed to reach it. + // + // Why a REFUSAL and not a silent drop: dropping the keys keeps the `201`, so a + // caller that believes it submitted a support ticket is told it succeeded and + // gets an empty row — the silence AGENTS.md's warn-vs-error rule names. Loud + // is also the only answer that reaches the AUTHOR, who is the one who can fix + // it. And a refusal that still INSERTED (a blank row per visitor) would be a + // worse bug than the one being fixed, so every case here pins `createData`. + + const BUSINESS_KEYS = { + subject: 'x', + email: 'a@b.c', + status: 'closed', + internal_tier: 'strategic', + internal_margin: '999', + not_even_a_field: 'accepted', + }; + + async function submitTo(sections: any[] | undefined, body: any) { + const { submit, createData } = buildServer(sections); + const res = mockRes(); + await submit.handler({ params: { slug: 'test' }, body } as any, res); + return { res, createData }; + } + + it('zero declared sections: the measured accepted set is REFUSED, envelope and all', async () => { + const { res } = await submitTo([], { ...BUSINESS_KEYS, ...FORGED }); + // Both halves of the ADR-0112 envelope. A `toThrow`-shaped assertion would + // not do here: this route never throws on the fall-through — it ANSWERED + // 201 — so only the code+status pair separates "refused" from "accepted", + // and only the code separates it from an unrelated 400 (a blank slug is + // `INVALID_REQUEST`, a failed object validator is `VALIDATION_FAILED`). + expect(res.statusCode).toBe(400); + expect(res.body.code).toBe('VALIDATION_ERROR'); + }); + + it('zero declared sections: NOTHING is written — the refusal does not insert a blank row', async () => { + const { createData } = await submitTo([], { ...BUSINESS_KEYS, ...FORGED }); + expect(createData, 'a refusal must not reach the data plane at all').toHaveBeenCalledTimes(0); + }); + + it('the refusal names the empty declaration and tells the author to wire sections', async () => { + // The wording is contract here (#5240): this is the only signal the AUTHOR + // gets, and "wire your sections" is the entire fix. Asserted on top of + // code+status, never instead of them. + const { res } = await submitTo([], BUSINESS_KEYS); + expect(res.body.error).toContain('declares no fields'); + expect(res.body.error).toContain('sections'); + }); + + it('the refusal leaks no object internals — it is readable by anyone on the internet', async () => { + const { res } = await submitTo([], BUSINESS_KEYS); + const wire = JSON.stringify(res.body); + for (const leak of ['ticket', 'internal_tier', 'internal_margin', 'not_even_a_field', 'owner_id']) { + expect(wire, `${leak} must not appear in an anonymous refusal`).not.toContain(leak); + } + }); + + it('sections that exist but declare no fields are refused too', async () => { + // Same degenerate `allowedFields`, reached by a different authoring shape — + // and the same case the read side already pins ('sections that exist but + // declare no fields publish nothing either'). + const { res, createData } = await submitTo([{ label: 'Details', fields: [] }, { label: 'More' }], BUSINESS_KEYS); + expect(res.statusCode).toBe(400); + expect(res.body.code).toBe('VALIDATION_ERROR'); + expect(createData).toHaveBeenCalledTimes(0); + }); + + it('`sections` omitted entirely is refused too', async () => { + const { res, createData } = await submitTo(undefined, BUSINESS_KEYS); + expect(res.statusCode).toBe(400); + expect(res.body.code).toBe('VALIDATION_ERROR'); + expect(createData).toHaveBeenCalledTimes(0); + }); + + it('the refusal is on the DECLARATION, not on the body: an empty body is refused identically', async () => { + // Gating on "the caller sent undeclared keys" instead would let an empty + // POST through and insert the blank row this whole card is about. The form + // collects nothing, so there is no body it can accept. + const { res, createData } = await submitTo([], {}); + expect(res.statusCode).toBe(400); + expect(res.body.code).toBe('VALIDATION_ERROR'); + expect(createData).toHaveBeenCalledTimes(0); + }); + + it('NO-REGRESSION: a form that declares sections accepts exactly those fields, as it always did', async () => { + // GUARD, not evidence. This assertion held BEFORE the change too — the + // declared path always took `allowedFields.has(k)` and never consulted the + // removed limb. It is here to prove working forms did not break. + const { res, createData } = await submitTo( + [{ fields: ['subject', { field: 'email' }] }], + { ...BUSINESS_KEYS, ...FORGED }, + ); + expect(res.statusCode).toBe(201); + expect(createData).toHaveBeenCalledTimes(1); + expect(createData.mock.calls[0][0].data).toEqual({ subject: 'x', email: 'a@b.c' }); + }); + + it('NO-REGRESSION: a declaring form still accepts an empty body (201, empty row)', async () => { + // GUARD. The refusal must key off the DECLARATION only — a declaring form + // with an empty body behaves exactly as before, letting the object's own + // validators and hooks have the last word rather than this filter. + const { res, createData } = await submitTo([{ fields: ['subject'] }], {}); + expect(res.statusCode).toBe(201); + expect(createData.mock.calls[0][0].data).toEqual({}); + }); +}); + describe('GET /forms/:slug — schema/sections agree with the submit boundary (#3022)', () => { // The zero-sections case used to live here as "the all-fields schema // expansion excludes managed anchors", asserting @@ -193,13 +344,13 @@ describe('GET /forms/:slug — the published schema IS the declared field set (# // form" was simply false there. // // Why the fix is "declare it or it is not published" rather than "publish - // what the submit route accepts": the submit route's accepted set degenerates - // the SAME way for a section-less form (`allowedFields.size === 0 ||`, pinned - // by 'zero declared sections: business fields fall through' above, which - // shows an undeclared `status` being accepted). Aligning the read surface to - // that write surface would have republished exactly the set we are removing. - // The submit-side whitelist is a WRITE control and never was the backstop for - // a READ disclosure. + // what the submit route accepts": when this landed, the submit route's + // accepted set degenerated the SAME way for a section-less form + // (`allowedFields.size === 0 ||`), so aligning the read surface to that write + // surface would have republished exactly the set being removed. The + // submit-side whitelist is a WRITE control and never was the backstop for a + // READ disclosure. #6920 has since closed the write-side twin, so both planes + // enforce the declaration — each on its own, not by deferring to the other. const SENSITIVE = ['internal_margin', 'internal_tier']; diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index c6daae931d..4323b91442 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -7125,12 +7125,14 @@ export class RestServer { // // Do NOT reach for the submit handler as the backstop here. // It enforces a field whitelist on WRITES, which cannot - // bound a READ disclosure — and its own accepted set - // degenerates identically for a section-less form - // (`allowedFields.size === 0 ||` below), so narrowing this - // schema to "what submit would accept" would have - // republished precisely the set being removed. That - // write-side twin is tracked separately in #6920. + // bound a READ disclosure — and when #6601 landed, its own + // accepted set still degenerated identically for a + // section-less form (`allowedFields.size === 0 ||`), so + // narrowing this schema to "what submit would accept" would + // have republished precisely the set being removed. #6920 + // has since closed that write-side twin, so the two planes + // now agree — but they agree by each enforcing the + // declaration itself, NOT by one deferring to the other. let objectSchema: any = null; try { const p = await this.resolveProtocol(environmentId, req); @@ -7277,14 +7279,56 @@ export class RestServer { else if (f?.field) allowedFields.add(f.field); } } + // [#6920] A form that declares NO fields collects nothing, so + // it has nothing to accept — refuse instead of inserting. + // + // The filter below used to read + // `allowedFields.size === 0 || allowedFields.has(k)`, and that + // fall-through was not "accept every field of the object": it + // accepted every KEY THE CALLER SENT, minus the anchors and the + // three prototype keys — measured as + // `["email","internal_margin","internal_tier","not_even_a_field", + // "status","subject"]` on a `sections: []` form, `not_even_a_field` + // not being a field of the object at all. On an ANONYMOUS surface + // that is unbounded mass assignment across the target object, and + // the form created-before-its-sections-are-wired mid-state reaches + // it without anything exotic. + // + // Symmetric with #6601 on the read side of the same pair: declare + // it or it is not published / not accepted, one rule on both planes + // (AGENTS.md "Explicit composition over default magic"). Post-#6601 + // such a form publishes `fields: {}`, so no legitimate client can + // even learn what to send here. + // + // REFUSAL, not a silent discard: dropping the keys would leave the + // `201` intact while swallowing data the caller believes it wrote — + // exactly the silence AGENTS.md's warn-vs-error rule forbids. The + // author's fix is to wire the sections, so the message says that; + // the code is the standard ADR-0112 catalog's generic 400 + // (`HttpStatusErrorCodeMap[400]`), not a minted synonym, and the + // message names no object, field or slug — this reply is readable + // by anyone on the internet. + // + // NOTE the #3022 pin ('zero declared sections: business fields fall + // through, anchors do NOT') asserted this fall-through as intended. + // Re-judged by maintainer ruling 5229989845 (2026-08-09): the + // anchor half is preserved and still pinned; the fall-through half + // was the wrong invariant. + if (allowedFields.size === 0) { + res.status(400).json({ + code: 'VALIDATION_ERROR', + error: 'This form declares no fields, so it cannot accept a submission. ' + + "Wire the fields it collects into the form's sections and publish it again.", + }); + return; + } // [#3022] System-managed anchors (owner_id, organization_id, // audit columns, id, …) are NEVER client-suppliable on this - // anonymous surface — not via an explicit section declaration, - // and not via the zero-declared-sections fallback below (which - // otherwise passes the raw body through and previously let a - // visitor forge record ownership). The SecurityPlugin's - // publicFormGrant branch strips the same set at the data layer, - // so this filter and the engine boundary cannot drift. + // anonymous surface, even when a FormView explicitly declares + // one in a section (the insert-forge of #3004, with no + // credentials at all). The SecurityPlugin's publicFormGrant + // branch strips the same set at the data layer, so this filter + // and the engine boundary cannot drift. const rawBody = (req.body && typeof req.body === 'object') ? req.body : {}; const filteredData: Record = {}; for (const [k, v] of Object.entries(rawBody)) { @@ -7293,7 +7337,7 @@ export class RestServer { // here would REPLACE filteredData's prototype and smuggle // inherited anchors past every own-property check downstream. if (k === '__proto__' || k === 'constructor' || k === 'prototype') continue; - if (allowedFields.size === 0 || allowedFields.has(k)) filteredData[k] = v; + if (allowedFields.has(k)) filteredData[k] = v; } // ADR-0056 (Option A): authorization DERIVED from the declared