From c8f7f06f7a52c12f04613ec2b60fbfe2433589e7 Mon Sep 17 00:00:00 2001 From: Mar Lopez Date: Tue, 8 Sep 2026 13:43:58 -0400 Subject: [PATCH 1/7] Add static frame ancestors check Assisted-By: devx/0434e4da-7c55-4ded-87bd-400bf4d8a927 --- .changeset/calm-frames-doctor.md | 5 ++ .../app-doctor-engine/capabilities/detect.ts | 2 + .../app-doctor-engine/rules/catalog.ts | 5 +- .../app-doctor-engine/rules/csp-rules.ts | 39 ++++++++++++++++ .../app-doctor-engine/scanners/index.ts | 5 ++ .../tests/deterministic-rules.test.ts | 37 ++++++++++++++- .../tests/rule-analysis.test.ts | 1 + .../tests/scan-contract.test.ts | 46 +++++++++++++++++++ .../app-doctor-engine/tests/trace.test.ts | 1 + .../cli/services/app-doctor-engine/types.ts | 1 + .../app-doctor-json-fixtures/compile.json | 1 + .../app-doctor-json-fixtures/scan.json | 1 + .../src/cli/services/app-doctor-json.test.ts | 1 + .../src/cli/services/doctor-output.test.ts | 1 + packages/app/src/cli/services/doctor.test.ts | 1 + 15 files changed, 144 insertions(+), 3 deletions(-) create mode 100644 .changeset/calm-frames-doctor.md create mode 100644 packages/app/src/cli/services/app-doctor-engine/rules/csp-rules.ts diff --git a/.changeset/calm-frames-doctor.md b/.changeset/calm-frames-doctor.md new file mode 100644 index 00000000000..a6cc8d7e78a --- /dev/null +++ b/.changeset/calm-frames-doctor.md @@ -0,0 +1,5 @@ +--- +'@shopify/app': patch +--- + +Add a narrow deterministic App Doctor check for wildcard `frame-ancestors` policies in embedded admin apps. diff --git a/packages/app/src/cli/services/app-doctor-engine/capabilities/detect.ts b/packages/app/src/cli/services/app-doctor-engine/capabilities/detect.ts index a3a2121d916..52cf4f27205 100644 --- a/packages/app/src/cli/services/app-doctor-engine/capabilities/detect.ts +++ b/packages/app/src/cli/services/app-doctor-engine/capabilities/detect.ts @@ -9,6 +9,7 @@ export function detectCapabilities( ): Capabilities { const themeExtension = extensions.some((extension) => extension.type === 'theme') const appEmbed = extensions.some((extension) => extension.type === 'theme' && hasAppEmbedBlock(extension)) + const embeddedApp = (appToml?.raw as Record | undefined)?.embedded === true const scriptTags = sourceFiles.some((file) => file.content ? /script[_-]?tags?|ScriptTag/i.test(file.content) : false, @@ -23,6 +24,7 @@ export function detectCapabilities( return { theme_app_extension: themeExtension, app_embed: appEmbed, + embedded_app: embeddedApp, script_tags: scriptTags, webhooks: Boolean(appToml?.webhooks.length), app_proxy: Boolean((appToml?.raw as Record)?.app_proxy), diff --git a/packages/app/src/cli/services/app-doctor-engine/rules/catalog.ts b/packages/app/src/cli/services/app-doctor-engine/rules/catalog.ts index eafc9f7a957..527680d39cc 100644 --- a/packages/app/src/cli/services/app-doctor-engine/rules/catalog.ts +++ b/packages/app/src/cli/services/app-doctor-engine/rules/catalog.ts @@ -268,9 +268,10 @@ export const RULE_CATALOG: RuleCatalogEntry[] = [ title: 'Embedded app frame-ancestors uses a wildcard', severity: 'high', points: -12, - description: 'Detects wildcard frame-ancestors CSP policies in Shopify app code.', - fix: 'Build frame-ancestors per request from the authenticated shop domain and admin.shopify.com.', + description: 'Detects literal wildcard or clearly permissive frame-ancestors policies in embedded app code.', + fix: 'Restrict frame-ancestors to Shopify Admin and the authenticated shop origin.', guide: 'https://shopify.dev/docs/apps/build/security/set-up-iframe-protection', + requires: 'embedded_app', }, { id: 'ACTIVE_UPLOADS_AND_PRIVILEGED_PREVIEWS', diff --git a/packages/app/src/cli/services/app-doctor-engine/rules/csp-rules.ts b/packages/app/src/cli/services/app-doctor-engine/rules/csp-rules.ts new file mode 100644 index 00000000000..272df93b67e --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-engine/rules/csp-rules.ts @@ -0,0 +1,39 @@ +import type {Issue} from '../types.js' +import type {SourceFile} from './types.js' + +const JAVASCRIPT_EXTENSIONS = new Set(['.js', '.jsx', '.mjs', '.cjs', '.ts', '.tsx', '.mts', '.cts']) +const CSP_FRAME_ANCESTORS_DIRECTIVE = + /\bContent-Security-Policy\b[^\r\n]{0,300}?\bframe-ancestors\b([^;\r\n]*)/gi +const CLEARLY_PERMISSIVE_SOURCE = /(^|\s)\*(?=\s|["'`;,}]|$)|https?:\/\/\*\.|\*\.myshopify\.com/i + +export function scanStaticFrameAncestors(files: SourceFile[]): Issue[] { + const issues: Issue[] = [] + for (const file of files) { + if (!file.content || !JAVASCRIPT_EXTENSIONS.has(file.ext)) continue + const source = stripCommentsOnly(file.content) + CSP_FRAME_ANCESTORS_DIRECTIVE.lastIndex = 0 + let directive = CSP_FRAME_ANCESTORS_DIRECTIVE.exec(source) + while (directive) { + if (CLEARLY_PERMISSIVE_SOURCE.test(directive[1] ?? '')) { + issues.push({ + id: 'STATIC_FRAME_ANCESTORS', + severity: 'high', + points: -12, + title: 'Embedded app frame-ancestors uses a wildcard', + message: 'A literal frame-ancestors directive allows arbitrary or wildcard embedding origins.', + location: {file: file.path, line: source.slice(0, directive.index).split('\n').length}, + fix: { + automated: false, + description: 'Restrict frame-ancestors to Shopify Admin and the authenticated shop origin.', + guide: 'https://shopify.dev/docs/apps/build/security/set-up-iframe-protection', + }, + }) + } + directive = CSP_FRAME_ANCESTORS_DIRECTIVE.exec(source) + } + } + return issues +} +function stripCommentsOnly(source: string): string { + return source.replace(/\/\*[\s\S]*?\*\//g, (comment) => comment.replace(/[^\n]/g, ' ')).replace(/(^|\s)\/\/[^\n]*/g, '$1') +} diff --git a/packages/app/src/cli/services/app-doctor-engine/scanners/index.ts b/packages/app/src/cli/services/app-doctor-engine/scanners/index.ts index fd2095b6bf7..6057755c5d3 100644 --- a/packages/app/src/cli/services/app-doctor-engine/scanners/index.ts +++ b/packages/app/src/cli/services/app-doctor-engine/scanners/index.ts @@ -29,6 +29,7 @@ import {scanDeprecatedScriptTagApi} from '../rules/shopify-rules.js' import {missingComplianceWebhooks, scanEolApiVersions} from '../rules/compliance-rules.js' import {scanAppProxyLiquidInjection} from '../rules/proxy-rules.js' import {scanExpiringOfflineTokens} from '../rules/token-rules.js' +import {scanStaticFrameAncestors} from '../rules/csp-rules.js' import {RULE_CATALOG} from '../rules/catalog.js' import {redactIssue} from '../trace/index.js' import {getEngineVersion} from '../version.js' @@ -199,6 +200,10 @@ const DETERMINISTIC_CHECK_DEFINITIONS: ReadonlyArray scanStaticFrameAncestors(context.sourceFiles)), + requires: 'embedded_app', + }, ] function unsafeInnerHtmlRunner(context: ScanContext): RunnerResult { diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts b/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts index 3004c8a3726..402ac2fcb39 100644 --- a/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts +++ b/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts @@ -11,6 +11,7 @@ import { } from '../rules/js-rules.js' import {scanLiquidSecurity} from '../rules/liquid-rules.js' import {auditKnownCves, parseAuditOutput} from '../rules/dependency-rules.js' +import {scanStaticFrameAncestors} from '../rules/csp-rules.js' import {describe, expect, test} from 'vitest' import {mkdtemp, rm, writeFile} from 'node:fs/promises' import {join} from 'node:path' @@ -32,6 +33,7 @@ const ACTIVE_IDS = [ 'LIQUID_UNSAFE_RENDER', 'UNSAFE_INNERHTML', 'APP_PROXY_LIQUID_INJECTION', + 'STATIC_FRAME_ANCESTORS', ].sort() const source = (content: string, path = 'app/routes/example.tsx'): SourceFile => ({ @@ -42,7 +44,7 @@ const source = (content: string, path = 'app/routes/example.tsx'): SourceFile => }) describe('deterministic rules product contract', () => { - test('has exactly fourteen active executable deterministic identities', () => { + test('has exactly fifteen active executable deterministic identities', () => { expect([...DETERMINISTIC_CHECKS.keys()].sort()).toEqual(ACTIVE_IDS) expect([...DETERMINISTIC_CHECKS.values()].every((check) => check.lifecycle === 'active' && check.runner)).toBe(true) const registry = getRegistry() @@ -162,6 +164,39 @@ describe('JavaScript regex mode', () => { }) }) +describe('STATIC_FRAME_ANCESTORS regex mode', () => { + test('flags literal wildcard frame-ancestors only', () => { + expect( + scanStaticFrameAncestors([ + source(`const headers = {'Content-Security-Policy': "frame-ancestors *"}`, 'app/root.tsx'), + ]), + ).toHaveLength(1) + expect( + scanStaticFrameAncestors([ + source( + `const headers = {'Content-Security-Policy': "frame-ancestors https://admin.shopify.com https://merchant.myshopify.com"}`, + 'app/root.tsx', + ), + ]), + ).toEqual([]) + expect( + scanStaticFrameAncestors([ + source(`const policy = buildPolicy(shop); const headers = {'Content-Security-Policy': policy}`, 'app/root.tsx'), + ]), + ).toEqual([]) + expect( + scanStaticFrameAncestors([ + source(`const note = 'frame-ancestors *'`, 'app/root.tsx'), + ]), + ).toEqual([]) + expect( + scanStaticFrameAncestors([ + source(`const headers = {'Content-Security-Policy': "frame-ancestors https://*.myshopify.com"}`, 'app/root.tsx'), + ]), + ).toHaveLength(1) + }) +}) + describe('Liquid AST mode', () => { test('uses context-appropriate output rules and reports parser failures', () => { expect( diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/rule-analysis.test.ts b/packages/app/src/cli/services/app-doctor-engine/tests/rule-analysis.test.ts index cd595a12489..b01753f5a82 100644 --- a/packages/app/src/cli/services/app-doctor-engine/tests/rule-analysis.test.ts +++ b/packages/app/src/cli/services/app-doctor-engine/tests/rule-analysis.test.ts @@ -62,6 +62,7 @@ function context( capabilities: { theme_app_extension: false, app_embed: false, + embedded_app: false, script_tags: false, webhooks: false, app_proxy: false, diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/scan-contract.test.ts b/packages/app/src/cli/services/app-doctor-engine/tests/scan-contract.test.ts index c5d1c700700..8578c4171d0 100644 --- a/packages/app/src/cli/services/app-doctor-engine/tests/scan-contract.test.ts +++ b/packages/app/src/cli/services/app-doctor-engine/tests/scan-contract.test.ts @@ -132,6 +132,52 @@ describe('framework and surface detection', () => { ).toMatchObject({status: 'executed', findings: 0}) }) + test('runs static frame-ancestors only for embedded admin apps', async () => { + const embedded = await scan( + await app({ + 'shopify.app.toml': `name = "Embedded app"\nembedded = true\n[access_scopes]\nscopes = ""\n`, + 'package.json': reactPackage, + 'app/shopify.server.ts': 'export const shopify = {}', + 'app/routes/index.tsx': `export const loader = () => null; const headers = {'Content-Security-Policy': 'frame-ancestors *'}`, + }), + ) + expect(embedded.scan.checks_executed.find((execution) => execution.id === 'STATIC_FRAME_ANCESTORS')).toMatchObject({ + status: 'executed', + findings: 1, + }) + + const plain = await scan( + await app({ + 'shopify.app.toml': `name = "Plain app"\nembedded = false\n[access_scopes]\nscopes = ""\n`, + 'package.json': reactPackage, + 'app/shopify.server.ts': 'export const shopify = {}', + 'app/routes/index.tsx': `export const loader = () => null; const headers = {'Content-Security-Policy': 'frame-ancestors *'}`, + }), + ) + expect(plain.scan.checks_executed.find((execution) => execution.id === 'STATIC_FRAME_ANCESTORS')).toMatchObject({ + status: 'not_applicable', + applicable: false, + }) + expect(plain.issues.some((issue) => issue.id === 'STATIC_FRAME_ANCESTORS')).toBe(false) + + const themeOnly = await scan( + await app({ + 'shopify.app.toml': `name = "Theme app"\nembedded = false\n[access_scopes]\nscopes = ""\n`, + 'package.json': reactPackage, + 'app/shopify.server.ts': 'export const shopify = {}', + 'app/routes/index.tsx': `export const loader = () => null; const headers = {'Content-Security-Policy': 'frame-ancestors *'}`, + 'extensions/theme/shopify.extension.toml': 'type = "theme"\n', + 'extensions/theme/blocks/app.liquid': `{% schema %}{"target":"body"}{% endschema %}`, + }), + ) + expect(themeOnly.capabilities.app_embed).toBe(true) + expect(themeOnly.capabilities.embedded_app).toBe(false) + expect(themeOnly.scan.checks_executed.find((execution) => execution.id === 'STATIC_FRAME_ANCESTORS')).toMatchObject({ + status: 'not_applicable', + applicable: false, + }) + }) + test('keeps React Router and theme implementations inside their supported file boundaries', async () => { const themeDirectory = await app({ 'shopify.app.toml': appConfig(), diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/trace.test.ts b/packages/app/src/cli/services/app-doctor-engine/tests/trace.test.ts index 26a2e7bf839..ba268ddce30 100644 --- a/packages/app/src/cli/services/app-doctor-engine/tests/trace.test.ts +++ b/packages/app/src/cli/services/app-doctor-engine/tests/trace.test.ts @@ -34,6 +34,7 @@ const result = (issues: Issue[] = []): ScanResult => ({ capabilities: { theme_app_extension: false, app_embed: false, + embedded_app: false, script_tags: false, webhooks: false, app_proxy: false, diff --git a/packages/app/src/cli/services/app-doctor-engine/types.ts b/packages/app/src/cli/services/app-doctor-engine/types.ts index eefa181d36c..0482c9e9fd2 100644 --- a/packages/app/src/cli/services/app-doctor-engine/types.ts +++ b/packages/app/src/cli/services/app-doctor-engine/types.ts @@ -37,6 +37,7 @@ export interface Fix { export interface Capabilities { theme_app_extension: boolean app_embed: boolean + embedded_app: boolean script_tags: boolean webhooks: boolean app_proxy: boolean diff --git a/packages/app/src/cli/services/app-doctor-json-fixtures/compile.json b/packages/app/src/cli/services/app-doctor-json-fixtures/compile.json index 2bfaf9317fe..e1c38362087 100644 --- a/packages/app/src/cli/services/app-doctor-json-fixtures/compile.json +++ b/packages/app/src/cli/services/app-doctor-json-fixtures/compile.json @@ -24,6 +24,7 @@ "capabilities": { "theme_app_extension": false, "app_embed": false, + "embedded_app": false, "script_tags": false, "webhooks": false, "app_proxy": false, diff --git a/packages/app/src/cli/services/app-doctor-json-fixtures/scan.json b/packages/app/src/cli/services/app-doctor-json-fixtures/scan.json index 72798016dae..3625e88346d 100644 --- a/packages/app/src/cli/services/app-doctor-json-fixtures/scan.json +++ b/packages/app/src/cli/services/app-doctor-json-fixtures/scan.json @@ -24,6 +24,7 @@ "capabilities": { "theme_app_extension": false, "app_embed": false, + "embedded_app": false, "script_tags": false, "webhooks": false, "app_proxy": false, diff --git a/packages/app/src/cli/services/app-doctor-json.test.ts b/packages/app/src/cli/services/app-doctor-json.test.ts index fb41062f907..c73fd936c25 100644 --- a/packages/app/src/cli/services/app-doctor-json.test.ts +++ b/packages/app/src/cli/services/app-doctor-json.test.ts @@ -23,6 +23,7 @@ const scan: ScanResult = { capabilities: { theme_app_extension: false, app_embed: false, + embedded_app: false, script_tags: false, webhooks: false, app_proxy: false, diff --git a/packages/app/src/cli/services/doctor-output.test.ts b/packages/app/src/cli/services/doctor-output.test.ts index 00729982b73..dc002689d7f 100644 --- a/packages/app/src/cli/services/doctor-output.test.ts +++ b/packages/app/src/cli/services/doctor-output.test.ts @@ -23,6 +23,7 @@ const scanWithIssues: ScanResult = { capabilities: { theme_app_extension: false, app_embed: false, + embedded_app: false, script_tags: false, webhooks: false, app_proxy: false, diff --git a/packages/app/src/cli/services/doctor.test.ts b/packages/app/src/cli/services/doctor.test.ts index f3eff79cd51..be966ad8fb2 100644 --- a/packages/app/src/cli/services/doctor.test.ts +++ b/packages/app/src/cli/services/doctor.test.ts @@ -15,6 +15,7 @@ const scan: ScanResult = { capabilities: { theme_app_extension: false, app_embed: false, + embedded_app: false, script_tags: false, webhooks: false, app_proxy: false, From 80e32be285764f1fb31c1ae5c53ac0a855b53d94 Mon Sep 17 00:00:00 2001 From: Mar Lopez Date: Tue, 8 Sep 2026 13:45:45 -0400 Subject: [PATCH 2/7] Narrow frame ancestors wildcard matching Assisted-By: devx/0434e4da-7c55-4ded-87bd-400bf4d8a927 --- .../src/cli/services/app-doctor-engine/rules/csp-rules.ts | 2 +- .../app-doctor-engine/tests/deterministic-rules.test.ts | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/app/src/cli/services/app-doctor-engine/rules/csp-rules.ts b/packages/app/src/cli/services/app-doctor-engine/rules/csp-rules.ts index 272df93b67e..a8885d9e194 100644 --- a/packages/app/src/cli/services/app-doctor-engine/rules/csp-rules.ts +++ b/packages/app/src/cli/services/app-doctor-engine/rules/csp-rules.ts @@ -4,7 +4,7 @@ import type {SourceFile} from './types.js' const JAVASCRIPT_EXTENSIONS = new Set(['.js', '.jsx', '.mjs', '.cjs', '.ts', '.tsx', '.mts', '.cts']) const CSP_FRAME_ANCESTORS_DIRECTIVE = /\bContent-Security-Policy\b[^\r\n]{0,300}?\bframe-ancestors\b([^;\r\n]*)/gi -const CLEARLY_PERMISSIVE_SOURCE = /(^|\s)\*(?=\s|["'`;,}]|$)|https?:\/\/\*\.|\*\.myshopify\.com/i +const CLEARLY_PERMISSIVE_SOURCE = /(^|\s)\*(?=\s|["'`;,}]|$)|https?:\/\/\*\.myshopify\.com/i export function scanStaticFrameAncestors(files: SourceFile[]): Issue[] { const issues: Issue[] = [] diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts b/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts index 402ac2fcb39..ede75ebd9cd 100644 --- a/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts +++ b/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts @@ -194,6 +194,11 @@ describe('STATIC_FRAME_ANCESTORS regex mode', () => { source(`const headers = {'Content-Security-Policy': "frame-ancestors https://*.myshopify.com"}`, 'app/root.tsx'), ]), ).toHaveLength(1) + expect( + scanStaticFrameAncestors([ + source(`const headers = {'Content-Security-Policy': "frame-ancestors https://*.mycompany.dev"}`, 'app/root.tsx'), + ]), + ).toEqual([]) }) }) From 66321e6fd824d47dc8aa191ae1f781105162eb69 Mon Sep 17 00:00:00 2001 From: Mar Lopez Date: Tue, 8 Sep 2026 13:51:58 -0400 Subject: [PATCH 3/7] Format static frame ancestors check Assisted-By: devx/0434e4da-7c55-4ded-87bd-400bf4d8a927 --- .../app-doctor-engine/capabilities/detect.ts | 2 +- .../app-doctor-engine/rules/csp-rules.ts | 7 ++++--- .../tests/deterministic-rules.test.ts | 16 +++++++++------- .../tests/scan-contract.test.ts | 10 ++++++---- 4 files changed, 20 insertions(+), 15 deletions(-) diff --git a/packages/app/src/cli/services/app-doctor-engine/capabilities/detect.ts b/packages/app/src/cli/services/app-doctor-engine/capabilities/detect.ts index 52cf4f27205..fc06c02ecd5 100644 --- a/packages/app/src/cli/services/app-doctor-engine/capabilities/detect.ts +++ b/packages/app/src/cli/services/app-doctor-engine/capabilities/detect.ts @@ -9,7 +9,7 @@ export function detectCapabilities( ): Capabilities { const themeExtension = extensions.some((extension) => extension.type === 'theme') const appEmbed = extensions.some((extension) => extension.type === 'theme' && hasAppEmbedBlock(extension)) - const embeddedApp = (appToml?.raw as Record | undefined)?.embedded === true + const embeddedApp = appToml?.raw.embedded === true const scriptTags = sourceFiles.some((file) => file.content ? /script[_-]?tags?|ScriptTag/i.test(file.content) : false, diff --git a/packages/app/src/cli/services/app-doctor-engine/rules/csp-rules.ts b/packages/app/src/cli/services/app-doctor-engine/rules/csp-rules.ts index a8885d9e194..c2e3eb412ec 100644 --- a/packages/app/src/cli/services/app-doctor-engine/rules/csp-rules.ts +++ b/packages/app/src/cli/services/app-doctor-engine/rules/csp-rules.ts @@ -2,8 +2,7 @@ import type {Issue} from '../types.js' import type {SourceFile} from './types.js' const JAVASCRIPT_EXTENSIONS = new Set(['.js', '.jsx', '.mjs', '.cjs', '.ts', '.tsx', '.mts', '.cts']) -const CSP_FRAME_ANCESTORS_DIRECTIVE = - /\bContent-Security-Policy\b[^\r\n]{0,300}?\bframe-ancestors\b([^;\r\n]*)/gi +const CSP_FRAME_ANCESTORS_DIRECTIVE = /\bContent-Security-Policy\b[^\r\n]{0,300}?\bframe-ancestors\b([^;\r\n]*)/gi const CLEARLY_PERMISSIVE_SOURCE = /(^|\s)\*(?=\s|["'`;,}]|$)|https?:\/\/\*\.myshopify\.com/i export function scanStaticFrameAncestors(files: SourceFile[]): Issue[] { @@ -35,5 +34,7 @@ export function scanStaticFrameAncestors(files: SourceFile[]): Issue[] { return issues } function stripCommentsOnly(source: string): string { - return source.replace(/\/\*[\s\S]*?\*\//g, (comment) => comment.replace(/[^\n]/g, ' ')).replace(/(^|\s)\/\/[^\n]*/g, '$1') + return source + .replace(/\/\*[\s\S]*?\*\//g, (comment) => comment.replace(/[^\n]/g, ' ')) + .replace(/(^|\s)\/\/[^\n]*/g, '$1') } diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts b/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts index ede75ebd9cd..bb5a52f6bdf 100644 --- a/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts +++ b/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts @@ -184,19 +184,21 @@ describe('STATIC_FRAME_ANCESTORS regex mode', () => { source(`const policy = buildPolicy(shop); const headers = {'Content-Security-Policy': policy}`, 'app/root.tsx'), ]), ).toEqual([]) + expect(scanStaticFrameAncestors([source(`const note = 'frame-ancestors *'`, 'app/root.tsx')])).toEqual([]) expect( scanStaticFrameAncestors([ - source(`const note = 'frame-ancestors *'`, 'app/root.tsx'), - ]), - ).toEqual([]) - expect( - scanStaticFrameAncestors([ - source(`const headers = {'Content-Security-Policy': "frame-ancestors https://*.myshopify.com"}`, 'app/root.tsx'), + source( + `const headers = {'Content-Security-Policy': "frame-ancestors https://*.myshopify.com"}`, + 'app/root.tsx', + ), ]), ).toHaveLength(1) expect( scanStaticFrameAncestors([ - source(`const headers = {'Content-Security-Policy': "frame-ancestors https://*.mycompany.dev"}`, 'app/root.tsx'), + source( + `const headers = {'Content-Security-Policy': "frame-ancestors https://*.mycompany.dev"}`, + 'app/root.tsx', + ), ]), ).toEqual([]) }) diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/scan-contract.test.ts b/packages/app/src/cli/services/app-doctor-engine/tests/scan-contract.test.ts index 8578c4171d0..d9a81d1d96b 100644 --- a/packages/app/src/cli/services/app-doctor-engine/tests/scan-contract.test.ts +++ b/packages/app/src/cli/services/app-doctor-engine/tests/scan-contract.test.ts @@ -172,10 +172,12 @@ describe('framework and surface detection', () => { ) expect(themeOnly.capabilities.app_embed).toBe(true) expect(themeOnly.capabilities.embedded_app).toBe(false) - expect(themeOnly.scan.checks_executed.find((execution) => execution.id === 'STATIC_FRAME_ANCESTORS')).toMatchObject({ - status: 'not_applicable', - applicable: false, - }) + expect(themeOnly.scan.checks_executed.find((execution) => execution.id === 'STATIC_FRAME_ANCESTORS')).toMatchObject( + { + status: 'not_applicable', + applicable: false, + }, + ) }) test('keeps React Router and theme implementations inside their supported file boundaries', async () => { From fb4d491e60b6b61b94db60ee5acf82e5871135e2 Mon Sep 17 00:00:00 2001 From: Mar Lopez Date: Tue, 8 Sep 2026 15:30:04 -0400 Subject: [PATCH 4/7] Tighten static frame ancestors detection Assisted-By: devx/0434e4da-7c55-4ded-87bd-400bf4d8a927 --- .../app-doctor-engine/capabilities/detect.ts | 3 +- .../app-doctor-engine/rules/csp-rules.ts | 269 ++++++++++++++++-- .../app-doctor-engine/scanners/index.ts | 30 +- .../tests/deterministic-rules.test.ts | 60 +++- .../tests/scan-contract.test.ts | 32 +++ 5 files changed, 355 insertions(+), 39 deletions(-) diff --git a/packages/app/src/cli/services/app-doctor-engine/capabilities/detect.ts b/packages/app/src/cli/services/app-doctor-engine/capabilities/detect.ts index fc06c02ecd5..0edd7df00e1 100644 --- a/packages/app/src/cli/services/app-doctor-engine/capabilities/detect.ts +++ b/packages/app/src/cli/services/app-doctor-engine/capabilities/detect.ts @@ -6,10 +6,11 @@ export function detectCapabilities( appToml: AppTomlContent | null, extensions: ExtensionInfo[], sourceFiles: SourceFile[], + appTomls: AppTomlContent[] = appToml ? [appToml] : [], ): Capabilities { const themeExtension = extensions.some((extension) => extension.type === 'theme') const appEmbed = extensions.some((extension) => extension.type === 'theme' && hasAppEmbedBlock(extension)) - const embeddedApp = appToml?.raw.embedded === true + const embeddedApp = appTomls.some((configuration) => configuration.raw.embedded === true) const scriptTags = sourceFiles.some((file) => file.content ? /script[_-]?tags?|ScriptTag/i.test(file.content) : false, diff --git a/packages/app/src/cli/services/app-doctor-engine/rules/csp-rules.ts b/packages/app/src/cli/services/app-doctor-engine/rules/csp-rules.ts index c2e3eb412ec..b480f6c00c6 100644 --- a/packages/app/src/cli/services/app-doctor-engine/rules/csp-rules.ts +++ b/packages/app/src/cli/services/app-doctor-engine/rules/csp-rules.ts @@ -2,39 +2,256 @@ import type {Issue} from '../types.js' import type {SourceFile} from './types.js' const JAVASCRIPT_EXTENSIONS = new Set(['.js', '.jsx', '.mjs', '.cjs', '.ts', '.tsx', '.mts', '.cts']) -const CSP_FRAME_ANCESTORS_DIRECTIVE = /\bContent-Security-Policy\b[^\r\n]{0,300}?\bframe-ancestors\b([^;\r\n]*)/gi -const CLEARLY_PERMISSIVE_SOURCE = /(^|\s)\*(?=\s|["'`;,}]|$)|https?:\/\/\*\.myshopify\.com/i +const HEADER_NAME = 'content-security-policy' +const HEADER_SETTER_PREFIX = /\b(?:headers|response\.headers|res)\.(?:set|append|setHeader)\s*\(\s*$/i +const ALL_ORIGIN_WILDCARD = /^https?:\/\/\*(?::(?:\*|\d+))?(?:\/.*)?$/i +const SHOPIFY_WILDCARD = /^(?:https?:\/\/)?\*\.myshopify\.com(?::(?:\*|\d+))?(?:\/.*)?$/i + +interface StaticHeaderValue { + index: number + value: string +} + +interface ParsedStringLiteral { + end: number + value: string + static: boolean +} export function scanStaticFrameAncestors(files: SourceFile[]): Issue[] { const issues: Issue[] = [] for (const file of files) { if (!file.content || !JAVASCRIPT_EXTENSIONS.has(file.ext)) continue - const source = stripCommentsOnly(file.content) - CSP_FRAME_ANCESTORS_DIRECTIVE.lastIndex = 0 - let directive = CSP_FRAME_ANCESTORS_DIRECTIVE.exec(source) - while (directive) { - if (CLEARLY_PERMISSIVE_SOURCE.test(directive[1] ?? '')) { - issues.push({ - id: 'STATIC_FRAME_ANCESTORS', - severity: 'high', - points: -12, - title: 'Embedded app frame-ancestors uses a wildcard', - message: 'A literal frame-ancestors directive allows arbitrary or wildcard embedding origins.', - location: {file: file.path, line: source.slice(0, directive.index).split('\n').length}, - fix: { - automated: false, - description: 'Restrict frame-ancestors to Shopify Admin and the authenticated shop origin.', - guide: 'https://shopify.dev/docs/apps/build/security/set-up-iframe-protection', - }, - }) - } - directive = CSP_FRAME_ANCESTORS_DIRECTIVE.exec(source) + const source = maskComments(file.content) + for (const header of staticCspHeaderValues(source)) { + if (!hasClearlyPermissiveFrameAncestors(header.value)) continue + issues.push({ + id: 'STATIC_FRAME_ANCESTORS', + severity: 'high', + points: -12, + title: 'Embedded app frame-ancestors uses a wildcard', + message: 'A literal frame-ancestors directive allows arbitrary or wildcard embedding origins.', + location: {file: file.path, line: source.slice(0, header.index).split('\n').length}, + fix: { + automated: false, + description: 'Restrict frame-ancestors to Shopify Admin and the authenticated shop origin.', + guide: 'https://shopify.dev/docs/apps/build/security/set-up-iframe-protection', + }, + }) } } return issues } -function stripCommentsOnly(source: string): string { - return source - .replace(/\/\*[\s\S]*?\*\//g, (comment) => comment.replace(/[^\n]/g, ' ')) - .replace(/(^|\s)\/\/[^\n]*/g, '$1') + +function staticCspHeaderValues(source: string): StaticHeaderValue[] { + const headers: StaticHeaderValue[] = [] + for (let index = 0; index < source.length; index++) { + const literal = parseStringLiteral(source, index) + if (!literal) continue + if (literal.static && literal.value.toLowerCase() === HEADER_NAME) { + const value = staticHeaderValueAfter(source, index, literal.end) + if (value !== undefined) headers.push({index, value}) + } + index = literal.end - 1 + } + return headers +} + +function staticHeaderValueAfter(source: string, headerStart: number, headerEnd: number): string | undefined { + const next = skipWhitespace(source, headerEnd) + if (source[next] === ':') { + const expression = readExpression(source, next + 1, new Set([',', '}'])) + return expression ? evaluateStaticStringExpression(expression.text) : undefined + } + if (source[next] === ',' && isHeaderSetterCall(source, headerStart)) { + const expression = readExpression(source, next + 1, new Set([',', ')'])) + return expression ? evaluateStaticStringExpression(expression.text) : undefined + } + return undefined +} + +function isHeaderSetterCall(source: string, headerStart: number): boolean { + return HEADER_SETTER_PREFIX.test(source.slice(Math.max(0, headerStart - 100), headerStart)) +} + +function hasClearlyPermissiveFrameAncestors(value: string): boolean { + return value.split(';').some((directive) => { + const [name, ...sources] = directive.trim().split(/\s+/) + return name?.toLowerCase() === 'frame-ancestors' && sources.some(isClearlyPermissiveSource) + }) +} + +function isClearlyPermissiveSource(source: string): boolean { + return source === '*' || ALL_ORIGIN_WILDCARD.test(source) || SHOPIFY_WILDCARD.test(source) +} + +function evaluateStaticStringExpression(expression: string): string | undefined { + const value = stripOuterParens(expression.trim()) + if (!value) return undefined + + const literal = parseStringLiteral(value, 0) + if (literal && literal.end === value.length && literal.static) return literal.value + + const concatenated = splitTopLevel(value, '+') + if (concatenated.length > 1) { + const parts = concatenated.map((part) => evaluateStaticStringExpression(part)) + if (parts.every((part): part is string => part !== undefined)) return parts.join('') + } + + if (value.startsWith('[')) { + const close = matchingDelimiter(value, 0, '[', ']') + if (close !== undefined) { + const joinMatch = /^\.join\s*\((.*)\)$/.exec(value.slice(close + 1).trim()) + if (joinMatch) { + const separator = joinMatch[1]!.trim() ? evaluateStaticStringExpression(joinMatch[1]!) : ',' + if (separator === undefined) return undefined + const parts = splitTopLevel(value.slice(1, close), ',') + .map((part) => part.trim()) + .filter(Boolean) + .map((part) => evaluateStaticStringExpression(part)) + if (parts.every((part): part is string => part !== undefined)) return parts.join(separator) + } + } + } + + return undefined +} + +function stripOuterParens(value: string): string { + let current = value + while (current.startsWith('(')) { + const close = matchingDelimiter(current, 0, '(', ')') + if (close !== current.length - 1) break + current = current.slice(1, -1).trim() + } + return current +} + +function readExpression( + source: string, + start: number, + terminators: Set, +): {text: string; end: number} | undefined { + const begin = skipWhitespace(source, start) + let depth = 0 + for (let index = begin; index < source.length; index++) { + const literal = parseStringLiteral(source, index) + if (literal) { + index = literal.end - 1 + continue + } + const character = source[index]! + if (character === '(' || character === '[' || character === '{') depth++ + else if (character === ')' || character === ']' || character === '}') { + if (depth === 0 && terminators.has(character)) return {text: source.slice(begin, index).trim(), end: index} + depth-- + } else if (depth === 0 && terminators.has(character)) return {text: source.slice(begin, index).trim(), end: index} + } + const text = source.slice(begin).trim() + return text ? {text, end: source.length} : undefined +} + +function splitTopLevel(source: string, delimiter: string): string[] { + const parts: string[] = [] + let start = 0 + let depth = 0 + for (let index = 0; index < source.length; index++) { + const literal = parseStringLiteral(source, index) + if (literal) { + index = literal.end - 1 + continue + } + const character = source[index]! + if (character === '(' || character === '[' || character === '{') depth++ + else if (character === ')' || character === ']' || character === '}') depth-- + else if (depth === 0 && character === delimiter) { + parts.push(source.slice(start, index)) + start = index + 1 + } + } + parts.push(source.slice(start)) + return parts +} + +function matchingDelimiter(source: string, start: number, open: string, close: string): number | undefined { + let depth = 0 + for (let index = start; index < source.length; index++) { + const literal = parseStringLiteral(source, index) + if (literal) { + index = literal.end - 1 + continue + } + if (source[index] === open) depth++ + else if (source[index] === close) { + depth-- + if (depth === 0) return index + } + } + return undefined +} + +function parseStringLiteral(source: string, start: number): ParsedStringLiteral | undefined { + const quote = source[start] + if (quote !== '"' && quote !== "'" && quote !== '`') return undefined + let staticValue = true + for (let index = start + 1; index < source.length; index++) { + const character = source[index]! + if (character === '\\') { + index++ + continue + } + if (quote === '`' && character === '$' && source[index + 1] === '{') staticValue = false + if (character === quote) { + return { + end: index + 1, + value: source.slice(start + 1, index), + static: staticValue, + } + } + } + return undefined +} + +function skipWhitespace(source: string, start: number): number { + let index = start + while (/\s/.test(source[index] ?? '')) index++ + return index +} + +/** Preserve offsets and string contents while blanking comments. */ +function maskComments(source: string): string { + const characters = [...source] + let quote: string | undefined + for (let index = 0; index < characters.length; index++) { + const character = characters[index]! + if (quote) { + if (character === '\\') index++ + else if (character === quote) quote = undefined + continue + } + if (character === '"' || character === "'" || character === '`') { + quote = character + continue + } + if (character === '/' && characters[index + 1] === '/') { + while (index < characters.length && characters[index] !== '\n') { + characters[index] = ' ' + index++ + } + } else if (character === '/' && characters[index + 1] === '*') { + characters[index] = ' ' + characters[index + 1] = ' ' + index += 2 + while (index < characters.length && !(characters[index] === '*' && characters[index + 1] === '/')) { + if (characters[index] !== '\n') characters[index] = ' ' + index++ + } + if (index < characters.length) { + characters[index] = ' ' + characters[index + 1] = ' ' + index++ + } + } + } + return characters.join('') } diff --git a/packages/app/src/cli/services/app-doctor-engine/scanners/index.ts b/packages/app/src/cli/services/app-doctor-engine/scanners/index.ts index 6057755c5d3..f4816ee85ca 100644 --- a/packages/app/src/cli/services/app-doctor-engine/scanners/index.ts +++ b/packages/app/src/cli/services/app-doctor-engine/scanners/index.ts @@ -50,7 +50,15 @@ import type { SkippedFile, } from '../types.js' -type CheckTarget = 'config' | 'source' | 'theme' | 'manifest' | 'secrets' | 'config_and_source' | 'source_and_theme' +type CheckTarget = + | 'config' + | 'source' + | 'app_source' + | 'theme' + | 'manifest' + | 'secrets' + | 'config_and_source' + | 'source_and_theme' interface RunnerImplementationResult { id: string analysisMode: AnalysisMode @@ -201,7 +209,7 @@ const DETERMINISTIC_CHECK_DEFINITIONS: ReadonlyArray scanStaticFrameAncestors(context.sourceFiles)), + ...jsCheck('STATIC_FRAME_ANCESTORS', (context) => scanStaticFrameAncestors(context.sourceFiles), 'app_source'), requires: 'embedded_app', }, ] @@ -350,11 +358,15 @@ function themeFiles(context: ScanContext): SourceFile[] { return context.extensions.filter((extension) => extension.type === 'theme').flatMap((extension) => extension.files) } -function reactRouterFiles(context: ScanContext): SourceFile[] { +function appSourceFiles(context: ScanContext): SourceFile[] { const themePaths = new Set(themeFiles(context).map((file) => file.path)) return context.sourceFiles.filter((file) => !themePaths.has(file.path)) } +function reactRouterFiles(context: ScanContext): SourceFile[] { + return appSourceFiles(context) +} + async function gitProject(appRoot: string): Promise { const run = async (args: string[]): Promise<{exitCode: number; stdout: string} | undefined> => { try { @@ -378,7 +390,7 @@ function selectedFiles(definition: DeterministicCheckDefinition, context: ScanCo if (definition.target === 'manifest') return context.manifests.map((manifest) => manifest.path) if (definition.target === 'secrets') return context.sensitiveFiles.filter((file) => file.content !== undefined).map((file) => file.path) - let files = reactRouterFiles(context) + let files = definition.target === 'app_source' ? appSourceFiles(context) : reactRouterFiles(context) if (definition.target === 'theme') files = themeFiles(context) else if (definition.target === 'source_and_theme') files = [...files, ...themeFiles(context)] const source = files @@ -495,7 +507,10 @@ function executionDisposition( applicable: true, reason: {code: 'parser_unavailable', message: 'No readable Shopify app configuration was available.'}, } - if (['source', 'theme', 'manifest', 'secrets', 'source_and_theme'].includes(definition.target) && files.length === 0) + if ( + ['source', 'app_source', 'theme', 'manifest', 'secrets', 'source_and_theme'].includes(definition.target) && + files.length === 0 + ) return { status: 'not_applicable', required: false, @@ -531,7 +546,7 @@ function skippedInputsForCheck( return skippedFiles.filter((file) => { if (definition.target === 'config') return isConfig(file.path) if (definition.target === 'config_and_source') return isConfig(file.path) || isSourcePath(file.path) - if (definition.target === 'source') return isSourcePath(file.path) + if (definition.target === 'source' || definition.target === 'app_source') return isSourcePath(file.path) if (definition.target === 'theme') return isThemePath(file.path) if (definition.target === 'source_and_theme') return isSourcePath(file.path) || isThemePath(file.path) if (definition.target === 'manifest') return isManifest(file.path) @@ -555,6 +570,7 @@ function skippedInputReason(definition: DeterministicCheckDefinition, files: Ski } function runnerContext(definition: DeterministicCheckDefinition, context: ScanContext): ScanContext { + if (definition.target === 'app_source') return {...context, sourceFiles: appSourceFiles(context)} return definition.target === 'source' || definition.target === 'config_and_source' ? {...context, sourceFiles: reactRouterFiles(context)} : context @@ -589,7 +605,7 @@ export async function scan( raw: Object.assign({}, ...appTomls.map((configuration) => configuration.raw)), } : null - const capabilities = detectCapabilities(mergedConfig, extensions, sourceFiles) + const capabilities = detectCapabilities(mergedConfig, extensions, sourceFiles, appTomls) const detection = detectProject(manifests, extensions, sourceCandidates) const context: ScanContext = { appRoot, diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts b/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts index bb5a52f6bdf..d410090e374 100644 --- a/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts +++ b/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts @@ -165,12 +165,30 @@ describe('JavaScript regex mode', () => { }) describe('STATIC_FRAME_ANCESTORS regex mode', () => { - test('flags literal wildcard frame-ancestors only', () => { + test('flags only literal clearly permissive frame-ancestors source tokens', () => { expect( scanStaticFrameAncestors([ source(`const headers = {'Content-Security-Policy': "frame-ancestors *"}`, 'app/root.tsx'), ]), ).toHaveLength(1) + expect( + scanStaticFrameAncestors([ + source(`const headers = {'Content-Security-Policy': "frame-ancestors https://*"}`, 'app/root.tsx'), + ]), + ).toHaveLength(1) + expect( + scanStaticFrameAncestors([ + source(`const headers = {'Content-Security-Policy': "frame-ancestors *.myshopify.com"}`, 'app/root.tsx'), + ]), + ).toHaveLength(1) + expect( + scanStaticFrameAncestors([ + source( + `const headers = {'Content-Security-Policy': "frame-ancestors https://*.myshopify.com"}`, + 'app/root.tsx', + ), + ]), + ).toHaveLength(1) expect( scanStaticFrameAncestors([ source( @@ -181,14 +199,46 @@ describe('STATIC_FRAME_ANCESTORS regex mode', () => { ).toEqual([]) expect( scanStaticFrameAncestors([ - source(`const policy = buildPolicy(shop); const headers = {'Content-Security-Policy': policy}`, 'app/root.tsx'), + source( + `const headers = {'Content-Security-Policy': "frame-ancestors https://*.myshopify.com.evil.test"}`, + 'app/root.tsx', + ), + ]), + ).toEqual([]) + expect( + scanStaticFrameAncestors([ + source( + `const headers = {'Content-Security-Policy': "frame-ancestors https://*.mycompany.dev"}`, + 'app/root.tsx', + ), + ]), + ).toEqual([]) + }) + + test('evaluates only static CSP header values and ignores commented examples', () => { + expect( + scanStaticFrameAncestors([ + source(`headers.set('Content-Security-Policy', policy); const example = 'frame-ancestors *'`, 'app/root.tsx'), ]), ).toEqual([]) expect(scanStaticFrameAncestors([source(`const note = 'frame-ancestors *'`, 'app/root.tsx')])).toEqual([]) + expect( + scanStaticFrameAncestors([ + source(`const safe = true;// const headers = {'Content-Security-Policy': 'frame-ancestors *'}`, 'app/root.tsx'), + ]), + ).toEqual([]) + }) + + test('handles multiline and long static CSP literal construction', () => { expect( scanStaticFrameAncestors([ source( - `const headers = {'Content-Security-Policy': "frame-ancestors https://*.myshopify.com"}`, + `const headers = { + 'Content-Security-Policy': [ + "default-src 'self';", + 'frame-ancestors *', + ].join(' '), +}`, 'app/root.tsx', ), ]), @@ -196,11 +246,11 @@ describe('STATIC_FRAME_ANCESTORS regex mode', () => { expect( scanStaticFrameAncestors([ source( - `const headers = {'Content-Security-Policy': "frame-ancestors https://*.mycompany.dev"}`, + `const headers = {'Content-Security-Policy': "default-src ${'https://cdn.example.com '.repeat(40)}; frame-ancestors *"}`, 'app/root.tsx', ), ]), - ).toEqual([]) + ).toHaveLength(1) }) }) diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/scan-contract.test.ts b/packages/app/src/cli/services/app-doctor-engine/tests/scan-contract.test.ts index d9a81d1d96b..c494c1f519e 100644 --- a/packages/app/src/cli/services/app-doctor-engine/tests/scan-contract.test.ts +++ b/packages/app/src/cli/services/app-doctor-engine/tests/scan-contract.test.ts @@ -180,6 +180,38 @@ describe('framework and surface detection', () => { ) }) + test('runs static frame-ancestors for embedded non-React-Router JavaScript apps', async () => { + const result = await scan( + await app({ + 'shopify.app.toml': `name = "Embedded generic app"\nembedded = true\n[access_scopes]\nscopes = ""\n`, + 'server.ts': `const headers = {'Content-Security-Policy': 'frame-ancestors *'}`, + }), + ) + + expect(result.detection.framework).toBe('unknown') + expect(result.scan.checks_executed.find((execution) => execution.id === 'STATIC_FRAME_ANCESTORS')).toMatchObject({ + status: 'executed', + findings: 1, + inspected_files: ['server.ts'], + }) + }) + + test('keeps embedded-app capability when any readable app config is embedded', async () => { + const result = await scan( + await app({ + 'shopify.app.toml': `name = "Embedded production"\nembedded = true\n[access_scopes]\nscopes = ""\n`, + 'shopify.app.staging.toml': `name = "Non-embedded staging"\nembedded = false\n[access_scopes]\nscopes = ""\n`, + 'server.ts': `const headers = {'Content-Security-Policy': 'frame-ancestors *'}`, + }), + ) + + expect(result.capabilities.embedded_app).toBe(true) + expect(result.scan.checks_executed.find((execution) => execution.id === 'STATIC_FRAME_ANCESTORS')).toMatchObject({ + status: 'executed', + findings: 1, + }) + }) + test('keeps React Router and theme implementations inside their supported file boundaries', async () => { const themeDirectory = await app({ 'shopify.app.toml': appConfig(), From e164900e834737338d12e06da747b96b6ad161b2 Mon Sep 17 00:00:00 2001 From: Mar Lopez Date: Wed, 9 Sep 2026 12:43:39 -0400 Subject: [PATCH 5/7] Add static frame ancestors setter coverage Assisted-By: devx/0434e4da-7c55-4ded-87bd-400bf4d8a927 --- .../tests/deterministic-rules.test.ts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts b/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts index d410090e374..ef61f1091b8 100644 --- a/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts +++ b/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts @@ -229,6 +229,23 @@ describe('STATIC_FRAME_ANCESTORS regex mode', () => { ).toEqual([]) }) + test('covers static header setters, concatenation, templates, and mixed source lists', () => { + expect( + scanStaticFrameAncestors([ + source(`headers.set('Content-Security-Policy', 'frame-ancestors *')`, 'app/root.tsx'), + source(`response.headers.append('Content-Security-Policy', 'frame-ancestors *')`, 'app/response.tsx'), + source(`res.setHeader('Content-Security-Policy', 'frame-ancestors *')`, 'app/server.tsx'), + ]), + ).toHaveLength(3) + expect( + scanStaticFrameAncestors([ + source(`const headers = {'Content-Security-Policy': 'frame-ancestors ' + '*'}`, 'app/root.tsx'), + source(`const headers = {'Content-Security-Policy': \`frame-ancestors *\`}`, 'app/template.tsx'), + source(`const headers = {'Content-Security-Policy': "frame-ancestors 'self' *"}`, 'app/mixed.tsx'), + ]), + ).toHaveLength(3) + }) + test('handles multiline and long static CSP literal construction', () => { expect( scanStaticFrameAncestors([ From f84268bec319ec7fe1e48ed1491221e008959fb1 Mon Sep 17 00:00:00 2001 From: Mar Lopez Date: Wed, 9 Sep 2026 17:36:01 -0400 Subject: [PATCH 6/7] Address static frame ancestors review comments Assisted-By: devx/0434e4da-7c55-4ded-87bd-400bf4d8a927 --- .../app-doctor-engine/rules/csp-rules.ts | 10 ++++++++-- .../tests/deterministic-rules.test.ts | 16 ++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/packages/app/src/cli/services/app-doctor-engine/rules/csp-rules.ts b/packages/app/src/cli/services/app-doctor-engine/rules/csp-rules.ts index b480f6c00c6..a6e72ad1f1f 100644 --- a/packages/app/src/cli/services/app-doctor-engine/rules/csp-rules.ts +++ b/packages/app/src/cli/services/app-doctor-engine/rules/csp-rules.ts @@ -6,6 +6,7 @@ const HEADER_NAME = 'content-security-policy' const HEADER_SETTER_PREFIX = /\b(?:headers|response\.headers|res)\.(?:set|append|setHeader)\s*\(\s*$/i const ALL_ORIGIN_WILDCARD = /^https?:\/\/\*(?::(?:\*|\d+))?(?:\/.*)?$/i const SHOPIFY_WILDCARD = /^(?:https?:\/\/)?\*\.myshopify\.com(?::(?:\*|\d+))?(?:\/.*)?$/i +const SCHEME_ONLY_SOURCE = /^(?:http|https):$/i interface StaticHeaderValue { index: number @@ -82,7 +83,12 @@ function hasClearlyPermissiveFrameAncestors(value: string): boolean { } function isClearlyPermissiveSource(source: string): boolean { - return source === '*' || ALL_ORIGIN_WILDCARD.test(source) || SHOPIFY_WILDCARD.test(source) + return ( + source === '*' || + SCHEME_ONLY_SOURCE.test(source) || + ALL_ORIGIN_WILDCARD.test(source) || + SHOPIFY_WILDCARD.test(source) + ) } function evaluateStaticStringExpression(expression: string): string | undefined { @@ -209,7 +215,7 @@ function parseStringLiteral(source: string, start: number): ParsedStringLiteral } } } - return undefined + return {end: source.length, value: '', static: false} } function skipWhitespace(source: string, start: number): number { diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts b/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts index ef61f1091b8..62191d00191 100644 --- a/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts +++ b/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts @@ -189,6 +189,16 @@ describe('STATIC_FRAME_ANCESTORS regex mode', () => { ), ]), ).toHaveLength(1) + expect( + scanStaticFrameAncestors([ + source(`const headers = {'Content-Security-Policy': "frame-ancestors https:"}`, 'app/root.tsx'), + ]), + ).toHaveLength(1) + expect( + scanStaticFrameAncestors([ + source(`const headers = {'Content-Security-Policy': "frame-ancestors 'self' https:"}`, 'app/root.tsx'), + ]), + ).toHaveLength(1) expect( scanStaticFrameAncestors([ source( @@ -269,6 +279,12 @@ describe('STATIC_FRAME_ANCESTORS regex mode', () => { ]), ).toHaveLength(1) }) + + test('consumes malformed string literals once', () => { + const malformed = `const broken = "${'\\"'.repeat(64_000)}` + + expect(scanStaticFrameAncestors([source(malformed, 'app/broken.tsx')])).toEqual([]) + }) }) describe('Liquid AST mode', () => { From 52bfc546cac394a90966e5156a1e3026b30b2e3e Mon Sep 17 00:00:00 2001 From: Mar Lopez Date: Wed, 9 Sep 2026 17:55:29 -0400 Subject: [PATCH 7/7] Fix App Doctor scanner gaps Assisted-By: devx/3ac90870-9844-4116-81bc-cd8883bd74f9 --- .../app-doctor-engine/rules/js-rules.ts | 8 +- .../app-doctor-engine/rules/proxy-rules.ts | 145 ++++++++++++++++-- .../tests/rule-analysis.test.ts | 132 ++++++++++++++++ 3 files changed, 266 insertions(+), 19 deletions(-) diff --git a/packages/app/src/cli/services/app-doctor-engine/rules/js-rules.ts b/packages/app/src/cli/services/app-doctor-engine/rules/js-rules.ts index 02ba9af77b1..2e78e35aa3d 100644 --- a/packages/app/src/cli/services/app-doctor-engine/rules/js-rules.ts +++ b/packages/app/src/cli/services/app-doctor-engine/rules/js-rules.ts @@ -215,10 +215,14 @@ function returnedShopExpressions(source: string): string[] { function isRequestShopHelperCall(expression: string, requestPattern: string, helpers: Set): boolean { return [...helpers].some((helper) => - new RegExp(`^(?:await\\s+)?${escapeRegExp(helper)}\\s*\\(\\s*(?:${requestPattern})\\s*\\)$`).test(expression), + new RegExp(`^${requestShopHelperCallPattern(helper, requestPattern)}$`).test(expression), ) } +function requestShopHelperCallPattern(helper: string, requestPattern: string): string { + return `(?:await\\s+)?${escapeRegExp(helper)}\\s*\\(\\s*(?:${requestPattern})(?:\\s*,[\\s\\S]*?)?\\s*\\)` +} + function isRequestControlledShop(expression: string, state: RequestShopState): boolean { const direct = new RegExp(`(?:${state.requestPattern})\\.(?:body|query|params)(?:\\?\\.|\\.|\\[\\s*["'])${SHOP_FIELD}`).test( @@ -247,7 +251,7 @@ function isRequestControlledShop(expression: string, state: RequestShopState): b function isRequestShopHelperMember(expression: string, state: RequestShopState): boolean { return [...state.requestShopHelpers].some((helper) => { - const call = `(?:\\(\\s*)?(?:await\\s+)?${escapeRegExp(helper)}\\s*\\(\\s*(?:${state.requestPattern})\\s*\\)(?:\\s*\\))?` + const call = `(?:\\(\\s*)?${requestShopHelperCallPattern(helper, state.requestPattern)}(?:\\s*\\))?` return new RegExp(`${call}\\s*(?:\\?\\.|\\.|\\[\\s*["'])${SHOP_FIELD}`).test(expression) }) } diff --git a/packages/app/src/cli/services/app-doctor-engine/rules/proxy-rules.ts b/packages/app/src/cli/services/app-doctor-engine/rules/proxy-rules.ts index d5ef92df2ef..08dc7b4eae0 100644 --- a/packages/app/src/cli/services/app-doctor-engine/rules/proxy-rules.ts +++ b/packages/app/src/cli/services/app-doctor-engine/rules/proxy-rules.ts @@ -59,16 +59,7 @@ export function scanAppProxyLiquidInjection(files: SourceFile[]): Issue[] { function collectRequestBindings(source: string): Set { const requestBindings = new Set() - const executableSource = maskLiteralTextPreservingTemplateExpressions(source) - const declarationPattern = new RegExp(`\\b(?:const|let|var)\\s+(${IDENTIFIER})\\s*=\\s*`, 'g') - const assignments: {name: string; expression: string}[] = [] - let declaration = declarationPattern.exec(executableSource) - while (declaration) { - const expression = statementExpression(executableSource, declarationPattern.lastIndex) - if (expression && !/^(?:async\s*)?\([^)]*\)\s*=>/.test(expression.text) && !/^function\b/.test(expression.text)) - assignments.push({name: declaration[1]!, expression: expression.text}) - declaration = declarationPattern.exec(executableSource) - } + const assignments = collectRequestAssignments(maskLiteralTextPreservingTemplateExpressions(source)) for (let pass = 0; pass < 5; pass++) { let changed = false @@ -87,20 +78,57 @@ function collectRequestBindings(source: string): Set { return requestBindings } +function collectRequestAssignments(source: string): {name: string; expression: string}[] { + const assignments: {name: string; expression: string}[] = [] + const declarationPattern = new RegExp(`\\b(?:const|let|var)\\s+(${IDENTIFIER})\\s*=\\s*`, 'g') + let declaration = declarationPattern.exec(source) + while (declaration) { + const expression = statementExpression(source, declarationPattern.lastIndex) + if (expression && !/^(?:async\s*)?\([^)]*\)\s*=>/.test(expression.text) && !/^function\b/.test(expression.text)) + assignments.push({name: declaration[1]!, expression: expression.text}) + declaration = declarationPattern.exec(source) + } + + const assignmentPattern = new RegExp(`(?:^|[;{}\\n])\\s*(${IDENTIFIER})\\s*=\\s*`, 'g') + let assignment = assignmentPattern.exec(source) + while (assignment) { + const expression = statementExpression(source, assignmentPattern.lastIndex) + if (expression && !/^(?:async\s*)?\([^)]*\)\s*=>/.test(expression.text) && !/^function\b/.test(expression.text)) + assignments.push({name: assignment[1]!, expression: expression.text}) + assignment = assignmentPattern.exec(source) + } + return assignments +} + function collectTrustedHtmlEscapers(source: string): Set { const escapers = new Set() const importPattern = /\bimport\s+([A-Za-z_$][\w$]*)\s+from\s+["']escape-html["']/g let imported = importPattern.exec(source) while (imported) { const name = imported[1]! - if (!hasLocalDefinition(source, name)) escapers.add(name) + if (!hasShadowingBinding(source, name)) escapers.add(name) imported = importPattern.exec(source) } return escapers } -function hasLocalDefinition(source: string, name: string): boolean { - return new RegExp(`\\b(?:function|const|let|var)\\s+${escapeRegExp(name)}\\b`).test(source) +function hasShadowingBinding(source: string, name: string): boolean { + if (new RegExp(`\\b(?:function|const|let|var|class)\\s+${escapeRegExp(name)}\\b`).test(source)) return true + const parameterPatterns = [ + /\bfunction(?:\s+[A-Za-z_$][\w$]*)?\s*\(([^)]*)\)/g, + /\(([^)]*)\)\s*(?::[^=]+)?=>/g, + /(?:^|[,{;]\s*)(?:async\s+)?(?:get\s+|set\s+)?[A-Za-z_$][\w$]*\s*\(([^)]*)\)\s*(?::[^={]+)?\s*\{/gm, + /\bcatch\s*\(([^)]*)\)/g, + ] + const bindingPattern = new RegExp(`(?:^|[,\\s{])${escapeRegExp(name)}(?=\\s*(?:[,}:=]|$))`) + for (const pattern of parameterPatterns) { + let parameters = pattern.exec(source) + while (parameters) { + if (bindingPattern.test(parameters[1] ?? '')) return true + parameters = pattern.exec(source) + } + } + return new RegExp(`\\b${escapeRegExp(name)}\\s*(?::[^=,)]*)?\\s*=>`).test(source) } function responseBodyCandidates(source: string): ResponseBodyCandidate[] { @@ -117,13 +145,93 @@ function responseBodyCandidates(source: string): ResponseBodyCandidate[] { responsePattern.lastIndex = call?.end ?? responsePattern.lastIndex response = responsePattern.exec(executableSource) } + + const responseMethodPattern = new RegExp(`\\b(${IDENTIFIER})\\.(?:send|end|write)\\s*\\(`, 'g') + let responseMethod = responseMethodPattern.exec(executableSource) + while (responseMethod) { + const openParen = responseMethod.index + responseMethod[0].lastIndexOf('(') + const call = callArguments(source, openParen + 1) + const responseType = activeResponseTypeForReceiver( + source, + executableSource, + responseMethod[1]!, + responseMethod.index, + ) + if (call && responseType && call.args[0] !== undefined) + candidates.push({index: responseMethod.index, expression: call.args[0], responseType}) + responseMethodPattern.lastIndex = call?.end ?? responseMethodPattern.lastIndex + responseMethod = responseMethodPattern.exec(executableSource) + } return candidates } +function activeResponseTypeForReceiver( + source: string, + executableSource: string, + receiver: string, + sinkIndex: number, +): ActiveResponseType | undefined { + const sinkPath = enclosingBlockPath(executableSource, sinkIndex) + const setterPattern = new RegExp(`\\b${escapeRegExp(receiver)}\\.(setHeader|set|header|type|contentType)\\s*\\(`, 'g') + let activeType: ActiveResponseType | undefined + let setter = setterPattern.exec(executableSource) + while (setter && setter.index < sinkIndex) { + const openParen = setter.index + setter[0].lastIndexOf('(') + const call = callArguments(source, openParen + 1) + if (call && isBlockPathPrefix(enclosingBlockPath(executableSource, setter.index), sinkPath)) { + const type = responseTypeForSetter(setter[1]!, call.args) + if (type) activeType = type + } + setterPattern.lastIndex = call?.end ?? setterPattern.lastIndex + setter = setterPattern.exec(executableSource) + } + return activeType +} + +function responseTypeForSetter(method: string, args: string[]): ActiveResponseType | undefined { + const firstExpression = args[0]?.trim() ?? '' + if ((method === 'set' || method === 'header') && args.length === 1) { + const objectType = responseTypeFor(firstExpression) + if (objectType) return objectType + } + const firstArg = /^["']([^"']+)["']$/.exec(firstExpression)?.[1] + if (!firstArg) return undefined + if (method === 'type' || method === 'contentType') { + if (/^html$/i.test(firstArg)) return 'html' + return activeResponseTypeForContentType(firstArg) + } + if (!/^content-type$/i.test(firstArg)) return undefined + const secondArg = /^["']([^"']+)["']$/.exec(args[1]?.trim() ?? '')?.[1] + return secondArg ? activeResponseTypeForContentType(secondArg) : undefined +} + +function enclosingBlockPath(source: string, end: number): number[] { + const path: number[] = [] + for (let index = 0; index < end; index++) { + const skipped = skipLexicalToken(source, index) + if (skipped !== undefined) { + index = skipped - 1 + continue + } + if (source[index] === '{') path.push(index) + else if (source[index] === '}') path.pop() + } + return path +} + +function isBlockPathPrefix(prefix: number[], path: number[]): boolean { + return prefix.length <= path.length && prefix.every((value, index) => value === path[index]) +} + function responseTypeFor(initExpression: string | undefined): ActiveResponseType | undefined { if (!initExpression) return undefined - const contentType = /(?:^|[,{]\s*)["']?Content-Type["']?\s*:\s*["']([^"']+)["']/i.exec(initExpression)?.[1] - if (!contentType) return undefined + const contentType = + /(?:^|[,{]\s*)["']?Content-Type["']?\s*:\s*["']([^"']+)["']/i.exec(initExpression)?.[1] ?? + /\[\s*["']Content-Type["']\s*,\s*["']([^"']+)["']\s*\]/i.exec(initExpression)?.[1] + return contentType ? activeResponseTypeForContentType(contentType) : undefined +} + +function activeResponseTypeForContentType(contentType: string): ActiveResponseType | undefined { if (LIQUID_RESPONSE_TYPE.test(contentType)) return 'liquid' if (HTML_RESPONSE_TYPE.test(contentType)) return 'html' return undefined @@ -148,6 +256,7 @@ function maskTrustedHtmlTextEscapes(source: string, trustedHtmlEscapers: Set { expect(findings.map((finding) => finding.location.line)).toEqual([8, 9, 10]) }) + test('follows request parsing helpers with additional arguments', () => { + const findings = scanRequestControlledAdminContext([ + source(`function readParams(request: Request, context: AppLoadContext): {shop: string} { + return {shop: new URL(request.url).searchParams.get("shop") ?? ""}; +} + +export const loader = async ({request, context}) => { + const params = readParams(request, context); + await unauthenticated.admin(params.shop); + await unauthenticated.admin(readParams(request, context).shop); +}`), + ]) + + expect(findings).toHaveLength(2) + expect(findings.map((finding) => finding.location.line)).toEqual([7, 8]) + }) + test('does not taint trusted session properties or helpers that return a rebound session shop', () => { const findings = scanRequestControlledAdminContext([ source(`export const loader = async ({request}) => { @@ -216,6 +233,71 @@ describe('APP_PROXY_LIQUID_INJECTION body flow', () => { ).toHaveLength(1) }) + test('follows post-declaration aliases and tuple-form Response headers', () => { + expect( + scanAppProxyLiquidInjection([ + source( + `export const loader = ({request}) => { + let shop; + shop = request.query.shop; + return new Response( + \`
\${shop}
\`, + {headers: [['Content-Type', 'text/html']]}, + ); +}`, + 'app/routes/proxy.ts', + ), + ]), + ).toHaveLength(1) + }) + + test('follows Express response setters and body sinks', () => { + expect( + scanAppProxyLiquidInjection([ + source( + `export const loader = ({request}, res) => { + res.setHeader('Content-Type', 'text/html'); + res.send(\`
\${request.query.shop}
\`); +}`, + 'app/routes/proxy.ts', + ), + ]), + ).toHaveLength(1) + expect( + scanAppProxyLiquidInjection([ + source( + `export const loader = ({request}, res) => { + res.type('html'); + res.end(\`
\${request.query.shop}
\`); +}`, + 'app/routes/proxy.ts', + ), + ]), + ).toHaveLength(1) + expect( + scanAppProxyLiquidInjection([ + source( + `export const loader = ({request}, res) => { + res.set({'Content-Type': 'text/html'}); + res.write(\`
\${request.query.shop}
\`); +}`, + 'app/routes/proxy.ts', + ), + ]), + ).toHaveLength(1) + expect( + scanAppProxyLiquidInjection([ + source( + `export const loader = ({request}, res) => { + res.setHeader('Content-Type', 'application/json'); + res.send(\`
\${request.query.shop}
\`); +}`, + 'app/routes/proxy.ts', + ), + ]), + ).toEqual([]) + }) + test('suppresses only imported HTML escapers, never local identities or Liquid bodies', () => { expect( scanAppProxyLiquidInjection([ @@ -254,6 +336,56 @@ export const loader = ({request}) => { ]), ).toHaveLength(1) }) + + test('does not suppress imported HTML escaping when earlier interpolation can change HTML context', () => { + expect( + scanAppProxyLiquidInjection([ + source( + `import escapeHtml from 'escape-html'; +export const loader = ({request}) => { + const opening = ''; + return new Response(\`\${opening}\${escapeHtml(request.query.action)}\${closing}\`, {headers: {'Content-Type': 'text/html'}}); +}`, + 'app/routes/proxy.ts', + ), + ]), + ).toHaveLength(1) + }) + + test('does not trust a shadowed escape-html binding', () => { + expect( + scanAppProxyLiquidInjection([ + source( + `import escapeHtml from 'escape-html'; +export const loader = ({request}) => { + const render = (escapeHtml) => new Response(\`
\${escapeHtml(request.query.shop)}
\`, {headers: {'Content-Type': 'text/html'}}); + return render((value) => value); +}`, + 'app/routes/proxy.ts', + ), + ]), + ).toHaveLength(1) + }) + + test('does not trust escape-html in method parameter scope', () => { + expect( + scanAppProxyLiquidInjection([ + source( + `import escapeHtml from 'escape-html'; +export const loader = ({request}) => { + const renderer = { + render(escapeHtml) { + return new Response(\`
\${escapeHtml(request.query.shop)}
\`, {headers: {'Content-Type': 'text/html'}}); + }, + }; + return renderer.render((value) => value); +}`, + 'app/routes/proxy.ts', + ), + ]), + ).toHaveLength(1) + }) test('does not suppress imported HTML escaping in executable attribute or URL contexts', () => { expect( scanAppProxyLiquidInjection([