From e9365a307231eb1fa6c7e7e65f045df26818d0a9 Mon Sep 17 00:00:00 2001 From: Craig Martin Date: Mon, 31 Aug 2026 12:37:19 -0400 Subject: [PATCH] Redact store signup JWTs from analytics payloads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `store stripe-auth` takes a signup JWT that is a bearer credential for the target store. Command arguments are reported to Monorail verbatim in `sensitive.args`, and `sanitizePayload` only knew about Theme Access passwords, so every invocation sent the credential off the machine — including the happy path where the browser opens and nothing is printed. Redact the signup credential in the three shapes the payload can carry it: as a command-line flag value, as a JSON key, and as a URL query parameter. The bare-value alternative now treats a JSON escape as part of the value. Applied to the existing store-password rule this also fixes a latent failure where a quoted value produced a string that no longer parsed, which threw inside `sanitizePayload` and dropped the whole event. Co-Authored-By: Claude Opus 5 (1M context) Assisted-By: devx/39092f35-5041-4a88-ab8d-808c98322495 --- .../cli-kit/src/public/node/analytics.test.ts | 147 ++++++++++++++++++ packages/cli-kit/src/public/node/analytics.ts | 10 +- 2 files changed, 155 insertions(+), 2 deletions(-) diff --git a/packages/cli-kit/src/public/node/analytics.test.ts b/packages/cli-kit/src/public/node/analytics.test.ts index 15f64abd05b..ea764c23fcc 100644 --- a/packages/cli-kit/src/public/node/analytics.test.ts +++ b/packages/cli-kit/src/public/node/analytics.test.ts @@ -429,6 +429,153 @@ describe('event tracking', () => { }) }) + test('does not send signup JWTs passed as command arguments to Monorail', async () => { + await inProjectWithFile('package.json', async (args) => { + // Given + const signupJwt = 'eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiJzdG9yZSJ9.s1gn4tur3' + const commandContent = {command: 'stripe-auth', topic: 'store'} + const argsWithSignup = args.concat(['--signup', signupJwt, '--scopes', `--signup=${signupJwt}`]) + await startAnalytics({commandContent, args: argsWithSignup, currentTime: currentDate.getTime() - 100}) + + // When + const config = { + runHook: vi.fn().mockResolvedValue({successes: [], failures: []}), + plugins: [], + } as any + await reportAnalyticsEvent({config, exitMode: 'ok'}) + await sendReportedAnalyticsPayload() + + // Then + expect(publishEventMock).toHaveBeenCalledOnce() + const sensitivePayload = publishEventMock.mock.calls[0]![2] + expect(sensitivePayload.args).toContain('--signup *****') + expect(sensitivePayload.args).toContain('--signup=*****') + expect(JSON.stringify(sensitivePayload)).not.toContain('s1gn4tur3') + }) + }) + + test('does not send signup JWTs that were quoted on the command line to Monorail', async () => { + await inProjectWithFile('package.json', async (args) => { + // Given + const signupJwt = 'eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiJzdG9yZSJ9.s1gn4tur3' + const commandContent = {command: 'stripe-auth', topic: 'store'} + const argsWithSignup = args.concat(['--signup', `"${signupJwt}"`]) + await startAnalytics({commandContent, args: argsWithSignup, currentTime: currentDate.getTime() - 100}) + + // When + const config = { + runHook: vi.fn().mockResolvedValue({successes: [], failures: []}), + plugins: [], + } as any + await reportAnalyticsEvent({config, exitMode: 'ok'}) + await sendReportedAnalyticsPayload() + + // Then + expect(publishEventMock).toHaveBeenCalledOnce() + const sensitivePayload = publishEventMock.mock.calls[0]![2] + expect(sensitivePayload.args).toContain('--signup *****') + expect(JSON.stringify(sensitivePayload)).not.toContain('s1gn4tur3') + }) + }) + + test('does not send signup environment flags to Monorail', async () => { + await inProjectWithFile('package.json', async (args) => { + const commandContent = {command: 'stripe-auth', topic: 'store'} + await startAnalytics({commandContent, args, currentTime: currentDate.getTime() - 100}) + await addSensitiveMetadata(() => ({ + environmentFlags: JSON.stringify({signup: 'eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiJzdG9yZSJ9.s1gn4tur3'}), + })) + + const config = { + runHook: vi.fn().mockResolvedValue({successes: [], failures: []}), + plugins: [], + } as any + await reportAnalyticsEvent({config, exitMode: 'ok'}) + await sendReportedAnalyticsPayload() + + expect(publishEventMock).toHaveBeenCalledOnce() + expect(publishEventMock.mock.calls[0]![2]).toMatchObject({ + cmd_all_environment_flags: JSON.stringify({signup: '*****'}), + }) + }) + }) + + test('does not send signup credentials carried in an authorization URL to Monorail', async () => { + await inProjectWithFile('package.json', async (args) => { + // Given + const commandContent = {command: 'stripe-auth', topic: 'store'} + await startAnalytics({commandContent, args, currentTime: currentDate.getTime() - 100}) + + // When + const config = { + runHook: vi.fn().mockResolvedValue({successes: [], failures: []}), + plugins: [], + } as any + await reportAnalyticsEvent({ + config, + errorMessage: + 'Could not open https://shop.myshopify.com/admin/oauth/authorize?client_id=abc&signup=eyJhbGciOiJIUzI1NiJ9.s1gn4tur3&state=xyz', + exitMode: 'unexpected_error', + }) + await sendReportedAnalyticsPayload() + + // Then + expect(publishEventMock).toHaveBeenCalledOnce() + const sensitivePayload = publishEventMock.mock.calls[0]![2] + expect(sensitivePayload.error_message).toContain('signup=*****&state=xyz') + expect(JSON.stringify(sensitivePayload)).not.toContain('s1gn4tur3') + }) + }) + + test('sends URLs whose path merely mentions signup without redacting them', async () => { + await inProjectWithFile('package.json', async (args) => { + // Given + const commandContent = {command: 'dev', topic: 'app'} + await startAnalytics({commandContent, args, currentTime: currentDate.getTime() - 100}) + + // When + const config = { + runHook: vi.fn().mockResolvedValue({successes: [], failures: []}), + plugins: [], + } as any + await reportAnalyticsEvent({ + config, + errorMessage: 'Create an account at https://partners.shopify.com/signup and retry with from_signup=true', + exitMode: 'unexpected_error', + }) + await sendReportedAnalyticsPayload() + + // Then + expect(publishEventMock).toHaveBeenCalledOnce() + expect(publishEventMock.mock.calls[0]![2].error_message).toBe( + 'Create an account at https://partners.shopify.com/signup and retry with from_signup=true', + ) + }) + }) + + test('sends analytics when a redacted flag value was quoted on the command line', async () => { + await inProjectWithFile('package.json', async (args) => { + // Given + const commandContent = {command: 'dev', topic: 'app'} + const argsWithPassword = args.concat(['--store-password', '"store secret"']) + await startAnalytics({commandContent, args: argsWithPassword, currentTime: currentDate.getTime() - 100}) + + // When + const config = { + runHook: vi.fn().mockResolvedValue({successes: [], failures: []}), + plugins: [], + } as any + await reportAnalyticsEvent({config, exitMode: 'ok'}) + await sendReportedAnalyticsPayload() + + // Then + expect(publishEventMock).toHaveBeenCalledOnce() + const sensitivePayload = publishEventMock.mock.calls[0]![2] + expect(sensitivePayload.args).toContain('--store-password *****') + expect(JSON.stringify(sensitivePayload)).not.toContain('store secret') + }) + }) + test('sends only allowlisted Shopify environment variables in sensitive payload', async () => { const originalShopifyInvokedBy = process.env.SHOPIFY_INVOKED_BY const originalShopifyCliAgent = process.env.SHOPIFY_CLI_AGENT diff --git a/packages/cli-kit/src/public/node/analytics.ts b/packages/cli-kit/src/public/node/analytics.ts index d20f7c90a8f..574783f4af5 100644 --- a/packages/cli-kit/src/public/node/analytics.ts +++ b/packages/cli-kit/src/public/node/analytics.ts @@ -267,11 +267,17 @@ async function buildPayload({ function sanitizePayload(payload: T): T { const payloadString = JSON.stringify(payload) - // Remove Theme Access passwords from the payload + // Redaction runs on the serialized payload, so a flag value that was quoted arrives with its quotes + // escaped. Every flag rule below matches that escaped form first, and its bare-value alternative + // treats a JSON escape as part of the value, so a value can never be redacted only halfway or leave + // behind a string that no longer parses. const sanitizedPayloadString = payloadString .replace(/shptka_\w*/g, '*****') - .replace(/(--store-password(?:=|\s+))(?:"[^"]*"|'[^']*'|[^\s"]+)/g, '$1*****') + .replace(/(--store-password(?:=|\s+))(?:\\"[^"\\]*\\"|'[^']*'|(?:\\.|[^\s"\\])+)/g, '$1*****') .replace(/((?:store-password|SHOPIFY_FLAG_STORE_PASSWORD)\\?":\\?")[^"\\]*/g, '$1*****') + .replace(/(--signup(?:=|\s+))(?:\\"[^"\\]*\\"|'[^']*'|(?:\\.|[^\s"\\])+)/g, '$1*****') + .replace(/((?:signup|SHOPIFY_FLAG_SIGNUP)\\?":\\?")[^"\\]*/g, '$1*****') + .replace(/(?