diff --git a/.changeset/app-doctor-secret-false-positives.md b/.changeset/app-doctor-secret-false-positives.md new file mode 100644 index 00000000000..5fa53730662 --- /dev/null +++ b/.changeset/app-doctor-secret-false-positives.md @@ -0,0 +1,5 @@ +--- +'@shopify/app': patch +--- + +The committed-secret check in `shopify app security` now only flags recognizable secret values (e.g. `shpss_`/`shpat_` tokens), not secret-sounding variable names or placeholders. diff --git a/packages/app/src/cli/services/app-security-engine/checks/COMMITTED_SECRET.md b/packages/app/src/cli/services/app-security-engine/checks/COMMITTED_SECRET.md index f2812bac2f4..b0b3cbec5f9 100644 --- a/packages/app/src/cli/services/app-security-engine/checks/COMMITTED_SECRET.md +++ b/packages/app/src/cli/services/app-security-engine/checks/COMMITTED_SECRET.md @@ -1,9 +1,11 @@ --- id: COMMITTED_SECRET -version: 1 +version: 2 severity: high --- # Committed Secret Inspect files skipped by deterministic secret scanning for committed credentials. Never quote or reproduce a secret; cite only the file and redacted credential kind, and recommend rotation. + +Do not report placeholders, public client identifiers (`SHOPIFY_API_KEY`, Stripe `pk_`), or files git confirms are untracked and ignored. Template env files (`.env.example`, `.sample`, `.template`, `.dist`) are findings only when they contain a known credential format. diff --git a/packages/app/src/cli/services/app-security-engine/checks/embedded.ts b/packages/app/src/cli/services/app-security-engine/checks/embedded.ts index 0f2c0192219..f6cafc209ea 100644 --- a/packages/app/src/cli/services/app-security-engine/checks/embedded.ts +++ b/packages/app/src/cli/services/app-security-engine/checks/embedded.ts @@ -7,7 +7,7 @@ export const EMBEDDED_CHECK_SOURCES: ReadonlyArray = [ "---\nid: ACTIVE_UPLOADS_AND_PRIVILEGED_PREVIEWS\nversion: 1\nseverity: high\n---\n\n# Active Uploads And Privileged Previews\n\nFind cases where merchant-, customer-, webhook-, or external-service-supplied\nfiles become active content in a privileged origin. Trace uploads, imports,\npreviews, and generated assets from ingestion through storage and final render.\n\nThe risk is not the upload alone. The risk is an untrusted-upload-to-active-render\npath: SVG, HTML, XML, PDF, blob/data URL, or another active format is accepted and\nlater rendered in a storefront, embedded admin, customer-account, theme-editor,\nor operator/admin context where it can execute or leak protected data.\n\n## What to look for\n\n1. **Find upload and import entry points.** Search for file uploads, import jobs,\n webhook attachments, remote fetches, document parsers, blob/data URL handling,\n and generated preview endpoints.\n\n2. **Trace file metadata and validation.** Check size limits, extension checks,\n declared MIME type, magic-byte/file-signature verification, filename handling,\n generated storage names, antivirus/sanitization, and any image/PDF re-encoding.\n\n3. **Inspect storage and serving boundaries.** Determine whether the object is\n stored on a non-executable origin, served with explicit `Content-Type` and\n `Content-Disposition`, and prevented from inheriting privileged cookies or\n browser authority.\n\n4. **Follow every final renderer.** Check storefront/theme renderers, embedded\n admin previews, customer-account views, email/PDF previews, admin/operator\n tools, iframe/srcdoc/blob/data URL renderers, and any browser code that inserts\n the uploaded content into the DOM.\n\n5. **Check sandboxing and isolation.** Verify iframes, preview origins, CSP,\n download headers, SVG sanitization, PDF handling, and re-encoding before\n deciding the content is safe.\n\n## What to report\n\nReport a finding only for a complete untrusted-upload-to-active-render path where\nthe uploaded or imported object is actually rendered or served into a privileged\nexecutable context. Show:\n- who controls the uploaded/imported content;\n- which validation or isolation boundary is missing;\n- where the content becomes active or executable;\n- which privileged origin or user is affected; and\n- file/line evidence for both the ingest path and the renderer/serving path.\n\nExample:\n\n```json\n{\n \"file\": \"app/controllers/previews_controller.rb\",\n \"line\": 28,\n \"message\": \"Uploaded SVG is rendered inline in the admin preview without sanitization or origin isolation\",\n \"evidence\": [\n { \"file\": \"app/controllers/uploads_controller.rb\", \"line\": 14, \"quote\": \"params[:file]\" },\n { \"file\": \"app/controllers/previews_controller.rb\", \"line\": 28, \"quote\": \"render inline: blob.download\" }\n ],\n \"confidence\": \"high\",\n \"reasoning\": \"The merchant-controlled SVG is stored without re-encoding and later rendered inline in the embedded admin origin, so script-capable SVG content can execute with merchant authority.\"\n}\n```\n\nDo not report:\n- files that are forced to download and never rendered in an active origin;\n- images/PDFs that are re-encoded or sanitized before serving;\n- isolated preview origins with no privileged cookies, storage, or message bridge;\n- missing deployment details where you cannot establish executable rendering or unsafe serving.\n- permissive content types, inline disposition, or storage/header hygiene issues\n without a concrete privileged renderer or execution surface.\n", "---\nid: APP_PROXY_LIQUID_INJECTION\nversion: 2\nseverity: high\n---\n\n# App Proxy Liquid Injection\n\nTrace verified app-proxy request values into active response bodies, including Liquid and HTML response types. Report only a request-controlled value that reaches an active response; static templates and inert JSON are not findings.\n", "---\nid: APP_PROXY_UNVERIFIED_SIGNATURE\nversion: 2\nseverity: high\n---\n\nFind app proxy endpoints that read proxy parameters without verifying\nthe Shopify signature, allowing an attacker to impersonate Shopify and\nsend fake proxy requests.\n\nApp proxies let an app serve content directly on the merchant's store\nvia a URL like `https://shop.example.com/apps/my-app/proxy`. Shopify\nsigns every proxy request with an HMAC using the app's shared secret.\nIf the app doesn't verify this signature, anyone can send requests to\nthe proxy endpoint with forged parameters — including `shop`,\n`logged_in_customer_id`, and `path_prefix`.\n\n## What to look for\n\n1. **Find app proxy route handlers.** These are endpoints configured as\n app proxies in `shopify.app.toml` under `[app_proxy]` or in the app's\n routing config. They typically read parameters like:\n - `shop` or `shop_id`\n - `logged_in_customer_id`\n - `path_prefix`\n - `signature`\n - `timestamp`\n\n2. **Check for signature verification.** The handler must verify the\n HMAC signature before trusting any proxy parameter. Look for:\n - **Remix:** `authenticate.public.appProxy(request)` — the official\n verification function\n - **Rails:** `verified_request?` or manual HMAC verification using\n `ShopifyApp` utilities\n - **Express:** Manual HMAC verification using the app secret\n - **PHP:** `ShopifyUtils::verifyProxyRequest()` or equivalent\n\n3. **If no verification is present, check whether the handler:**\n - Reads `shop` from the query string and uses it to scope data\n - Reads `logged_in_customer_id` and uses it for authorisation\n - Returns any shop-specific data\n\n If any of these are true and there's no signature check, it's a real\n finding.\n\n4. **Check for the HMAC pattern even if the function name isn't obvious.**\n Some apps implement custom verification:\n - `crypto.createHmac('sha256', API_SECRET)`\n - `OpenSSL::HMAC.digest`\n - `hash_hmac('sha256', ...)`\n - Comparison with `timingSafeEqual` or `secure_compare`\n\n5. **Separate app-local findings from protocol hardening signals.** Missing\n verification is a finding when the handler trusts signed parameters without\n any verification boundary. Weak comparison, unusual canonicalization, or\n delimiterless concatenation is not automatically an app finding: keep it\n unresolved unless you can show a usable victim-signed request path or another\n concrete exploit condition in this app.\n\n## What to report\n\nFor each proxy handler that reads shop/customer parameters without\nsignature verification, or where you can demonstrate a usable victim-signed\nrequest path through a weak verification implementation:\n\n```json\n{\n \"file\": \"app/routes/proxy.ts\",\n \"line\": 15,\n \"message\": \"App proxy handler reads shop parameter without signature verification\",\n \"snippet\": \"const shop = url.searchParams.get('shop')\",\n \"evidence\": [\n {\n \"file\": \"app/routes/proxy.ts\",\n \"line\": 15,\n \"quote\": \"const shop = url.searchParams.get('shop')\"\n },\n {\n \"file\": \"app/routes/proxy.ts\",\n \"line\": 1,\n \"quote\": \"no authenticate.public.appProxy or HMAC verification found\"\n }\n ],\n \"confidence\": \"high\",\n \"reasoning\": \"The handler reads the shop parameter from the query string and uses it to query shop data, but no signature verification is present. An attacker can send requests with any shop parameter.\"\n}\n```\n\nDo not report:\n\n- Handlers that call `authenticate.public.appProxy(request)` (Remix)\n- Handlers with manual HMAC verification\n- Handlers that return only static content (no shop-specific data)\n- Protocol-only canonicalization concerns with no demonstrated app-local exploit path\n- Test handlers\n", - "---\nid: COMMITTED_SECRET\nversion: 1\nseverity: high\n---\n\n# Committed Secret\n\nInspect files skipped by deterministic secret scanning for committed credentials. Never quote or reproduce a secret; cite only the file and redacted credential kind, and recommend rotation.\n", + "---\nid: COMMITTED_SECRET\nversion: 2\nseverity: high\n---\n\n# Committed Secret\n\nInspect files skipped by deterministic secret scanning for committed credentials. Never quote or reproduce a secret; cite only the file and redacted credential kind, and recommend rotation.\n\nDo not report placeholders, public client identifiers (`SHOPIFY_API_KEY`, Stripe `pk_`), or files git confirms are untracked and ignored. Template env files (`.env.example`, `.sample`, `.template`, `.dist`) are findings only when they contain a known credential format.\n", "---\nid: CREDENTIAL_BROWSER_LEAKAGE\nversion: 1\nseverity: high\n---\n\n# Credential Browser Leakage\n\nTrace credentials, access tokens, session tokens, and client secrets into loader/HTTP responses, browser globals, DOM values, client bundles, or external requests. Do not report server-only use or safe boolean/redacted/hash-derived values.\n", "---\nid: CREDENTIAL_LOG_LEAKAGE\nversion: 1\nseverity: high\n---\n\n# Credential Log Leakage\n\nTrace credentials, access tokens, session tokens, and client secrets through aliases and helpers to console, logger, telemetry, or error-reporting sinks. Do not report boolean presence checks, deliberate redaction, or one-way hashes.\n", "---\nid: CSRF_MISSING_PROTECTION\nversion: 2\nseverity: medium\n---\n\nFind state-changing endpoints (POST, PUT, DELETE, PATCH) that don't\nverify CSRF protection, allowing an attacker to forge requests on\nbehalf of an authenticated user.\n\nCSRF (Cross-Site Request Forgery) occurs when an app accepts\nstate-changing requests without checking that the request came from\nthe app's own UI. In Shopify apps, embedded apps use session tokens\n(JWT) that provide some CSRF protection, but server-rendered apps and\napp proxies still need explicit CSRF checks.\n\n## What to look for\n\n1. **Find state-changing handlers.** Search for:\n - Rails: controller actions responding to POST/PUT/PATCH/DELETE\n (check `routes.rb` or controller method names like `create`,\n `update`, `destroy`)\n - Remix: `action` exports in route files\n - Express: `app.post()`, `app.put()`, `app.delete()`\n - PHP: form handlers, POST routes\n\n2. **Check for CSRF protection on each.** Look for:\n - Rails: `protect_from_forgery` (default in Rails, but check for\n `skip_forgery_protection` or `protect_from_forgery with: :null_session`)\n - Remix: session token validation (`authenticate.admin(request)`)\n - Express: `csurf` middleware or equivalent\n - PHP: CSRF token in form, `VerifyCsrfToken` middleware\n\n3. **Flag explicit opt-outs.** Search for:\n - `skip_forgery_protection` — disables CSRF entirely for a controller\n - `protect_from_forgery with: :null_session` — used for webhooks, but\n if on a non-webhook endpoint, CSRF is missing\n - `skip_before_action :verify_authenticity_token` — skips the Rails\n CSRF check\n\n4. **Distinguish webhooks from user-facing endpoints.** Webhooks use\n HMAC verification instead of CSRF tokens — `protect_from_forgery\n with: :null_session` is correct for webhooks. But the same pattern\n on a user-facing POST handler is a CSRF vulnerability.\n\n5. **Check Shopify-specific patterns.** Embedded apps that use\n `authenticate.admin(request)` get session token validation that\n prevents CSRF. But if an action skips `authenticate.admin` and still\n processes state changes, CSRF protection may be missing.\n\n6. **Require a concrete sensitive action.** A missing anti-CSRF signal is only a\n finding when the forged request can change privileged state, access protected\n data, or trigger another security-relevant action. A harmless no-op or public\n write endpoint is not enough by itself.\n\n## What to report\n\nFor each state-changing endpoint without CSRF protection that reaches a concrete sensitive action:\n\n```json\n{\n \"file\": \"app/controllers/settings_controller.rb\",\n \"line\": 5,\n \"message\": \"POST handler with CSRF protection disabled\",\n \"snippet\": \"skip_forgery_protection\",\n \"evidence\": [\n {\n \"file\": \"app/controllers/settings_controller.rb\",\n \"line\": 5,\n \"quote\": \"skip_forgery_protection\"\n },\n {\n \"file\": \"app/controllers/settings_controller.rb\",\n \"line\": 10,\n \"quote\": \"def update\"\n }\n ],\n \"confidence\": \"medium\",\n \"reasoning\": \"The update action accepts POST requests but CSRF protection is explicitly skipped. This is not a webhook handler, and the action mutates privileged state, so an attacker can forge the request from another site.\"\n}\n```\n\nDo not report:\n\n- Webhook handlers with `protect_from_forgery with: :null_session`\n (HMAC is the CSRF protection for webhooks)\n- Endpoints protected by `authenticate.admin(request)` (session\n token provides CSRF protection)\n- GET-only handlers (not state-changing)\n- API endpoints that use bearer token auth (not cookie-based, so\n CSRF doesn't apply)\n- State-changing handlers where no privileged or security-relevant effect is reachable\n- Test controllers\n", diff --git a/packages/app/src/cli/services/app-security-engine/rules/secret-rules.ts b/packages/app/src/cli/services/app-security-engine/rules/secret-rules.ts index 8d145604b1c..bcea64eb60e 100644 --- a/packages/app/src/cli/services/app-security-engine/rules/secret-rules.ts +++ b/packages/app/src/cli/services/app-security-engine/rules/secret-rules.ts @@ -25,25 +25,22 @@ interface SecretPattern { } export const SECRET_PATTERNS: SecretPattern[] = [ - // Shopify API secret (shpss_ prefix or 32 hex) + // Shopify credentials are recognized by value prefix, never by variable or + // key name — a secret-sounding name with a placeholder value (as in + // committed `.env.example` files) is not evidence of a leak. + // shpat_/shpca_/shppa_/shpss_ bodies are hex; shprt_/shpsb_/shptka_/shpua_ + // are alphanumeric. The first seven prefixes are already public via + // shopify.dev docs and published secret-scanning rules (gitleaks, GitHub + // partner patterns); shpua_ marks tokens issued while an app is still in + // development — the most likely to be committed. { - regex: /(?:api[_-]?secret|SHOPIFY_API_SECRET)\s*[:=]\s*['"](shpss_[a-f0-9]+|[a-f0-9]{32})['"]/i, - name: 'Shopify API secret', - }, - // Shopify access token (shpat_ / shpca_ / shppa_) - { - regex: /(?:access[_-]?token|SHOPIFY_ACCESS_TOKEN)\s*[:=]\s*['"](shp(?:at|ca|pa)_[a-zA-Z0-9]+)['"]/i, - name: 'Shopify access token', - }, - // Bare Shopify tokens, even without an assignment context - { - regex: /shp(?:at|ca|pa|ss)_[a-fA-F0-9]{16,}/, + regex: /shp(?:(?:at|ca|pa|ss)_[a-fA-F0-9]{16,}|(?:rt|sb|tka|ua)_[a-zA-Z0-9]{16,})/, name: 'Shopify token', wholeMatch: true, }, - // Stripe keys + // Stripe secret/restricted keys. Publishable `pk_` keys are public by design. { - regex: /(?:sk|pk|rk)_(?:live|test)_[a-zA-Z0-9]{20,}/, + regex: /(?:sk|rk)_(?:live|test)_[a-zA-Z0-9]{20,}/, name: 'Stripe API key', wholeMatch: true, }, @@ -125,37 +122,51 @@ export function redactText(text: string): string { return redacted } -const ENV_FILE_PATTERN = /(^|\/)\.env(?:\.[^/]+)?$/ const NAMED_SECRET_FILE_PATTERN = /(^|\/)(?:\.env\.(?:secrets|keys)|(?:secrets|credentials)\.json)$/ -const SECRET_ASSIGNMENT_PATTERN = - /(?:api[_-]?key|api[_-]?secret|access[_-]?token|secret[_-]?key|private[_-]?key|password|SHOPIFY_API_KEY|SHOPIFY_API_SECRET)\s*[:=]\s*(?:"[^"\r\n]+"|'[^'\r\n]+'|[^\s#'"\r\n][^#\r\n]*)/i -function containsSecretLikeValue(content: string): boolean { - return SECRET_ASSIGNMENT_PATTERN.test(content) || SECRET_PATTERNS.some((pattern) => pattern.regex.test(content)) +function envFileBasename(path: string): string | undefined { + const basename = path.slice(path.lastIndexOf('/') + 1) + if (basename === '.env' || basename.startsWith('.env.')) return basename + return undefined +} + +function isEnvFile(path: string): boolean { + return envFileBasename(path) !== undefined } function committedSecretFileIssue(file: SourceFile, status: GitFileStatus, environmentFile: boolean): Issue { + const kind = environmentFile ? 'Environment file with secrets' : 'Secret file' const tracked = status.tracked === true + const untrackedAndNotIgnored = status.tracked === false && status.ignored === false + let title: string - if (tracked) - title = environmentFile ? 'Environment file with secrets is tracked by git' : 'Secret file is tracked by git' - else title = environmentFile ? 'Environment file with secrets committed to repository' : 'Secret file in repository' + let message: string + let fixDescription: string + if (tracked) { + title = `${kind} is tracked by git` + message = `${file.path} IS TRACKED BY GIT (confirmed via git ls-files), so its contents are in the repository history. Rotate every exposed secret and purge the file from history.` + fixDescription = `git rm --cached ${file.path}, add it to .gitignore, purge it from history, and rotate every exposed secret` + } else if (untrackedAndNotIgnored) { + title = `${kind} is not ignored by git` + message = `${file.path} is untracked but not ignored. If committed, its contents enter repository history.` + fixDescription = `Add ${file.path} to .gitignore, confirm with 'git check-ignore ${file.path}', and rotate any exposed secrets` + } else { + title = `${kind} could not be confirmed as ignored` + message = `${file.path} could not be confirmed as untracked-and-ignored${status.reason ? ` (${status.reason})` : ''}. Confirm it is gitignored before treating this as clean.` + fixDescription = `Add ${file.path} to .gitignore, confirm with 'git ls-files ${file.path}', and rotate any exposed secrets` + } return { id: 'COMMITTED_SECRET', severity: 'high', points: -50, title, - message: tracked - ? `${file.path} IS TRACKED BY GIT (confirmed via git ls-files), so its contents are in the repository history. Rotate every exposed secret and purge the file from history.` - : `${file.path} could not be confirmed as untracked-and-ignored${status.reason ? ` (${status.reason})` : ''}. Treating it as exposed.`, + message, location: {file: file.path}, detection_evidence: status.evidence, fix: { automated: false, - description: tracked - ? `git rm --cached ${file.path}, add it to .gitignore, purge it from history, and rotate every exposed secret` - : `Add ${file.path} to .gitignore, confirm with 'git ls-files ${file.path}', and rotate any exposed secrets`, + description: fixDescription, }, } } @@ -165,24 +176,37 @@ export async function scanCommittedSecrets(secretEvidenceFiles: SourceFile[], ap const issues: Issue[] = [] for (const file of secretEvidenceFiles) { - if (file.content === undefined) continue - const environmentFile = ENV_FILE_PATTERN.test(file.path) + const content = file.content + if (content === undefined) continue + const environmentFile = isEnvFile(file.path) const namedSecretFile = NAMED_SECRET_FILE_PATTERN.test(file.path) if (!environmentFile && !namedSecretFile) continue - if (environmentFile && !namedSecretFile && !containsSecretLikeValue(file.content)) continue + + // Only a recognizable secret value is evidence. A secret-sounding + // variable or key name proves nothing (see SECRET_PATTERNS). + const hasEvidence = SECRET_PATTERNS.some((pattern) => pattern.regex.test(content)) + const emptyNamedSecret = namedSecretFile && content.trim() === '' + if (!hasEvidence && !emptyNamedSecret) continue // Keep git probes sequential to avoid spawning competing processes for one repository. // eslint-disable-next-line no-await-in-loop const status = await gitStatusFor(appRoot, file.path) - // A safe local secret file is not a vulnerability or scoring event. Tracked - // and indeterminate states remain fail-closed, including empty named secret - // files whose history cannot be inferred from their current contents. + // A safe local secret file is not a vulnerability or scoring event. if (status.tracked === false && status.ignored === true) continue + // Empty named secret files stay fail-closed only when git confirms they are + // tracked — history may still contain prior secrets. Unknown git plus empty + // contents is not a provable leak. + if (!hasEvidence) { + if (emptyNamedSecret && status.tracked === true) { + issues.push(committedSecretFileIssue(file, status, environmentFile)) + } + continue + } issues.push(committedSecretFileIssue(file, status, environmentFile)) } for (const file of secretEvidenceFiles) { - if (!file.content || ENV_FILE_PATTERN.test(file.path) || NAMED_SECRET_FILE_PATTERN.test(file.path)) continue + if (!file.content || isEnvFile(file.path) || NAMED_SECRET_FILE_PATTERN.test(file.path)) continue const lines = file.content.split('\n') for (const [index, line] of lines.entries()) { diff --git a/packages/app/src/cli/services/app-security-engine/scanners/index.ts b/packages/app/src/cli/services/app-security-engine/scanners/index.ts index 275c4f1cfce..bd947512a1b 100644 --- a/packages/app/src/cli/services/app-security-engine/scanners/index.ts +++ b/packages/app/src/cli/services/app-security-engine/scanners/index.ts @@ -168,7 +168,7 @@ const DETERMINISTIC_CHECK_DEFINITIONS: ReadonlyArray { expect(DETERMINISTIC_CHECKS.get('REQUEST_CONTROLLED_ADMIN_CONTEXT')?.version).toBe(3) expect(DETERMINISTIC_CHECKS.get('APP_PROXY_LIQUID_INJECTION')?.version).toBe(2) expect(DETERMINISTIC_CHECKS.get('INSECURE_WEBHOOK_URL')?.version).toBe(2) + expect(DETERMINISTIC_CHECKS.get('COMMITTED_SECRET')?.version).toBe(2) }) test('extracts security fields from parsed TOML without source regexes', () => { diff --git a/packages/app/src/cli/services/app-security-engine/tests/secret-safety.test.ts b/packages/app/src/cli/services/app-security-engine/tests/secret-safety.test.ts index 4e4ad2b50c4..b443ff94e14 100644 --- a/packages/app/src/cli/services/app-security-engine/tests/secret-safety.test.ts +++ b/packages/app/src/cli/services/app-security-engine/tests/secret-safety.test.ts @@ -50,6 +50,7 @@ const PROBES = { awsAccessKey: compose('AKIA', 'IOSFODNN7EXAMPLE'), awsSecretKey: compose('wJalrXUtnFEMI', 'K7MDENGbPxRfiCYEXAMPLEKEY12'), stripeLive: compose('sk_', `live_51H8xQ2eZvKYlo2C${ALNUM.slice(0, 24)}`), + stripePublishable: compose('pk_', `live_51H8xQ2eZvKYlo2C${ALNUM.slice(0, 24)}`), shopifyToken: compose('shp', `at_${HEX32}`), shopifySecret: compose('shp', `ss_${HEX32}`), githubToken: compose('gh', `p_${ALNUM.repeat(2).slice(0, 36)}`), @@ -58,6 +59,8 @@ const PROBES = { pemHeader: compose('-----BEGIN ', 'RSA PRIVATE KEY-----'), } +const trackedEnvSecret = () => `SHOPIFY_API_SECRET=${PROBES.shopifySecret}\n` + const TOML = `name = "t" client_id = "abc123" application_url = "https://example.com" @@ -176,7 +179,7 @@ describe('git status drives severity, not .gitignore text', () => { // The classic leak: commit the file, then gitignore it and assume safety. const dir = makeApp({}) git(dir, ['init', '-q', '.']) - writeFileSync(join(dir, '.env'), 'SHOPIFY_API_SECRET=placeholder-value-here\n') + writeFileSync(join(dir, '.env'), trackedEnvSecret()) git(dir, ['add', '-f', '.env']) git(dir, ['commit', '-qm', 'oops']) writeFileSync(join(dir, '.gitignore'), '.env\n') @@ -198,7 +201,7 @@ describe('git status drives severity, not .gitignore text', () => { writeFileSync(join(dir, '.gitignore'), '.env\n') git(dir, ['add', '.gitignore']) git(dir, ['commit', '-qm', 'init']) - writeFileSync(join(dir, '.env'), 'SHOPIFY_API_SECRET=placeholder-value-here\n') + writeFileSync(join(dir, '.env'), trackedEnvSecret()) const result = await scan(dir) const finding = result.issues.find((i) => i.id === 'COMMITTED_SECRET') @@ -221,24 +224,52 @@ describe('git status drives severity, not .gitignore text', () => { rmSync(dir, {recursive: true, force: true}) }) - test('fails closed for an empty named secret file when git status is unknown', async () => { + test('does not score an empty named secret file when git status is unknown', async () => { const dir = makeApp({'secrets.json': ''}) + const result = await scan(dir) + expect(result.issues.find((issue) => issue.id === 'COMMITTED_SECRET')).toBeUndefined() + rmSync(dir, {recursive: true, force: true}) + }) + + test('fails closed for an empty named secret file that git confirms is tracked', async () => { + const dir = makeApp({}) + git(dir, ['init', '-q', '.']) + writeFileSync(join(dir, 'secrets.json'), '') + git(dir, ['add', 'secrets.json', 'shopify.app.toml']) + git(dir, ['commit', '-qm', 'init']) + const result = await scan(dir) const finding = result.issues.find((issue) => issue.id === 'COMMITTED_SECRET') - expect(finding).toMatchObject({severity: 'high', location: {file: 'secrets.json'}}) + expect(finding).toMatchObject({ + severity: 'high', + location: {file: 'secrets.json'}, + title: 'Secret file is tracked by git', + }) rmSync(dir, {recursive: true, force: true}) }) - test('fails closed when git cannot answer (no repository)', async () => { - // Unknown status must never be treated as safe. + test('does not score placeholder env values when git cannot answer', async () => { const dir = makeApp({}) writeFileSync(join(dir, '.env'), 'SHOPIFY_API_SECRET=placeholder-value-here\n') writeFileSync(join(dir, '.gitignore'), '.env\n') + const result = await scan(dir) + expect(result.issues.find((i) => i.id === 'COMMITTED_SECRET')).toBeUndefined() + rmSync(dir, {recursive: true, force: true}) + }) + + test('reports unverified exposure separately from confirmed tracked files', async () => { + const dir = makeApp({}) + writeFileSync(join(dir, '.env'), trackedEnvSecret()) + const result = await scan(dir) const finding = result.issues.find((i) => i.id === 'COMMITTED_SECRET') - expect(finding).toBeDefined() - expect(finding!.severity).toBe('high') + expect(finding).toMatchObject({ + severity: 'high', + points: -50, + title: 'Environment file with secrets could not be confirmed as ignored', + }) + expect(finding!.message).not.toContain('IS TRACKED BY GIT') rmSync(dir, {recursive: true, force: true}) }) @@ -266,6 +297,173 @@ describe('git status drives severity, not .gitignore text', () => { }) }) +describe('committed secret classification', () => { + test('detects every supported Shopify credential prefix as a bare value', () => { + const prefixes = ['shpat_', 'shpca_', 'shppa_', 'shpss_', 'shprt_', 'shpsb_', 'shptka_', 'shpua_'] + for (const prefix of prefixes) { + const token = compose(prefix, HEX32) + expect( + SECRET_PATTERNS.some((pattern) => pattern.regex.test(token)), + `${prefix} not detected`, + ).toBe(true) + } + }) + + test('does not score template env files with placeholder values', async () => { + const dir = makeApp({ + '.env.example': 'SHOPIFY_API_SECRET=your-secret-here\nSHOPIFY_API_KEY=your-key-here\n', + }) + git(dir, ['init', '-q', '.']) + git(dir, ['add', '.']) + git(dir, ['commit', '-qm', 'init']) + + const result = await scan(dir) + expect(result.issues.filter((issue) => issue.id === 'COMMITTED_SECRET')).toEqual([]) + rmSync(dir, {recursive: true, force: true}) + }) + + test('scores a real token format in a template env file', async () => { + const dir = makeApp({'.env.example': trackedEnvSecret()}) + git(dir, ['init', '-q', '.']) + git(dir, ['add', '.']) + git(dir, ['commit', '-qm', 'init']) + + const result = await scan(dir) + expect(result.issues.find((issue) => issue.id === 'COMMITTED_SECRET')).toMatchObject({ + severity: 'high', + location: {file: '.env.example'}, + }) + rmSync(dir, {recursive: true, force: true}) + }) + + test('does not score public Shopify API keys in env files', async () => { + const dir = makeApp({'.env': `SHOPIFY_API_KEY=${HEX32}\n`}) + git(dir, ['init', '-q', '.']) + git(dir, ['add', '-f', '.env']) + git(dir, ['commit', '-qm', 'init']) + + const result = await scan(dir) + expect(result.issues.filter((issue) => issue.id === 'COMMITTED_SECRET')).toEqual([]) + rmSync(dir, {recursive: true, force: true}) + }) + + test('does not score placeholder assignments in a tracked .env', async () => { + const dir = makeApp({'.env': 'SHOPIFY_API_SECRET=placeholder-value-here\npassword=changeme\n'}) + git(dir, ['init', '-q', '.']) + git(dir, ['add', '-f', '.env']) + git(dir, ['commit', '-qm', 'init']) + + const result = await scan(dir) + expect(result.issues.filter((issue) => issue.id === 'COMMITTED_SECRET')).toEqual([]) + rmSync(dir, {recursive: true, force: true}) + }) + + test('scores a real token format in a multi-suffix template env file', async () => { + const dir = makeApp({'.env.local.example': trackedEnvSecret()}) + git(dir, ['init', '-q', '.']) + git(dir, ['add', '.']) + git(dir, ['commit', '-qm', 'init']) + + const result = await scan(dir) + expect(result.issues.find((issue) => issue.id === 'COMMITTED_SECRET')).toMatchObject({ + severity: 'high', + location: {file: '.env.local.example'}, + }) + rmSync(dir, {recursive: true, force: true}) + }) + + test('does not score a value identified only by a secret-sounding key name', async () => { + // Key names are not evidence: `password` in JSON could be anything, and + // flagging it is what flooded partners with false positives. + const dir = makeApp({'secrets.json': '{ "password": "correct-horse-battery-staple" }\n'}) + git(dir, ['init', '-q', '.']) + git(dir, ['add', '.']) + git(dir, ['commit', '-qm', 'init']) + + const result = await scan(dir) + expect(result.issues.filter((issue) => issue.id === 'COMMITTED_SECRET')).toEqual([]) + rmSync(dir, {recursive: true, force: true}) + }) + + test('does not score quoted JSON placeholder assignments in a tracked named secret file', async () => { + const dir = makeApp({'secrets.json': '{ "password": "changeme" }\n'}) + git(dir, ['init', '-q', '.']) + git(dir, ['add', '.']) + git(dir, ['commit', '-qm', 'init']) + + const result = await scan(dir) + expect(result.issues.filter((issue) => issue.id === 'COMMITTED_SECRET')).toEqual([]) + rmSync(dir, {recursive: true, force: true}) + }) + + test('does not score a 32-hex value under a secret-sounding name, quoted or not', async () => { + // Legacy Shopify API secrets are 32 hex chars, but so are client IDs, + // webhook ids, and content hashes — without a prefix the value is not + // provably a secret. + for (const assignment of [`SHOPIFY_API_SECRET="${HEX32}"\n`, `SHOPIFY_API_SECRET=${HEX32}\n`]) { + const dir = makeApp({'.env': assignment}) + git(dir, ['init', '-q', '.']) + git(dir, ['add', '-f', '.env']) + git(dir, ['commit', '-qm', 'init']) + + // eslint-disable-next-line no-await-in-loop + const result = await scan(dir) + expect(result.issues.filter((issue) => issue.id === 'COMMITTED_SECRET')).toEqual([]) + rmSync(dir, {recursive: true, force: true}) + } + }) + + test('scores a prefixed Shopify token in source regardless of variable name', async () => { + const dir = makeApp({'config.js': `const token = "${PROBES.shopifySecret}"\n`}) + const result = await scan(dir) + expect(result.issues.find((issue) => issue.id === 'COMMITTED_SECRET')).toMatchObject({ + severity: 'high', + location: {file: 'config.js'}, + }) + rmSync(dir, {recursive: true, force: true}) + }) + + test('does not score a 32-hex value under a secret-sounding name in source', async () => { + const dir = makeApp({'config.js': `SHOPIFY_API_SECRET=${HEX32}\n`}) + const result = await scan(dir) + expect(result.issues.filter((issue) => issue.id === 'COMMITTED_SECRET')).toEqual([]) + rmSync(dir, {recursive: true, force: true}) + }) + + test('does not treat a blank secret assignment as the next line', async () => { + const dir = makeApp({'.env': 'SHOPIFY_API_SECRET=\nPORT=3000\n'}) + git(dir, ['init', '-q', '.']) + git(dir, ['add', '-f', '.env']) + git(dir, ['commit', '-qm', 'init']) + + const result = await scan(dir) + expect(result.issues.filter((issue) => issue.id === 'COMMITTED_SECRET')).toEqual([]) + rmSync(dir, {recursive: true, force: true}) + }) + + test('does not score boolean flags whose names merely contain password', async () => { + const dir = makeApp({'.env': 'HAS_PASSWORD=true\n'}) + git(dir, ['init', '-q', '.']) + git(dir, ['add', '-f', '.env']) + git(dir, ['commit', '-qm', 'init']) + + const result = await scan(dir) + expect(result.issues.filter((issue) => issue.id === 'COMMITTED_SECRET')).toEqual([]) + rmSync(dir, {recursive: true, force: true}) + }) + + test('does not treat Stripe publishable keys as secrets', async () => { + const line = `const k = "${PROBES.stripePublishable}";` + expect(SECRET_PATTERNS.some((pattern) => pattern.regex.test(line))).toBe(false) + + const dir = makeApp({'config.js': `${line}\n`}) + const result = await scan(dir) + expect(result.issues.filter((issue) => issue.id === 'COMMITTED_SECRET')).toEqual([]) + expect(JSON.stringify(result)).not.toContain(PROBES.stripePublishable) + rmSync(dir, {recursive: true, force: true}) + }) +}) + describe('secret evidence coverage', () => { test('scans common repository text formats and unsupported source languages', async () => { const files = { @@ -321,10 +519,6 @@ describe('incomplete coverage is reported, not hidden', () => { /** Probe strings with realistic shape, assembled at runtime. See note above. */ function probeFor(name: string): string | undefined { switch (name) { - case 'Shopify API secret': - return `api_secret = "${PROBES.shopifySecret}"` - case 'Shopify access token': - return `access_token = "${PROBES.shopifyToken}"` case 'Shopify token': return `x = ${PROBES.shopifyToken}` case 'Stripe API key':