From f5f737ad3f16d45abea6e1a0fe53ed2675820896 Mon Sep 17 00:00:00 2001 From: Josh Larson Date: Wed, 7 Oct 2026 19:14:20 +0000 Subject: [PATCH 1/4] Calibrate app security agent checks against the React Router template A fresh React Router app came back with METAFIELD_OFFLINE_TOKEN, MISSING_AUTHORIZATION_CHECK and STATIC_FRAME_ANCESTORS unresolved for some agents, though the template code is safe. The instructions told the agent to record unresolved whenever it couldn't prove exploitability, and the three prompts didn't describe the React Router SDK defaults. - Record executed when the code establishes the boundary a check asks about, and unresolved only for an incomplete investigation or a named candidate. - METAFIELD_OFFLINE_TOKEN v2 checks token provenance, so an offline session from authenticate.admin is no longer a finding by itself, and its catalog text no longer contradicts its fix. - MISSING_AUTHORIZATION_CHECK v3 treats authenticate.admin as Shopify's standard model, while still reporting skipped app-defined roles, other users' records and online-to-offline fallbacks. - STATIC_FRAME_ANCESTORS v2 accepts addDocumentResponseHeaders and still inspects headers set after or instead of it, including values built from variables. The deterministic version moves with it. - Add a deterministic fixture test of the template at a pinned commit, and a dev smoke script that runs codex and claude against the template and a planted-bug negative control. Co-authored-by: Qlaw --- .../app-security-unresolved-calibration.md | 6 + bin/app-security-smoke/README.md | 47 ++ bin/app-security-smoke/run.js | 404 ++++++++++++++++++ .../app-security-engine/INSTRUCTIONS.md | 5 +- .../checks/METAFIELD_OFFLINE_TOKEN.md | 11 +- .../checks/MISSING_AUTHORIZATION_CHECK.md | 12 +- .../checks/STATIC_FRAME_ANCESTORS.md | 6 +- .../app-security-engine/checks/embedded.ts | 8 +- .../app-security-engine/checks/index.ts | 3 +- .../app-security-engine/rules/catalog.ts | 6 +- .../app-security-engine/scanners/index.ts | 2 +- .../app-security-engine/tests/checks.test.ts | 30 ++ .../tests/deterministic-rules.test.ts | 1 + .../tests/fixtures/react-router-template.ts | 76 ++++ .../tests/react-router-template.test.ts | 71 +++ 15 files changed, 673 insertions(+), 15 deletions(-) create mode 100644 .changeset/app-security-unresolved-calibration.md create mode 100644 bin/app-security-smoke/README.md create mode 100755 bin/app-security-smoke/run.js create mode 100644 packages/app/src/cli/services/app-security-engine/tests/fixtures/react-router-template.ts create mode 100644 packages/app/src/cli/services/app-security-engine/tests/react-router-template.test.ts diff --git a/.changeset/app-security-unresolved-calibration.md b/.changeset/app-security-unresolved-calibration.md new file mode 100644 index 00000000000..17a89f5f28d --- /dev/null +++ b/.changeset/app-security-unresolved-calibration.md @@ -0,0 +1,6 @@ +--- +'@shopify/app': patch +'@shopify/cli': patch +--- + +Stop app security agent checks from leaving the React Router app template's metafield, authorization and frame-ancestors checks unresolved diff --git a/bin/app-security-smoke/README.md b/bin/app-security-smoke/README.md new file mode 100644 index 00000000000..85dc423ee96 --- /dev/null +++ b/bin/app-security-smoke/README.md @@ -0,0 +1,47 @@ +# App security check smoke run + +`run.js` runs real coding agents through `shopify app security` against a fresh React Router app template, then reports how each agent recorded the checks this calibration covers: `METAFIELD_OFFLINE_TOKEN`, `MISSING_AUTHORIZATION_CHECK` and `STATIC_FRAME_ANCESTORS`. Use it before and after changing agent check prompts or `INSTRUCTIONS.md`. It isn't run in CI: agent results vary from run to run, and each pass costs model time. + +## What it runs + +The script prepares two apps from [`Shopify/shopify-app-template-react-router`](https://github.com/Shopify/shopify-app-template-react-router) `main-cli`, pinned to the commit in `TEMPLATE_REF`: + +- **template**: the app as `shopify app init` leaves it, with placeholder values for the fields that linking writes to `shopify.app.toml`. Every focus check should be recorded as `executed` with no findings. Apart from the focus checks, the template is expected to have findings only for `MISSING_COMPLIANCE_WEBHOOKS` and `MISSING_DEPENDENCY_SECURITY_AUTOMATION`. Any other unresolved check or finding is listed as `other`, but doesn't fail the run. +- **negative**: the template plus one planted bug per focus check. Each focus check should report a finding in its planted file: + - `app/routes/api.demo-info.tsx` writes a request-controlled metafield through `unauthenticated.admin(shop)`. + - `app/routes/app.cleanup.tsx` enforces an app-admin allowlist in its loader but not in its action, which deletes products. + - `app/entry.server.tsx` replaces the SDK header with a `*.myshopify.com` frame-ancestors policy built from a variable. + +Each pass gets its own copy of the app and runs the agent with a prompt to follow `shopify app security instructions`. A `shopify` shim on `PATH` points to this checkout's CLI. + +## Usage + +Run it in a disposable sandbox such as Aquifer or Spin, not on your laptop. Agents run with approvals and sandboxing turned off. You need `git`, `pnpm`, and the agent CLIs (`codex` and/or `claude`) already authenticated. + +```sh +pnpm nx build cli +node bin/app-security-smoke/run.js --runs 3 --agents codex,claude +``` + +Options: + +- `--runs`: passes per agent and scenario. Default 3. +- `--agents`: `codex`, `claude`, or both. Default both. +- `--scenarios`: `template`, `negative`, or both. Default both. +- `--concurrency`: number of passes run at the same time. Default 4. +- `--timeout-minutes`: per-pass timeout. Default 20. +- `--work-dir`: where to put apps, logs and `summary.json`. Defaults to a new temporary directory. + +Agent commands default to `codex exec --dangerously-bypass-approvals-and-sandbox "$SMOKE_PROMPT"` and `claude -p --dangerously-skip-permissions "$SMOKE_PROMPT"`. Override them with `APP_SECURITY_SMOKE_CODEX_COMMAND` or `APP_SECURITY_SMOKE_CLAUDE_COMMAND`, for example to choose a model or a provider. The command runs through `sh -c` in the app directory, with the prompt in `$SMOKE_PROMPT`. + +The script prints `PASS` or `FAIL` for each pass, then counts per scenario and check, for example `template: clean 5/6` and `negative: all planted bugs caught 6/6`. It exits 1 when any pass fails. + +## Updating the template + +The deterministic fixture test (`packages/app/src/cli/services/app-security-engine/tests/react-router-template.test.ts`) scans the same template without an agent. To move both to a new template commit, update `TEMPLATE_REF` in `run.js`, then run: + +```sh +node bin/app-security-smoke/run.js write-fixture +``` + +This regenerates `tests/fixtures/react-router-template.ts`. Run the fixture test and this smoke run before committing. diff --git a/bin/app-security-smoke/run.js b/bin/app-security-smoke/run.js new file mode 100755 index 00000000000..9cae1b9e34c --- /dev/null +++ b/bin/app-security-smoke/run.js @@ -0,0 +1,404 @@ +#! /usr/bin/env node + +/** + * Smoke run for `shopify app security` agent checks against the React Router app template. + * See README.md in this directory. Not run in CI: it drives real coding agents. + */ + +import {spawn, execFileSync} from 'node:child_process' +import {cpSync, existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync, chmodSync} from 'node:fs' +import {tmpdir} from 'node:os' +import {dirname, join, resolve as resolvePath} from 'node:path' +import {fileURLToPath} from 'node:url' +import {parseArgs} from 'node:util' + +const REPO_ROOT = resolvePath(dirname(fileURLToPath(import.meta.url)), '../..') +const CLI_ENTRY = join(REPO_ROOT, 'packages/cli/bin/dev.js') +const FIXTURE_PATH = join( + REPO_ROOT, + 'packages/app/src/cli/services/app-security-engine/tests/fixtures/react-router-template.ts', +) + +const TEMPLATE_REPOSITORY = 'https://github.com/Shopify/shopify-app-template-react-router.git' +const TEMPLATE_BRANCH = 'main-cli' +const TEMPLATE_REF = 'ea3008dad983c5509371b5130a1bcbefe8346b43' + +const RESULTS_FILE = '.shopify/app-security/shopify.app/agent-findings.json' + +/** The checks this smoke run calibrates. */ +const FOCUS_CHECKS = ['METAFIELD_OFFLINE_TOKEN', 'MISSING_AUTHORIZATION_CHECK', 'STATIC_FRAME_ANCESTORS'] + +/** Findings the unmodified template is expected to have; see the fixture test for why. */ +const EXPECTED_TEMPLATE_FINDINGS = ['MISSING_COMPLIANCE_WEBHOOKS', 'MISSING_DEPENDENCY_SECURITY_AUTOMATION'] + +/** One planted bug per focus check, added to the template for the negative control. */ +const PLANTED_FILES = { + METAFIELD_OFFLINE_TOKEN: 'app/routes/api.demo-info.tsx', + MISSING_AUTHORIZATION_CHECK: 'app/routes/app.cleanup.tsx', + STATIC_FRAME_ANCESTORS: 'app/entry.server.tsx', +} + +const PROMPT = + 'Run a Shopify app security check on this app and record the results. The Shopify CLI is available as `shopify` on PATH. ' + + 'Start by running `shopify app security instructions` and follow those instructions completely, including recording ' + + 'results and running review. Do not modify app source files.' + +const DEFAULT_AGENT_COMMANDS = { + codex: 'codex exec --dangerously-bypass-approvals-and-sandbox "$SMOKE_PROMPT" < /dev/null', + claude: 'claude -p --dangerously-skip-permissions "$SMOKE_PROMPT" < /dev/null', +} + +const USAGE = `Usage: + node bin/app-security-smoke/run.js [run] [--runs 3] [--agents codex,claude] [--scenarios template,negative] + [--concurrency 4] [--timeout-minutes 20] [--work-dir DIR] + node bin/app-security-smoke/run.js write-fixture + +Agent commands default to: + codex: ${DEFAULT_AGENT_COMMANDS.codex} + claude: ${DEFAULT_AGENT_COMMANDS.claude} +Override them with APP_SECURITY_SMOKE_CODEX_COMMAND and APP_SECURITY_SMOKE_CLAUDE_COMMAND. The prompt is in $SMOKE_PROMPT.` + +function run(command, args, options = {}) { + return execFileSync(command, args, {encoding: 'utf-8', stdio: ['ignore', 'pipe', 'inherit'], ...options}) +} + +function commit(directory, message) { + const identity = ['-c', 'user.name=app-security-smoke', '-c', 'user.email=app-security-smoke@example.com'] + run('git', ['add', '-A'], {cwd: directory}) + run('git', [...identity, 'commit', '--quiet', '--no-verify', '-m', message], {cwd: directory}) +} + +/** + * Fills the fields `shopify app init` writes when it links the app, so `shopify app security check` can parse the + * configuration without a Partners account. + */ +function linkAppConfiguration(appDirectory) { + const path = join(appDirectory, 'shopify.app.toml') + const original = readFileSync(path, 'utf-8') + const linked = original.replace( + 'client_id = ""\n', + [ + 'client_id = "0123456789abcdef0123456789abcdef"', + 'name = "app-security-smoke"', + 'application_url = "https://example.com"', + 'embedded = true', + '', + ].join('\n'), + ) + if (linked === original) throw new Error(`Couldn't find an empty client_id in ${path}`) + writeFileSync(path, `${linked.trimEnd()}\n\n[auth]\nredirect_urls = [ "https://example.com/api/auth" ]\n`) +} + +/** Clones the template at the pinned commit and applies the parts of `shopify app init` that change scanned files. */ +function scaffoldTemplate(appDirectory) { + run('git', ['clone', '--quiet', '--branch', TEMPLATE_BRANCH, TEMPLATE_REPOSITORY, appDirectory]) + run('git', ['checkout', '--quiet', TEMPLATE_REF], {cwd: appDirectory}) + // `shopify app init` removes these (services/init/template/cleanup.ts). + for (const path of ['.git', '.github', 'LICENSE.md']) rmSync(join(appDirectory, path), {recursive: true, force: true}) + linkAppConfiguration(appDirectory) +} + +const PLANTED_METAFIELD_ROUTE = `import type { ActionFunctionArgs } from "react-router"; +import { unauthenticated } from "../shopify.server"; + +// Lets a storefront widget update the demo info metafield on a product. +export const action = async ({ request }: ActionFunctionArgs) => { + const url = new URL(request.url); + const shop = url.searchParams.get("shop")!; + const form = await request.formData(); + const { admin } = await unauthenticated.admin(shop); + await admin.graphql( + \`#graphql + mutation setDemoInfo($metafields: [MetafieldsSetInput!]!) { + metafieldsSet(metafields: $metafields) { userErrors { message } } + }\`, + { + variables: { + metafields: [ + { + ownerId: String(form.get("productId")), + namespace: "$app", + key: "demo_info", + value: String(form.get("value")), + }, + ], + }, + }, + ); + return Response.json({ ok: true }); +}; +` + +const PLANTED_AUTHORIZATION_ROUTE = `import type { ActionFunctionArgs, LoaderFunctionArgs } from "react-router"; +import { authenticate } from "../shopify.server"; + +// Only app administrators (by Shopify staff user ID) may use bulk cleanup. +const APP_ADMIN_USER_IDS = new Set((process.env.APP_ADMIN_USER_IDS ?? "").split(",")); + +export const loader = async ({ request }: LoaderFunctionArgs) => { + const { sessionToken } = await authenticate.admin(request); + if (!APP_ADMIN_USER_IDS.has(String(sessionToken?.sub))) { + throw new Response("Forbidden", { status: 403 }); + } + return null; +}; + +export const action = async ({ request }: ActionFunctionArgs) => { + const { admin } = await authenticate.admin(request); + const form = await request.formData(); + const ids = String(form.get("productIds")).split(","); + for (const id of ids) { + await admin.graphql( + \`#graphql + mutation deleteProduct($input: ProductDeleteInput!) { + productDelete(input: $input) { deletedProductId } + }\`, + { variables: { input: { id } } }, + ); + } + return { deleted: ids.length }; +}; + +export default function Cleanup() { + return null; +} +` + +/** Adds one bug per focus check. The CSP is built from a variable, so only the agent can see the wildcard. */ +function plantBugs(appDirectory) { + writeFileSync(join(appDirectory, PLANTED_FILES.METAFIELD_OFFLINE_TOKEN), PLANTED_METAFIELD_ROUTE) + writeFileSync(join(appDirectory, PLANTED_FILES.MISSING_AUTHORIZATION_CHECK), PLANTED_AUTHORIZATION_ROUTE) + const entryPath = join(appDirectory, PLANTED_FILES.STATIC_FRAME_ANCESTORS) + const entry = readFileSync(entryPath, 'utf-8') + const anchor = ' addDocumentResponseHeaders(request, responseHeaders);\n' + if (!entry.includes(anchor)) throw new Error(`Couldn't find addDocumentResponseHeaders in ${entryPath}`) + const override = + ' const frameAncestors = ["https://*.myshopify.com", "https://admin.shopify.com"];\n' + + ' responseHeaders.set("Content-Security-Policy", `frame-ancestors ${frameAncestors.join(" ")};`);\n' + writeFileSync(entryPath, entry.replace(anchor, anchor + override)) +} + +function prepareBases(workDirectory) { + const template = join(workDirectory, 'base-template') + console.log(`Scaffolding ${TEMPLATE_BRANCH}@${TEMPLATE_REF.slice(0, 7)} in ${template}`) + scaffoldTemplate(template) + run('pnpm', ['install', '--silent'], {cwd: template, stdio: ['ignore', 'ignore', 'inherit']}) + run('git', ['init', '--quiet'], {cwd: template}) + commit(template, 'Template') + + const negative = join(workDirectory, 'base-negative') + cpSync(template, negative, {recursive: true, verbatimSymlinks: true}) + plantBugs(negative) + commit(negative, 'Planted bugs') + return {template, negative} +} + +function writeShopifyShim(workDirectory) { + const binDirectory = join(workDirectory, 'bin') + mkdirSync(binDirectory, {recursive: true}) + const shim = join(binDirectory, 'shopify') + writeFileSync(shim, `#!/bin/sh\nexec node "${CLI_ENTRY}" "$@"\n`) + chmodSync(shim, 0o755) + return binDirectory +} + +function runAgent({name, agent, baseDirectory, workDirectory, binDirectory, timeoutMinutes}) { + const appDirectory = join(workDirectory, 'runs', name) + cpSync(baseDirectory, appDirectory, {recursive: true, verbatimSymlinks: true}) + const command = + process.env[`APP_SECURITY_SMOKE_${agent.toUpperCase()}_COMMAND`] ?? DEFAULT_AGENT_COMMANDS[agent] + const logPath = join(workDirectory, 'runs', `${name}.log`) + const started = Date.now() + + return new Promise((resolve) => { + const child = spawn('sh', ['-c', `${command} > "${logPath}" 2>&1`], { + cwd: appDirectory, + env: { + ...process.env, + PATH: `${binDirectory}:${process.env.PATH}`, + SHOPIFY_CLI_NO_ANALYTICS: '1', + SMOKE_PROMPT: PROMPT, + }, + stdio: 'ignore', + }) + const timer = setTimeout(() => child.kill('SIGTERM'), timeoutMinutes * 60 * 1000) + child.on('exit', (code, signal) => { + clearTimeout(timer) + const seconds = Math.round((Date.now() - started) / 1000) + resolve({name, agent, appDirectory, logPath, exit: signal ?? code, seconds}) + }) + }) +} + +function outcomeFor(check, scenario) { + if (!check) return 'missing' + if (scenario === 'negative') { + const planted = PLANTED_FILES[check.id] + if (check.findings.some((finding) => finding.location.file === planted)) return 'caught' + } + if (check.findings.length > 0) return 'finding' + return check.status === 'executed' ? 'clean' : check.status +} + +function evaluate(result, scenario) { + const resultsPath = join(result.appDirectory, RESULTS_FILE) + if (!existsSync(resultsPath)) return {...result, scenario, recorded: false, passed: false, focus: {}, other: []} + const document = JSON.parse(readFileSync(resultsPath, 'utf-8')) + const checks = new Map(document.checks.map((check) => [check.id, check])) + const focus = Object.fromEntries(FOCUS_CHECKS.map((id) => [id, outcomeFor(checks.get(id), scenario)])) + const other = document.checks + .filter((check) => !FOCUS_CHECKS.includes(check.id)) + .filter((check) => check.status === 'unresolved' || check.findings.length > 0) + .filter((check) => scenario !== 'template' || !EXPECTED_TEMPLATE_FINDINGS.includes(check.id) || check.status !== 'executed') + .map((check) => `${check.id}=${check.findings.length > 0 ? `${check.findings.length} finding(s)` : check.status}`) + const wanted = scenario === 'negative' ? 'caught' : 'clean' + const passed = Object.values(focus).every((outcome) => outcome === wanted) + return {...result, scenario, recorded: true, passed, focus, other} +} + +async function runWithConcurrency(tasks, concurrency) { + const results = [] + const queue = [...tasks] + const workers = Array.from({length: Math.min(concurrency, queue.length)}, async () => { + while (queue.length > 0) { + const task = queue.shift() + results.push(await task()) + } + }) + await Promise.all(workers) + return results +} + +function printSummary(results) { + console.log('') + for (const result of results.sort((left, right) => left.name.localeCompare(right.name))) { + const focus = result.recorded + ? FOCUS_CHECKS.map((id) => `${id}=${result.focus[id]}`).join(' ') + : 'no agent-findings.json' + const other = result.other.length > 0 ? ` | other: ${result.other.join(' ')}` : '' + console.log(`${result.passed ? 'PASS' : 'FAIL'} ${result.name} (${result.seconds}s) ${focus}${other}`) + } + console.log('') + for (const scenario of ['template', 'negative']) { + const scenarioResults = results.filter((result) => result.scenario === scenario) + if (scenarioResults.length === 0) continue + const label = scenario === 'template' ? 'clean' : 'all planted bugs caught' + const passed = scenarioResults.filter((result) => result.passed).length + console.log(`${scenario}: ${label} ${passed}/${scenarioResults.length}`) + for (const id of FOCUS_CHECKS) { + const wanted = scenario === 'negative' ? 'caught' : 'clean' + const count = scenarioResults.filter((result) => result.focus[id] === wanted).length + console.log(` ${id}: ${wanted} ${count}/${scenarioResults.length}`) + } + } +} + +async function smoke(values) { + if (!existsSync(join(REPO_ROOT, 'packages/app/dist'))) { + throw new Error('Build the CLI first: pnpm nx build cli') + } + const workDirectory = resolvePath(values['work-dir'] ?? mkdtempSync(join(tmpdir(), 'app-security-smoke-'))) + mkdirSync(join(workDirectory, 'runs'), {recursive: true}) + const bases = prepareBases(workDirectory) + const binDirectory = writeShopifyShim(workDirectory) + const agents = values.agents.split(',').filter(Boolean) + const scenarios = values.scenarios.split(',').filter(Boolean) + for (const agent of agents) { + if (!DEFAULT_AGENT_COMMANDS[agent]) throw new Error(`Unknown agent ${agent}. Use codex or claude.`) + } + + const tasks = [] + for (const scenario of scenarios) { + if (!bases[scenario]) throw new Error(`Unknown scenario ${scenario}. Use template or negative.`) + for (const agent of agents) { + for (let index = 1; index <= Number(values.runs); index++) { + const name = `${scenario}-${agent}-${index}` + tasks.push(async () => { + const result = await runAgent({ + name, + agent, + baseDirectory: bases[scenario], + workDirectory, + binDirectory, + timeoutMinutes: Number(values['timeout-minutes']), + }) + const evaluated = evaluate(result, scenario) + console.log(`${evaluated.passed ? 'PASS' : 'FAIL'} ${name} (${result.seconds}s)`) + return evaluated + }) + } + } + } + console.log(`Running ${tasks.length} agent passes, ${values.concurrency} at a time. Logs: ${join(workDirectory, 'runs')}`) + const results = await runWithConcurrency(tasks, Number(values.concurrency)) + printSummary(results) + writeFileSync(join(workDirectory, 'summary.json'), `${JSON.stringify(results, null, 2)}\n`) + console.log(`\nSummary: ${join(workDirectory, 'summary.json')}`) + return results.every((result) => result.passed) ? 0 : 1 +} + +/** Text files a fresh app has that the deterministic scan reads; docs, styles, and binaries don't affect it. */ +function isFixtureFile(path) { + return !/\.(md|css|ico|png|svg)$/.test(path) +} + +function writeFixture() { + const scratch = mkdtempSync(join(tmpdir(), 'app-security-fixture-')) + try { + const appDirectory = join(scratch, 'app') + scaffoldTemplate(appDirectory) + const files = run('find', ['.', '-type', 'f', '-not', '-path', './node_modules/*'], {cwd: appDirectory}) + .split('\n') + .filter(Boolean) + .map((path) => path.replace(/^\.\//, '')) + .filter(isFixtureFile) + .sort() + const entries = files + .map((path) => ` ${JSON.stringify(path)}: ${JSON.stringify(readFileSync(join(appDirectory, path), 'utf-8'))},`) + .join('\n') + const output = `// AUTO-GENERATED by bin/app-security-smoke/run.js write-fixture — do not edit. +/* eslint-disable no-template-curly-in-string */ + +/** + * Shopify/shopify-app-template-react-router \`${TEMPLATE_BRANCH}\` at the commit below, as \`shopify app init\` leaves it + * (no \`.git\`, \`.github\` or \`LICENSE.md\`), with placeholder values in \`shopify.app.toml\` for the fields that linking + * the app writes. Docs, styles and binaries are left out. + */ +export const REACT_ROUTER_TEMPLATE_REF = '${TEMPLATE_REF}' + +export const REACT_ROUTER_TEMPLATE_FILES: {[path: string]: string} = { +${entries} +} +` + writeFileSync(FIXTURE_PATH, output) + run(join(REPO_ROOT, 'node_modules/.bin/prettier'), ['--write', FIXTURE_PATH], {cwd: REPO_ROOT}) + console.log(`Wrote ${files.length} files to ${FIXTURE_PATH}`) + } finally { + rmSync(scratch, {recursive: true, force: true}) + } +} + +const {positionals, values} = parseArgs({ + allowPositionals: true, + options: { + runs: {type: 'string', default: '3'}, + agents: {type: 'string', default: 'codex,claude'}, + scenarios: {type: 'string', default: 'template,negative'}, + concurrency: {type: 'string', default: '4'}, + 'timeout-minutes': {type: 'string', default: '20'}, + 'work-dir': {type: 'string'}, + help: {type: 'boolean', default: false}, + }, +}) + +const command = positionals[0] ?? 'run' +if (values.help) { + console.log(USAGE) +} else if (command === 'write-fixture') { + writeFixture() +} else if (command === 'run') { + process.exitCode = await smoke(values) +} else { + console.error(USAGE) + process.exitCode = 1 +} diff --git a/packages/app/src/cli/services/app-security-engine/INSTRUCTIONS.md b/packages/app/src/cli/services/app-security-engine/INSTRUCTIONS.md index 1b26956561c..aacaacf4660 100644 --- a/packages/app/src/cli/services/app-security-engine/INSTRUCTIONS.md +++ b/packages/app/src/cli/services/app-security-engine/INSTRUCTIONS.md @@ -44,7 +44,10 @@ For each check: 5. Keep the check `id` and `version` exactly as they appear in {{AGENT_CHECKS_PATH}}. 6. Include concise evidence citations. Never include a detected secret value or unnecessary personal data. -A check with no verified issue must not produce a fabricated finding. If you cannot establish exploitability or affected authority, record the check as `unresolved` with a reason instead. +A check with no verified issue must not produce a fabricated finding. Choose the status from what you were able to investigate, not from whether you can prove a negative: + +- `executed`: you traced the paths the check directs you to and found no concrete issue. This includes code that follows the safe pattern the check or its `docs_url` describes. A candidate is ruled out when the code establishes the boundary the check asks about, even if you can't rule out hypothetical policies or requirements that the repository doesn't define. +- `unresolved`: you couldn't complete the investigation, for example because required code was unreadable, out of scope, or outside the repository and you couldn't inspect it, or you found a specific candidate whose boundary you could neither establish nor show to be missing. Name the candidate's file and line, or the missing input, in the reason. ### 4. Write one findings document diff --git a/packages/app/src/cli/services/app-security-engine/checks/METAFIELD_OFFLINE_TOKEN.md b/packages/app/src/cli/services/app-security-engine/checks/METAFIELD_OFFLINE_TOKEN.md index 30d21809bcc..9f76c811f26 100644 --- a/packages/app/src/cli/services/app-security-engine/checks/METAFIELD_OFFLINE_TOKEN.md +++ b/packages/app/src/cli/services/app-security-engine/checks/METAFIELD_OFFLINE_TOKEN.md @@ -1,9 +1,16 @@ --- id: METAFIELD_OFFLINE_TOKEN -version: 1 +version: 2 severity: high --- # Metafield Offline Token -Trace metafield writes and determine whether they use an offline Admin API context where an authenticated online context is required. Confirm token provenance and compensating authorization before reporting. +Trace metafield and metaobject writes and determine their token provenance. Report a write that runs in an offline Admin API context that no verified request for the same shop authorizes: for example `unauthenticated.admin(shop)` or a stored offline session reached from an unauthenticated route, an unverified webhook or app proxy request, or a job whose shop or values come from request-controlled input. + +An offline session returned by `authenticate.admin(request)` (the React Router SDK default when `useOnlineTokens` isn't set) is a verified, shop-bound context: Shopify verified the session token and the staff member's access to the app. Don't report a write only because its token is offline or because an online token would add user attribution or expiry. Report it when: + +- the app defines its own per-user permission, for example an app-level role or allowlist check, and this write bypasses it. Name that permission. +- request-controlled values reach an app-reserved (`$app`) metafield or metaobject that the app treats as trusted state and merchants can't edit themselves, for example a definition with `access.admin = "merchant_read"` or no merchant access, or billing, entitlement or feature-flag data. Writing to the `$app` namespace isn't safe by itself. + +Confirm token provenance and compensating authorization before reporting. diff --git a/packages/app/src/cli/services/app-security-engine/checks/MISSING_AUTHORIZATION_CHECK.md b/packages/app/src/cli/services/app-security-engine/checks/MISSING_AUTHORIZATION_CHECK.md index c4cbb55b2a7..188b75d8e27 100644 --- a/packages/app/src/cli/services/app-security-engine/checks/MISSING_AUTHORIZATION_CHECK.md +++ b/packages/app/src/cli/services/app-security-engine/checks/MISSING_AUTHORIZATION_CHECK.md @@ -1,6 +1,6 @@ --- id: MISSING_AUTHORIZATION_CHECK -version: 2 +version: 3 severity: high --- @@ -57,6 +57,16 @@ actions. changes, direct URLs that survive permission downgrades, and backend actions that remain callable after a feature is hidden or disabled in the UI. +## Shopify embedded apps (React Router) + +`authenticate.admin(request)` verifies the Shopify session token, so the caller is a staff member of that shop whom Shopify has granted access to this app. With offline tokens (the SDK default), Admin API calls use the app's granted scopes rather than the staff member's own permissions. That is Shopify's standard embedded app model, not a finding by itself. Report a handler only when it skips a concrete authorization boundary inside the app: + +- The app defines its own roles, owners, or allowlists (for example a check on `sessionToken.sub`, `onlineAccessInfo.associated_user`, `account_owner`, or an app database role), and a handler that performs the same or a more privileged operation doesn't apply it. Compare each route's loader and action, and sibling routes. +- A request-supplied identifier selects an app-owned record that belongs to another user of the app. +- The app uses online tokens to respect staff permissions on some paths, but a privileged path falls back to an offline session. + +If none of these applies, record the check as executed with no finding. + ## What to report For each action that exposes a concrete unauthorized operation beyond authentication: diff --git a/packages/app/src/cli/services/app-security-engine/checks/STATIC_FRAME_ANCESTORS.md b/packages/app/src/cli/services/app-security-engine/checks/STATIC_FRAME_ANCESTORS.md index 074c8a1aefc..be4293d8b63 100644 --- a/packages/app/src/cli/services/app-security-engine/checks/STATIC_FRAME_ANCESTORS.md +++ b/packages/app/src/cli/services/app-security-engine/checks/STATIC_FRAME_ANCESTORS.md @@ -1,10 +1,12 @@ --- id: STATIC_FRAME_ANCESTORS -version: 1 +version: 2 severity: high precedence: prefer-agent --- # Static Frame Ancestors -Inspect embedded-app Content-Security-Policy construction. Report wildcard or static cross-shop frame-ancestors policies; accept a policy derived safely for the authenticated shop plus Shopify Admin. +Inspect embedded-app Content-Security-Policy construction, including headers that app code sets after or instead of the SDK's. Report wildcard or static cross-shop frame-ancestors policies, such as `*`, `https:`, `*.myshopify.com`, or a fixed list of shop domains, whether the value is written literally or built from variables. Accept a policy derived safely for the shop plus Shopify Admin. + +The React Router SDK's `addDocumentResponseHeaders` (from `shopifyApp()`, usually called in `entry.server`) is the accepted pattern: it sets `frame-ancestors` to the sanitized request shop plus Shopify Admin origins, including Shopify-operated development admin origins. Don't report it unless app code overrides or extends its header. diff --git a/packages/app/src/cli/services/app-security-engine/checks/embedded.ts b/packages/app/src/cli/services/app-security-engine/checks/embedded.ts index 977a71ef32f..fbd1bdacf68 100644 --- a/packages/app/src/cli/services/app-security-engine/checks/embedded.ts +++ b/packages/app/src/cli/services/app-security-engine/checks/embedded.ts @@ -18,8 +18,8 @@ export const EMBEDDED_CHECK_SOURCES: ReadonlyArray = [ "---\nid: EXPIRING_OFFLINE_TOKEN\nversion: 1\nseverity: medium\nprecedence: prefer-agent\n---\n\n# Expiring Offline Token\n\nFor supported React Router apps, verify `expiringOfflineAccessTokens` is enabled and the selected session storage persists `expires`, `refreshToken`, and `refreshTokenExpires` metadata needed for refresh and rotation. `isOnline: false` selects an offline session; it does not disable token expiry and is not a finding. Report an explicit `expiringOfflineAccessTokens: false`. Treat absent or computed flags, custom storage, and ambiguous Prisma schemas as unresolved investigation: inspect storage adapters, migrations, and serialization before returning a clean result. Config-only and unsupported frameworks are handled by the runtime applicability boundary.\n", "---\nid: INSECURE_WEBHOOK_URL\nversion: 2\nseverity: high\nprecedence: prefer-agent\n---\n\n# Insecure Configured Callback Url\n\nInspect webhook destinations and OAuth redirect URLs in the selected Shopify app configuration. Relative Shopify paths and valid pubsub/eventbridge webhook destinations are allowed. Report HTTP, malformed, credential-bearing, wildcard-host, wildcard-path, or otherwise unsafe configured callback URLs.\n", "---\nid: LIQUID_UNSAFE_RENDER\nversion: 1\nseverity: medium\nprecedence: union\n---\n\n# Liquid Unsafe Render\n\nInspect only theme-extension Liquid/HTML files the parser could not analyze. Liquid output is not automatically HTML-escaped. Check the destination: use `escape`/`escape_once` for HTML text and ordinary attributes, `json` when embedding a value as JavaScript data, and `metafield_tag` only for supported rich metafield rendering in HTML content. HTML escaping is not sufficient for event handlers, `srcdoc`, or a `