diff --git a/.changeset/bright-doctors-scan.md b/.changeset/bright-doctors-scan.md new file mode 100644 index 00000000000..7d7b16d6df5 --- /dev/null +++ b/.changeset/bright-doctors-scan.md @@ -0,0 +1,5 @@ +--- +'@shopify/app': minor +--- + +Add `shopify app doctor scan` for Shopify-specific security reviews. diff --git a/packages/app/package.json b/packages/app/package.json index 4a3ed74a086..1c05bc2d306 100644 --- a/packages/app/package.json +++ b/packages/app/package.json @@ -42,6 +42,7 @@ "scripts": { "build": "nx build", "clean": "nx clean", + "generate:app-doctor-checks": "node src/cli/services/app-doctor-engine/embed-checks.mjs", "lint": "nx lint", "lint:fix": "nx lint:fix", "prepack": "NODE_ENV=production pnpm nx build && cp ../../README.md README.md", @@ -55,6 +56,7 @@ }, "dependencies": { "@graphql-typed-document-node/core": "3.2.0", + "@iarna/toml": "2.2.5", "@luckycatfactory/esbuild-graphql-loader": "3.8.1", "@oclif/core": "4.8.3", "@shopify/cli-kit": "4.7.0", @@ -63,9 +65,12 @@ "@shopify/theme": "4.7.0", "@shopify/theme-check-node": "3.29.0", "@shopify/toml-patch": "0.3.0", + "acorn": "8.17.0", + "acorn-walk": "8.3.5", "chokidar": "3.6.0", "diff": "5.2.2", "esbuild": "0.28.1", + "fast-glob": "3.3.3", "graphql-request": "6.1.0", "h3": "1.15.11", "http-proxy-node16": "1.0.6", diff --git a/packages/app/src/cli/commands/app/doctor/scan.test.ts b/packages/app/src/cli/commands/app/doctor/scan.test.ts new file mode 100644 index 00000000000..c69ad03b39d --- /dev/null +++ b/packages/app/src/cli/commands/app/doctor/scan.test.ts @@ -0,0 +1,70 @@ +import DoctorScan from './scan.js' +import doctor from '../../../services/doctor.js' +import AppLinkedCommand from '../../../utilities/app-linked-command.js' +import BaseCommand from '@shopify/cli-kit/node/base-command' +import {resolvePath} from '@shopify/cli-kit/node/path' +import {describe, expect, test, vi} from 'vitest' + +vi.mock('../../../services/doctor.js') + +describe('app doctor scan command', () => { + test('does not require linked app context', () => { + expect(DoctorScan.prototype).toBeInstanceOf(BaseCommand) + expect(DoctorScan.prototype).not.toBeInstanceOf(AppLinkedCommand) + }) + + test('forwards the directory and flags to the service', async () => { + await DoctorScan.run( + ['./fixtures/unlinked-app', '--json', '--verbose', '--blocking', 'high', '--skip-skill'], + import.meta.url, + ) + + expect(doctor).toHaveBeenCalledWith({ + directory: resolvePath('./fixtures/unlinked-app'), + json: true, + verbose: true, + blocking: 'high', + yes: false, + skipSkill: true, + findingsPath: undefined, + }) + }) + + test('forwards --yes without requiring an app configuration', async () => { + await DoctorScan.run(['/tmp/directory-without-shopify-toml', '--yes'], import.meta.url) + + expect(doctor).toHaveBeenCalledWith({ + directory: '/tmp/directory-without-shopify-toml', + json: false, + verbose: false, + blocking: 'none', + yes: true, + skipSkill: false, + findingsPath: undefined, + }) + }) + + test('resolves and forwards an agent findings file', async () => { + await DoctorScan.run(['.', '--findings', './findings.json', '--skip-skill'], import.meta.url) + + expect(doctor).toHaveBeenCalledWith(expect.objectContaining({findingsPath: resolvePath('./findings.json')})) + }) + + test('describes --yes as showing instructions and keeps it mutually exclusive with --skip-skill', () => { + expect(DoctorScan.flags.yes.description).toBe( + 'Show optional App Doctor skill setup instructions without prompting.', + ) + expect(DoctorScan.flags['skip-skill'].description).toBe("Don't offer App Doctor skill setup instructions.") + expect(DoctorScan.flags.yes.exclusive).toEqual(['skip-skill']) + expect(DoctorScan.flags['skip-skill'].exclusive).toEqual(['yes']) + expect(DoctorScan.descriptionWithMarkdown).toContain( + "Shopify CLI only shows instructions; it doesn't install or configure the skill.", + ) + }) + + test('allows --yes in JSON mode while preserving non-interactive output behavior', async () => { + await DoctorScan.run(['--json', '--yes'], import.meta.url) + + expect(doctor).toHaveBeenCalledWith(expect.objectContaining({json: true, yes: true})) + }) +}) diff --git a/packages/app/src/cli/commands/app/doctor/scan.ts b/packages/app/src/cli/commands/app/doctor/scan.ts new file mode 100644 index 00000000000..4bf06a3a482 --- /dev/null +++ b/packages/app/src/cli/commands/app/doctor/scan.ts @@ -0,0 +1,67 @@ +import doctor from '../../../services/doctor.js' +import {Args, Flags} from '@oclif/core' +import BaseCommand from '@shopify/cli-kit/node/base-command' +import {globalFlags, jsonFlag} from '@shopify/cli-kit/node/cli' +import {cwd, resolvePath} from '@shopify/cli-kit/node/path' +import type {AppDoctorBlockingLevel} from '../../../services/app-doctor-api.js' + +const blockingLevels: AppDoctorBlockingLevel[] = ['critical', 'high', 'medium', 'low', 'none'] + +export default class DoctorScan extends BaseCommand { + static summary = 'Check an app for Shopify-specific security issues.' + + static descriptionWithMarkdown = `Runs Shopify App Doctor locally and creates its review pack and trace. + +Pass \`--findings\` after completing the review pack to validate agent findings and compile them into the trace. In CI and other non-interactive environments, skill setup instructions aren't offered unless you pass \`--yes\`. JSON output never prompts or prints those instructions. Shopify CLI only shows instructions; it doesn't install or configure the skill.` + + static description = this.descriptionWithoutMarkdown() + + static args = { + directory: Args.string({ + description: 'The app directory to check. Defaults to the current directory.', + parse: async (input) => resolvePath(input), + }), + } + + static flags = { + ...globalFlags, + ...jsonFlag, + findings: Flags.string({ + description: 'Validate agent findings from a JSON file and compile them into the trace.', + parse: async (input) => resolvePath(input), + env: 'SHOPIFY_FLAG_APP_DOCTOR_FINDINGS', + }), + blocking: Flags.string({ + description: 'The minimum finding severity that causes a non-zero exit code.', + options: blockingLevels, + default: 'none', + env: 'SHOPIFY_FLAG_APP_DOCTOR_BLOCKING', + }), + yes: Flags.boolean({ + description: 'Show optional App Doctor skill setup instructions without prompting.', + default: false, + exclusive: ['skip-skill'], + env: 'SHOPIFY_FLAG_YES', + }), + 'skip-skill': Flags.boolean({ + description: "Don't offer App Doctor skill setup instructions.", + default: false, + exclusive: ['yes'], + env: 'SHOPIFY_FLAG_SKIP_SKILL', + }), + } + + public async run(): Promise { + const {args, flags} = await this.parse(DoctorScan) + + await doctor({ + directory: args.directory ?? cwd(), + json: flags.json, + verbose: Boolean(flags.verbose), + blocking: flags.blocking as AppDoctorBlockingLevel, + yes: flags.yes, + skipSkill: flags['skip-skill'], + findingsPath: flags.findings, + }) + } +} diff --git a/packages/app/src/cli/index.test.ts b/packages/app/src/cli/index.test.ts new file mode 100644 index 00000000000..6c424c63cb0 --- /dev/null +++ b/packages/app/src/cli/index.test.ts @@ -0,0 +1,9 @@ +import {commands} from './index.js' +import DoctorScan from './commands/app/doctor/scan.js' +import {describe, expect, test} from 'vitest' + +describe('@shopify/app command registration', () => { + test('registers app:doctor:scan', () => { + expect(commands['app:doctor:scan']).toBe(DoctorScan) + }) +}) diff --git a/packages/app/src/cli/index.ts b/packages/app/src/cli/index.ts index fc2d9c42b10..7df5089d0cc 100644 --- a/packages/app/src/cli/index.ts +++ b/packages/app/src/cli/index.ts @@ -7,6 +7,7 @@ import ConfigPull from './commands/app/config/pull.js' import DemoWatcher from './commands/app/demo/watcher.js' import Deploy from './commands/app/deploy.js' import Dev from './commands/app/dev.js' +import DoctorScan from './commands/app/doctor/scan.js' import Logs from './commands/app/logs.js' import Sources from './commands/app/app-logs/sources.js' import EnvPull from './commands/app/env/pull.js' @@ -48,6 +49,7 @@ export const commands: {[key: string]: typeof AppLinkedCommand | typeof AppUnlin 'app:deploy': Deploy, 'app:dev': Dev, 'app:dev:clean': DevClean, + 'app:doctor:scan': DoctorScan, 'app:logs': Logs, 'app:logs:sources': Sources, 'app:import-custom-data-definitions': ImportCustomDataDefinitions, diff --git a/packages/app/src/cli/services/app-doctor-api.test.ts b/packages/app/src/cli/services/app-doctor-api.test.ts new file mode 100644 index 00000000000..3f6d7de7cd4 --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-api.test.ts @@ -0,0 +1,91 @@ +import {runAppDoctor} from './app-doctor-api.js' +import {loadChecks} from './app-doctor-engine/index.js' +import {inTemporaryDirectory, mkdir, readFile, writeFile} from '@shopify/cli-kit/node/fs' +import {joinPath} from '@shopify/cli-kit/node/path' +import {describe, expect, test} from 'vitest' + +async function createApp(directory: string, source = 'export const loader = () => ({ok: true})'): Promise { + const sourceDirectory = joinPath(directory, 'app', 'routes') + const sourcePath = joinPath(sourceDirectory, 'index.ts') + await mkdir(sourceDirectory) + await writeFile(joinPath(directory, 'shopify.app.toml'), 'name = "Test app"\nclient_id = "test"\n') + await writeFile(joinPath(directory, 'package.json'), '{"name":"test-app"}\n') + await writeFile(sourcePath, source) + return sourcePath +} + +describe('App Doctor CLI integration', () => { + test('runs the in-tree engine and writes the review pack and trace', async () => { + await inTemporaryDirectory(async (directory) => { + await createApp(directory) + + const result = await runAppDoctor({directory, format: 'human', verbose: true, blocking: 'none'}) + const review = JSON.parse(await readFile(joinPath(directory, 'app-doctor-review.json'))) + const trace = JSON.parse(await readFile(joinPath(directory, 'app-doctor-trace.json'))) + + expect(review.checks).toHaveLength(16) + expect(review.checks.every((check: {prompt: string}) => check.prompt.length > 0)).toBe(true) + expect(trace.schema_version).toBe(1) + expect(trace.engine.name).toBe('shopify-app-doctor') + expect(result.engine).toEqual(trace.engine) + expect(result.output).toContain('shopify app doctor scan --findings ') + expect(result.exitCode).toBe(0) + }) + }) + + test('preserves JSON output and applies the requested blocking severity', async () => { + await inTemporaryDirectory(async (directory) => { + const testToken = ['shpat', '0123456789abcdef0123456789abcdef'].join('_') + await createApp(directory, `const access_token = "${testToken}"`) + + const result = await runAppDoctor({directory, format: 'json', verbose: false, blocking: 'high'}) + + expect(() => JSON.parse(result.output)).not.toThrow() + expect(result.output).not.toContain(testToken) + expect(result.exitCode).toBe(1) + }) + }) + + test('validates agent findings and compiles them into the trace', async () => { + await inTemporaryDirectory(async (directory) => { + await createApp(directory) + const check = loadChecks().get('MISSING_TENANT_ISOLATION')! + const findingsPath = joinPath(directory, 'findings.json') + await writeFile( + findingsPath, + `${JSON.stringify({ + checks_executed: [{check_id: check.id, check_version: check.version, prompt_hash: check.prompt_hash}], + findings: [ + { + check_id: check.id, + check_version: check.version, + prompt_hash: check.prompt_hash, + file: 'app/routes/index.ts', + line: 1, + message: 'The query is not scoped to the current shop.', + evidence: [{file: 'app/routes/index.ts', line: 1, quote: 'loader'}], + }, + ], + })}\n`, + ) + + const result = await runAppDoctor({ + directory, + findingsPath, + format: 'json', + verbose: false, + blocking: 'none', + }) + const trace = JSON.parse(result.output) + + expect(trace.findings).toEqual( + expect.arrayContaining([expect.objectContaining({source: 'agent', check_id: 'MISSING_TENANT_ISOLATION'})]), + ) + expect(trace.checks_executed).toEqual( + expect.arrayContaining([expect.objectContaining({id: 'MISSING_TENANT_ISOLATION', status: 'executed'})]), + ) + expect(JSON.parse(await readFile(joinPath(directory, 'app-doctor-trace.json')))).toEqual(trace) + expect(result.exitCode).toBe(0) + }) + }) +}) diff --git a/packages/app/src/cli/services/app-doctor-api.ts b/packages/app/src/cli/services/app-doctor-api.ts new file mode 100644 index 00000000000..3c8958cccea --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-api.ts @@ -0,0 +1,154 @@ +import { + buildReviewPack, + compileTrace, + formatConsole, + formatJson, + getEngineVersion, + mergeFindings, + scan, + validateAgentChecksExecuted, +} from './app-doctor-engine/index.js' +import {computeResultHash} from './app-doctor-engine/scorer/index.js' +import {AbortError} from '@shopify/cli-kit/node/error' +import {readFile, writeFile} from '@shopify/cli-kit/node/fs' +import {joinPath} from '@shopify/cli-kit/node/path' +import type {CheckExecution, Severity, Suppression} from './app-doctor-engine/types.js' +import type {AgentFindingsDocument} from './app-doctor-engine/checks/index.js' + +const REVIEW_FILENAME = 'app-doctor-review.json' +const TRACE_FILENAME = 'app-doctor-trace.json' + +export interface AppDoctorEngineMetadata { + name: string + version: string + ruleset: string +} + +export type AppDoctorBlockingLevel = Severity | 'none' + +export interface AppDoctorRunOptions { + directory: string + format: 'human' | 'json' + verbose: boolean + blocking: AppDoctorBlockingLevel + findingsPath?: string +} + +export interface AppDoctorRunResult { + output: string + engine: AppDoctorEngineMetadata + exitCode: number +} + +interface FindingsDocument extends AgentFindingsDocument { + suppressions?: Suppression[] +} + +const severityRank: Record = { + critical: 4, + high: 3, + medium: 2, + low: 1, +} + +function shouldBlock(issues: {severity: Severity}[], blocking: AppDoctorBlockingLevel): boolean { + if (blocking === 'none') return false + return issues.some((issue) => severityRank[issue.severity] >= severityRank[blocking]) +} + +function humanScanOutput(scanOutput: string, checkCount: number, reviewPath: string, tracePath: string): string { + return [ + scanOutput.trimEnd(), + '', + 'Agentic review', + `${checkCount} check(s) ready for your coding agent.`, + `Wrote ${reviewPath}`, + `Trace written to ${tracePath}`, + '', + 'After investigating the review pack, compile the final trace with:', + ` shopify app doctor scan --findings `, + ].join('\n') +} + +function humanFindingsOutput(scanOutput: string, accepted: number, rejected: string[], tracePath: string): string { + return [ + scanOutput.trimEnd(), + '', + `Merged ${accepted} agent finding(s) into the trace.`, + ...rejected.map((reason) => `Rejected: ${reason}`), + `Trace written to ${tracePath}`, + ].join('\n') +} + +async function loadFindings(path: string): Promise { + let parsed: unknown + try { + parsed = JSON.parse(await readFile(path)) + } catch (error) { + throw new AbortError( + `Could not read App Doctor findings from ${path}.`, + error instanceof Error ? error.message : undefined, + ) + } + + if (!parsed || typeof parsed !== 'object' || !('findings' in parsed) || !Array.isArray(parsed.findings)) { + throw new AbortError('The App Doctor findings file must contain a findings array.') + } + if ('suppressions' in parsed && parsed.suppressions !== undefined && !Array.isArray(parsed.suppressions)) { + throw new AbortError('The App Doctor findings file suppressions field must be an array.') + } + + return parsed as FindingsDocument +} + +export async function runAppDoctor(options: AppDoctorRunOptions): Promise { + const startTime = Date.now() + const result = await scan(options.directory) + const elapsedMilliseconds = Date.now() - startTime + const engineVersion = getEngineVersion() + const reviewPath = joinPath(options.directory, REVIEW_FILENAME) + const tracePath = joinPath(options.directory, TRACE_FILENAME) + const scanOutput = formatConsole(result, {verbose: options.verbose, elapsedMilliseconds}) + + let rejected: string[] = [] + let accepted = 0 + let agentChecksExecuted: CheckExecution[] = [] + let suppressions: Suppression[] = [] + + if (options.findingsPath) { + const document = await loadFindings(options.findingsPath) + const merged = mergeFindings(result.issues, document.findings, { + knownFiles: new Set(Object.keys(result.scan.file_hashes ?? {})), + }) + const executed = validateAgentChecksExecuted(document) + accepted = merged.accepted + rejected = [...merged.rejected, ...executed.rejected] + agentChecksExecuted = executed.executions + suppressions = document.suppressions ?? [] + result.scan.result_hash = computeResultHash(result.issues, result.score) + } + + const trace = compileTrace(result, {engineVersion, agentChecksExecuted, suppressions}) + await writeFile(tracePath, `${JSON.stringify(trace, null, 2)}\n`) + + let output: string + if (options.findingsPath) { + output = + options.format === 'json' + ? JSON.stringify(trace, null, 2) + : humanFindingsOutput(scanOutput, accepted, rejected, tracePath) + } else { + const reviewPack = buildReviewPack(engineVersion) + await writeFile(reviewPath, `${JSON.stringify(reviewPack, null, 2)}\n`) + output = + options.format === 'json' + ? formatJson(result) + : humanScanOutput(scanOutput, reviewPack.checks.length, reviewPath, tracePath) + } + + let exitCode = 0 + if (rejected.length > 0) exitCode = 2 + else if (shouldBlock(result.issues, options.blocking)) exitCode = 1 + + return {output, engine: trace.engine, exitCode} +} 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 new file mode 100644 index 00000000000..3863b43a7a0 --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-engine/capabilities/detect.ts @@ -0,0 +1,92 @@ +import type {Capabilities} from '../types.js' +import type {SourceFile, AppTomlContent, ExtensionInfo} from '../rules/types.js' + +/** + * Detect what the app does by examining config and source files. + * This determines which rules run and which are skipped. + */ +export function detectCapabilities( + appToml: AppTomlContent | null, + extensions: ExtensionInfo[], + sourceFiles: SourceFile[], +): Capabilities { + // Shopify CLI uses type = "theme" for theme app extensions (not "theme_app_extension"). + // See https://shopify.dev/docs/api/cli/app#extension-types + const themeExtension = extensions.some((extension) => extension.type === 'theme') + const appEmbed = extensions.some((extension) => extension.type === 'theme' && hasAppEmbedBlock(extension)) + + const scriptTags = sourceFiles.some((file) => { + if (!file.content) return false + // Match scriptTag, script_tag, ScriptTag in any language + return /script[_-]?tags?|ScriptTag/i.test(file.content) + }) + + const webhooks = Boolean(appToml?.webhooks?.length) + + const appProxy = Boolean((appToml?.raw as Record)?.app_proxy) + + const storefrontMetafieldWrites = sourceFiles.some((file) => { + if (!file.content) return false + // Match metafield write patterns + return /metafields?Set|metafields?\/.*(?:POST|PUT|create|update)|write.*metafield|metafield.*write/i.test( + file.content, + ) + }) + + const hasBackend = sourceFiles.some((file) => { + if (!file.content) return false + return detectRouteDefinitions(file) + }) + + const declaredIpAllowlist = Boolean(appToml?.ip_allowlist?.length) + + // Shopify CLI uses type = "checkout_ui" for checkout UI extensions. + const checkoutExtension = extensions.some( + (extension) => extension.type === 'checkout_ui' || extension.type === 'checkout_ui_extension', + ) + + return { + theme_app_extension: themeExtension, + app_embed: appEmbed, + script_tags: scriptTags, + webhooks, + app_proxy: appProxy, + storefront_metafield_writes: storefrontMetafieldWrites, + has_backend: hasBackend, + declared_ip_allowlist: declaredIpAllowlist, + checkout_extension: checkoutExtension, + } +} + +function hasAppEmbedBlock(extension: ExtensionInfo): boolean { + return extension.files.some( + (file) => file.ext === '.liquid' && file.content?.includes('"target"') && file.content?.includes('body'), + ) +} + +/** + * Detect route definitions across frameworks. + * Express: app.get/post/put/delete, router.get/post + * Rails: get/post/match in routes.rb + * Remix: export const loader/action + * PHP: Route::get/post + */ +function detectRouteDefinitions(file: SourceFile): boolean { + const content = file.content + if (!content) return false + + // Express / Remix + if (/\b(?:app|router)\.(get|post|put|delete|patch)\s*\(/.test(content)) return true + if (/export\s+(?:async\s+)?(?:function|const)\s+(?:loader|action)\b/.test(content)) return true + + // Rails + if (file.ext === '.rb' && /\b(?:get|post|put|delete|match)\s+['"]/.test(content)) return true + + // PHP Laravel + if (file.ext === '.php' && /Route::(?:get|post|put|delete)\s*\(/.test(content)) return true + + // Flask + if (file.ext === '.py' && /@(?:app|bp)\.route\s*\(/.test(content)) return true + + return false +} diff --git a/packages/app/src/cli/services/app-doctor-engine/checks/APP_PROXY_UNVERIFIED_SIGNATURE.md b/packages/app/src/cli/services/app-doctor-engine/checks/APP_PROXY_UNVERIFIED_SIGNATURE.md new file mode 100644 index 00000000000..3e65846b6f9 --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-engine/checks/APP_PROXY_UNVERIFIED_SIGNATURE.md @@ -0,0 +1,87 @@ +--- +id: APP_PROXY_UNVERIFIED_SIGNATURE +version: 1 +tier: agentic +severity: high +--- + +Find app proxy endpoints that read proxy parameters without verifying +the Shopify signature, allowing an attacker to impersonate Shopify and +send fake proxy requests. + +App proxies let an app serve content directly on the merchant's store +via a URL like `https://shop.example.com/apps/my-app/proxy`. Shopify +signs every proxy request with an HMAC using the app's shared secret. +If the app doesn't verify this signature, anyone can send requests to +the proxy endpoint with forged parameters — including `shop`, +`logged_in_customer_id`, and `path_prefix`. + +## What to look for + +1. **Find app proxy route handlers.** These are endpoints configured as + app proxies in `shopify.app.toml` under `[app_proxy]` or in the app's + routing config. They typically read parameters like: + - `shop` or `shop_id` + - `logged_in_customer_id` + - `path_prefix` + - `signature` + - `timestamp` + +2. **Check for signature verification.** The handler must verify the + HMAC signature before trusting any proxy parameter. Look for: + - **Remix:** `authenticate.public.appProxy(request)` — the official + verification function + - **Rails:** `verified_request?` or manual HMAC verification using + `ShopifyApp` utilities + - **Express:** Manual HMAC verification using the app secret + - **PHP:** `ShopifyUtils::verifyProxyRequest()` or equivalent + +3. **If no verification is present, check whether the handler:** + - Reads `shop` from the query string and uses it to scope data + - Reads `logged_in_customer_id` and uses it for authorisation + - Returns any shop-specific data + + If any of these are true and there's no signature check, it's a real + finding. + +4. **Check for the HMAC pattern even if the function name isn't obvious.** + Some apps implement custom verification: + - `crypto.createHmac('sha256', API_SECRET)` + - `OpenSSL::HMAC.digest` + - `hash_hmac('sha256', ...)` + - Comparison with `timingSafeEqual` or `secure_compare` + +## What to report + +For each proxy handler that reads shop/customer parameters without +signature verification: + +```json +{ + "file": "app/routes/proxy.ts", + "line": 15, + "message": "App proxy handler reads shop parameter without signature verification", + "snippet": "const shop = url.searchParams.get('shop')", + "evidence": [ + { + "file": "app/routes/proxy.ts", + "line": 15, + "quote": "const shop = url.searchParams.get('shop')" + }, + { + "file": "app/routes/proxy.ts", + "line": 1, + "quote": "no authenticate.public.appProxy or HMAC verification found" + } + ], + "confidence": "high", + "reasoning": "The handler reads the shop parameter from the query string and uses it to query shop data, but no signature verification is present. An attacker can send requests with any shop parameter." +} +``` + +Do not report: + +- Handlers that call `authenticate.public.appProxy(request)` (Remix) +- Handlers with manual HMAC verification +- Handlers that return only static content (no shop-specific data) +- Test handlers diff --git a/packages/app/src/cli/services/app-doctor-engine/checks/CSRF_MISSING_PROTECTION.md b/packages/app/src/cli/services/app-doctor-engine/checks/CSRF_MISSING_PROTECTION.md new file mode 100644 index 00000000000..781c0c76eb9 --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-engine/checks/CSRF_MISSING_PROTECTION.md @@ -0,0 +1,88 @@ +--- +id: CSRF_MISSING_PROTECTION +version: 1 +tier: agentic +severity: medium +--- + +Find state-changing endpoints (POST, PUT, DELETE, PATCH) that don't +verify CSRF protection, allowing an attacker to forge requests on +behalf of an authenticated user. + +CSRF (Cross-Site Request Forgery) occurs when an app accepts +state-changing requests without checking that the request came from +the app's own UI. In Shopify apps, embedded apps use session tokens +(JWT) that provide some CSRF protection, but server-rendered apps and +app proxies still need explicit CSRF checks. + +## What to look for + +1. **Find state-changing handlers.** Search for: + - Rails: controller actions responding to POST/PUT/PATCH/DELETE + (check `routes.rb` or controller method names like `create`, + `update`, `destroy`) + - Remix: `action` exports in route files + - Express: `app.post()`, `app.put()`, `app.delete()` + - PHP: form handlers, POST routes + +2. **Check for CSRF protection on each.** Look for: + - Rails: `protect_from_forgery` (default in Rails, but check for + `skip_forgery_protection` or `protect_from_forgery with: :null_session`) + - Remix: session token validation (`authenticate.admin(request)`) + - Express: `csurf` middleware or equivalent + - PHP: CSRF token in form, `VerifyCsrfToken` middleware + +3. **Flag explicit opt-outs.** Search for: + - `skip_forgery_protection` — disables CSRF entirely for a controller + - `protect_from_forgery with: :null_session` — used for webhooks, but + if on a non-webhook endpoint, CSRF is missing + - `skip_before_action :verify_authenticity_token` — skips the Rails + CSRF check + +4. **Distinguish webhooks from user-facing endpoints.** Webhooks use + HMAC verification instead of CSRF tokens — `protect_from_forgery +with: :null_session` is correct for webhooks. But the same pattern + on a user-facing POST handler is a CSRF vulnerability. + +5. **Check Shopify-specific patterns.** Embedded apps that use + `authenticate.admin(request)` get session token validation that + prevents CSRF. But if an action skips `authenticate.admin` and still + processes state changes, CSRF protection may be missing. + +## What to report + +For each state-changing endpoint without CSRF protection: + +```json +{ + "file": "app/controllers/settings_controller.rb", + "line": 5, + "message": "POST handler with CSRF protection disabled", + "snippet": "skip_forgery_protection", + "evidence": [ + { + "file": "app/controllers/settings_controller.rb", + "line": 5, + "quote": "skip_forgery_protection" + }, + { + "file": "app/controllers/settings_controller.rb", + "line": 10, + "quote": "def update" + } + ], + "confidence": "medium", + "reasoning": "The update action accepts POST requests but CSRF protection is explicitly skipped. This is not a webhook handler (no HMAC verification), so an attacker can forge a POST request from another site." +} +``` + +Do not report: + +- Webhook handlers with `protect_from_forgery with: :null_session` + (HMAC is the CSRF protection for webhooks) +- Endpoints protected by `authenticate.admin(request)` (session + token provides CSRF protection) +- GET-only handlers (not state-changing) +- API endpoints that use bearer token auth (not cookie-based, so + CSRF doesn't apply) +- Test controllers diff --git a/packages/app/src/cli/services/app-doctor-engine/checks/MISSING_AUTHORIZATION_CHECK.md b/packages/app/src/cli/services/app-doctor-engine/checks/MISSING_AUTHORIZATION_CHECK.md new file mode 100644 index 00000000000..dee1293b9b6 --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-engine/checks/MISSING_AUTHORIZATION_CHECK.md @@ -0,0 +1,94 @@ +--- +id: MISSING_AUTHORIZATION_CHECK +version: 1 +tier: agentic +severity: high +--- + +Find controller actions or route handlers that access resources without +checking whether the current user is authorized to access them, beyond +authentication. Authentication verifies WHO you are; authorization verifies +WHAT you can do. An app can be authenticated but still access resources +belonging to another merchant if authorization checks are missing. + +This is distinct from `MISSING_TENANT_ISOLATION` (database query +scoping) — this check looks for missing policy/permission checks on +actions, even when the data access is scoped. For example, an app might +scope queries by shop but not check whether the merchant has the right to +delete a resource, or whether a staff member can access admin-only +actions. + +## What to look for + +1. **Find authorization frameworks.** Check what the app uses: + - Rails: Pundit (`authorize`, `policy`, `Pundit`), CanCanCan + (`can?`, `ability`), action_access filters + - Remix/Express: middleware that checks roles/permissions + - Custom: `before_action :check_admin`, `if current_user.can?` + +2. **Find actions without authorization checks.** For each controller + action or route handler, determine: + - Is there a `before_action` that checks authorization (not just + authentication)? + - Is there a Pundit `authorize` call? + - Is there a CanCanCan `authorize!` or `can?` check? + - Is there a custom permission check? + +3. **Check for `skip_idor_protection` or equivalent opt-outs.** These + disable IDOR/authorization checks. For each, determine: + - Is the skip justified? (e.g., public endpoint, webhook, health check) + - Does the skip expose a state-changing action to unauthorised users? + - Is there a compensating control (HMAC, session token, etc.)? + +4. **Check for admin-only functionality reachable by merchants.** Look for: + - Controllers under `admin/` namespace that don't check staff vs merchant + - Actions that modify app configuration without checking the caller's role + - Staff-only operations accessible through the merchant-facing UI + +5. **Check for missing object-level authorization.** Even if the query + is scoped by shop, does the handler verify that the specific resource + belongs to the current merchant? + - `Order.find(params[:id])` scoped by shop — but does it check the + merchant can access this specific order? + - `Product.find(params[:id])` — is there a policy check, or just + tenant scoping? + +## What to report + +For each action that accesses resources without authorization checks: + +```json +{ + "file": "app/controllers/orders_controller.rb", + "line": 15, + "message": "Destroy action has no authorization check beyond authentication", + "snippet": "def destroy\n Order.find(params[:id]).destroy\nend", + "evidence": [ + { + "file": "app/controllers/orders_controller.rb", + "line": 15, + "quote": "def destroy" + }, + { + "file": "app/controllers/orders_controller.rb", + "line": 5, + "quote": "before_action :authenticate_user (no authorize check)" + } + ], + "confidence": "medium", + "reasoning": "The destroy action authenticates the user but does not call authorize or check a policy. Any authenticated merchant can delete any order within their shop, even if they shouldn't have delete permissions." +} +``` + +Do not report: + +- Actions with explicit `authorize` / `can?` / policy checks +- Actions protected by a `before_action` that checks authorization +- Public endpoints (health checks, static content) +- Webhook handlers (HMAC is the authorization) +- Actions that only read data the merchant owns (scoped by session.shop + AND no object-level access control needed) +- Internal/staff-only controllers (under `Internal::` namespace, behind + employee SSO like `EmployeeIdentity`, `IdentityClient`, etc.) +- Test files (under test/ or \*\_test.rb) +- Test controllers diff --git a/packages/app/src/cli/services/app-doctor-engine/checks/MISSING_EMBEDDED_CSP.md b/packages/app/src/cli/services/app-doctor-engine/checks/MISSING_EMBEDDED_CSP.md new file mode 100644 index 00000000000..5cd0b6fe1af --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-engine/checks/MISSING_EMBEDDED_CSP.md @@ -0,0 +1,74 @@ +--- +id: MISSING_EMBEDDED_CSP +version: 2 +tier: agentic +severity: medium +--- + +Find embedded Shopify apps that are missing a Content-Security-Policy +`frame-ancestors` directive, allowing any origin to iframe the app. + +Shopify apps run inside an iframe in the admin. Without a +`frame-ancestors` directive in the CSP header, any website can embed the +app in an iframe — a clickjacking risk. The attacker overlays invisible +elements on top of the app's UI to trick the merchant into clicking +buttons they can't see. + +## What to look for + +1. **Determine if the app is embedded.** Check `shopify.app.toml` for + `app_embed` or `theme_app_extension` in the capabilities. If the app + is not embedded, this check does not apply. + +2. **Find where HTTP response headers are set.** Search for: + - `Content-Security-Policy` in any file + - `addDocumentResponseHeaders` (Shopify Remix helper) + - `response.headers.set` + - `frame-ancestors` + - CSP middleware configuration + +3. **If CSP headers are set, check for `frame-ancestors`.** The directive + must be present and must restrict embedding to: + - `https://admin.shopify.com` + - The authenticated shop's domain (e.g. `https://my-shop.myshopify.com`) + + A wildcard `frame-ancestors *` is not safe. An absent `frame-ancestors` + is not safe (browsers default to allowing any origin). + +4. **Check for the Shopify Remix helper.** If the app uses + `@shopify/shopify-app-remix`, the `addDocumentResponseHeaders` function + sets the correct CSP automatically. If it's called, the app is safe. + +5. **Check for `X-Frame-Options` as a fallback.** Some apps use + `X-Frame-Options: ALLOW-FROM https://admin.shopify.com` instead of + CSP `frame-ancestors`. This is deprecated but functional in some + browsers. Note it but don't flag if CSP is also present. + +## What to report + +For embedded apps with no `frame-ancestors` directive: + +```json +{ + "file": "app/root.tsx", + "line": 1, + "message": "Embedded app has no frame-ancestors CSP directive — any origin can iframe it", + "evidence": [ + { "file": "shopify.app.toml", "line": 5, "quote": "app_embed = true" }, + { + "file": "app/root.tsx", + "line": 1, + "quote": "no addDocumentResponseHeaders or CSP header found" + } + ], + "confidence": "medium", + "reasoning": "The app declares app_embed capability but no file sets a Content-Security-Policy with frame-ancestors. Without it, any website can iframe the app." +} +``` + +Do not report: + +- Non-embedded apps (no app_embed or theme_app_extension) +- Apps that call `addDocumentResponseHeaders` (handles CSP automatically) +- Apps with an explicit `frame-ancestors` directive in their CSP +- Test files (under test/ or \*\_test.rb) diff --git a/packages/app/src/cli/services/app-doctor-engine/checks/MISSING_TENANT_ISOLATION.md b/packages/app/src/cli/services/app-doctor-engine/checks/MISSING_TENANT_ISOLATION.md new file mode 100644 index 00000000000..1f4a8833328 --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-engine/checks/MISSING_TENANT_ISOLATION.md @@ -0,0 +1,86 @@ +--- +id: MISSING_TENANT_ISOLATION +version: 3 +tier: agentic +severity: high +--- + +Find controller actions where a database query can read or modify a row +belonging to a shop other than the one making the request. + +This is a multi-tenant app: every merchant's data must be isolated by +shop. A query that doesn't filter on the current shop is a cross-tenant +leak. Static analysis can't catch these reliably because the scoping is +often indirect — applied by a `before_action`, inherited from a parent +controller, or baked into a default scope on the model. Your job is to +follow those threads. + +## What to look for + +Search for ActiveRecord queries that filter on a column other than +`shop_id` / `shop`, or that take no tenant filter at all: + +```ruby +Product.where(id: params[:id]) +Order.where(shopify_id: params[:order_id]) +Token.where(shop_id: params[:shop_id]).delete_all +``` + +The last one looks scoped but isn't — `params[:shop_id]` comes from the +request, not from the authenticated session. The caller can pass any +shop's id. + +## How to investigate each candidate + +1. **Read the enclosing method and the whole controller.** The scope may + be applied on an adjacent line, or the flagged line may be a fragment + of a longer chain (`.or(...)`, `.merge(...)`) whose base scope is above. + +2. **Follow the receiver.** If the query is on a variable rather than a + model constant, find where it comes from. A relation passed in as a + method parameter may already be scoped by its caller — go look. + +3. **Read the controller's ancestors.** Authentication and tenant scoping + are usually inherited: `before_action`, `around_action`, a mixin, or a + parent class. Follow the chain to the top before concluding there's no + protection. + +4. **Check whether the model is tenant-scoped at all.** Read the model and + its schema. If the table has no shop/tenant column, there is nothing to + scope by. Global reference or catalog tables are a correct design. + +5. **Consider whether cross-tenant access is the deliberate purpose.** + Some queries exist to resolve which tenant owns a resource. Scoping + those by tenant is circular. If so, the risk is enumeration, not + isolation — note it but don't report it under this check. + +6. **Check for an explicit opt-out** like `skip_idor_protection`. That + tells you the author considered it. Decide whether their reasoning + holds — an unguessable capability token is a real control; a sequential + integer id is not. + +## What to report + +For each genuine cross-tenant risk you find, report: + +```json +{ + "file": "app/controllers/...", + "line": 42, + "message": "Query on Product is not scoped to the current shop", + "snippet": "Product.where(id: params[:id])", + "evidence": [ + { "file": "path", "line": 12, "quote": "the line that shows the gap" } + ], + "confidence": "high", + "reasoning": "what you read and why it's a real risk" +} +``` + +Be precise about the gap. "No shop filter" is not enough — explain where +the scoping _should_ have come from and why it's missing. If you read a +file and it turns out the query IS scoped, don't report it. You are not +trying to find problems — you are trying to find the real ones. + +Every finding must cite at least one file and line you actually read. +An finding with no evidence is not a finding. diff --git a/packages/app/src/cli/services/app-doctor-engine/checks/OPEN_REDIRECT.md b/packages/app/src/cli/services/app-doctor-engine/checks/OPEN_REDIRECT.md new file mode 100644 index 00000000000..468cdcfdc1c --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-engine/checks/OPEN_REDIRECT.md @@ -0,0 +1,66 @@ +--- +id: OPEN_REDIRECT +version: 1 +tier: agentic +severity: medium +--- + +Find redirect URLs that are built from user input without validation, +allowing an attacker to redirect users to a malicious site. + +An open redirect occurs when a web application redirects to a URL that +comes from an untrusted source (query parameters, form fields, headers) +without checking that the destination is safe. In Shopify apps, this is +particularly dangerous because the app runs inside an iframe in the admin +— a redirect to an external site can be used for phishing. + +## What to look for + +1. **Find redirect calls.** Search for: + - Rails: `redirect_to`, `head :redirect`, `redirect` + - Remix/Express: `redirect()`, `Response.redirect()`, `res.redirect()` + - PHP: `header("Location: ...")`, `Redirect::to()` + - Python: `redirect()`, `HttpResponseRedirect()` + +2. **Trace the URL source.** For each redirect, determine where the + destination URL comes from: + - `params[:return_url]`, `params[:redirect_url]`, `request.query_params` + - `url.searchParams.get("return_url")` + - `$_GET['redirect']`, `request.args.get('next')` + +3. **Check for validation.** Is the URL checked against an allowlist? Is + it restricted to relative paths? Is it compared to a known-safe list of + domains? If none of these, it's an open redirect. + +4. **Consider the `flow_redirect_url` pattern.** Shopify Flow connectors + use signed URLs for redirects — the URL is HMAC-signed, so it's not + user-controlled even though it comes from params. Verify the signature + check exists before flagging. + +## What to report + +```json +{ + "file": "app/controllers/...", + "line": 42, + "message": "Redirect to user-supplied URL without validation", + "snippet": "redirect_to(params[:return_url])", + "evidence": [ + { "file": "path", "line": 42, "quote": "redirect_to(params[:return_url])" }, + { + "file": "path", + "line": 30, + "quote": "no allowlist or validation found in this controller" + } + ], + "confidence": "high", + "reasoning": "The redirect target comes from params[:return_url] with no allowlist, path validation, or signature check." +} +``` + +Do not report: + +- Redirects to hardcoded paths (`redirect_to("/dashboard")`) +- Redirects with allowlist validation (`if ALLOWED_HOSTS.include?(uri.host)`) +- Signed redirect URLs (verify the HMAC check first) +- Test controllers diff --git a/packages/app/src/cli/services/app-doctor-engine/checks/OVERBROAD_DATA_ACCESS.md b/packages/app/src/cli/services/app-doctor-engine/checks/OVERBROAD_DATA_ACCESS.md new file mode 100644 index 00000000000..e1a4ac61d21 --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-engine/checks/OVERBROAD_DATA_ACCESS.md @@ -0,0 +1,87 @@ +--- +id: OVERBROAD_DATA_ACCESS +version: 1 +tier: agentic +severity: medium +--- + +Find cases where an app returns more data than necessary in API +responses, exposing sensitive information that the caller doesn't need. + +Overbroad data access is a privacy risk: returning full customer records +when only an order status is needed, exposing PII (email, phone, address) +in error messages, or selecting all fields in a GraphQL query when only +a subset is required. This is how information disclosure happens in +practice — not through a single vulnerability, but through +carelessly broad data returns. + +## What to look for + +1. **Find API response patterns.** Search for: + - Rails: `render json: @orders`, `render json: order`, + `respond_with @resource`, `as_json` + - Remix: `return json(data)`, `return Response(data)` + - GraphQL: query resolvers that return full objects + - Any serialization that includes all model fields + +2. **Check what fields are returned.** For each API response: + - Does it return the full model (all columns) or a filtered set? + - Does it include sensitive fields like: + - `email`, `phone`, `address`, `name` (PII) + - `api_key`, `access_token`, `secret` (credentials) + - `shop_id`, `tenant_id` (internal identifiers) + - `password`, `password_digest` (auth data) + - Is there a serializer or field selection that limits the output? + +3. **Find GraphQL over-selection.** Search for: + - Queries that select all fields: `query { products { ...AllFields } }` + - Queries without field selection: `query { orders }` (returns everything) + - Mutations that return the full object after creation/update + +4. **Check error messages for information disclosure.** Search for: + - Error responses that include stack traces + - Error messages that reveal internal paths (`/app/services/...`) + - Error messages that include database details (table names, column names) + - Debug endpoints that expose app configuration + +5. **Check for missing field-level authorization.** Even if the caller + can access the resource, should they see all fields? + - A merchant can see their orders, but should they see internal + `cost` or `profit_margin` fields? + - A customer can see their order, but should they see the merchant's + internal notes? + +## What to report + +For each response that returns sensitive data unnecessarily: + +```json +{ + "file": "app/controllers/api/orders_controller.rb", + "line": 20, + "message": "API response returns full order including customer PII", + "snippet": "render json: @order", + "evidence": [ + { + "file": "app/controllers/api/orders_controller.rb", + "line": 20, + "quote": "render json: @order" + }, + { + "file": "app/models/order.rb", + "line": 15, + "quote": "has_many :line_items (includes customer email and shipping address)" + } + ], + "confidence": "medium", + "reasoning": "The response serializes the full order model including related customer PII (email, phone, address). No field selection or serializer limits the output. The caller only needs order status, but receives the customer's personal information." +} +``` + +Do not report: + +- Responses with explicit field selection (serializers, `only:`, `except:`) +- Responses that return only public/non-sensitive fields +- Admin-only endpoints where full data access is intended +- Internal diagnostic endpoints behind staff auth +- Test files (under test/ or \*\_test.rb) diff --git a/packages/app/src/cli/services/app-doctor-engine/checks/REQUEST_DERIVED_SHOP_SCOPE.md b/packages/app/src/cli/services/app-doctor-engine/checks/REQUEST_DERIVED_SHOP_SCOPE.md new file mode 100644 index 00000000000..4374edd3cd1 --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-engine/checks/REQUEST_DERIVED_SHOP_SCOPE.md @@ -0,0 +1,122 @@ +--- +id: REQUEST_DERIVED_SHOP_SCOPE +version: 2 +tier: agentic +severity: high +--- + +Find cases where a shop identifier comes from request input (form data, +query params, headers) instead of the authenticated session, and is used +to scope a database query or select an Admin API context. + +The key insight: a shop filter that uses an attacker-controlled value is +no filter at all. The attacker can pass any shop's identifier and access +that shop's data. This is distinct from `MISSING_TENANT_ISOLATION` (no +shop filter at all) — here the filter or context selection exists, but +the value comes from the request, not the session. + +This bug appears in two forms: + +**Form 1: Database query scoped by request input.** + +```ruby +# Rails — shop_id from params, not session +Token.where(shop_id: params[:shop_id]).delete_all +Order.find_by(shop_id: params[:shop]) +``` + +**Form 2: Admin API context selected by request input.** + +```typescript +// Remix — shop from formData, not session +const shop = formData.get("shop"); +const { admin } = await unauthenticated.admin(shop); +// Now admin is scoped to whatever shop the caller passed +``` + +Both are the same vulnerability: the caller chooses which shop's data to +access. In Form 1, the query filter is attacker-controlled. In Form 2, +the Admin API context is attacker-controlled. `unauthenticated.admin()` +deliberately takes a shop parameter (it's for offline/background jobs), +so using it with request input is an IDOR — the caller selects the shop. + +## What to look for + +1. **Find database queries that filter on a shop/tenant column.** Search for: + - `where(shop_id:`, `where(shop:`, `where(store_id:`, `where(tenant_id:` + - `.find_by(shop_id:`, `.find_or_initialize_by(shop_id:` + +2. **Find `unauthenticated.admin()` calls.** Search for: + - `unauthenticated.admin(` — this function takes a shop domain/id as + its argument. If that argument comes from request input, it's an IDOR. + - `unauthenticated.admin(shop)` where `shop` is traced to `formData.get()`, + `request.json()`, `url.searchParams.get()`, `params.shop`, etc. + +3. **Trace the shop value for every query or admin context call.** Determine + where it comes from: + - `params[:shop_id]`, `formData.get("shop")`, `url.searchParams.get("shop")` + — request input, attacker-controlled + - `request.headers["X-Shopify-Shop-Domain"]` — header, attacker-controlled + - `session.shop`, `current_shop.shop_id`, `shop.shop_id` — session-derived, + safe + - A local variable — trace it back to its assignment + +4. **Check for compensating controls.** The shop value may be safe even + if it comes from params, IF there's a prior verification: + - An HMAC signature on the URL (e.g., `validate_path` with a signing key) + - A `before_action` that validates the shop against the session + - A Pundit policy check + - The params were set by trusted backend code, not the client + + Follow the control to its definition and verify it actually covers + this query's shop_id. + +5. **Check for the OAuth callback pattern.** In Shopify OAuth flows, + `shop_id` often comes from a signed URL that was generated by the + app's own backend using the session shop. The HMAC on that URL is + the control. This is safe — but verify the signing key isn't + hardcoded or leaked. + +6. **Distinguish `authenticate.admin` from `unauthenticated.admin`.** + `authenticate.admin(request)` derives the shop from the session — safe. + `unauthenticated.admin(shop)` takes the shop as an argument — only safe + if the argument is session-derived or verified, NOT if it comes from + request input. + +## What to report + +For each query or admin context call where the shop value is +attacker-controlled with no compensating control: + +```json +{ + "file": "app/routes/api.orders.ts", + "line": 6, + "message": "Shop from formData passed to unauthenticated.admin() — IDOR", + "snippet": "const shop = formData.get(\"shop\"); const { admin } = await unauthenticated.admin(shop);", + "evidence": [ + { + "file": "app/routes/api.orders.ts", + "line": 5, + "quote": "const shop = formData.get(\"shop\")" + }, + { + "file": "app/routes/api.orders.ts", + "line": 6, + "quote": "unauthenticated.admin(shop)" + } + ], + "confidence": "high", + "reasoning": "Shop comes from formData (request input) and is passed to unauthenticated.admin(). No session verification. An attacker can set shop to any value and access that shop's Admin API context." +} +``` + +Do not report: + +- Calls to `authenticate.admin(request)` — the shop comes from the + session, not from request input +- Queries where shop_id comes from `current_shop`, `session.shop`, or + other session-derived sources +- Queries guarded by an HMAC signature (verify the signature check first) +- Queries on the Shop model itself (looking up a shop by id is normal) +- Queries in webhook handlers (the HMAC verification covers the payload) diff --git a/packages/app/src/cli/services/app-doctor-engine/checks/SCOPE_OVER_REQUEST.md b/packages/app/src/cli/services/app-doctor-engine/checks/SCOPE_OVER_REQUEST.md new file mode 100644 index 00000000000..5c8a2d1386a --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-engine/checks/SCOPE_OVER_REQUEST.md @@ -0,0 +1,90 @@ +--- +id: SCOPE_OVER_REQUEST +version: 1 +tier: agentic +severity: high +--- + +Find cases where an app requests OAuth scopes it does not use, or uses +scopes in ways that exceed what the merchant authorised. + +When a merchant installs an app, they grant a set of access scopes (e.g. +`read_orders`, `write_products`). The app should only access data covered +by those scopes. Two risks: + +1. **Over-requested scopes:** the app declares scopes in its config that it + never references in code. This is a privacy violation — the merchant + granted access to data the app doesn't need. + +2. **Under-verified usage:** the app calls an API endpoint that requires a + scope, but doesn't check that the scope was granted before making the + call. This can fail at runtime or, worse, access data the merchant + didn't authorise if the scope was added by a different code path. + +## What to look for + +1. **Find the declared scopes.** Look in `shopify.app.toml` under + `[access_scopes]` → `scopes`, or in the app's OAuth redirect URL, or + in environment variables like `SCOPES`. + +2. **Find where scopes are used.** Search for API calls that reference + Shopify resources: `admin.rest.get`, `admin.graphql`, REST resource + classes, GraphQL queries on `orders`, `products`, `customers`, etc. + +3. **Match scopes to usage.** Each scope should map to at least one API + call: + - `read_orders` → queries on orders + - `write_products` → mutations on products + - `read_customers` → queries on customers + - etc. + +4. **Flag scopes with no matching usage.** If `read_analytics` is declared + but no code references analytics, that's an over-requested scope. + +5. **Flag API calls with no matching scope.** If code queries customers + but `read_customers` isn't declared, that's an under-verified usage. + +## What to report + +```json +{ + "file": "shopify.app.toml", + "line": 10, + "message": "Scope 'read_analytics' is declared but never referenced in app code", + "evidence": [ + { + "file": "shopify.app.toml", + "line": 10, + "quote": "scopes = \"read_orders,read_analytics\"" + } + ], + "confidence": "medium", + "reasoning": "Searched all source files for 'analytics' and found no API calls referencing analytics endpoints or resources." +} +``` + +For under-verified usage, report the code location, not the TOML: + +```json +{ + "file": "app/services/customer_export.rb", + "line": 15, + "message": "Queries customers but 'read_customers' is not in declared scopes", + "evidence": [ + { + "file": "app/services/customer_export.rb", + "line": 15, + "quote": "Customer.all" + }, + { + "file": "shopify.app.toml", + "line": 10, + "quote": "scopes = \"read_orders\"" + } + ], + "confidence": "high" +} +``` + +Note: if the app has zero source files (config-only app), do not report +over-requested scopes — you cannot verify usage from an empty corpus. diff --git a/packages/app/src/cli/services/app-doctor-engine/checks/SCRIPT_TAG_URL_INJECTION.md b/packages/app/src/cli/services/app-doctor-engine/checks/SCRIPT_TAG_URL_INJECTION.md new file mode 100644 index 00000000000..3a2e68a40ce --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-engine/checks/SCRIPT_TAG_URL_INJECTION.md @@ -0,0 +1,81 @@ +--- +id: SCRIPT_TAG_URL_INJECTION +version: 1 +tier: agentic +severity: critical +--- + +Find cases where the ScriptTag API is used with a URL derived from user +input, allowing an attacker to inject arbitrary scripts into every +merchant's storefront. + +The ScriptTag API injects a `