From 8fa0d6a0d037f3eb519026deaf126d00daabf4b8 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 17 Aug 2026 14:39:08 +0000 Subject: [PATCH] =?UTF-8?q?fix(console):=20PublicFormsPage=20=E7=9A=84=20r?= =?UTF-8?q?edirect=20=E6=8E=88=E6=9D=83=E9=97=A8=E6=94=B9=E9=97=AE?= =?UTF-8?q?=E5=A5=91=E7=BA=A6,=E8=B6=8A=E5=A5=91=E7=BA=A6=E5=80=BC?= =?UTF-8?q?=E5=9C=A8=E4=BF=9D=E5=AD=98=E6=97=B6=E5=88=BB=E8=A2=AB=E6=8B=92?= =?UTF-8?q?=20(#4990)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit objectstack#7496(2026-08-11 裁定,objectstack#7657 落 spec,本仓 17.0.0 GA pin 已覆盖)把 `submitBehavior.url` 裁为仅相对路径,spec 对七族越契约值各给一句 author-facing 处方。PublicFormsPage 的 redirect 分支只执行了其中第一族(非空), 其余六族原样写进 view 元数据 —— 管理员输入 `https://example.com/thanks` 或 `javascript:alert(1)` 得不到任何提示,而这个界面自己刚教过他这个值可以用:字段是 `type="url"`(它自己认的「合法」恰是本键拒绝的绝对 URL),占位符是 `https://example.com/thanks`。 - 保存时刻拒绝,文案来自 spec。判定复用 #4992 落地的同一条 `FormViewSchema` 解析路径:`submitRedirect` 新导出 `checkSubmitRedirectUrl`,消费侧 `resolveSubmitRedirect` 与授权门共用它。绝对 URL、script/data scheme、 协议相对 `//host`、反斜杠、空白与控制字符、畸形 `{{record.field_name}}` token、 文档相对路径、空值各自得到 spec 自己那句处方,显示在字段旁。 规则不在此处重写一份:镜像拷贝在 spec 改动前的每次取值比较里都是绿的,正是 `scripts/check-spec-symbol-derivation.mjs` 要劝退的形状;裁定日后放宽,跟着 pin 走而不需要改这里。 - 本地的 `Redirect URL is required` 去掉。空值本就是七族之一,一并走契约,作者 读到的是「目的地长什么样」而不是「必填」。 - 字段不再教错值:改回纯文本输入,占位符 `/thanks`,并给出规则提示(应用内路径、 `{{record.field_name}}` 插值、故意外跳请用 app navigation item)。 保存的是 schema 接受后回读的值,所以授权门与渲染侧不可能对同一个目的地有两种 意见。`thank-you` 的 `title`/`message` 刻意不校验:spec 里两者就是自由串,没有 可供门陈述的契约。 服务端已测(只读判定,未起服务):`saveMetaItem` 的 spec-conformance gate 走 `getMetadataTypeSchema('view')`(同一个 17.0.0 GA pin),对这些 body 返回 `422 invalid_metadata`,issue 落在 `submitBehavior.url`。所以本卡收口的是错误 路径而非静默入库:纠正现在出现在管理员能改的字段里,而不是一次失败的往返。 Claude-Session: https://claude.ai/code/session_01GTRjn8xBqp75dk7kFupVRt Co-authored-by: Claude --- ...blic-forms-redirect-authoring-door-4990.md | 17 ++ .../src/components/submitRedirect.test.ts | 54 +++- apps/console/src/components/submitRedirect.ts | 104 ++++--- .../PublicFormsPage.redirect.test.tsx | 253 ++++++++++++++++++ .../src/pages/developer/PublicFormsPage.tsx | 85 +++++- 5 files changed, 471 insertions(+), 42 deletions(-) create mode 100644 .changeset/public-forms-redirect-authoring-door-4990.md create mode 100644 apps/console/src/pages/developer/PublicFormsPage.redirect.test.tsx diff --git a/.changeset/public-forms-redirect-authoring-door-4990.md b/.changeset/public-forms-redirect-authoring-door-4990.md new file mode 100644 index 0000000000..430fc578b8 --- /dev/null +++ b/.changeset/public-forms-redirect-authoring-door-4990.md @@ -0,0 +1,17 @@ +--- +"@object-ui/console": patch +--- + +The Public Forms dialog now refuses an out-of-contract `submitBehavior.url` at the moment it is authored, with the contract's own prescription shown next to the field (objectui#4990). + +The redirect branch validated one thing — that the field was not empty — and wrote whatever else was typed into the view metadata. objectstack#7496 rules this key **relative-only** and refuses seven families of value; this door enforced the first. An admin could type `https://example.com/thanks`, or `javascript:alert(1)`, and be told nothing by the surface that had just taught them the value was acceptable — the field was `type="url"`, whose own notion of valid is an absolute URL, under a `https://example.com/thanks` placeholder. + +What changed: + +- **The save is refused, with the spec's sentence.** The verdict comes from `checkSubmitRedirectUrl`, the same `@objectstack/spec` `FormViewSchema` parse the renderer already asks at submit time — now exported from `submitRedirect` and called by the door. An absolute URL, a script or data scheme, a protocol-relative `//host`, a backslash, whitespace or a control character, a malformed `{{record.field_name}}` token, a document-relative path and an empty value each get their own author-facing prescription, naming the rule and what to write instead. The rule is not restated here: a second copy in the dialog would pass every value comparison right up to the release that moved the original, so a later widening of the ruling is followed by the pin rather than by an edit. +- **`Redirect URL is required` is gone.** Empty is one of the seven families, so it routes through the contract too and the author reads a sentence that says what a destination looks like. +- **The field no longer teaches the wrong value.** It is a plain text input with a `/thanks` placeholder, and a hint stating the rule — an in-app path, `{{record.field_name}}` interpolation, and the app navigation item that is declared for a deliberately external destination. + +The saved value is the one the schema accepted, read back off the parse, so the door and the renderer cannot hold different opinions about a destination. `thank-you`'s `title` and `message` stay unvalidated deliberately: the spec declares both as free-form strings, so there is no contract for a door to state about them. + +The server's own metadata gate already refused these bodies (`422 invalid_metadata` on `submitBehavior.url`, from the same schema), so this closes an error path rather than a silent-save hole: the correction now arrives in the field the admin can fix instead of as a failed round-trip. diff --git a/apps/console/src/components/submitRedirect.test.ts b/apps/console/src/components/submitRedirect.test.ts index ffddf98b69..c2d22e81d3 100644 --- a/apps/console/src/components/submitRedirect.test.ts +++ b/apps/console/src/components/submitRedirect.test.ts @@ -58,7 +58,7 @@ import { describe, expect, it } from 'vitest'; import { FormViewSchema } from '@objectstack/spec/ui'; -import { resolveSubmitRedirect } from './submitRedirect'; +import { checkSubmitRedirectUrl, resolveSubmitRedirect } from './submitRedirect'; /** * The contract's verdict on one authored value — the same minimal parse the @@ -69,6 +69,21 @@ function specAccepts(url: string): boolean { return FormViewSchema.safeParse({ submitBehavior: { kind: 'redirect', url } }).success; } +/** + * The contract's own prescription for a value it refuses, read off the same + * parse. Used to pin message PROVENANCE rather than message wording: a reworded + * spec keeps these tests green, a hand-written copy in either caller does not. + */ +function specRefusalMessage(url: string): string { + const parsed = FormViewSchema.safeParse({ submitBehavior: { kind: 'redirect', url } }); + if (parsed.success) throw new Error(`fixture is IN contract, not out of it: ${url}`); + const onUrl = parsed.error.issues.find( + (issue) => issue.path[0] === 'submitBehavior' && issue.path[1] === 'url', + ); + if (!onUrl) throw new Error(`the schema refused ${url} on some other path`); + return onUrl.message; +} + /** Values the ruling allows: rooted, relative, optionally interpolated. */ const IN_CONTRACT = [ '/thanks', @@ -227,3 +242,40 @@ describe('interpolation — the consumer’s half of the ruling', () => { } }); }); + +/** + * `checkSubmitRedirectUrl` — the shape half of the ruling, exported for the + * authoring door (objectui#4990). + * + * The console's Public Forms dialog validated one of the seven families and + * saved the rest into view metadata unexamined. It now calls this, so the + * property under test is not "the door has a rule" but "the door and the + * renderer cannot hold different opinions about a value, because there is one + * parse". That is what a second hand-written copy in the dialog would take away + * while passing every value comparison until the spec moved. + */ +describe('the authoring door asks the same question (#4990)', () => { + it.each(IN_CONTRACT)('accepts %j, handing back the value the schema accepted', (url) => { + expect(checkSubmitRedirectUrl(url)).toEqual({ ok: true, url }); + }); + + it.each(OUT_OF_CONTRACT)('refuses %s with the spec’s own prescription', (_label, url) => { + // Direction first: the contract itself rejects this value, so the refusal + // below is the contract's and not this module's private opinion. + expect(specAccepts(url)).toBe(false); + + const verdict = checkSubmitRedirectUrl(url); + expect(verdict.ok).toBe(false); + if (verdict.ok) return; + expect(verdict.refusal).toBe(specRefusalMessage(url)); + }); + + it('never disagrees with the renderer about a value', () => { + for (const url of [...IN_CONTRACT, ...OUT_OF_CONTRACT.map(([, u]) => u)]) { + const door = checkSubmitRedirectUrl(url); + const renderer = resolveSubmitRedirect(url, { id: 'x', slug: 's', status: 'open' }); + expect(door.ok).toBe(renderer.ok); + if (!door.ok && !renderer.ok) expect(door.refusal).toBe(renderer.refusal); + } + }); +}); diff --git a/apps/console/src/components/submitRedirect.ts b/apps/console/src/components/submitRedirect.ts index e3b0d96efb..ae1e6d8e96 100644 --- a/apps/console/src/components/submitRedirect.ts +++ b/apps/console/src/components/submitRedirect.ts @@ -25,12 +25,27 @@ * reference-integrity family owns that. Point 2's "when the redirect is built" * and point 3's consumption are runtime, and this module is where they happen. * + * ## Both layers ask the same question here (objectui#4990) + * + * "The spec enforces point 1 at the authoring door" is only true of a door that + * ASKS it. This repo ships one — the console's Public Forms dialog — and it + * enforced exactly one of the seven refusal families (empty), writing the other + * six into view metadata unexamined. So the parse below is exported as + * {@link checkSubmitRedirectUrl} and called there at save time: one parse, two + * callers, one spelling of a security rule. The author now reads the spec's + * prescription at the moment they can still fix the value, and the submitter + * never meets a destination the door let past. + * + * The door needs the verdict on the string alone — there is no record at + * authoring time — which is why the shape check and the substitution are + * separate exports rather than one function with an optional scope. + * * ## The shape verdict is the spec's, not a copy of it * * `@objectstack/spec` does not export its URL check as a function, but it does * export the schema the check lives in, so the verdict here is produced by * PARSING the smallest form view that carries this behavior. That costs one - * parse per submit and buys the property that matters: there is no second + * parse per submit or save and buys the property that matters: there is no second * spelling of a security rule in this repo to drift from the first. When the * ruling widens — it says an allowlist of absolute origins "waits for measured * demand" — the console follows the pin with no edit here, which is the same @@ -42,12 +57,8 @@ * objectui#4074/#4588/#4592): a hand copy passes every value comparison right * up to the release that moves the original. * - * Parsing a MINIMAL view is load-bearing, not laziness. A whole stored - * `FormView` refuses on any unrelated key the strict schema does not know, and - * a redirect must not be refused because some other part of the metadata - * drifted. The question asked here is narrow — "is this url a value the - * contract allows?" — so only that value is submitted for judgement, and only - * issues on that value's path are read back. + * That the parsed view is a MINIMAL one is load-bearing, not laziness — see + * {@link checkSubmitRedirectUrl}, which is where the parse lives. * * Neither the import nor the parse is new ground in this repo, which is worth * knowing before weighing the cost: @@ -123,6 +134,56 @@ interface SubmitRedirectRefused { export type SubmitRedirectVerdict = SubmitRedirectAccepted | SubmitRedirectRefused; +/** + * The contract's verdict on one authored `url`, before any substitution: `url` + * is the value the schema accepted, and the refused arm is the same + * author-facing prose the renderer quotes. + */ +export type SubmitRedirectUrlVerdict = { ok: true; url: string } | SubmitRedirectRefused; + +/** + * Ask the contract whether an authored `submitBehavior.url` is a value it + * accepts, and get its own prescription back when it is not. + * + * This is the shape half of the ruling — the half that needs only the string — + * so it is what an authoring door calls before writing the value, and what + * {@link resolveSubmitRedirect} calls before substituting into it. + * + * Parsing a MINIMAL view is load-bearing, not laziness. A whole stored + * `FormView` refuses on any unrelated key the strict schema does not know, and + * neither a redirect nor a save dialog must be refused because some other part + * of the metadata drifted. The question asked here is narrow — "is this url a + * value the contract allows?" — so only that value is submitted for judgement, + * and only issues on that value's path are read back. + */ +export function checkSubmitRedirectUrl(url: string): SubmitRedirectUrlVerdict { + const parsed = FormViewSchema.safeParse({ submitBehavior: { kind: 'redirect', url } }); + + if (!parsed.success) { + // Only this value was submitted for judgement, so an issue on its path is + // the answer; the two fallbacks exist so a refusal is never silent, not + // because either is expected to be reached. + const onUrl = parsed.error.issues.find( + (issue) => issue.path[0] === 'submitBehavior' && issue.path[1] === 'url', + ); + return { + ok: false, + refusal: + onUrl?.message + ?? parsed.error.issues[0]?.message + ?? `\`submitBehavior.url\` is not a value this contract accepts: ${JSON.stringify(url)}.`, + }; + } + + // Read the value back off the parse rather than reusing the input: the schema + // is the authority on what it accepted, so if it ever normalises the string + // both callers follow without a second edit. Today the two are identical — + // the key is a plain string with a refinement, deliberately, so that what is + // saved and what reaches the renderer stay the string the author wrote. + const behavior = parsed.data.submitBehavior; + return { ok: true, url: behavior?.kind === 'redirect' ? behavior.url : url }; +} + /** * The string form of a record value inside a URL. * @@ -179,33 +240,10 @@ export function resolveSubmitRedirect( url: string, record: Record, ): SubmitRedirectVerdict { - const parsed = FormViewSchema.safeParse({ submitBehavior: { kind: 'redirect', url } }); - - if (!parsed.success) { - // Only this value was submitted for judgement, so an issue on its path is - // the answer; the two fallbacks exist so a refusal is never silent, not - // because either is expected to be reached. - const onUrl = parsed.error.issues.find( - (issue) => issue.path[0] === 'submitBehavior' && issue.path[1] === 'url', - ); - return { - ok: false, - refusal: - onUrl?.message - ?? parsed.error.issues[0]?.message - ?? `\`submitBehavior.url\` is not a value this contract accepts: ${JSON.stringify(url)}.`, - }; - } - - // Read the value back off the parse rather than reusing the input: the schema - // is the authority on what it accepted, so if it ever normalises the string - // this follows without a second edit. Today the two are identical — the key - // is a plain string with a refinement, deliberately, so that what reaches - // this renderer stays the string the author wrote. - const behavior = parsed.data.submitBehavior; - const accepted = behavior?.kind === 'redirect' ? behavior.url : url; + const verdict = checkSubmitRedirectUrl(url); + if (!verdict.ok) return verdict; - const path = accepted.replace(RECORD_TOKEN_RE, (_token, field: string) => + const path = verdict.url.replace(RECORD_TOKEN_RE, (_token, field: string) => encodeURIComponent(urlValue(record[field])), ); diff --git a/apps/console/src/pages/developer/PublicFormsPage.redirect.test.tsx b/apps/console/src/pages/developer/PublicFormsPage.redirect.test.tsx new file mode 100644 index 0000000000..89e3c32293 --- /dev/null +++ b/apps/console/src/pages/developer/PublicFormsPage.redirect.test.tsx @@ -0,0 +1,253 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * objectui#4990 — the Public Forms dialog is an authoring door for + * `submitBehavior.url`, so it must refuse what the contract refuses. + * + * The ruling is objectstack#7496 (2026-08-11), landed in `@objectstack/spec` by + * objectstack#7657 and live on this repo's 17.0.0 GA pin: the key is a RELATIVE + * in-app path, interpolated only as `{{record.field_name}}`. The spec refuses + * seven families of value, each with its own author-facing prescription. This + * dialog enforced ONE of them — non-empty — and wrote the other six into view + * metadata unexamined. + * + * ## What is pinned, and against what + * + * The oracle is the contract itself. `specRefusal()` below performs the same + * minimal parse an authoring door performs and reads the message off it, so + * every expectation here is "the dialog showed the SPEC's sentence", never "the + * dialog showed this string I typed into a test". That distinction is the whole + * point of the fix: a local copy of the seven prescriptions would pass a + * literal-text assertion right up to the spec release that reworded one, which + * is the drift `scripts/check-spec-symbol-derivation.mjs` exists to discourage. + * These tests follow the pin instead — reword the spec and they still pass; + * hand-write the message in the dialog and they fail. + * + * The second half of every refusal case is that `meta.saveItem` was NOT called. + * A dialog that showed the refusal and saved anyway would be the same corpus + * defect wearing an error's clothes. + * + * ## Reverse verification — predicted first, then measured + * + * 1. **Reverting the door to the non-empty check** (`if (!editBehaviorUrl)` + + * `url: editBehaviorUrl`): all eight out-of-contract cases go RED, and each + * fails on BOTH assertions — `saveItem` is called with the rejected value, + * and no alert renders to read a message from. The empty case stays GREEN in + * its save-blocking half (the old code refused empty too) and goes RED on the + * message, because `Redirect URL is required` is not the spec's sentence. + * The in-contract cases and the thank-you boundary case stay green. + * 2. **Keeping the parse but showing a local string** (`setEditUrlRefusal('That + * URL is not allowed')`): every refusal case stays green on `saveItem` and + * goes RED on the message comparison — the narrow change detector for message + * PROVENANCE, which is the property a second copy of the rule would silently + * take away. + * + * Control characters in the fixtures below are written as escape sequences on + * purpose — a raw one makes the whole file read as binary to grep, and this repo + * has paid for that four times (objectui#4890 among them). + */ + +import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest'; +import { render, screen, cleanup, waitFor, fireEvent } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { FormViewSchema } from '@objectstack/spec/ui'; + +/** + * One published public form: `isForm` needs a form-ish shape, and the page only + * lists a row when `sharing.allowAnonymous` and a parseable `publicLink` are + * both present. + */ +const { saveItem, ADAPTER } = vi.hoisted(() => { + const spec = { + name: 'showcase_task.public', + label: 'Log Time', + object: 'showcase_task', + type: 'simple', + sections: [{ label: 'Task', fields: ['title'] }], + sharing: { enabled: true, allowAnonymous: true, publicLink: '/forms/log-time' }, + }; + const saveItem = vi.fn(async () => ({ ok: true })); + // A STABLE singleton: a fresh object per render loops the page's load effect. + const ADAPTER = { + getClient: () => ({ + meta: { + getItems: async (type: string) => (type === 'view' ? [{ spec }] : []), + saveItem, + }, + }), + }; + return { saveItem, ADAPTER }; +}); + +vi.mock('@object-ui/app-shell', () => ({ useAdapter: () => ADAPTER })); +vi.mock('sonner', () => ({ toast: { success: vi.fn(), error: vi.fn() } })); + +// Imported AFTER the mocks so the page picks them up. +import { PublicFormsPage } from './PublicFormsPage'; + +/** + * The contract's own prescription for one authored value — the same minimal + * parse the door performs, spelled out here independently so the test states the + * question rather than borrowing the door's answer. + */ +function specRefusal(url: string): string { + const parsed = FormViewSchema.safeParse({ submitBehavior: { kind: 'redirect', url } }); + if (parsed.success) throw new Error(`fixture is IN contract, not out of it: ${url}`); + const onUrl = parsed.error.issues.find( + (issue) => issue.path[0] === 'submitBehavior' && issue.path[1] === 'url', + ); + if (!onUrl) throw new Error(`the schema refused ${url} on some other path`); + return onUrl.message; +} + +/** + * Put a value in the field with `fireEvent.change` rather than `user.type`. + * + * Not a shortcut: userEvent's keyboard DSL reads `{` and `[` as key descriptors + * — `{{` types ONE literal brace and `{record.id}` reads as an unknown key — + * and half of these fixtures exist to be about braces. What the door reacts to + * is the change event, so dispatching it directly asks the same question of the + * same handler without fighting the DSL over the fixture's own content. + */ +function setUrl(field: HTMLElement, value: string) { + fireEvent.change(field, { target: { value } }); +} + +/** Open the row's editor and switch the post-submit behavior to `redirect`. */ +async function openRedirectEditor(user: ReturnType) { + render(); + await user.click( + await screen.findByRole('button', { name: /Edit sharing & post-submit behavior/i }), + ); + await user.selectOptions(await screen.findByLabelText(/After submit/i), 'redirect'); + return screen.getByLabelText(/Redirect URL/i); +} + +beforeEach(() => saveItem.mockClear()); +afterEach(cleanup); + +/** + * One fixture per family the spec's check defends, named by what saving it would + * have put into the metadata corpus. + */ +const OUT_OF_CONTRACT: Array<[label: string, url: string]> = [ + ['an absolute URL — the open redirect the ruling closed', 'https://example.com/thanks'], + ['a script scheme, the same refusal for a stronger reason', 'javascript:alert(1)'], + ['protocol-relative: another origin despite the leading slash', '//evil.example/thanks'], + ['a backslash, which browsers normalise to a slash while resolving', '/\\evil.example'], + ['whitespace, stripped before resolving and hiding the real start', '/ thanks'], + ['a tab, the same smuggle in a form that is easy to miss', '/\u0009thanks'], + ['a single-brace near-miss of the token spelling', '/thanks?x={record.id}'], + ['document-relative, so one form lands in different places', 'thanks'], +]; + +describe('the redirect door refuses what the contract refuses (#4990)', () => { + it.each(OUT_OF_CONTRACT)('refuses %s', async (_label, url) => { + const user = userEvent.setup(); + const field = await openRedirectEditor(user); + + setUrl(field, url); + await user.click(screen.getByRole('button', { name: /^Save$/ })); + + // The value never reaches the metadata corpus. + await waitFor(() => expect(screen.getByRole('alert')).toBeInTheDocument()); + expect(saveItem).not.toHaveBeenCalled(); + + // And the sentence the author reads is the SPEC's, not this dialog's. This + // is the assertion a hand-written mirror of the rule would fail. + expect(screen.getByRole('alert')).toHaveTextContent(specRefusal(url)); + }); + + /** + * Empty is the one family the old door DID catch, with a local + * `Redirect URL is required`. It routes through the contract now for the same + * reason as the other seven: the spec's sentence says what to write instead. + */ + it('refuses empty with the contract’s prescription, not a local “required”', async () => { + const user = userEvent.setup(); + const field = await openRedirectEditor(user); + + setUrl(field, ''); + await user.click(screen.getByRole('button', { name: /^Save$/ })); + + await waitFor(() => expect(screen.getByRole('alert')).toBeInTheDocument()); + expect(saveItem).not.toHaveBeenCalled(); + expect(screen.getByRole('alert')).toHaveTextContent(specRefusal('')); + }); + + it('marks the field invalid and describes it by the refusal', async () => { + const user = userEvent.setup(); + const field = await openRedirectEditor(user); + + setUrl(field, 'https://example.com/thanks'); + await user.click(screen.getByRole('button', { name: /^Save$/ })); + + await waitFor(() => expect(field).toHaveAttribute('aria-invalid', 'true')); + expect(field).toHaveAttribute('aria-describedby', 'edit-url-refusal'); + }); + + it('clears the refusal when the author starts fixing the value', async () => { + const user = userEvent.setup(); + const field = await openRedirectEditor(user); + + setUrl(field, 'https://example.com/thanks'); + await user.click(screen.getByRole('button', { name: /^Save$/ })); + await waitFor(() => expect(screen.getByRole('alert')).toBeInTheDocument()); + + setUrl(field, '/thanks'); + await waitFor(() => expect(screen.queryByRole('alert')).not.toBeInTheDocument()); + }); +}); + +describe('in-contract values still save (#4990)', () => { + it.each([ + ['a rooted in-app path', '/thanks'], + ['a path with a query', '/thanks?ref=42'], + ['an interpolated path, the one form the ruling allows', '/t/{{record.slug}}?r={{record.id}}'], + ])('saves %s unchanged', async (_label, url) => { + const user = userEvent.setup(); + const field = await openRedirectEditor(user); + + setUrl(field, url); + await user.click(screen.getByRole('button', { name: /^Save$/ })); + + await waitFor(() => expect(saveItem).toHaveBeenCalledTimes(1)); + expect(screen.queryByRole('alert')).not.toBeInTheDocument(); + expect(saveItem).toHaveBeenCalledWith( + 'view', + 'showcase_task.public', + expect.objectContaining({ submitBehavior: { kind: 'redirect', url } }), + ); + }); + + /** + * The scope boundary, pinned so a later reading of #4990 does not widen it by + * accident: `thank-you`'s `title` and `message` are free-form strings in the + * spec, so there is no contract for this door to state about them — even when + * the text happens to look like the address the redirect key refuses. + */ + it('leaves thank-you title/message unvalidated — the spec declares them free text', async () => { + const user = userEvent.setup(); + render(); + await user.click( + await screen.findByRole('button', { name: /Edit sharing & post-submit behavior/i }), + ); + + setUrl(await screen.findByLabelText(/^Title$/i), 'https://example.com/thanks'); + setUrl(screen.getByLabelText(/^Message$/i), 'See javascript:alert(1) below'); + await user.click(screen.getByRole('button', { name: /^Save$/ })); + + await waitFor(() => expect(saveItem).toHaveBeenCalledTimes(1)); + expect(saveItem).toHaveBeenCalledWith( + 'view', + 'showcase_task.public', + expect.objectContaining({ + submitBehavior: { + kind: 'thank-you', + title: 'https://example.com/thanks', + message: 'See javascript:alert(1) below', + }, + }), + ); + }); +}); diff --git a/apps/console/src/pages/developer/PublicFormsPage.tsx b/apps/console/src/pages/developer/PublicFormsPage.tsx index fc04a6107e..97440deb26 100644 --- a/apps/console/src/pages/developer/PublicFormsPage.tsx +++ b/apps/console/src/pages/developer/PublicFormsPage.tsx @@ -6,6 +6,29 @@ * Console is not project-scoped, so there is no `useParams().package`, no * `` to the legacy metadata editor, and no `useMetadataHmr` polling — * refresh is driven by the explicit Refresh button. + * + * ## The redirect field is an authoring door, so it states the contract + * + * `submitBehavior.url` is ruled relative-only (objectstack#7496, landed by + * objectstack#7657 in the `@objectstack/spec` 17.0.0 GA pin this repo installs), + * and the spec refuses seven families of value with an author-facing + * prescription for each. This dialog used to enforce one of them — non-empty — + * and save the rest into view metadata unexamined (objectui#4990), so an admin + * could type `https://example.com/thanks`, or `javascript:alert(1)`, and be + * told nothing. + * + * `checkSubmitRedirectUrl` (from the renderer's own `submitRedirect`) is asked + * instead, at save time, and its refusal — the spec's prose, verbatim — is shown + * next to the field. That reuse is the point: a hand-written mirror of the seven + * families here is exactly the shape `scripts/check-spec-symbol-derivation.mjs` + * exists to discourage, and it would be a second spelling of a security rule + * that passes every value comparison right up to the release that moves the + * original. The door and the renderer now refuse identically because they are + * one parse. + * + * `thank-you`'s `title` / `message` stay unvalidated deliberately: the spec + * declares both as free-form strings, so there is no contract for a door to + * state. */ import { useEffect, useState } from 'react'; @@ -35,6 +58,7 @@ import { } from '@object-ui/components'; import { Copy, ExternalLink, FormInput, RefreshCw, Code2, Link2, Settings2, Plus } from 'lucide-react'; import { toast } from 'sonner'; +import { checkSubmitRedirectUrl } from '../../components/submitRedirect'; interface PublicFormRow { name: string; @@ -84,6 +108,13 @@ export function PublicFormsPage() { const [editBehaviorTitle, setEditBehaviorTitle] = useState(''); const [editBehaviorMessage, setEditBehaviorMessage] = useState(''); const [editBehaviorUrl, setEditBehaviorUrl] = useState(''); + /** + * The contract's refusal for the value currently in the Redirect URL field, + * or null while there is nothing to say. Set only by a save attempt — typing + * clears it, so the author is corrected once, at the moment they asked to + * commit, rather than nagged mid-keystroke. + */ + const [editUrlRefusal, setEditUrlRefusal] = useState(null); const [saving, setSaving] = useState(false); const load = async () => { @@ -209,6 +240,7 @@ export function PublicFormsPage() { setEditBehaviorTitle(sb?.title ?? ''); setEditBehaviorMessage(sb?.message ?? ''); setEditBehaviorUrl(sb?.url ?? ''); + setEditUrlRefusal(null); setEditOpen(true); }; @@ -219,6 +251,7 @@ export function PublicFormsPage() { toast.error('Invalid slug'); return; } + setEditUrlRefusal(null); let submitBehavior: any; switch (editBehavior) { case 'thank-you': @@ -226,13 +259,20 @@ export function PublicFormsPage() { if (editBehaviorTitle) submitBehavior.title = editBehaviorTitle; if (editBehaviorMessage) submitBehavior.message = editBehaviorMessage; break; - case 'redirect': - if (!editBehaviorUrl) { - toast.error('Redirect URL is required'); + case 'redirect': { + // The contract's verdict, not this dialog's: empty is one of the seven + // families the spec refuses, so it comes back through here too rather + // than keeping a local `required` message that says less. + const verdict = checkSubmitRedirectUrl(editBehaviorUrl); + if (!verdict.ok) { + setEditUrlRefusal(verdict.refusal); return; } - submitBehavior = { kind: 'redirect', url: editBehaviorUrl }; + // The value the schema accepted, so a future normalisation in the spec + // is what gets saved — the same read-back the renderer does. + submitBehavior = { kind: 'redirect', url: verdict.url }; break; + } case 'continue': case 'next-record': submitBehavior = { kind: editBehavior }; @@ -480,7 +520,10 @@ export function PublicFormsPage() { id="edit-behavior" className="flex h-9 w-full rounded-md border border-input bg-transparent px-3 py-1 text-sm shadow-sm" value={editBehavior} - onChange={(e) => setEditBehavior(e.target.value as any)} + onChange={(e) => { + setEditBehavior(e.target.value as any); + setEditUrlRefusal(null); + }} > @@ -513,13 +556,39 @@ export function PublicFormsPage() { {editBehavior === 'redirect' && (
+ {/* + Not `type="url"`: that type's own notion of valid is an + ABSOLUTE URL, which is the one thing this key refuses, so it + pulled the author the wrong way — as did the former + `https://example.com/thanks` placeholder. + */} setEditBehaviorUrl(e.target.value)} + onChange={(e) => { + setEditBehaviorUrl(e.target.value); + setEditUrlRefusal(null); + }} + aria-invalid={editUrlRefusal ? true : undefined} + aria-describedby={editUrlRefusal ? 'edit-url-refusal' : 'edit-url-hint'} /> + {editUrlRefusal ? ( + + ) : ( +

+ An in-app path, starting with / — interpolate a field of + the submitted record as {'{{record.field_name}}'}. To send + the browser out of the app, use an app navigation item instead. +

+ )}
)}