Limit SHOPIFY_ environment variables and credential flags in analytics

packages/cli-kit/src/private/node/analytics/redact-output.ts

@@ -0,0 +1,19 @@
+/**
+ * Redacts the copy of the analytics payload that gets printed.
+ *
+ * The payload itself is already sanitized for Monorail. This is the extra pass for
+ * the `outputDebug` sinks that `--verbose` turns on, where the audience is a
+ * terminal and whatever scrapes it rather than a sensitive Monorail field.
+ *
+ * `key` is the whole reason this exists. It marks a credential in the environment
+ * -- SHOPIFY_PROXY_KEY holds a signed token, SHOPIFY_FLAG_GRAPHIQL_KEY is derived
+ * from the app secret -- but `api_key` is an app's public client ID that Monorail
+ * is meant to receive, so the payload rules leave the name alone.
+ *
+ * @param payload - The already-sanitized analytics payload.
+ * @returns A copy with the values of `key`-named entries replaced.
+ */
+export function redactForOutput<T>(payload: T): T {
+  const payloadString = JSON.stringify(payload)
+  return JSON.parse(payloadString.replace(/([\w.-]*key[\w.-]*\\*":\\*")[^"\\]*/gi, '$1*****'))
+}

packages/cli-kit/src/public/node/analytics.test.ts

@@ -32,6 +32,12 @@ vi.mock('../../version.js')
 vi.mock('./monorail.js')
 vi.mock('./cli.js')
 vi.mock('./error-handler.js')
+// Rate limiting can short-circuit reporting, which would make these tests depend
+// on how many times the suite has run locally today.
+vi.mock('../../private/node/conf-store.js', async (importOriginal) => ({
+  ...(await importOriginal<typeof import('../../private/node/conf-store.js')>()),
+  runWithRateLimit: vi.fn(async ({task}: {task: () => Promise<void>}) => task()),
+}))
 
 function restoreEnvVariable(key: string, value: string | undefined): void {
   if (value === undefined) {
@@ -251,18 +257,74 @@ describe('event tracking', () => {
     })
   })
 
-  test('sends SHOPIFY_ environment variables in sensitive payload', async () => {
-    const originalShopifyTestVar = process.env.SHOPIFY_TEST_VAR
-    const originalShopifyAnotherVar = process.env.SHOPIFY_ANOTHER_VAR
-    const originalShopifyFlagStorePassword = process.env.SHOPIFY_FLAG_STORE_PASSWORD
-    const originalNotShopifyVar = process.env.NOT_SHOPIFY_VAR
-    process.env.SHOPIFY_TEST_VAR = 'test_value'
-    process.env.SHOPIFY_ANOTHER_VAR = 'another_value'
-    process.env.SHOPIFY_FLAG_STORE_PASSWORD = 'store-secret'
-    process.env.NOT_SHOPIFY_VAR = 'should_not_appear'
+  async function withEnvironment(variables: {[key: string]: string}, execute: () => Promise<void>): Promise<void> {
+    const originalValues = Object.keys(variables).map((key): [string, string | undefined] => [key, process.env[key]])
+    Object.entries(variables).forEach(([key, value]) => {
+      process.env[key] = value
+    })
 
     try {
+      await execute()
+    } finally {
+      originalValues.forEach(([key, value]) => restoreEnvVariable(key, value))
+    }
+  }
+
+  // Every SHOPIFY_ variable is reported -- callers identify themselves by setting
+  // their own, and shopify-function-test-helpers sets SHOPIFY_INVOKED_BY -- so it
+  // is the sanitizer that has to keep credential values out.
+  test('sends SHOPIFY_ environment variables with credential values redacted', async () => {
+    await withEnvironment(
+      {
+        SHOPIFY_CLI_AGENT: 'some-agent',
+        SHOPIFY_INVOKED_BY: 'function-test-helpers',
+        SHOPIFY_CLI_PARTNERS_TOKEN: 'partners_secret',
+        NOT_SHOPIFY_VAR: 'should_not_appear',
+      },
+      async () => {
+        await inProjectWithFile('package.json', async (args) => {
+          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, exitMode: 'ok'})
+
+          // Then
+          const sensitivePayload = publishEventMock.mock.calls[0]![2]
+          expect(publishEventMock).toHaveBeenCalledOnce()
+
+          const shopifyVars = JSON.parse(sensitivePayload.env_shopify_variables as string)
+          expect(shopifyVars).toMatchObject({
+            SHOPIFY_CLI_AGENT: 'some-agent',
+            SHOPIFY_INVOKED_BY: 'function-test-helpers',
+            SHOPIFY_CLI_PARTNERS_TOKEN: '*****',
+          })
+          expect(shopifyVars).not.toHaveProperty('NOT_SHOPIFY_VAR')
+        })
+      },
+    )
+  })
+
+  // The allowlist keeps these out of env_shopify_variables, but they should not
+  // reach the payload through any other field either. Kept in sync with the
+  // credentials in `environmentVariables` in private/node/constants.ts.
+  test('does not send credential environment variables anywhere in the payload', async () => {
+    const credentials = {
+      SHOPIFY_APP_AUTOMATION_TOKEN: 'app_automation_secret',
+      SHOPIFY_CLI_PARTNERS_TOKEN: 'partners_secret',
+      SHOPIFY_CLI_IDENTITY_TOKEN: 'identity_secret',
+      SHOPIFY_CLI_REFRESH_TOKEN: 'refresh_secret',
+      SHOPIFY_CLI_THEME_TOKEN: 'theme_secret',
+      SHOPIFY_FLAG_STORE_PASSWORD: 'store_password_secret',
+    }
+
+    await withEnvironment(credentials, async () => {
       await inProjectWithFile('package.json', async (args) => {
+        // Given
         const commandContent = {command: 'dev', topic: 'app'}
         await startAnalytics({commandContent, args, currentTime: currentDate.getTime() - 100})
 
@@ -274,23 +336,210 @@ describe('event tracking', () => {
         await reportAnalyticsEvent({config, exitMode: 'ok'})
 
         // Then
-        const sensitivePayload = publishEventMock.mock.calls[0]![2]
         expect(publishEventMock).toHaveBeenCalledOnce()
-        expect(sensitivePayload).toHaveProperty('env_shopify_variables')
-        expect(sensitivePayload.env_shopify_variables).toBeDefined()
-
-        const shopifyVars = JSON.parse(sensitivePayload.env_shopify_variables as string)
-        expect(shopifyVars).toHaveProperty('SHOPIFY_TEST_VAR', 'test_value')
-        expect(shopifyVars).toHaveProperty('SHOPIFY_ANOTHER_VAR', 'another_value')
-        expect(shopifyVars).toHaveProperty('SHOPIFY_FLAG_STORE_PASSWORD', '*****')
-        expect(shopifyVars).not.toHaveProperty('NOT_SHOPIFY_VAR')
+        const [, publicPayload, sensitivePayload] = publishEventMock.mock.calls[0]!
+        const serializedPayload = JSON.stringify({publicPayload, sensitivePayload})
+        Object.values(credentials).forEach((secret) => {
+          expect(serializedPayload).not.toContain(secret)
+        })
       })
-    } finally {
-      restoreEnvVariable('SHOPIFY_TEST_VAR', originalShopifyTestVar)
-      restoreEnvVariable('SHOPIFY_ANOTHER_VAR', originalShopifyAnotherVar)
-      restoreEnvVariable('SHOPIFY_FLAG_STORE_PASSWORD', originalShopifyFlagStorePassword)
-      restoreEnvVariable('NOT_SHOPIFY_VAR', originalNotShopifyVar)
-    }
+    })
+  })
+
+  // `key` marks a credential in the environment -- SHOPIFY_PROXY_KEY holds a signed
+  // token -- but `api_key` is an app's public client ID, so the name is only
+  // redacted on the way to a terminal, not on the way to Monorail.
+  test('sends key-named environment variables to Monorail but does not print them', async () => {
+    await withEnvironment({SHOPIFY_PROXY_KEY: 'proxy_key_value'}, async () => {
+      await inProjectWithFile('package.json', async (args) => {
+        // Given
+        const commandContent = {command: 'dev', topic: 'app'}
+        await startAnalytics({commandContent, args, currentTime: currentDate.getTime() - 100})
+        const config = {
+          runHook: vi.fn().mockResolvedValue({successes: [], failures: []}),
+          plugins: [],
+        } as any
+
+        // When
+        await reportAnalyticsEvent({config, exitMode: 'ok'})
+
+        // Then
+        const reportedVars = publishEventMock.mock.calls[0]![2].env_shopify_variables as string
+        expect(JSON.parse(reportedVars)).toMatchObject({SHOPIFY_PROXY_KEY: 'proxy_key_value'})
+
+        // And when the same payload is printed instead of sent
+        vi.mocked(analyticsDisabled).mockReturnValue(true)
+        const outputMock = mockAndCaptureOutput()
+        await reportAnalyticsEvent({config, exitMode: 'ok'})
+
+        const debugOutput = outputMock.debug()
+        expect(debugOutput).toMatch('Skipping command analytics, payload:')
+        expect(debugOutput).not.toContain('proxy_key_value')
+      })
+    })
+  })
+
+  // The payload is printed by outputDebug even when analytics are disabled, so
+  // asserting on the payload object alone would miss this sink.
+  test('does not print credentials when analytics are disabled', async () => {
+    await inProjectWithFile('package.json', async (args) => {
+      // Given
+      vi.mocked(analyticsDisabled).mockReturnValue(true)
+      const commandContent = {command: 'dev', topic: 'app'}
+      const argsWithCredentials = args.concat(['--client-secret', 'client_secret_value'])
+      await startAnalytics({commandContent, args: argsWithCredentials, currentTime: currentDate.getTime() - 100})
+      const outputMock = mockAndCaptureOutput()
+
+      // When
+      const config = {
+        runHook: vi.fn().mockResolvedValue({successes: [], failures: []}),
+        plugins: [],
+      } as any
+      await reportAnalyticsEvent({config, exitMode: 'ok'})
+
+      // Then
+      const debugOutput = outputMock.debug()
+      expect(debugOutput).toMatch('Skipping command analytics, payload:')
+      expect(debugOutput).not.toContain('client_secret_value')
+    })
+  })
+
+  test('does not send credentials passed as flags to Monorail', async () => {
+    await inProjectWithFile('package.json', async (args) => {
+      // Given
+      const commandContent = {command: 'dev', topic: 'app'}
+      const argsWithCredentials = args.concat([
+        '--client-secret',
+        'client_secret_value',
+        '--token=token_value',
+        '--password',
+        'plain_password_value',
+      ])
+      await startAnalytics({commandContent, args: argsWithCredentials, currentTime: currentDate.getTime() - 100})
+
+      // When
+      const config = {
+        runHook: vi.fn().mockResolvedValue({successes: [], failures: []}),
+        plugins: [],
+      } as any
+      await reportAnalyticsEvent({config, exitMode: 'ok'})
+
+      // Then
+      expect(publishEventMock).toHaveBeenCalledOnce()
+      const reportedArgs = publishEventMock.mock.calls[0]![2].args as string
+      expect(reportedArgs).toContain('--client-secret *****')
+      expect(reportedArgs).toContain('--token=*****')
+      expect(reportedArgs).toContain('--password *****')
+      expect(reportedArgs).not.toContain('client_secret_value')
+      expect(reportedArgs).not.toContain('token_value')
+      expect(reportedArgs).not.toContain('plain_password_value')
+    })
+  })
+
+  // Values that sit near the boundaries the rule has to respect: JSON escapes
+  // quotes and backslashes, so a replacement that cuts one in half would leave the
+  // payload unparseable. A value containing a space is not covered -- the space
+  // ends the match -- but that needs quotes inside argv to arise.
+  test.each([
+    ['is empty', ''],
+    ['contains quotes', '"cs_secret_value"'],
+    ['contains a backslash', 'cs\\secret_value'],
+  ])('redacts a credential flag whose value %s', async (_description, credentialValue) => {
+    await inProjectWithFile('package.json', async (args) => {
+      // Given
+      const commandContent = {command: 'dev', topic: 'app'}
+      await startAnalytics({
+        commandContent,
+        args: args.concat(['--client-secret', credentialValue]),
+        currentTime: currentDate.getTime() - 100,
+      })
+
+      // When
+      const config = {
+        runHook: vi.fn().mockResolvedValue({successes: [], failures: []}),
+        plugins: [],
+      } as any
+      await reportAnalyticsEvent({config, exitMode: 'ok'})
+
+      // Then
+      expect(publishEventMock).toHaveBeenCalledOnce()
+      expect(sendErrorToBugsnag).not.toHaveBeenCalled()
+      const reportedArgs = publishEventMock.mock.calls[0]![2].args as string
+      expect(reportedArgs).toContain('--client-secret *****')
+      expect(reportedArgs).not.toContain('secret_value')
+    })
+  })
+
+  // A failing subprocess reports the command line it ran, so a credential flag can
+  // reach the payload as part of some other value rather than through `args`.
+  test('does not send credential flags quoted inside an error message', 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: 'Command failed: shopify theme dev --store-password store_secret_value',
+        exitMode: 'expected_error',
+      })
+
+      // Then
+      expect(publishEventMock).toHaveBeenCalledOnce()
+      const reportedError = publishEventMock.mock.calls[0]![2].error_message as string
+      expect(reportedError).toContain('--store-password *****')
+      expect(reportedError).not.toContain('store_secret_value')
+    })
+  })
+
+  // `metadata` is serialised JSON, and a plugin is free to put serialised JSON
+  // inside it, so credential keys can sit more than one escaping level deep.
+  test('does not send credentials nested inside serialized metadata', async () => {
+    await inProjectWithFile('package.json', async (args) => {
+      // Given
+      const commandContent = {command: 'dev', topic: 'app'}
+      await startAnalytics({commandContent, args, currentTime: currentDate.getTime() - 100})
+      await addSensitiveMetadata(() => ({
+        environmentFlags: JSON.stringify({nested: JSON.stringify({'client-secret': 'nested_secret_value'})}),
+      }))
+
+      // When
+      const config = {
+        runHook: vi.fn().mockResolvedValue({successes: [], failures: []}),
+        plugins: [],
+      } as any
+      await reportAnalyticsEvent({config, exitMode: 'ok'})
+
+      // Then
+      expect(publishEventMock).toHaveBeenCalledOnce()
+      const environmentFlags = publishEventMock.mock.calls[0]![2].cmd_all_environment_flags as string
+      expect(environmentFlags).not.toContain('nested_secret_value')
+      expect(JSON.parse(JSON.parse(environmentFlags).nested)).toStrictEqual({'client-secret': '*****'})
+    })
+  })
+
+  test('keeps reporting the auth method, which is not a credential', async () => {
+    await inProjectWithFile('package.json', async (args) => {
+      // Given
+      const commandContent = {command: 'dev', topic: 'app'}
+      await startAnalytics({commandContent, args, currentTime: currentDate.getTime() - 100})
+      setLastSeenAuthMethod('partners_token')
+
+      // When
+      const config = {
+        runHook: vi.fn().mockResolvedValue({successes: [], failures: []}),
+        plugins: [],
+      } as any
+      await reportAnalyticsEvent({config, exitMode: 'ok'})
+
+      // Then
+      expect(publishEventMock.mock.calls[0]![1]).toMatchObject({env_auth_method: 'partners_token'})
+    })
   })
 
   test('does nothing when analytics are disabled', async () => {

packages/cli-kit/src/public/node/analytics.ts

@@ -13,6 +13,7 @@ import {
   RuntimeData,
 } from '../../private/node/analytics/storage.js'
 import {getEnvironmentData, getSensitiveEnvironmentData} from '../../private/node/analytics.js'
+import {redactForOutput} from '../../private/node/analytics/redact-output.js'
 import {CLI_KIT_VERSION} from '../common/version.js'
 import {recordMetrics} from '../../private/node/otel-metrics.js'
 import {runWithRateLimit} from '../../private/node/conf-store.js'
@@ -59,14 +60,18 @@ export async function reportAnalyticsEvent(options: ReportAnalyticsEventOptions)
       },
     })
     if (!withinRateLimit) {
-      outputDebug(outputContent`Skipping command analytics due to rate limiting, payload: ${outputToken.json(payload)}`)
+      outputDebug(
+        outputContent`Skipping command analytics due to rate limiting, payload: ${outputToken.json(
+          redactForOutput(payload),
+        )}`,
+      )
       return
     }
 
     const skipMonorailAnalytics = !alwaysLogAnalytics() && analyticsDisabled()
     const skipMetricAnalytics = !alwaysLogMetrics() && analyticsDisabled()
     if (skipMonorailAnalytics || skipMetricAnalytics) {
-      outputDebug(outputContent`Skipping command analytics, payload: ${outputToken.json(payload)}`)
+      outputDebug(outputContent`Skipping command analytics, payload: ${outputToken.json(redactForOutput(payload))}`)
     }
 
     const doMonorail = async () => {
@@ -197,11 +202,16 @@ async function buildPayload({config, errorMessage, exitMode}: ReportAnalyticsEve
 
 function sanitizePayload<T>(payload: T): T {
   const payloadString = JSON.stringify(payload)
-  // Remove Theme Access passwords from the payload
   const sanitizedPayloadString = payloadString
+    // Remove Theme Access passwords from the payload
     .replace(/shptka_\w*/g, '*****')
-    .replace(/(--store-password(?:=|\s+))(?:"[^"]*"|'[^']*'|[^\s"]+)/g, '$1*****')
-    .replace(/((?:store-password|SHOPIFY_FLAG_STORE_PASSWORD)\\?":\\?")[^"\\]*/g, '$1*****')
+    // Credential flags, in `args` and in any command line quoted into a message.
+    // `\\.` matches an escape sequence whole so a replacement can't cut a `\"` in
+    // half and leave the payload unparseable.
+    .replace(/(--[\w-]*(?:password|token|secret|credential)(?:=|\s+))(?:\\.|[^\s"\\])*/gi, '$1*****')
+    // Credential keys, e.g. `"store-password":"abc"`. The quotes take any number of
+    // backslashes so keys inside already-serialised JSON are covered at any depth.
+    .replace(/([\w.-]*(?:password|token|secret|credential)[\w.-]*\\*":\\*")[^"\\]*/gi, '$1*****')
   return JSON.parse(sanitizedPayloadString)
 }