From 5f772205ed57218299c53549bdae26dfe40d86b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Wed, 29 Jul 2026 14:00:38 +0200 Subject: [PATCH 01/13] Limit SHOPIFY_ environment variables and credential flags in analytics The analytics payload collected every SHOPIFY_* environment variable, which includes the credentials the CLI reads from the environment, and the payload sanitizer only recognised the shptka_ theme token format. Values for other credential env vars and for credential-bearing flags were reported verbatim and printed by the verbose debug sinks. - Collect an allowlist of the agent identification variables the field was added for, instead of matching on the SHOPIFY_ prefix. - Generalise the flag redaction from --store-password to any flag whose name contains password, token, secret or credential. - Apply the same name matching to object keys, so credentials arriving through plugin metadata or environment flags are covered too. Excludes `key` and `auth` from the name matching on purpose: api_key is an app's public client ID and env_auth_method is legitimate telemetry. Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/analytics-redact-shopify-env.md | 5 + .../cli-kit/src/private/node/analytics.ts | 28 ++- .../cli-kit/src/public/node/analytics.test.ts | 159 +++++++++++++++--- packages/cli-kit/src/public/node/analytics.ts | 21 ++- 4 files changed, 182 insertions(+), 31 deletions(-) create mode 100644 .changeset/analytics-redact-shopify-env.md diff --git a/.changeset/analytics-redact-shopify-env.md b/.changeset/analytics-redact-shopify-env.md new file mode 100644 index 00000000000..1fbbfb61800 --- /dev/null +++ b/.changeset/analytics-redact-shopify-env.md @@ -0,0 +1,5 @@ +--- +'@shopify/cli-kit': patch +--- + +Only include allowlisted SHOPIFY_ environment variables and redact credential flag values in CLI analytics diff --git a/packages/cli-kit/src/private/node/analytics.ts b/packages/cli-kit/src/private/node/analytics.ts index 499fc8ef24d..977b40d088a 100644 --- a/packages/cli-kit/src/private/node/analytics.ts +++ b/packages/cli-kit/src/private/node/analytics.ts @@ -107,14 +107,28 @@ export async function getSensitiveEnvironmentData(config: Interfaces.Config) { } } +// Agent callers can identify themselves today via SHOPIFY_* environment +// variables. The current contract is intentionally lightweight and is kept in +// the sensitive payload until we prove which dimensions deserve first-class +// Monorail fields. +// +// This is an allowlist rather than a `SHOPIFY_*` prefix match on purpose: the CLI +// reads several of its own credentials from the environment (see +// `environmentVariables` in ./constants.ts), so a prefix match collects access +// tokens along with the agent dimensions we actually want. Adding a new variable +// here should come with a check that it can never hold a secret. +const REPORTED_SHOPIFY_ENVIRONMENT_VARIABLES = new Set([ + 'SHOPIFY_CLI_AGENT', + 'SHOPIFY_CLI_AGENT_VERSION', + 'SHOPIFY_CLI_AGENT_RUN_ID', + 'SHOPIFY_CLI_AGENT_SESSION_ID', + 'SHOPIFY_CLI_AGENT_PROVIDER', +]) + function getShopifyEnvironmentVariables() { - // Agent callers can identify themselves today via SHOPIFY_* environment - // variables. The current contract is intentionally lightweight and is kept in - // the sensitive payload until we prove which dimensions deserve first-class - // Monorail fields, e.g. SHOPIFY_CLI_AGENT, SHOPIFY_CLI_AGENT_VERSION, - // SHOPIFY_CLI_AGENT_RUN_ID, SHOPIFY_CLI_AGENT_SESSION_ID, and - // SHOPIFY_CLI_AGENT_PROVIDER. - return Object.fromEntries(Object.entries(process.env).filter(([key]) => key.startsWith('SHOPIFY_'))) + return Object.fromEntries( + Object.entries(process.env).filter(([key]) => REPORTED_SHOPIFY_ENVIRONMENT_VARIABLES.has(key)), + ) } function getPluginNames(config: Interfaces.Config) { diff --git a/packages/cli-kit/src/public/node/analytics.test.ts b/packages/cli-kit/src/public/node/analytics.test.ts index 97c5d95d202..52ec10b1121 100644 --- a/packages/cli-kit/src/public/node/analytics.test.ts +++ b/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 short-circuits reporting before the analytics-disabled branch is +// reached, which would make the assertions below depend on prior local runs. +vi.mock('../../private/node/conf-store.js', async (importOriginal) => ({ + ...(await importOriginal()), + runWithRateLimit: vi.fn(async ({task}: {task: () => Promise}) => task()), +})) function restoreEnvVariable(key: string, value: string | undefined): void { if (value === undefined) { @@ -251,17 +257,32 @@ 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' + // Every SHOPIFY_* variable the CLI reads a credential from. Kept in sync with + // `environmentVariables` in private/node/constants.ts. + const credentialEnvironmentVariables = { + SHOPIFY_APP_AUTOMATION_TOKEN: 'atkn_app_automation_secret', + SHOPIFY_CLI_PARTNERS_TOKEN: 'atkn_partners_secret', + SHOPIFY_CLI_IDENTITY_TOKEN: 'identity_secret', + SHOPIFY_CLI_REFRESH_TOKEN: 'refresh_secret', + SHOPIFY_CLI_THEME_TOKEN: 'shptka_theme_secret', + SHOPIFY_FLAG_STORE_PASSWORD: 'store_secret', + } + + async function withEnvironment(variables: {[key: string]: string}, execute: () => Promise): Promise { + 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)) + } + } + + test('only sends allowlisted SHOPIFY_ environment variables in sensitive payload', async () => { + await withEnvironment({SHOPIFY_CLI_AGENT: 'some-agent', SHOPIFY_TEST_VAR: 'test_value'}, async () => { await inProjectWithFile('package.json', async (args) => { const commandContent = {command: 'dev', topic: 'app'} await startAnalytics({commandContent, args, currentTime: currentDate.getTime() - 100}) @@ -276,21 +297,117 @@ describe('event tracking', () => { // 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') + expect(shopifyVars).toStrictEqual({SHOPIFY_CLI_AGENT: 'some-agent'}) }) - } finally { - restoreEnvVariable('SHOPIFY_TEST_VAR', originalShopifyTestVar) - restoreEnvVariable('SHOPIFY_ANOTHER_VAR', originalShopifyAnotherVar) - restoreEnvVariable('SHOPIFY_FLAG_STORE_PASSWORD', originalShopifyFlagStorePassword) - restoreEnvVariable('NOT_SHOPIFY_VAR', originalNotShopifyVar) - } + }) + }) + + test('does not send credential environment variables to Monorail', async () => { + await withEnvironment(credentialEnvironmentVariables, 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 + expect(publishEventMock).toHaveBeenCalledOnce() + const [, publicPayload, sensitivePayload] = publishEventMock.mock.calls[0]! + const serializedPayload = JSON.stringify({publicPayload, sensitivePayload}) + Object.values(credentialEnvironmentVariables).forEach((secret) => { + expect(serializedPayload).not.toContain(secret) + }) + }) + }) + }) + + // 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 withEnvironment(credentialEnvironmentVariables, 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() + // Asserts on the analytics-disabled sink specifically, not the rate-limited one. + expect(debugOutput).toMatch('Skipping command analytics, payload:') + Object.values(credentialEnvironmentVariables).forEach((secret) => { + expect(debugOutput).not.toContain(secret) + }) + 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') + }) + }) + + 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 () => { diff --git a/packages/cli-kit/src/public/node/analytics.ts b/packages/cli-kit/src/public/node/analytics.ts index dcaef13d51c..559687d99e2 100644 --- a/packages/cli-kit/src/public/node/analytics.ts +++ b/packages/cli-kit/src/public/node/analytics.ts @@ -195,13 +195,28 @@ async function buildPayload({config, errorMessage, exitMode}: ReportAnalyticsEve return sanitizePayload(payload) } +// Names that identify a credential wherever they show up in the payload -- as a +// flag (`--client-secret`), an object key (`"store-password"`), or an environment +// variable name (`SHOPIFY_CLI_PARTNERS_TOKEN`). +// +// Deliberately excludes `key` and `auth`: `api_key` is an app's public client ID +// (redacted separately in monorail.ts) and `env_auth_method` reports which auth +// method was used, so matching those would drop legitimate telemetry. +const CREDENTIAL_NAME_PATTERN = 'password|token|secret|credential' + function sanitizePayload(payload: T): T { const payloadString = JSON.stringify(payload) - // Remove Theme Access passwords from the payload const sanitizedPayloadString = payloadString + // Theme Access passwords, which are recognisable wherever they appear. .replace(/shptka_\w*/g, '*****') - .replace(/(--store-password(?:=|\s+))(?:"[^"]*"|'[^']*'|[^\s"]+)/g, '$1*****') - .replace(/((?:store-password|SHOPIFY_FLAG_STORE_PASSWORD)\\?":\\?")[^"\\]*/g, '$1*****') + // Credentials passed as flags, e.g. `--password abc`, `--client-secret=abc`. + .replace( + new RegExp(`(--[\\w-]*(?:${CREDENTIAL_NAME_PATTERN})(?:=|\\s+))(?:"[^"]*"|'[^']*'|[^\\s"]+)`, 'gi'), + '$1*****', + ) + // Credentials held under a credential-shaped key. Keys are matched with an + // optional backslash because nested JSON strings arrive escaped. + .replace(new RegExp(`([\\w.-]*(?:${CREDENTIAL_NAME_PATTERN})[\\w.-]*\\\\?":\\\\?")[^"\\\\]*`, 'gi'), '$1*****') return JSON.parse(sanitizedPayloadString) } From 9042775e9ebf18ea822092c7d80cadd70831a7d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Thu, 30 Jul 2026 12:09:44 +0200 Subject: [PATCH 02/13] Keep the analytics payload parseable and cover nested credential keys The flag rule matched a value as `"[^"]*"` or `[^\s"]+`, neither of which accounts for the payload already being JSON: a literal quote inside a value is `\"` there, so the quoted alternative could only fire by matching the closing quote of `args` and running into the next key. `--client-secret ""` was enough to produce unbalanced JSON, and the JSON.parse at the end of sanitizePayload then threw, dropping the whole event and reporting to Bugsnag. Match escape sequences as a unit instead. The key rule allowed one optional backslash before each quote, so it only reached keys nested a single serialised JSON string deep. `metadata` and `cmd_all_environment_flags` are serialised JSON whose values may be serialised JSON in turn, leaving those credentials unredacted. Allow any number. --- .../cli-kit/src/public/node/analytics.test.ts | 59 +++++++++++++++++++ packages/cli-kit/src/public/node/analytics.ts | 17 ++++-- 2 files changed, 72 insertions(+), 4 deletions(-) diff --git a/packages/cli-kit/src/public/node/analytics.test.ts b/packages/cli-kit/src/public/node/analytics.test.ts index 52ec10b1121..7983ec1fa6f 100644 --- a/packages/cli-kit/src/public/node/analytics.test.ts +++ b/packages/cli-kit/src/public/node/analytics.test.ts @@ -391,6 +391,65 @@ describe('event tracking', () => { }) }) + // Redaction rewrites the serialised payload, so a replacement that lands inside + // an escape sequence or past a closing quote makes the whole payload unparseable + // and drops the event. These values are the ones that get near those boundaries. + test.each([ + ['an empty value', ['--client-secret', '']], + ['a value containing quotes', ['--client-secret', '"cs_secret_value"']], + ['a value containing a backslash', ['--client-secret', 'cs\\secret\\value']], + ])('still reports the event when a credential flag has %s', async (_description, credentialFlag) => { + await inProjectWithFile('package.json', async (args) => { + // Given + const commandContent = {command: 'dev', topic: 'app'} + await startAnalytics({ + commandContent, + args: args.concat(credentialFlag), + 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') + }) + }) + + // `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 diff --git a/packages/cli-kit/src/public/node/analytics.ts b/packages/cli-kit/src/public/node/analytics.ts index 559687d99e2..692ba94ca45 100644 --- a/packages/cli-kit/src/public/node/analytics.ts +++ b/packages/cli-kit/src/public/node/analytics.ts @@ -204,6 +204,13 @@ async function buildPayload({config, errorMessage, exitMode}: ReportAnalyticsEve // method was used, so matching those would drop legitimate telemetry. const CREDENTIAL_NAME_PATTERN = 'password|token|secret|credential' +// A single space-delimited flag value as it appears in the serialised payload: +// either a JSON escape sequence such as `\"` or `\\`, or a character that is +// neither whitespace nor part of one. Consuming escape sequences whole is what +// keeps the replacement from cutting a `\"` in half, which would unbalance the +// surrounding string and make the payload fail to parse. +const SERIALIZED_FLAG_VALUE_PATTERN = '(?:\\\\.|[^\\s"\\\\])*' + function sanitizePayload(payload: T): T { const payloadString = JSON.stringify(payload) const sanitizedPayloadString = payloadString @@ -211,12 +218,14 @@ function sanitizePayload(payload: T): T { .replace(/shptka_\w*/g, '*****') // Credentials passed as flags, e.g. `--password abc`, `--client-secret=abc`. .replace( - new RegExp(`(--[\\w-]*(?:${CREDENTIAL_NAME_PATTERN})(?:=|\\s+))(?:"[^"]*"|'[^']*'|[^\\s"]+)`, 'gi'), + new RegExp(`(--[\\w-]*(?:${CREDENTIAL_NAME_PATTERN})(?:=|\\s+))${SERIALIZED_FLAG_VALUE_PATTERN}`, 'gi'), '$1*****', ) - // Credentials held under a credential-shaped key. Keys are matched with an - // optional backslash because nested JSON strings arrive escaped. - .replace(new RegExp(`([\\w.-]*(?:${CREDENTIAL_NAME_PATTERN})[\\w.-]*\\\\?":\\\\?")[^"\\\\]*`, 'gi'), '$1*****') + // Credentials held under a credential-shaped key. Quotes carry any number of + // leading backslashes so that keys nested one or more serialised JSON strings + // deep -- `metadata` is already-serialised JSON, and its values can be too -- + // are covered as well as top-level ones. + .replace(new RegExp(`([\\w.-]*(?:${CREDENTIAL_NAME_PATTERN})[\\w.-]*\\\\*":\\\\*")[^"\\\\]*`, 'gi'), '$1*****') return JSON.parse(sanitizedPayloadString) } From fd14dc6c973c26a2efea3c28db82130a9b2c43bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Thu, 30 Jul 2026 12:26:03 +0200 Subject: [PATCH 03/13] Trim the redaction comments and a redundant test The allowlist test already asserts the exact contents of env_shopify_variables, so the separate credential-env-var test added nothing. --- .../cli-kit/src/private/node/analytics.ts | 14 +--- .../cli-kit/src/public/node/analytics.test.ts | 79 +++++-------------- packages/cli-kit/src/public/node/analytics.ts | 33 +++----- 3 files changed, 33 insertions(+), 93 deletions(-) diff --git a/packages/cli-kit/src/private/node/analytics.ts b/packages/cli-kit/src/private/node/analytics.ts index 977b40d088a..9367c2e5509 100644 --- a/packages/cli-kit/src/private/node/analytics.ts +++ b/packages/cli-kit/src/private/node/analytics.ts @@ -107,16 +107,10 @@ export async function getSensitiveEnvironmentData(config: Interfaces.Config) { } } -// Agent callers can identify themselves today via SHOPIFY_* environment -// variables. The current contract is intentionally lightweight and is kept in -// the sensitive payload until we prove which dimensions deserve first-class -// Monorail fields. -// -// This is an allowlist rather than a `SHOPIFY_*` prefix match on purpose: the CLI -// reads several of its own credentials from the environment (see -// `environmentVariables` in ./constants.ts), so a prefix match collects access -// tokens along with the agent dimensions we actually want. Adding a new variable -// here should come with a check that it can never hold a secret. +// How agent callers identify themselves, kept in the sensitive payload until we +// prove which dimensions deserve first-class Monorail fields. An allowlist rather +// than a `SHOPIFY_*` prefix match because the CLI also reads its own tokens from +// the environment (see `environmentVariables` in ./constants.ts). const REPORTED_SHOPIFY_ENVIRONMENT_VARIABLES = new Set([ 'SHOPIFY_CLI_AGENT', 'SHOPIFY_CLI_AGENT_VERSION', diff --git a/packages/cli-kit/src/public/node/analytics.test.ts b/packages/cli-kit/src/public/node/analytics.test.ts index 7983ec1fa6f..3bcc15e3eab 100644 --- a/packages/cli-kit/src/public/node/analytics.test.ts +++ b/packages/cli-kit/src/public/node/analytics.test.ts @@ -32,8 +32,8 @@ vi.mock('../../version.js') vi.mock('./monorail.js') vi.mock('./cli.js') vi.mock('./error-handler.js') -// Rate limiting short-circuits reporting before the analytics-disabled branch is -// reached, which would make the assertions below depend on prior local runs. +// 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()), runWithRateLimit: vi.fn(async ({task}: {task: () => Promise}) => task()), @@ -257,17 +257,6 @@ describe('event tracking', () => { }) }) - // Every SHOPIFY_* variable the CLI reads a credential from. Kept in sync with - // `environmentVariables` in private/node/constants.ts. - const credentialEnvironmentVariables = { - SHOPIFY_APP_AUTOMATION_TOKEN: 'atkn_app_automation_secret', - SHOPIFY_CLI_PARTNERS_TOKEN: 'atkn_partners_secret', - SHOPIFY_CLI_IDENTITY_TOKEN: 'identity_secret', - SHOPIFY_CLI_REFRESH_TOKEN: 'refresh_secret', - SHOPIFY_CLI_THEME_TOKEN: 'shptka_theme_secret', - SHOPIFY_FLAG_STORE_PASSWORD: 'store_secret', - } - async function withEnvironment(variables: {[key: string]: string}, execute: () => Promise): Promise { const originalValues = Object.keys(variables).map((key): [string, string | undefined] => [key, process.env[key]]) Object.entries(variables).forEach(([key, value]) => { @@ -304,58 +293,28 @@ describe('event tracking', () => { }) }) - test('does not send credential environment variables to Monorail', async () => { - await withEnvironment(credentialEnvironmentVariables, 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 - expect(publishEventMock).toHaveBeenCalledOnce() - const [, publicPayload, sensitivePayload] = publishEventMock.mock.calls[0]! - const serializedPayload = JSON.stringify({publicPayload, sensitivePayload}) - Object.values(credentialEnvironmentVariables).forEach((secret) => { - expect(serializedPayload).not.toContain(secret) - }) - }) - }) - }) - // 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 withEnvironment(credentialEnvironmentVariables, 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() + 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'}) + // When + const config = { + runHook: vi.fn().mockResolvedValue({successes: [], failures: []}), + plugins: [], + } as any + await reportAnalyticsEvent({config, exitMode: 'ok'}) - // Then - const debugOutput = outputMock.debug() - // Asserts on the analytics-disabled sink specifically, not the rate-limited one. - expect(debugOutput).toMatch('Skipping command analytics, payload:') - Object.values(credentialEnvironmentVariables).forEach((secret) => { - expect(debugOutput).not.toContain(secret) - }) - expect(debugOutput).not.toContain('client_secret_value') - }) + // Then + const debugOutput = outputMock.debug() + expect(debugOutput).toMatch('Skipping command analytics, payload:') + expect(debugOutput).not.toContain('client_secret_value') }) }) diff --git a/packages/cli-kit/src/public/node/analytics.ts b/packages/cli-kit/src/public/node/analytics.ts index 692ba94ca45..5e908c86538 100644 --- a/packages/cli-kit/src/public/node/analytics.ts +++ b/packages/cli-kit/src/public/node/analytics.ts @@ -195,36 +195,23 @@ async function buildPayload({config, errorMessage, exitMode}: ReportAnalyticsEve return sanitizePayload(payload) } -// Names that identify a credential wherever they show up in the payload -- as a -// flag (`--client-secret`), an object key (`"store-password"`), or an environment -// variable name (`SHOPIFY_CLI_PARTNERS_TOKEN`). -// -// Deliberately excludes `key` and `auth`: `api_key` is an app's public client ID -// (redacted separately in monorail.ts) and `env_auth_method` reports which auth -// method was used, so matching those would drop legitimate telemetry. +// Excludes `key` and `auth`, which would drop legitimate telemetry: `api_key` is +// an app's public client ID and `env_auth_method` is the auth method used. const CREDENTIAL_NAME_PATTERN = 'password|token|secret|credential' -// A single space-delimited flag value as it appears in the serialised payload: -// either a JSON escape sequence such as `\"` or `\\`, or a character that is -// neither whitespace nor part of one. Consuming escape sequences whole is what -// keeps the replacement from cutting a `\"` in half, which would unbalance the -// surrounding string and make the payload fail to parse. -const SERIALIZED_FLAG_VALUE_PATTERN = '(?:\\\\.|[^\\s"\\\\])*' +// One flag value, as an escape sequence or a plain character. Matching escapes +// whole stops a replacement from cutting a `\"` in half and unbalancing the string. +const FLAG_VALUE_PATTERN = '(?:\\\\.|[^\\s"\\\\])*' function sanitizePayload(payload: T): T { const payloadString = JSON.stringify(payload) const sanitizedPayloadString = payloadString - // Theme Access passwords, which are recognisable wherever they appear. + // Theme Access passwords, recognisable wherever they appear. .replace(/shptka_\w*/g, '*****') - // Credentials passed as flags, e.g. `--password abc`, `--client-secret=abc`. - .replace( - new RegExp(`(--[\\w-]*(?:${CREDENTIAL_NAME_PATTERN})(?:=|\\s+))${SERIALIZED_FLAG_VALUE_PATTERN}`, 'gi'), - '$1*****', - ) - // Credentials held under a credential-shaped key. Quotes carry any number of - // leading backslashes so that keys nested one or more serialised JSON strings - // deep -- `metadata` is already-serialised JSON, and its values can be too -- - // are covered as well as top-level ones. + // Credential flags, e.g. `--password abc`, `--client-secret=abc`. + .replace(new RegExp(`(--[\\w-]*(?:${CREDENTIAL_NAME_PATTERN})(?:=|\\s+))${FLAG_VALUE_PATTERN}`, 'gi'), '$1*****') + // Credential keys. Quotes take any number of backslashes so that keys inside + // already-serialised JSON, such as `metadata`, are covered at any depth. .replace(new RegExp(`([\\w.-]*(?:${CREDENTIAL_NAME_PATTERN})[\\w.-]*\\\\*":\\\\*")[^"\\\\]*`, 'gi'), '$1*****') return JSON.parse(sanitizedPayloadString) } From 9ef86f04f96c287af9701a31ddc8c6954a2723d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Fri, 31 Jul 2026 13:37:27 +0200 Subject: [PATCH 04/13] Redact credential flags before joining the arguments The flag regex was complicated only because it ran after JSON.stringify, where it had to recognise escape sequences to avoid cutting one in half. Redacting the argument list before it is joined removes the need for it entirely, and covers a value containing a space, which was impossible to delimit once joined. --- .../cli-kit/src/public/node/analytics.test.ts | 17 ++++++----- packages/cli-kit/src/public/node/analytics.ts | 30 +++++++++++++------ 2 files changed, 30 insertions(+), 17 deletions(-) diff --git a/packages/cli-kit/src/public/node/analytics.test.ts b/packages/cli-kit/src/public/node/analytics.test.ts index 3bcc15e3eab..194f0417cc1 100644 --- a/packages/cli-kit/src/public/node/analytics.test.ts +++ b/packages/cli-kit/src/public/node/analytics.test.ts @@ -350,20 +350,21 @@ describe('event tracking', () => { }) }) - // Redaction rewrites the serialised payload, so a replacement that lands inside - // an escape sequence or past a closing quote makes the whole payload unparseable - // and drops the event. These values are the ones that get near those boundaries. + // Values that used to be hard to delimit once the arguments had been joined + // into one string: quotes and backslashes are escaped by JSON, and a space + // makes the end of the value ambiguous. test.each([ - ['an empty value', ['--client-secret', '']], - ['a value containing quotes', ['--client-secret', '"cs_secret_value"']], - ['a value containing a backslash', ['--client-secret', 'cs\\secret\\value']], - ])('still reports the event when a credential flag has %s', async (_description, credentialFlag) => { + ['is empty', ''], + ['contains quotes', '"cs_secret_value"'], + ['contains a backslash', 'cs\\secret_value'], + ['contains a space', '"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(credentialFlag), + args: args.concat(['--client-secret', credentialValue]), currentTime: currentDate.getTime() - 100, }) diff --git a/packages/cli-kit/src/public/node/analytics.ts b/packages/cli-kit/src/public/node/analytics.ts index 5e908c86538..74048ecd9f4 100644 --- a/packages/cli-kit/src/public/node/analytics.ts +++ b/packages/cli-kit/src/public/node/analytics.ts @@ -168,7 +168,7 @@ async function buildPayload({config, errorMessage, exitMode}: ReportAnalyticsEve request_ids: requestIdsCollection.getRequestIds(), }, sensitive: { - args: startArgs.join(' '), + args: redactCredentialFlags(startArgs).join(' '), cmd_all_environment_flags: environmentFlags, error_message: errorMessage, ...internalPluginsSensitive, @@ -197,22 +197,34 @@ async function buildPayload({config, errorMessage, exitMode}: ReportAnalyticsEve // Excludes `key` and `auth`, which would drop legitimate telemetry: `api_key` is // an app's public client ID and `env_auth_method` is the auth method used. -const CREDENTIAL_NAME_PATTERN = 'password|token|secret|credential' +const CREDENTIAL_NAME = /password|token|secret|credential/i +const REDACTED = '*****' -// One flag value, as an escape sequence or a plain character. Matching escapes -// whole stops a replacement from cutting a `\"` in half and unbalancing the string. -const FLAG_VALUE_PATTERN = '(?:\\\\.|[^\\s"\\\\])*' +function isCredentialFlag(arg: string): boolean { + return arg.startsWith('--') && CREDENTIAL_NAME.test(arg.split('=')[0]!) +} + +// Redacting the arguments while they're still a list means we never have to work +// out where a value ended once they've been joined into one string. +function redactCredentialFlags(args: string[]): string[] { + return args.map((arg, index) => { + const previous = args[index - 1] + // `--client-secret abc`: the value is a separate argument. + if (previous !== undefined && isCredentialFlag(previous) && !previous.includes('=')) return REDACTED + // `--client-secret=abc`: the value is in this one. + if (isCredentialFlag(arg) && arg.includes('=')) return `${arg.split('=')[0]}=${REDACTED}` + return arg + }) +} function sanitizePayload(payload: T): T { const payloadString = JSON.stringify(payload) const sanitizedPayloadString = payloadString // Theme Access passwords, recognisable wherever they appear. - .replace(/shptka_\w*/g, '*****') - // Credential flags, e.g. `--password abc`, `--client-secret=abc`. - .replace(new RegExp(`(--[\\w-]*(?:${CREDENTIAL_NAME_PATTERN})(?:=|\\s+))${FLAG_VALUE_PATTERN}`, 'gi'), '$1*****') + .replace(/shptka_\w*/g, REDACTED) // Credential keys. Quotes take any number of backslashes so that keys inside // already-serialised JSON, such as `metadata`, are covered at any depth. - .replace(new RegExp(`([\\w.-]*(?:${CREDENTIAL_NAME_PATTERN})[\\w.-]*\\\\*":\\\\*")[^"\\\\]*`, 'gi'), '$1*****') + .replace(new RegExp(`([\\w.-]*(?:${CREDENTIAL_NAME.source})[\\w.-]*\\\\*":\\\\*")[^"\\\\]*`, 'gi'), `$1${REDACTED}`) return JSON.parse(sanitizedPayloadString) } From af79db0dc0794c2c003c7705df3cb3e5037ee0a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Fri, 31 Jul 2026 13:41:44 +0200 Subject: [PATCH 05/13] Pin that credential env vars reach no field of the payload The allowlist test only covers env_shopify_variables; this covers the rest. --- .../cli-kit/src/public/node/analytics.test.ts | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/packages/cli-kit/src/public/node/analytics.test.ts b/packages/cli-kit/src/public/node/analytics.test.ts index 194f0417cc1..f540877a043 100644 --- a/packages/cli-kit/src/public/node/analytics.test.ts +++ b/packages/cli-kit/src/public/node/analytics.test.ts @@ -293,6 +293,43 @@ describe('event tracking', () => { }) }) + // 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}) + + // When + const config = { + runHook: vi.fn().mockResolvedValue({successes: [], failures: []}), + plugins: [], + } as any + await reportAnalyticsEvent({config, exitMode: 'ok'}) + + // Then + expect(publishEventMock).toHaveBeenCalledOnce() + const [, publicPayload, sensitivePayload] = publishEventMock.mock.calls[0]! + const serializedPayload = JSON.stringify({publicPayload, sensitivePayload}) + Object.values(credentials).forEach((secret) => { + expect(serializedPayload).not.toContain(secret) + }) + }) + }) + }) + // 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 () => { From 3fbef39f666e71d7a5d27f1a8dfead144ca43304 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Fri, 31 Jul 2026 13:57:07 +0200 Subject: [PATCH 06/13] Filter SHOPIFY_ variables by name instead of an allowlist env_shopify_variables exists so a caller can set its own SHOPIFY_* variable and have it reach Monorail -- SHOPIFY_INVOKED_BY, from shopify-function-test-helpers in PR #6509, is set outside this repo entirely. An allowlist of the names we know about drops those callers silently. Filter on the name instead, with `key` treated as a credential marker for environment variables specifically, since SHOPIFY_PROXY_KEY holds a signed token. --- .changeset/analytics-redact-shopify-env.md | 2 +- .../cli-kit/src/private/node/analytics.ts | 32 ++++++---- .../cli-kit/src/public/node/analytics.test.ts | 62 ++++++++++++------- packages/cli-kit/src/public/node/analytics.ts | 5 +- 4 files changed, 62 insertions(+), 39 deletions(-) diff --git a/.changeset/analytics-redact-shopify-env.md b/.changeset/analytics-redact-shopify-env.md index 1fbbfb61800..f2cb40ce5ec 100644 --- a/.changeset/analytics-redact-shopify-env.md +++ b/.changeset/analytics-redact-shopify-env.md @@ -2,4 +2,4 @@ '@shopify/cli-kit': patch --- -Only include allowlisted SHOPIFY_ environment variables and redact credential flag values in CLI analytics +Exclude credential-shaped environment variables and redact credential flag values in CLI analytics diff --git a/packages/cli-kit/src/private/node/analytics.ts b/packages/cli-kit/src/private/node/analytics.ts index 9367c2e5509..3322ef06b5c 100644 --- a/packages/cli-kit/src/private/node/analytics.ts +++ b/packages/cli-kit/src/private/node/analytics.ts @@ -107,21 +107,27 @@ export async function getSensitiveEnvironmentData(config: Interfaces.Config) { } } -// How agent callers identify themselves, kept in the sensitive payload until we -// prove which dimensions deserve first-class Monorail fields. An allowlist rather -// than a `SHOPIFY_*` prefix match because the CLI also reads its own tokens from -// the environment (see `environmentVariables` in ./constants.ts). -const REPORTED_SHOPIFY_ENVIRONMENT_VARIABLES = new Set([ - 'SHOPIFY_CLI_AGENT', - 'SHOPIFY_CLI_AGENT_VERSION', - 'SHOPIFY_CLI_AGENT_RUN_ID', - 'SHOPIFY_CLI_AGENT_SESSION_ID', - 'SHOPIFY_CLI_AGENT_PROVIDER', -]) - +// Names that mark a value as a credential, wherever it appears in the payload. +// Excludes `key` and `auth`, which would drop legitimate telemetry: `api_key` is +// an app's public client ID and `env_auth_method` is the auth method used. +export const CREDENTIAL_NAME = /password|token|secret|credential/i + +// Environment variables get the stricter check. `key` marks a credential here -- +// SHOPIFY_PROXY_KEY holds a signed token -- and unlike `api_key` in the payload +// there is nothing we want from the environment that is named for one. +const CREDENTIAL_ENVIRONMENT_NAME = new RegExp(`${CREDENTIAL_NAME.source}|key`, 'i') + +// Callers identify themselves by setting their own SHOPIFY_* variable -- agents +// use SHOPIFY_CLI_AGENT and friends, shopify-function-test-helpers uses +// SHOPIFY_INVOKED_BY -- so this stays a prefix match rather than an allowlist of +// the names we happen to know about. What it drops is the variables holding the +// CLI's own credentials (see `environmentVariables` in ./constants.ts), all of +// which say so in their name. function getShopifyEnvironmentVariables() { return Object.fromEntries( - Object.entries(process.env).filter(([key]) => REPORTED_SHOPIFY_ENVIRONMENT_VARIABLES.has(key)), + Object.entries(process.env).filter( + ([name]) => name.startsWith('SHOPIFY_') && !CREDENTIAL_ENVIRONMENT_NAME.test(name), + ), ) } diff --git a/packages/cli-kit/src/public/node/analytics.test.ts b/packages/cli-kit/src/public/node/analytics.test.ts index f540877a043..5d87033c91c 100644 --- a/packages/cli-kit/src/public/node/analytics.test.ts +++ b/packages/cli-kit/src/public/node/analytics.test.ts @@ -270,27 +270,47 @@ describe('event tracking', () => { } } - test('only sends allowlisted SHOPIFY_ environment variables in sensitive payload', async () => { - await withEnvironment({SHOPIFY_CLI_AGENT: 'some-agent', SHOPIFY_TEST_VAR: 'test_value'}, 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).toStrictEqual({SHOPIFY_CLI_AGENT: 'some-agent'}) - }) - }) + // Callers set their own SHOPIFY_* variable to identify themselves, so the field + // has to keep reporting names it doesn't know about -- SHOPIFY_INVOKED_BY comes + // from shopify-function-test-helpers, outside this repo. + test('sends SHOPIFY_ environment variables other than credentials', async () => { + await withEnvironment( + { + SHOPIFY_CLI_AGENT: 'some-agent', + SHOPIFY_INVOKED_BY: 'function-test-helpers', + SHOPIFY_CLI_PARTNERS_TOKEN: 'partners_secret', + SHOPIFY_PROXY_KEY: 'proxy_key_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', + }) + // `key` counts as a credential marker for environment variables: + // SHOPIFY_PROXY_KEY holds a signed token. + expect(shopifyVars).not.toHaveProperty('SHOPIFY_CLI_PARTNERS_TOKEN') + expect(shopifyVars).not.toHaveProperty('SHOPIFY_PROXY_KEY') + expect(shopifyVars).not.toHaveProperty('NOT_SHOPIFY_VAR') + }) + }, + ) }) // The allowlist keeps these out of env_shopify_variables, but they should not diff --git a/packages/cli-kit/src/public/node/analytics.ts b/packages/cli-kit/src/public/node/analytics.ts index 74048ecd9f4..509947d9e73 100644 --- a/packages/cli-kit/src/public/node/analytics.ts +++ b/packages/cli-kit/src/public/node/analytics.ts @@ -12,7 +12,7 @@ import { compileData as storageCompileData, RuntimeData, } from '../../private/node/analytics/storage.js' -import {getEnvironmentData, getSensitiveEnvironmentData} from '../../private/node/analytics.js' +import {CREDENTIAL_NAME, getEnvironmentData, getSensitiveEnvironmentData} from '../../private/node/analytics.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' @@ -195,9 +195,6 @@ async function buildPayload({config, errorMessage, exitMode}: ReportAnalyticsEve return sanitizePayload(payload) } -// Excludes `key` and `auth`, which would drop legitimate telemetry: `api_key` is -// an app's public client ID and `env_auth_method` is the auth method used. -const CREDENTIAL_NAME = /password|token|secret|credential/i const REDACTED = '*****' function isCredentialFlag(arg: string): boolean { From b5807e5ab8532942ae821b710d2a04d13f6b93e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Fri, 31 Jul 2026 16:03:48 +0200 Subject: [PATCH 07/13] Report every SHOPIFY_ variable again, masking credential values Dropping credential variables from the payload also dropped the fact that they were set, and an allowlist would have dropped callers this repo has never heard of -- SHOPIFY_INVOKED_BY comes from shopify-function-test-helpers. Go back to reporting every SHOPIFY_ variable, as main does, and mask the value when the name marks it as a credential. That is what main already did for SHOPIFY_FLAG_STORE_PASSWORD; the rule was just too narrow to cover the rest. --- .changeset/analytics-redact-shopify-env.md | 2 +- .../cli-kit/src/private/node/analytics.ts | 20 +++++++++---------- .../cli-kit/src/public/node/analytics.test.ts | 16 +++++++-------- packages/cli-kit/src/public/node/analytics.ts | 9 ++++++--- 4 files changed, 25 insertions(+), 22 deletions(-) diff --git a/.changeset/analytics-redact-shopify-env.md b/.changeset/analytics-redact-shopify-env.md index f2cb40ce5ec..8d0937c52f1 100644 --- a/.changeset/analytics-redact-shopify-env.md +++ b/.changeset/analytics-redact-shopify-env.md @@ -2,4 +2,4 @@ '@shopify/cli-kit': patch --- -Exclude credential-shaped environment variables and redact credential flag values in CLI analytics +Redact credential environment variables and flag values in CLI analytics diff --git a/packages/cli-kit/src/private/node/analytics.ts b/packages/cli-kit/src/private/node/analytics.ts index 3322ef06b5c..06ac240dae6 100644 --- a/packages/cli-kit/src/private/node/analytics.ts +++ b/packages/cli-kit/src/private/node/analytics.ts @@ -107,6 +107,8 @@ export async function getSensitiveEnvironmentData(config: Interfaces.Config) { } } +export const REDACTED = '*****' + // Names that mark a value as a credential, wherever it appears in the payload. // Excludes `key` and `auth`, which would drop legitimate telemetry: `api_key` is // an app's public client ID and `env_auth_method` is the auth method used. @@ -114,20 +116,18 @@ export const CREDENTIAL_NAME = /password|token|secret|credential/i // Environment variables get the stricter check. `key` marks a credential here -- // SHOPIFY_PROXY_KEY holds a signed token -- and unlike `api_key` in the payload -// there is nothing we want from the environment that is named for one. +// there is nothing we report from the environment that is named for one. const CREDENTIAL_ENVIRONMENT_NAME = new RegExp(`${CREDENTIAL_NAME.source}|key`, 'i') -// Callers identify themselves by setting their own SHOPIFY_* variable -- agents -// use SHOPIFY_CLI_AGENT and friends, shopify-function-test-helpers uses -// SHOPIFY_INVOKED_BY -- so this stays a prefix match rather than an allowlist of -// the names we happen to know about. What it drops is the variables holding the -// CLI's own credentials (see `environmentVariables` in ./constants.ts), all of -// which say so in their name. +// Every SHOPIFY_* variable is reported, because callers identify themselves by +// setting their own: agents use SHOPIFY_CLI_AGENT and friends, and +// shopify-function-test-helpers uses SHOPIFY_INVOKED_BY. Knowing a variable was +// set is the telemetry; the value only matters when it isn't a credential. function getShopifyEnvironmentVariables() { return Object.fromEntries( - Object.entries(process.env).filter( - ([name]) => name.startsWith('SHOPIFY_') && !CREDENTIAL_ENVIRONMENT_NAME.test(name), - ), + Object.entries(process.env) + .filter(([name]) => name.startsWith('SHOPIFY_')) + .map(([name, value]) => [name, CREDENTIAL_ENVIRONMENT_NAME.test(name) ? REDACTED : value]), ) } diff --git a/packages/cli-kit/src/public/node/analytics.test.ts b/packages/cli-kit/src/public/node/analytics.test.ts index 5d87033c91c..36ea320f5c5 100644 --- a/packages/cli-kit/src/public/node/analytics.test.ts +++ b/packages/cli-kit/src/public/node/analytics.test.ts @@ -270,10 +270,10 @@ describe('event tracking', () => { } } - // Callers set their own SHOPIFY_* variable to identify themselves, so the field - // has to keep reporting names it doesn't know about -- SHOPIFY_INVOKED_BY comes - // from shopify-function-test-helpers, outside this repo. - test('sends SHOPIFY_ environment variables other than credentials', async () => { + // Callers set their own SHOPIFY_* variable to identify themselves -- agents use + // SHOPIFY_CLI_AGENT, shopify-function-test-helpers uses SHOPIFY_INVOKED_BY -- so + // every variable is still reported. Only credential values are masked. + test('sends SHOPIFY_ environment variables with credential values redacted', async () => { await withEnvironment( { SHOPIFY_CLI_AGENT: 'some-agent', @@ -302,11 +302,11 @@ describe('event tracking', () => { expect(shopifyVars).toMatchObject({ SHOPIFY_CLI_AGENT: 'some-agent', SHOPIFY_INVOKED_BY: 'function-test-helpers', + SHOPIFY_CLI_PARTNERS_TOKEN: '*****', + // `key` counts as a credential marker for environment variables: + // SHOPIFY_PROXY_KEY holds a signed token. + SHOPIFY_PROXY_KEY: '*****', }) - // `key` counts as a credential marker for environment variables: - // SHOPIFY_PROXY_KEY holds a signed token. - expect(shopifyVars).not.toHaveProperty('SHOPIFY_CLI_PARTNERS_TOKEN') - expect(shopifyVars).not.toHaveProperty('SHOPIFY_PROXY_KEY') expect(shopifyVars).not.toHaveProperty('NOT_SHOPIFY_VAR') }) }, diff --git a/packages/cli-kit/src/public/node/analytics.ts b/packages/cli-kit/src/public/node/analytics.ts index 509947d9e73..972221411ac 100644 --- a/packages/cli-kit/src/public/node/analytics.ts +++ b/packages/cli-kit/src/public/node/analytics.ts @@ -12,7 +12,12 @@ import { compileData as storageCompileData, RuntimeData, } from '../../private/node/analytics/storage.js' -import {CREDENTIAL_NAME, getEnvironmentData, getSensitiveEnvironmentData} from '../../private/node/analytics.js' +import { + CREDENTIAL_NAME, + REDACTED, + getEnvironmentData, + getSensitiveEnvironmentData, +} from '../../private/node/analytics.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' @@ -195,8 +200,6 @@ async function buildPayload({config, errorMessage, exitMode}: ReportAnalyticsEve return sanitizePayload(payload) } -const REDACTED = '*****' - function isCredentialFlag(arg: string): boolean { return arg.startsWith('--') && CREDENTIAL_NAME.test(arg.split('=')[0]!) } From 522c813ef85d69bff9ec9d53604cd185fe3a7a36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Fri, 31 Jul 2026 16:45:03 +0200 Subject: [PATCH 08/13] Leave getShopifyEnvironmentVariables as it is on main The sanitizer already redacts every credential the CLI reads from the environment, since they are all named for what they hold. Filtering at collection time as well was a second mechanism for the same job. --- .../cli-kit/src/private/node/analytics.ts | 28 +++++-------------- .../cli-kit/src/public/node/analytics.test.ts | 10 ++----- packages/cli-kit/src/public/node/analytics.ts | 13 +++++---- 3 files changed, 17 insertions(+), 34 deletions(-) diff --git a/packages/cli-kit/src/private/node/analytics.ts b/packages/cli-kit/src/private/node/analytics.ts index 06ac240dae6..499fc8ef24d 100644 --- a/packages/cli-kit/src/private/node/analytics.ts +++ b/packages/cli-kit/src/private/node/analytics.ts @@ -107,28 +107,14 @@ export async function getSensitiveEnvironmentData(config: Interfaces.Config) { } } -export const REDACTED = '*****' - -// Names that mark a value as a credential, wherever it appears in the payload. -// Excludes `key` and `auth`, which would drop legitimate telemetry: `api_key` is -// an app's public client ID and `env_auth_method` is the auth method used. -export const CREDENTIAL_NAME = /password|token|secret|credential/i - -// Environment variables get the stricter check. `key` marks a credential here -- -// SHOPIFY_PROXY_KEY holds a signed token -- and unlike `api_key` in the payload -// there is nothing we report from the environment that is named for one. -const CREDENTIAL_ENVIRONMENT_NAME = new RegExp(`${CREDENTIAL_NAME.source}|key`, 'i') - -// Every SHOPIFY_* variable is reported, because callers identify themselves by -// setting their own: agents use SHOPIFY_CLI_AGENT and friends, and -// shopify-function-test-helpers uses SHOPIFY_INVOKED_BY. Knowing a variable was -// set is the telemetry; the value only matters when it isn't a credential. function getShopifyEnvironmentVariables() { - return Object.fromEntries( - Object.entries(process.env) - .filter(([name]) => name.startsWith('SHOPIFY_')) - .map(([name, value]) => [name, CREDENTIAL_ENVIRONMENT_NAME.test(name) ? REDACTED : value]), - ) + // Agent callers can identify themselves today via SHOPIFY_* environment + // variables. The current contract is intentionally lightweight and is kept in + // the sensitive payload until we prove which dimensions deserve first-class + // Monorail fields, e.g. SHOPIFY_CLI_AGENT, SHOPIFY_CLI_AGENT_VERSION, + // SHOPIFY_CLI_AGENT_RUN_ID, SHOPIFY_CLI_AGENT_SESSION_ID, and + // SHOPIFY_CLI_AGENT_PROVIDER. + return Object.fromEntries(Object.entries(process.env).filter(([key]) => key.startsWith('SHOPIFY_'))) } function getPluginNames(config: Interfaces.Config) { diff --git a/packages/cli-kit/src/public/node/analytics.test.ts b/packages/cli-kit/src/public/node/analytics.test.ts index 36ea320f5c5..93018623900 100644 --- a/packages/cli-kit/src/public/node/analytics.test.ts +++ b/packages/cli-kit/src/public/node/analytics.test.ts @@ -270,16 +270,15 @@ describe('event tracking', () => { } } - // Callers set their own SHOPIFY_* variable to identify themselves -- agents use - // SHOPIFY_CLI_AGENT, shopify-function-test-helpers uses SHOPIFY_INVOKED_BY -- so - // every variable is still reported. Only credential values are masked. + // 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', - SHOPIFY_PROXY_KEY: 'proxy_key_secret', NOT_SHOPIFY_VAR: 'should_not_appear', }, async () => { @@ -303,9 +302,6 @@ describe('event tracking', () => { SHOPIFY_CLI_AGENT: 'some-agent', SHOPIFY_INVOKED_BY: 'function-test-helpers', SHOPIFY_CLI_PARTNERS_TOKEN: '*****', - // `key` counts as a credential marker for environment variables: - // SHOPIFY_PROXY_KEY holds a signed token. - SHOPIFY_PROXY_KEY: '*****', }) expect(shopifyVars).not.toHaveProperty('NOT_SHOPIFY_VAR') }) diff --git a/packages/cli-kit/src/public/node/analytics.ts b/packages/cli-kit/src/public/node/analytics.ts index 972221411ac..6c5bead6484 100644 --- a/packages/cli-kit/src/public/node/analytics.ts +++ b/packages/cli-kit/src/public/node/analytics.ts @@ -12,12 +12,7 @@ import { compileData as storageCompileData, RuntimeData, } from '../../private/node/analytics/storage.js' -import { - CREDENTIAL_NAME, - REDACTED, - getEnvironmentData, - getSensitiveEnvironmentData, -} from '../../private/node/analytics.js' +import {getEnvironmentData, getSensitiveEnvironmentData} from '../../private/node/analytics.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' @@ -200,6 +195,12 @@ async function buildPayload({config, errorMessage, exitMode}: ReportAnalyticsEve return sanitizePayload(payload) } +// Names that mark a value as a credential, wherever it appears in the payload. +// Excludes `key` and `auth`, which would drop legitimate telemetry: `api_key` is +// an app's public client ID and `env_auth_method` is the auth method used. +const CREDENTIAL_NAME = /password|token|secret|credential/i +const REDACTED = '*****' + function isCredentialFlag(arg: string): boolean { return arg.startsWith('--') && CREDENTIAL_NAME.test(arg.split('=')[0]!) } From 77888def981f6d53d5fc32422cebfca33dfcaa87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Fri, 31 Jul 2026 17:25:22 +0200 Subject: [PATCH 09/13] Keep redacting credential flags quoted inside other values Removing the string-level flag rule was not the pure simplification it looked like. Redacting the argument list covers `args`, but the old rule also matched anywhere else in the serialised payload -- a failing subprocess reports the command line it ran, and that reaches Monorail through error_message. Keep both: the list-level pass for `args`, where it can delimit values exactly, and an escape-aware string-level pass as the backstop for copies elsewhere. --- .../cli-kit/src/public/node/analytics.test.ts | 27 +++++++++++++++++++ packages/cli-kit/src/public/node/analytics.ts | 9 +++++++ 2 files changed, 36 insertions(+) diff --git a/packages/cli-kit/src/public/node/analytics.test.ts b/packages/cli-kit/src/public/node/analytics.test.ts index 93018623900..46e548b0602 100644 --- a/packages/cli-kit/src/public/node/analytics.test.ts +++ b/packages/cli-kit/src/public/node/analytics.test.ts @@ -437,6 +437,33 @@ describe('event tracking', () => { }) }) + // 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 () => { diff --git a/packages/cli-kit/src/public/node/analytics.ts b/packages/cli-kit/src/public/node/analytics.ts index 6c5bead6484..4ce22d88ad1 100644 --- a/packages/cli-kit/src/public/node/analytics.ts +++ b/packages/cli-kit/src/public/node/analytics.ts @@ -223,6 +223,15 @@ function sanitizePayload(payload: T): T { const sanitizedPayloadString = payloadString // Theme Access passwords, recognisable wherever they appear. .replace(/shptka_\w*/g, REDACTED) + // Credential flags quoted inside some other value -- a failing subprocess + // reports the command line it ran, and that ends up in `error_message`. + // `args` is redacted as a list before it gets here; this catches the copies. + // Escape sequences are matched whole so a replacement can't cut a `\"` in + // half and leave the payload unparseable. + .replace( + new RegExp(`(--[\\w-]*(?:${CREDENTIAL_NAME.source})(?:=|\\s+))(?:\\\\.|[^\\s"\\\\])*`, 'gi'), + `$1${REDACTED}`, + ) // Credential keys. Quotes take any number of backslashes so that keys inside // already-serialised JSON, such as `metadata`, are covered at any depth. .replace(new RegExp(`([\\w.-]*(?:${CREDENTIAL_NAME.source})[\\w.-]*\\\\*":\\\\*")[^"\\\\]*`, 'gi'), `$1${REDACTED}`) From 4750b85351222235b68e41c243eb171b6c55afb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Fri, 31 Jul 2026 17:47:09 +0200 Subject: [PATCH 10/13] Drop the changeset Internal hardening of what the analytics payload reports; nothing here needs a line in the changelog. --- .changeset/analytics-redact-shopify-env.md | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 .changeset/analytics-redact-shopify-env.md diff --git a/.changeset/analytics-redact-shopify-env.md b/.changeset/analytics-redact-shopify-env.md deleted file mode 100644 index 8d0937c52f1..00000000000 --- a/.changeset/analytics-redact-shopify-env.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -'@shopify/cli-kit': patch ---- - -Redact credential environment variables and flag values in CLI analytics From cce9c7109eb3e0490be500c71a94080c8d8f36a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Fri, 31 Jul 2026 17:50:12 +0200 Subject: [PATCH 11/13] Write the redaction rules as literal regexes The doubled backslashes came from building the patterns with new RegExp from a string, not from the matching itself. As literals they are the same shape as the two --store-password rules they replace, with the name generalised. --- packages/cli-kit/src/public/node/analytics.ts | 24 ++++++++----------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/packages/cli-kit/src/public/node/analytics.ts b/packages/cli-kit/src/public/node/analytics.ts index 4ce22d88ad1..ab27cc335dc 100644 --- a/packages/cli-kit/src/public/node/analytics.ts +++ b/packages/cli-kit/src/public/node/analytics.ts @@ -221,20 +221,16 @@ function redactCredentialFlags(args: string[]): string[] { function sanitizePayload(payload: T): T { const payloadString = JSON.stringify(payload) const sanitizedPayloadString = payloadString - // Theme Access passwords, recognisable wherever they appear. - .replace(/shptka_\w*/g, REDACTED) - // Credential flags quoted inside some other value -- a failing subprocess - // reports the command line it ran, and that ends up in `error_message`. - // `args` is redacted as a list before it gets here; this catches the copies. - // Escape sequences are matched whole so a replacement can't cut a `\"` in - // half and leave the payload unparseable. - .replace( - new RegExp(`(--[\\w-]*(?:${CREDENTIAL_NAME.source})(?:=|\\s+))(?:\\\\.|[^\\s"\\\\])*`, 'gi'), - `$1${REDACTED}`, - ) - // Credential keys. Quotes take any number of backslashes so that keys inside - // already-serialised JSON, such as `metadata`, are covered at any depth. - .replace(new RegExp(`([\\w.-]*(?:${CREDENTIAL_NAME.source})[\\w.-]*\\\\*":\\\\*")[^"\\\\]*`, 'gi'), `$1${REDACTED}`) + // Remove Theme Access passwords from the payload + .replace(/shptka_\w*/g, '*****') + // Credential flags quoted inside another value, e.g. `--store-password abc` in + // the command line a failing subprocess reports. `args` is redacted as a list + // before it gets here; this catches the copies. `\\.` matches an escape + // sequence whole so a replacement can't cut a `\"` in half. + .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) } From be539ad823db0333bf249f788e8f8ff51cb2a9ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Mon, 3 Aug 2026 13:33:06 +0200 Subject: [PATCH 12/13] Redact key-named values in the printed payload only `key` marks a credential in the environment -- SHOPIFY_PROXY_KEY holds a signed token and 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. Add the name to an extra pass over the copies that outputDebug prints, which is what --verbose turns on. Telemetry keeps the values; a terminal, and whatever scrapes it, does not see them. --- .../private/node/analytics/redact-output.ts | 19 +++++++++++ .../cli-kit/src/public/node/analytics.test.ts | 33 +++++++++++++++++++ packages/cli-kit/src/public/node/analytics.ts | 9 +++-- packages/cli-kit/src/public/node/monorail.ts | 3 +- 4 files changed, 61 insertions(+), 3 deletions(-) create mode 100644 packages/cli-kit/src/private/node/analytics/redact-output.ts diff --git a/packages/cli-kit/src/private/node/analytics/redact-output.ts b/packages/cli-kit/src/private/node/analytics/redact-output.ts new file mode 100644 index 00000000000..57e3dfef7ad --- /dev/null +++ b/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(payload: T): T { + const payloadString = JSON.stringify(payload) + return JSON.parse(payloadString.replace(/([\w.-]*key[\w.-]*\\*":\\*")[^"\\]*/gi, '$1*****')) +} diff --git a/packages/cli-kit/src/public/node/analytics.test.ts b/packages/cli-kit/src/public/node/analytics.test.ts index 46e548b0602..b480c38a329 100644 --- a/packages/cli-kit/src/public/node/analytics.test.ts +++ b/packages/cli-kit/src/public/node/analytics.test.ts @@ -346,6 +346,39 @@ describe('event tracking', () => { }) }) + // `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 () => { diff --git a/packages/cli-kit/src/public/node/analytics.ts b/packages/cli-kit/src/public/node/analytics.ts index ab27cc335dc..d9ff7403976 100644 --- a/packages/cli-kit/src/public/node/analytics.ts +++ b/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 () => { diff --git a/packages/cli-kit/src/public/node/monorail.ts b/packages/cli-kit/src/public/node/monorail.ts index c797cfe2e88..77a762d9aee 100644 --- a/packages/cli-kit/src/public/node/monorail.ts +++ b/packages/cli-kit/src/public/node/monorail.ts @@ -1,6 +1,7 @@ import {fetch} from './http.js' import {outputDebug, outputContent, outputToken} from './output.js' import {JsonMap} from '../../private/common/json.js' +import {redactForOutput} from '../../private/node/analytics/redact-output.js' import {DeepRequired} from '../common/ts/deep-required.js' export {DeepRequired} @@ -224,7 +225,7 @@ export async function publishMonorailEvent Date: Mon, 3 Aug 2026 13:41:30 +0200 Subject: [PATCH 13/13] Cut the redaction down to two passes Redacting the argument list before joining was a second mechanism for what the flag rule already does. It only added a value containing a space, which needs quotes inside argv to arise, at the cost of a helper, a predicate and the wiring into buildPayload. redactForOutput also subsumes monorail's api_key masker, since api_key is a key-named entry, so that function goes too. --- .../cli-kit/src/public/node/analytics.test.ts | 8 ++--- packages/cli-kit/src/public/node/analytics.ts | 32 +++---------------- .../cli-kit/src/public/node/monorail.test.ts | 2 +- packages/cli-kit/src/public/node/monorail.ts | 17 +--------- 4 files changed, 10 insertions(+), 49 deletions(-) diff --git a/packages/cli-kit/src/public/node/analytics.test.ts b/packages/cli-kit/src/public/node/analytics.test.ts index b480c38a329..3c0555517b4 100644 --- a/packages/cli-kit/src/public/node/analytics.test.ts +++ b/packages/cli-kit/src/public/node/analytics.test.ts @@ -436,14 +436,14 @@ describe('event tracking', () => { }) }) - // Values that used to be hard to delimit once the arguments had been joined - // into one string: quotes and backslashes are escaped by JSON, and a space - // makes the end of the value ambiguous. + // 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'], - ['contains a space', '"cs secret_value"'], ])('redacts a credential flag whose value %s', async (_description, credentialValue) => { await inProjectWithFile('package.json', async (args) => { // Given diff --git a/packages/cli-kit/src/public/node/analytics.ts b/packages/cli-kit/src/public/node/analytics.ts index d9ff7403976..74fc48d7242 100644 --- a/packages/cli-kit/src/public/node/analytics.ts +++ b/packages/cli-kit/src/public/node/analytics.ts @@ -173,7 +173,7 @@ async function buildPayload({config, errorMessage, exitMode}: ReportAnalyticsEve request_ids: requestIdsCollection.getRequestIds(), }, sensitive: { - args: redactCredentialFlags(startArgs).join(' '), + args: startArgs.join(' '), cmd_all_environment_flags: environmentFlags, error_message: errorMessage, ...internalPluginsSensitive, @@ -200,38 +200,14 @@ async function buildPayload({config, errorMessage, exitMode}: ReportAnalyticsEve return sanitizePayload(payload) } -// Names that mark a value as a credential, wherever it appears in the payload. -// Excludes `key` and `auth`, which would drop legitimate telemetry: `api_key` is -// an app's public client ID and `env_auth_method` is the auth method used. -const CREDENTIAL_NAME = /password|token|secret|credential/i -const REDACTED = '*****' - -function isCredentialFlag(arg: string): boolean { - return arg.startsWith('--') && CREDENTIAL_NAME.test(arg.split('=')[0]!) -} - -// Redacting the arguments while they're still a list means we never have to work -// out where a value ended once they've been joined into one string. -function redactCredentialFlags(args: string[]): string[] { - return args.map((arg, index) => { - const previous = args[index - 1] - // `--client-secret abc`: the value is a separate argument. - if (previous !== undefined && isCredentialFlag(previous) && !previous.includes('=')) return REDACTED - // `--client-secret=abc`: the value is in this one. - if (isCredentialFlag(arg) && arg.includes('=')) return `${arg.split('=')[0]}=${REDACTED}` - return arg - }) -} - function sanitizePayload(payload: T): T { const payloadString = JSON.stringify(payload) const sanitizedPayloadString = payloadString // Remove Theme Access passwords from the payload .replace(/shptka_\w*/g, '*****') - // Credential flags quoted inside another value, e.g. `--store-password abc` in - // the command line a failing subprocess reports. `args` is redacted as a list - // before it gets here; this catches the copies. `\\.` matches an escape - // sequence whole so a replacement can't cut a `\"` in half. + // 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. diff --git a/packages/cli-kit/src/public/node/monorail.test.ts b/packages/cli-kit/src/public/node/monorail.test.ts index 72b7e89bb62..91dd53ae13b 100644 --- a/packages/cli-kit/src/public/node/monorail.test.ts +++ b/packages/cli-kit/src/public/node/monorail.test.ts @@ -49,7 +49,7 @@ describe('monorail', () => { const outputMock = mockAndCaptureOutput() const res = await publishMonorailEvent('fake_schema/0.0', {api_key: 'some-api-key'}, {baz: 'abc'}) expect(res.type).toEqual('ok') - expect(outputMock.debug()).toContain('"api_key": "****"') + expect(outputMock.debug()).toContain('"api_key": "*****"') expect(outputMock.debug()).not.toContain('some-api-key') }) diff --git a/packages/cli-kit/src/public/node/monorail.ts b/packages/cli-kit/src/public/node/monorail.ts index 77a762d9aee..acdcc2686fd 100644 --- a/packages/cli-kit/src/public/node/monorail.ts +++ b/packages/cli-kit/src/public/node/monorail.ts @@ -225,7 +225,7 @@ export async function publishMonorailEvent(payload: T): T { - const result = {...payload} - if ('api_key' in result) { - result.api_key = '****' - } - - return result -} - const buildHeaders = (currentTime: number) => { return { 'Content-Type': 'application/json; charset=utf-8',