Add static frame-ancestors check to App Doctor

.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.

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<string, unknown>)?.app_proxy),

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',

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<string>,
+): {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('')
+}

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<string>): 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)
   })
 }