cloud-hosting-platform/site-builder
4
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(site-builder): review findings on the webhook destination controls

Browse Source 
I-1: WebhookSecretField now mounts with key={selectedId}. GuidedStyles renders
FormStylePanel with no key, so a selection change re-rendered rather than
remounted it and React kept `draft`/`status`. Since a failed store deliberately
retains the draft, clicking a second contact form showed node A's raw secret in
node B's field, and the next blur assigned the returned secret_id to the wrong
form (burning a slot against the per-site cap). The status banner leaked the
same way. Both pinned by the reviewer's repro sequence.

I-2: the button is "Clear", not "Remove", and says so -- nothing deletes a
stored key file, so the 50-per-site cap counts stores-ever. Labelling it Remove
told the customer they had reclaimed a slot right up until the 429 that said
otherwise. Actually deleting the file is Task 8's territory.

M-1: inline warning when the webhook URL is blank or not absolute https. The
publish step does refuse these, but into an error_log the customer never reads.
Warning only -- isHttpsWebhookUrl never edits the value or blocks the publish,
since either would trade a loud server-side refusal for a silently inert form.

M-2: the BYTE-IDENTITY test now compares against a literal marker captured by
executing the emitter at 071f3447, not against another head-revision output. The
self-comparison could only catch a drift affecting one side; a uniform one
passed it. Verified: a uniform `<!-- WHP-FORM` drift now fails this assertion.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
shadowdao
2026-08-10 15:57:38 -07:00
co-authored by Claude Opus 5
parent d1c57db967
commit 422697acec
3 changed files with 183 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files
craft/src/components/forms/ContactForm.toHtml.test.ts
+14 -4
View File
@@ -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 =
'<!--WHP-FORM id="F_3hodg" recipient="a@example.com" thankyou="/thx"-->';
const legacy = toHtml({ ...defaultProps, recipientEmail: 'a@example.com', thankYouUrl: '/thx' }, '', 'n1');
expect(legacy.html.startsWith(`${FROZEN_LEGACY_MARKER}<form `)).toBe(true);
// ...and the new props, set to their defaults, change nothing about it.
const explicit = toHtml(
{ ...defaultProps, destinationType: 'email', webhookUrl: '', webhookSecretId: '',
webhookAuthMode: 'signature', recipientEmail: 'a@example.com', thankYouUrl: '/thx' }, '', 'n1');
expect(explicit.html).toBe(legacy.html);
// ...and the marker itself is the exact narrow shape, anchored.
expect(legacy.html).toMatch(/^<!--WHP-FORM id="F_[0-9a-z]+" recipient="a@example\.com" thankyou="\/thx"--><form /);
});
test('webhook destination emits type, url, secret id and auth mode', () => {
craft/src/panels/right/styles/FormStylePanel.tsx
+66 -11
View File
@@ -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 <FormStylePanel>
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 && (
<div style={{ display: 'flex', alignItems: 'center', gap: 6, marginTop: 4 }}>
<span data-testid="webhook-secret-status" style={{ fontSize: 10, color: '#22c55e' }}>Secret stored</span>
<button
data-testid="webhook-secret-remove"
onClick={onRemoved}
style={{ ...moveBtnStyle, flex: 'none', padding: '3px 8px' }}
>
Remove
</button>
</div>
<>
<div style={{ display: 'flex', alignItems: 'center', gap: 6, marginTop: 4 }}>
<span data-testid="webhook-secret-status" style={{ fontSize: 10, color: '#22c55e' }}>Secret stored</span>
{/* "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. */}
<button
data-testid="webhook-secret-remove"
onClick={onRemoved}
style={{ ...moveBtnStyle, flex: 'none', padding: '3px 8px' }}
>
Clear
</button>
</div>
<p style={hintStyle}>
Clearing stops this form using the secret; the stored key stays on the server.
</p>
</>
)}
{status === 'saving' && <p style={hintStyle}>Storing…</p>}
{status === 'saved' && <p data-testid="webhook-secret-saved" style={{ ...hintStyle, color: '#22c55e' }}>Secret stored.</p>}
@@ -268,6 +306,19 @@ export const FormStylePanel: React.FC<StylePanelProps> = ({ 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) && (
<p data-testid="webhook-url-warning" style={{ ...hintStyle, color: '#fbbf24' }}>
{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.'}
</p>
)}
<p style={hintStyle}>
Must be an absolute <strong>https://</strong> 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<StylePanelProps> = ({ selectedId, nodeProp
</select>
</div>
<WebhookSecretField
/* Remounts on every selection change, so a retained draft (and its
status banner) can never follow the customer to another form --
see the comment on WebhookSecretField. */
key={selectedId}
secretId={nodeProps.webhookSecretId || ''}
onStored={(id) => setProp('webhookSecretId', id)}
onRemoved={() => setProp('webhookSecretId', '')}
craft/src/panels/right/styles/FormStylePanel.webhook.test.tsx
+103 -2
View File
@@ -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(<FormStylePanel selectedId="n1" nodeProps={lastProps} />);
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(<FormStylePanel selectedId="n1" nodeProps={lastProps} />);
// "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 <FormStylePanel> 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(<FormStylePanel selectedId="nodeA" nodeProps={nodeA} />);
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(<FormStylePanel selectedId="nodeB" nodeProps={nodeB} />);
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(<FormStylePanel selectedId="nodeA" nodeProps={lastProps} />);
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(<FormStylePanel selectedId="nodeB" nodeProps={nodeB} />);
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(<FormStylePanel selectedId="n1" nodeProps={lastProps} />);
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(<FormStylePanel selectedId="n1" nodeProps={lastProps} />);
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(<FormStylePanel selectedId="n1" nodeProps={lastProps} />);
expect(testid('webhook-url-warning')).toBeNull();
});
});