diff --git a/craft/src/components/forms/ContactForm.toHtml.test.ts b/craft/src/components/forms/ContactForm.toHtml.test.ts index e72c4aa..d32201b 100644 --- a/craft/src/components/forms/ContactForm.toHtml.test.ts +++ b/craft/src/components/forms/ContactForm.toHtml.test.ts @@ -235,15 +235,25 @@ describe('ContactForm.toHtml destination marker', () => { test('BYTE-IDENTITY: an email destination emits exactly what a pre-feature form emits', () => { // The guarantee every already-published site depends on: a marker with no // `type` still provisions an email endpoint, so its bytes must not drift by - // so much as a space. Compared against a form that has no destination props - // at all (a page saved before this feature existed). + // so much as a space. + // + // FROZEN LITERAL, not a self-comparison. Comparing two head-revision outputs + // to each other only catches a drift that affects ONE of them -- a uniform + // change passes it. This string was captured from `071f3447` (the revision + // deployed to production before this feature) and is the actual reference: + // if it has to be edited, every already-published site's forms have changed + // shape and that is the thing to stop, not the test. + const FROZEN_LEGACY_MARKER = + ''; + const legacy = toHtml({ ...defaultProps, recipientEmail: 'a@example.com', thankYouUrl: '/thx' }, '', 'n1'); + expect(legacy.html.startsWith(`${FROZEN_LEGACY_MARKER}
{ diff --git a/craft/src/panels/right/styles/FormStylePanel.tsx b/craft/src/panels/right/styles/FormStylePanel.tsx index 8122c86..77e5a47 100644 --- a/craft/src/panels/right/styles/FormStylePanel.tsx +++ b/craft/src/panels/right/styles/FormStylePanel.tsx @@ -61,12 +61,40 @@ const SPACING_SIDE_KEYS: { side: 'top' | 'right' | 'bottom' | 'left'; suffix: 'T const hintStyle: React.CSSProperties = { fontSize: 10, color: '#71717a', margin: '4px 0 0' }; +/** + * Does this look like a webhook URL the publish step will accept? + * + * Mirrors the shape `FormRelayProvisioner::upsertWebhookToken()` enforces + * (absolute https, a host, no whitespace/control characters) closely enough to + * warn in the panel. It is a HINT, not a gate -- the server-side check is the + * real one, and this deliberately never edits or blocks the value. + */ +export function isHttpsWebhookUrl(value: unknown): boolean { + const v = typeof value === 'string' ? value.trim() : ''; + if (v === '' || /[\s\x00-\x1F\x7F]/.test(v)) return false; + try { + const u = new URL(v); + return u.protocol === 'https:' && u.hostname !== ''; + } catch { + return false; + } +} + /* ---------- Webhook shared secret: WRITE-ONLY field ---------- The raw secret lives in this component's local state and nowhere else. On blur it is POSTed to the panel endpoint, which returns an opaque id; only that id is handed to `onStored` (and thence to a craft prop). The field is then cleared, because there is no read route and nothing to show back -- - the UI offers set / replace / remove, never "view". */ + the UI offers set / replace / clear, never "view". + + MOUNT THIS WITH `key={selectedId}`. GuidedStyles renders + with no key, so a selection change re-renders this component rather than + remounting it, and React keeps `draft`/`status`. A failed store deliberately + RETAINS the draft (so a 429 or a network blip doesn't make the customer + retype a pasted key) -- which means without the key, clicking a second + contact form shows node A's raw secret in node B's field, and the next blur + assigns the returned id to the wrong form and burns a slot against the + per-site cap. The `status` banner leaks the same way. */ export const WebhookSecretField: React.FC<{ secretId: string; onStored: (secretId: string) => void; @@ -109,16 +137,26 @@ export const WebhookSecretField: React.FC<{ style={inputStyle} /> {secretId && ( -
- Secret stored - -
+ <> +
+ Secret stored + {/* "Clear", not "Remove": this only drops the form's reference to the + key. The stored key file stays on the server and still counts + toward the per-site cap -- nothing deletes one, so a button + labelled Remove would be telling the customer they had reclaimed + a slot right up until the 429 that says otherwise. */} + +
+

+ Clearing stops this form using the secret; the stored key stays on the server. +

+ )} {status === 'saving' &&

Storing…

} {status === 'saved' &&

Secret stored.

} @@ -268,6 +306,19 @@ export const FormStylePanel: React.FC = ({ selectedId, nodeProp placeholder="https://hooks.example.com/..." style={inputStyle} /> + {/* The publish step DOES refuse a blank/non-https URL -- loudly, but + into an error_log the customer never reads, leaving them with a + form that just doesn't work. Warn here instead. Deliberately a + warning only: not blanking the value and not blocking the + publish, since either would trade a loud server-side refusal for + a silently inert form. */} + {!isHttpsWebhookUrl(nodeProps.webhookUrl) && ( +

+ {nodeProps.webhookUrl + ? 'This must be an absolute https:// URL — submissions to this form won\'t be delivered until it is.' + : 'Enter the https:// URL to POST submissions to — this form won\'t deliver anything until you do.'} +

+ )}

Must be an absolute https:// URL. Each submission is POSTed as JSON; failures are retried, then emailed to the fallback address below. @@ -286,6 +337,10 @@ export const FormStylePanel: React.FC = ({ selectedId, nodeProp setProp('webhookSecretId', id)} onRemoved={() => setProp('webhookSecretId', '')} diff --git a/craft/src/panels/right/styles/FormStylePanel.webhook.test.tsx b/craft/src/panels/right/styles/FormStylePanel.webhook.test.tsx index c3fde75..f04105d 100644 --- a/craft/src/panels/right/styles/FormStylePanel.webhook.test.tsx +++ b/craft/src/panels/right/styles/FormStylePanel.webhook.test.tsx @@ -17,7 +17,7 @@ vi.mock('@craftjs/core', () => ({ useEditor: () => ({ actions: { setProp: setPropSpy } }), })); -import { FormStylePanel } from './FormStylePanel'; +import { FormStylePanel, isHttpsWebhookUrl } from './FormStylePanel'; const RAW_SECRET = 'hunter2-SUPER-SECRET-VALUE'; @@ -197,7 +197,7 @@ describe('FormStylePanel webhook secret field is WRITE-ONLY', () => { expect(JSON.stringify(lastProps)).not.toContain(RAW_SECRET); }); - test('the stored secret is never shown -- only its state, with a Remove action', () => { + test('the stored secret is never shown -- only its state, with a Clear action', () => { lastProps = contactFormProps({ destinationType: 'webhook', webhookSecretId: 'whs_42_deadbeefdeadbeef' }); render(); expect(testid('webhook-secret-status')!.textContent).toContain('Secret stored'); @@ -219,4 +219,105 @@ describe('FormStylePanel webhook secret field is WRITE-ONLY', () => { await blur(testid('webhook-secret-input')!); expect(fetchMock).not.toHaveBeenCalled(); }); + + test('the Clear action says what it does: the stored key stays on the server', () => { + lastProps = contactFormProps({ destinationType: 'webhook', webhookSecretId: 'whs_42_deadbeefdeadbeef' }); + render(); + // "Remove" would be a lie: nothing deletes the key file, and it keeps + // counting toward the per-site cap the customer eventually 429s against. + expect(testid('webhook-secret-remove')!.textContent).toBe('Clear'); + expect(container.textContent).toContain('the stored key stays on the server'); + }); +}); + +describe('FormStylePanel: a retained secret draft never follows the selection to another form', () => { + /* GuidedStyles renders with no key, so a selection change + re-renders rather than remounts. A failed store deliberately KEEPS the + draft, so without a remount the next form's field would open holding the + previous form's raw secret -- and the next blur would POST it and assign + the returned id to the wrong node. */ + test('after a failed store on node A, selecting node B shows an empty field and no status', async () => { + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ + ok: false, status: 429, + json: async () => ({ success: false, error: 'Too many webhook secrets stored for this site — please contact support.' }), + })); + + const nodeA = contactFormProps({ destinationType: 'webhook', webhookUrl: 'https://a.example/x' }); + lastProps = nodeA; + render(); + const input = testid('webhook-secret-input') as HTMLInputElement; + setValue(input, RAW_SECRET); + await blur(input); + + // Precondition: the draft really was retained on node A (otherwise this + // test would pass for the wrong reason). + expect((testid('webhook-secret-input') as HTMLInputElement).value).toBe(RAW_SECRET); + expect(testid('webhook-secret-error')).toBeTruthy(); + + const nodeB = contactFormProps({ destinationType: 'webhook', webhookUrl: 'https://b.example/y' }); + lastProps = nodeB; + rerender(); + + expect((testid('webhook-secret-input') as HTMLInputElement).value).toBe(''); + expect(container.textContent).not.toContain(RAW_SECRET); + expect(testid('webhook-secret-error')).toBeNull(); + }); + + test('a "Secret stored." banner does not follow the selection either', async () => { + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ + ok: true, status: 200, + json: async () => ({ success: true, secret_id: 'whs_42_deadbeefdeadbeef' }), + })); + + lastProps = contactFormProps({ destinationType: 'webhook' }); + render(); + const input = testid('webhook-secret-input') as HTMLInputElement; + setValue(input, RAW_SECRET); + await blur(input); + expect(testid('webhook-secret-saved')).toBeTruthy(); + + const nodeB = contactFormProps({ destinationType: 'webhook' }); + lastProps = nodeB; + rerender(); + expect(testid('webhook-secret-saved')).toBeNull(); + expect(testid('webhook-secret-status')).toBeNull(); + }); +}); + +describe('isHttpsWebhookUrl / the inline URL warning', () => { + test.each([ + ['https://hooks.example.com/x', true], + ['https://hooks.example.com/x?a=1&b=2', true], + ['http://hooks.example.com/x', false], + ['hooks.example.com/x', false], + ['/relative/path', false], + ['', false], + [' ', false], + ['https://hooks.example.com/x\nHost: evil', false], + ['javascript:alert(1)', false], + ])('%s -> %s', (value, expected) => { + expect(isHttpsWebhookUrl(value)).toBe(expected); + }); + + test('a blank URL warns that nothing will be delivered', () => { + lastProps = contactFormProps({ destinationType: 'webhook' }); + render(); + expect(testid('webhook-url-warning')!.textContent).toContain("won't deliver anything"); + }); + + test('a non-https URL warns without altering the value', () => { + lastProps = contactFormProps({ destinationType: 'webhook', webhookUrl: 'http://hooks.example.com/x' }); + render(); + expect(testid('webhook-url-warning')!.textContent).toContain('absolute https:// URL'); + // Warning only -- the panel must not blank or rewrite the prop, which would + // turn the publish step's loud refusal into a silently inert form. + expect(lastProps.webhookUrl).toBe('http://hooks.example.com/x'); + expect((testid('webhook-url') as HTMLInputElement).value).toBe('http://hooks.example.com/x'); + }); + + test('a valid https URL shows no warning', () => { + lastProps = contactFormProps({ destinationType: 'webhook', webhookUrl: 'https://hooks.example.com/x' }); + render(); + expect(testid('webhook-url-warning')).toBeNull(); + }); });