From ba3a5f5322bbe26f6b7c25431973e5231fd1f6c4 Mon Sep 17 00:00:00 2001 From: Matteo Date: Mon, 5 Oct 2026 10:58:44 +0200 Subject: [PATCH] Etsy setup: catch wrong app keys at setup, not at the first call Etsy's sign-in succeeds with a wrong shared secret (its token exchange does not use it), so the guided setup said "is ready" and every call then failed with 403. Seen in the weekly stuck-users report: a secret pasted as "keystring:secret", the keystring pasted twice. - After the sign-in, the setup page already called the connector test; a refusal (auth_failed) now sends the user back to the keys with the provider's message instead of "is ready". - Etsy's Keystring and Shared secret get a pattern and a message saying what to paste (lowercase letters and digits, no colon). Checked against production: every working Etsy connector has a 24-character keystring and a 10- or 25-character secret; no value with a colon or space ever worked. - envVarMeta gains patternMessage; settings passed from the chat (setup_install_connector) are checked against the pattern too. --- .../adapters/connector-setup.service.spec.ts | 10 +++++- .../src/adapters/connector-setup.service.ts | 22 +++++++++++- packages/backend/src/adapters/env-var-meta.ts | 5 ++- packages/backend/src/adapters/intl/etsy.json | 10 ++++-- .../src/app/connectors/setup/[slug]/page.tsx | 14 ++++++-- packages/frontend/src/lib/api.ts | 2 ++ .../tests/e2e/connector-guided-setup.spec.ts | 34 +++++++++++++++---- scripts/validate-adapters.mjs | 2 +- 8 files changed, 84 insertions(+), 15 deletions(-) diff --git a/packages/backend/src/adapters/connector-setup.service.spec.ts b/packages/backend/src/adapters/connector-setup.service.spec.ts index 5b5fd892..a1762497 100644 --- a/packages/backend/src/adapters/connector-setup.service.spec.ts +++ b/packages/backend/src/adapters/connector-setup.service.spec.ts @@ -124,7 +124,7 @@ describe('ConnectorSetupService — install', () => { it('asks for the sign-in when an OAuth connector has its app keys', async () => { const { service, ctx } = build(); - const out: any = await service.install(ctx, { adapter: 'etsy', settings: { ETSY_CLIENT_ID: 'ks' } }); + const out: any = await service.install(ctx, { adapter: 'etsy', settings: { ETSY_CLIENT_ID: 'a1b2c3d4e5f6g7h8i9j0k1l2' } }); expect(out.body.status).toBe('needs_input'); // the shared secret still has to be entered on the page expect(out.body.whatTheUserDoes).toMatch(/enter Shared secret, then sign in to .+ and approve\./); expect(out.body.finishSetupUrl).toBeDefined(); @@ -138,6 +138,14 @@ describe('ConnectorSetupService — install', () => { expect(JSON.stringify(out.body)).toContain('an hour'); }); + it('refuses a setting that does not match its pattern, with what to check', async () => { + const { service, ctx, adapters } = build(); + const out: any = await service.install(ctx, { adapter: 'etsy', settings: { ETSY_CLIENT_ID: 'abc123:secret' } }); + expect(out.isError).toBe(true); + expect(out.body.error).toMatch(/Keystring.*does not look right.*24 lowercase/); + expect(adapters.importAdapter).not.toHaveBeenCalled(); + }); + it('respects the trial limit', async () => { const { service, ctx, licenseGuard, adapters } = build(); licenseGuard.checkCanCreateConnector.mockRejectedValueOnce(new Error('Trial limit reached (2 connectors).')); diff --git a/packages/backend/src/adapters/connector-setup.service.ts b/packages/backend/src/adapters/connector-setup.service.ts index 79e985a6..6ad9a93d 100644 --- a/packages/backend/src/adapters/connector-setup.service.ts +++ b/packages/backend/src/adapters/connector-setup.service.ts @@ -162,7 +162,17 @@ export class ConnectorSetupService implements SharedSetupProvider, OnModuleInit }, }; } - if (value !== undefined && value !== null && String(value).trim() !== '') settings[name] = String(value).trim(); + const text = value !== undefined && value !== null ? String(value).trim() : ''; + if (text && d.pattern && !matchesPattern(d.pattern, text)) { + return { + isError: true, + body: { + error: `'${d.label}' does not look right. ${d.patternMessage ?? d.help ?? ''}`.trim(), + hint: 'Ask the user to check the value, or install without it: they can enter it on the page linked in the answer.', + }, + }; + } + if (text) settings[name] = text; } if (!this.takeInstallSlot(ctx.userId)) { @@ -395,3 +405,13 @@ export class ConnectorSetupService implements SharedSetupProvider, OnModuleInit }); } } + +/** A broken pattern in an adapter never blocks an install. */ +function matchesPattern(pattern: string, value: string): boolean { + try { + return new RegExp(pattern).test(value); + } catch { + return true; + } +} + diff --git a/packages/backend/src/adapters/env-var-meta.ts b/packages/backend/src/adapters/env-var-meta.ts index cde302d1..01fcb67b 100644 --- a/packages/backend/src/adapters/env-var-meta.ts +++ b/packages/backend/src/adapters/env-var-meta.ts @@ -19,8 +19,10 @@ export interface EnvVarMeta { /** Where to find the value, in a sentence. */ help?: string; example?: string; - /** Regular expression the value must match (validated in the form). */ + /** Regular expression the value must match (validated in the form and in the chat). */ pattern?: string; + /** What to tell the user when the value does not match `pattern`. */ + patternMessage?: string; /** Page of the provider where the value is created or shown. */ link?: string; /** Rarely needed: shown collapsed (e.g. a refresh token the authorization fills in). */ @@ -33,6 +35,7 @@ export interface EnvVarDescriptor extends Required { if (c?.setupStatus === 'ready') { const test = await connectors.test(existingId, token).catch(() => null); - setResult({ connectorId: existingId, status: (test as any)?.message }); + // The sign-in can succeed with wrong app keys (Etsy's token exchange + // does not check the shared secret), so only a call to the API tells. + // A refusal sends the user back to the keys, not to "is ready". + if (test && test.ok === false && test.kind === 'auth_failed') { + productEvents.track('setup_verify_failed', token, { adapterSlug: slug, kind: 'after_authorization' }); + setVerifyFailed({ ok: false, kind: 'auth_failed', message: test.message } as VerifyResult); + setError(''); + setPhase('form'); + return; + } + setResult({ connectorId: existingId, status: test?.message }); setPhase('done'); productEvents.track('setup_completed', token, { adapterSlug: slug, kind: 'oauth_browser' }); } else { @@ -143,7 +153,7 @@ function SetupContent() { if (f.required && !f.advanced && !v && !storedSecrets.includes(f.name)) errs[f.name] = 'Required'; else if (v && f.pattern) { try { - if (!new RegExp(f.pattern).test(v)) errs[f.name] = f.example ? `Looks wrong. Example: ${f.example}` : 'Looks wrong'; + if (!new RegExp(f.pattern).test(v)) errs[f.name] = f.patternMessage ?? (f.example ? `Looks wrong. Example: ${f.example}` : 'Looks wrong'); } catch { /* a broken pattern never blocks the form */ } diff --git a/packages/frontend/src/lib/api.ts b/packages/frontend/src/lib/api.ts index a1a67e0e..486a06c9 100644 --- a/packages/frontend/src/lib/api.ts +++ b/packages/frontend/src/lib/api.ts @@ -467,6 +467,8 @@ export interface EnvVarDescriptor { help?: string; example?: string; pattern?: string; + /** Shown when the value does not match `pattern`. */ + patternMessage?: string; link?: string; advanced?: boolean; } diff --git a/packages/frontend/tests/e2e/connector-guided-setup.spec.ts b/packages/frontend/tests/e2e/connector-guided-setup.spec.ts index d4a5f57e..f6a008ee 100644 --- a/packages/frontend/tests/e2e/connector-guided-setup.spec.ts +++ b/packages/frontend/tests/e2e/connector-guided-setup.spec.ts @@ -30,7 +30,7 @@ const ETSY = { setupKind: 'oauth_browser', envVars: [ { name: 'ETSY_CLIENT_ID', required: true, label: 'Keystring', kind: 'credential', secret: false }, - { name: 'ETSY_CLIENT_SECRET', required: true, label: 'Shared secret', kind: 'credential', secret: true }, + { name: 'ETSY_CLIENT_SECRET', required: true, label: 'Shared secret', kind: 'credential', secret: true, pattern: '^[a-z0-9]{8,32}$', patternMessage: 'Paste the Shared secret alone: lowercase letters and digits, without the Keystring and without a colon.' }, { name: 'ETSY_REFRESH_TOKEN', required: false, label: 'Refresh token', kind: 'credential', secret: true, advanced: true }, ], }; @@ -42,7 +42,7 @@ interface Calls { authorize: any[]; } -async function setup(page: Page, opts: { verify?: any; connector?: any } = {}): Promise { +async function setup(page: Page, opts: { verify?: any; connector?: any; test?: any } = {}): Promise { const calls: Calls = { verify: [], imports: [], envVars: [], authorize: [] }; await page.context().addCookies([{ name: 'amcp_token', value: 'test-token', url: 'http://localhost:3100' }]); await page.addInitScript((user) => { @@ -73,7 +73,7 @@ async function setup(page: Page, opts: { verify?: any; connector?: any } = {}): calls.envVars.push(req.postDataJSON()); return json({}); } - if (url.includes('/api/connectors/c9/test')) return json({ ok: true, message: 'Connected' }); + if (url.includes('/api/connectors/c9/test')) return json(opts.test ?? { ok: true, message: 'Connected' }); if (url.includes('/api/connectors/c9')) return json(opts.connector ?? { id: 'c9', setupStatus: 'ready', envVars: {}, maskedEnvVars: [] }); if (url.includes('/api/product-events')) return json({ ok: true }); if (url.includes('/api/users/me/onboarding-state')) return json({ onboardingCompletedAt: '2026-01-01T00:00:00Z' }); @@ -128,11 +128,11 @@ test('OAuth: saves the app keys and goes straight to the sign-in, with the way b await expect(page.getByLabel('Keystring')).toHaveAttribute('autocomplete', 'off'); await expect(page.getByLabel('Shared secret')).toHaveAttribute('autocomplete', 'new-password'); await expect(page.getByLabel('Shared secret')).toHaveAttribute('data-1p-ignore', 'true'); - await page.getByLabel('Keystring').fill('ks'); - await page.getByLabel('Shared secret').fill('ss'); + await page.getByLabel('Keystring').fill('a1b2c3d4e5f6g7h8i9j0k1l2'); + await page.getByLabel('Shared secret').fill('s3cr3t0abc'); await page.getByRole('button', { name: 'Save and sign in to Etsy' }).click(); await page.waitForURL('https://www.etsy.com/oauth/connect?state=s1'); - expect(calls.imports[0].credentials).toMatchObject({ ETSY_CLIENT_ID: 'ks', ETSY_CLIENT_SECRET: 'ss' }); + expect(calls.imports[0].credentials).toMatchObject({ ETSY_CLIENT_ID: 'a1b2c3d4e5f6g7h8i9j0k1l2', ETSY_CLIENT_SECRET: 's3cr3t0abc' }); expect(calls.authorize).toEqual([{ returnTo: '/connectors/setup/etsy?connector=c9&step=done' }]); }); @@ -143,6 +143,28 @@ test('OAuth: back from the provider, shows the connector ready', async ({ page } await expect(page.getByRole('link', { name: 'Back to Claude' })).toBeVisible(); }); +test('OAuth: a sign-in that worked with wrong app keys goes back to the keys, not to "ready"', async ({ page }) => { + // Etsy's token exchange does not check the shared secret; the first API + // call does. Seen in production: users told "is ready", then every call 403. + await setup(page, { + test: { ok: false, kind: 'auth_failed', httpStatus: 403, message: 'Invalid API key: should be in the format keystring:shared_secret.' }, + }); + await page.goto('/connectors/setup/etsy?connector=c9&step=done&from=claude'); + await expect(page.getByText('Etsy Open API v3 did not accept these credentials.')).toBeVisible(); + await expect(page.getByText('should be in the format keystring:shared_secret')).toBeVisible(); + await expect(page.getByRole('heading', { name: 'Etsy Open API v3 is ready' })).toHaveCount(0); + await expect(page.getByLabel('Shared secret')).toBeVisible(); +}); + +test('a value that does not match the pattern says what to check', async ({ page }) => { + await setup(page); + await page.goto('/connectors/setup/etsy'); + await page.getByLabel('Keystring').fill('a1b2c3d4e5f6g7h8i9j0k1l2'); + await page.getByLabel('Shared secret').fill('a1b2c3d4e5f6g7h8i9j0k1l2:abc'); + await page.getByRole('button', { name: 'Save and sign in to Etsy' }).click(); + await expect(page.getByText('Paste the Shared secret alone')).toBeVisible(); +}); + test('finishing an existing connector keeps a stored secret left empty', async ({ page }) => { const calls = await setup(page, { connector: { id: 'c9', setupStatus: 'needs_input', envVars: { LEXWARE_API_KEY: '' }, maskedEnvVars: ['LEXWARE_API_KEY'] }, diff --git a/scripts/validate-adapters.mjs b/scripts/validate-adapters.mjs index c4862549..9fab723e 100644 --- a/scripts/validate-adapters.mjs +++ b/scripts/validate-adapters.mjs @@ -249,7 +249,7 @@ export function validateAdapter(adapter, file, region) { const meta = adapter.envVarMeta; const declared = [...(adapter.requiredEnvVars || []), ...(Array.isArray(adapter.optionalEnvVars) ? adapter.optionalEnvVars : [])]; const KINDS = new Set(['address', 'credential', 'setting']); - const FIELDS = new Set(['label', 'kind', 'secret', 'help', 'example', 'pattern', 'link', 'advanced']); + const FIELDS = new Set(['label', 'kind', 'secret', 'help', 'example', 'pattern', 'patternMessage', 'link', 'advanced']); if (!meta || typeof meta !== 'object' || Array.isArray(meta)) { errors.push(error('env-meta-shape', 'envVarMeta', 'envVarMeta must map a variable name to its description', 'Use { "MY_VAR": { "label": "…", "help": "…" } }.', 'adapter-fields')); } else {