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..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,9 +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 = appTomls.some((configuration) => configuration.raw.embedded === true) const scriptTags = sourceFiles.some((file) => file.content ? /script[_-]?tags?|ScriptTag/i.test(file.content) : false, @@ -23,6 +25,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..a6e72ad1f1f --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-engine/rules/csp-rules.ts @@ -0,0 +1,263 @@ +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 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 + 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 = 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 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 === '*' || + SCHEME_ONLY_SOURCE.test(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 {end: source.length, value: '', static: false} +} + +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/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 scanStaticFrameAncestors(context.sourceFiles), 'app_source'), + requires: 'embedded_app', + }, ] function unsafeInnerHtmlRunner(context: ScanContext): RunnerResult { @@ -345,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 { @@ -373,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 @@ -490,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, @@ -526,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) @@ -550,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 @@ -584,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 3004c8a3726..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 @@ -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,129 @@ describe('JavaScript regex mode', () => { }) }) +describe('STATIC_FRAME_ANCESTORS regex mode', () => { + 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(`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( + `const headers = {'Content-Security-Policy': "frame-ancestors https://admin.shopify.com https://merchant.myshopify.com"}`, + 'app/root.tsx', + ), + ]), + ).toEqual([]) + expect( + scanStaticFrameAncestors([ + 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('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([ + source( + `const headers = { + 'Content-Security-Policy': [ + "default-src 'self';", + 'frame-ancestors *', + ].join(' '), +}`, + 'app/root.tsx', + ), + ]), + ).toHaveLength(1) + expect( + scanStaticFrameAncestors([ + source( + `const headers = {'Content-Security-Policy': "default-src ${'https://cdn.example.com '.repeat(40)}; frame-ancestors *"}`, + 'app/root.tsx', + ), + ]), + ).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', () => { 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..d7eea85cc3f 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, @@ -132,6 +133,23 @@ export const loader = async ({request}) => { 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}) => { @@ -215,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([ @@ -253,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([ 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..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 @@ -132,6 +132,86 @@ 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('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(), 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,