From 9f46946291774ddbbe7e5adb683de8c74730288f Mon Sep 17 00:00:00 2001 From: Jason Kirtland Date: Mon, 28 Sep 2026 17:23:02 -0700 Subject: [PATCH] App Security: combined review and submit v2 Converge deterministic-findings.json (renamed from scan.json) and agent-findings.json on one stored schema with versioned translation. Combine both sources in the engine, honoring per-check precedence, and fall back to union when the agent result is older than the deterministic one. Load both files through one shared loader. Rewrite `review` to render or encode the combined results, and move `submit` to a v2 payload projected from both sources with privacy filtering and new copy. --- .../commands/app/security/blocking-flag.ts | 17 + .../app/security/check.integration.test.ts | 6 +- .../src/cli/commands/app/security/check.ts | 13 +- .../cli/commands/app/security/review.test.ts | 48 +- .../src/cli/commands/app/security/review.ts | 22 +- .../app/security/submit.integration.test.ts | 74 +- .../cli/commands/app/security/submit.test.ts | 26 +- .../src/cli/commands/app/security/submit.ts | 8 +- .../src/cli/services/app-security-api.test.ts | 13 +- .../app/src/cli/services/app-security-api.ts | 9 +- .../services/app-security-artifacts.test.ts | 169 ++-- .../cli/services/app-security-artifacts.ts | 75 +- .../services/app-security-commands.test.ts | 19 + .../src/cli/services/app-security-commands.ts | 2 + .../app-security-engine/INSTRUCTIONS.md | 10 +- .../checks/APP_PROXY_LIQUID_INJECTION.md | 1 + .../checks/COMMITTED_SECRET.md | 1 + .../checks/CREDENTIAL_BROWSER_LEAKAGE.md | 1 + .../checks/CREDENTIAL_LOG_LEAKAGE.md | 1 + .../checks/DEPRECATED_SCRIPT_TAG_SCOPE.md | 1 + .../checks/EOL_API_VERSION.md | 1 + .../checks/EXPIRING_OFFLINE_TOKEN.md | 1 + .../checks/INSECURE_WEBHOOK_URL.md | 1 + .../checks/LIQUID_UNSAFE_RENDER.md | 1 + .../checks/MISSING_COMPLIANCE_WEBHOOKS.md | 1 + .../REQUEST_CONTROLLED_ADMIN_CONTEXT.md | 1 + .../checks/STATIC_FRAME_ANCESTORS.md | 1 + .../checks/UNAUTHENTICATED_ENDPOINT.md | 1 + .../checks/UNSAFE_INNERHTML.md | 1 + .../app-security-engine/checks/embedded.ts | 30 +- .../app-security-engine/checks/index.ts | 68 +- .../cli/services/app-security-engine/index.ts | 67 +- .../output/group-issues.ts | 6 +- .../app-security-engine/registry/index.ts | 8 +- .../app-security-engine/results/combine.ts | 292 +++++++ .../app-security-engine/results/order.ts | 27 + .../app-security-engine/results/schema.ts | 155 ++++ .../app-security-engine/results/translate.ts | 48 ++ .../cli/services/app-security-engine/run.ts | 8 +- .../scan-artifact/index.ts | 110 +-- .../app-security-engine/submission/index.ts | 252 ++++-- .../app-security-engine/tests/combine.test.ts | 574 +++++++++++++ .../tests/dependency-automation.test.ts | 30 +- .../tests/fixtures/findings-documents.ts | 245 ++++++ .../security-submit-dry-run-result.json | 2 +- .../fixtures/security-submit-result.json | 2 +- .../tests/fixtures/submission-agent-only.json | 94 +++ .../submission-deterministic-only.json | 134 +++ .../fixtures/submission-forbidden-values.json | 44 +- .../tests/fixtures/submission-scan.ts | 101 --- .../tests/fixtures/submission.json | 286 +++++-- .../app-security-engine/tests/record.test.ts | 136 ++- .../tests/registry.test.ts | 80 +- .../tests/scan-artifact.test.ts | 245 ++++-- .../tests/scan-contract.test.ts | 2 +- .../tests/submission.test.ts | 386 ++++++++- .../tests/translate.test.ts | 141 ++++ .../cli/services/app-security-engine/types.ts | 152 ++-- .../src/cli/services/app-security-format.ts | 25 + .../{scan.json => check.json} | 6 +- .../review-filtered.json | 148 ++++ .../app-security-json-fixtures/review.json | 356 ++++++++ .../cli/services/app-security-json.test.ts | 12 +- .../app-security-results.test-data.ts | 29 + .../cli/services/app-security-results.test.ts | 260 ++++++ .../src/cli/services/app-security-results.ts | 128 +++ .../app-security-submission-payload.test.ts | 10 +- .../services/app-security-submit-api.test.ts | 2 +- .../src/cli/services/security-check.test.ts | 21 +- .../app/src/cli/services/security-check.ts | 12 +- .../app/src/cli/services/security-json.ts | 4 +- .../src/cli/services/security-output.test.ts | 2 +- .../app/src/cli/services/security-output.ts | 2 +- .../src/cli/services/security-record.test.ts | 6 +- .../app/src/cli/services/security-record.ts | 22 +- .../cli/services/security-review-json.test.ts | 107 +++ .../src/cli/services/security-review-json.ts | 151 +++- .../services/security-review-output.test.ts | 790 ++++++++++++++++++ .../cli/services/security-review-output.ts | 496 +++++++++++ .../src/cli/services/security-review.test.ts | 341 ++++++-- .../app/src/cli/services/security-review.ts | 177 ++-- .../cli/services/security-submit-json.test.ts | 11 +- .../services/security-submit-output.test.ts | 168 +++- .../cli/services/security-submit-output.ts | 84 +- .../cli/services/security-submit-result.ts | 10 +- .../src/cli/services/security-submit.test.ts | 183 +++- .../app/src/cli/services/security-submit.ts | 70 +- packages/cli/oclif.manifest.json | 37 +- 88 files changed, 6698 insertions(+), 1222 deletions(-) create mode 100644 packages/app/src/cli/commands/app/security/blocking-flag.ts create mode 100644 packages/app/src/cli/services/app-security-engine/results/combine.ts create mode 100644 packages/app/src/cli/services/app-security-engine/results/order.ts create mode 100644 packages/app/src/cli/services/app-security-engine/results/schema.ts create mode 100644 packages/app/src/cli/services/app-security-engine/results/translate.ts create mode 100644 packages/app/src/cli/services/app-security-engine/tests/combine.test.ts create mode 100644 packages/app/src/cli/services/app-security-engine/tests/fixtures/findings-documents.ts create mode 100644 packages/app/src/cli/services/app-security-engine/tests/fixtures/submission-agent-only.json create mode 100644 packages/app/src/cli/services/app-security-engine/tests/fixtures/submission-deterministic-only.json delete mode 100644 packages/app/src/cli/services/app-security-engine/tests/fixtures/submission-scan.ts create mode 100644 packages/app/src/cli/services/app-security-engine/tests/translate.test.ts create mode 100644 packages/app/src/cli/services/app-security-format.ts rename packages/app/src/cli/services/app-security-json-fixtures/{scan.json => check.json} (92%) create mode 100644 packages/app/src/cli/services/app-security-json-fixtures/review-filtered.json create mode 100644 packages/app/src/cli/services/app-security-json-fixtures/review.json create mode 100644 packages/app/src/cli/services/app-security-results.test-data.ts create mode 100644 packages/app/src/cli/services/app-security-results.test.ts create mode 100644 packages/app/src/cli/services/app-security-results.ts create mode 100644 packages/app/src/cli/services/security-review-json.test.ts create mode 100644 packages/app/src/cli/services/security-review-output.test.ts create mode 100644 packages/app/src/cli/services/security-review-output.ts diff --git a/packages/app/src/cli/commands/app/security/blocking-flag.ts b/packages/app/src/cli/commands/app/security/blocking-flag.ts new file mode 100644 index 00000000000..5141bd44727 --- /dev/null +++ b/packages/app/src/cli/commands/app/security/blocking-flag.ts @@ -0,0 +1,17 @@ +import {Flags} from '@oclif/core' +import type {AppSecurityBlockingLevel} from '../../../services/app-security-api.js' + +const blockingLevels: AppSecurityBlockingLevel[] = ['high', 'medium', 'low', 'none'] + +/** + * `--blocking`, shared by `check` and `review` so both accept the same levels and default. + * `Flags.custom` types the parsed value as an `AppSecurityBlockingLevel`, so the commands don't cast it. + */ +export const appSecurityBlockingFlag = { + blocking: Flags.custom({ + description: 'The minimum finding severity that causes a non-zero exit code.', + options: blockingLevels, + default: 'none', + env: 'SHOPIFY_FLAG_APP_SECURITY_BLOCKING', + })(), +} diff --git a/packages/app/src/cli/commands/app/security/check.integration.test.ts b/packages/app/src/cli/commands/app/security/check.integration.test.ts index a19493a3c8b..f73ce0c321f 100644 --- a/packages/app/src/cli/commands/app/security/check.integration.test.ts +++ b/packages/app/src/cli/commands/app/security/check.integration.test.ts @@ -102,8 +102,9 @@ describe('app security check command boundary', () => { await expect(readJson(paths.deterministicFindingsPath)).resolves.toEqual(output.deterministic_findings) await expect(readJson(paths.deterministicFindingsPath)).resolves.toMatchObject({ schema_version: 1, + source: 'deterministic', engine: {name: 'shopify-app-security'}, - findings: expect.any(Array), + checks: expect.any(Array), }) await expect(readJson(paths.agentChecksPath)).resolves.toMatchObject({ schema_version: 1, @@ -136,7 +137,8 @@ describe('app security check command boundary', () => { expect(unstyled(rescan.stdout)).not.toMatch(/discard/i) await expect(readJson(paths.deterministicFindingsPath)).resolves.toMatchObject({ schema_version: 1, - findings: expect.any(Array), + source: 'deterministic', + checks: expect.any(Array), }) await expect(readJson(paths.agentChecksPath)).resolves.toMatchObject({ schema_version: 1, diff --git a/packages/app/src/cli/commands/app/security/check.ts b/packages/app/src/cli/commands/app/security/check.ts index 0219ced71ba..99d151f8b39 100644 --- a/packages/app/src/cli/commands/app/security/check.ts +++ b/packages/app/src/cli/commands/app/security/check.ts @@ -1,3 +1,4 @@ +import {appSecurityBlockingFlag} from './blocking-flag.js' import {appFlags} from '../../../flags.js' import {ignorePatternProblem} from '../../../services/app-security-engine/index.js' import securityCheck from '../../../services/security-check.js' @@ -5,9 +6,6 @@ import {Flags} from '@oclif/core' import BaseCommand from '@shopify/cli-kit/node/base-command' import {globalFlags, jsonFlag} from '@shopify/cli-kit/node/cli' import {AbortError} from '@shopify/cli-kit/node/error' -import type {AppSecurityBlockingLevel} from '../../../services/app-security-api.js' - -const blockingLevels: AppSecurityBlockingLevel[] = ['high', 'medium', 'low', 'none'] export default class SecurityCheck extends BaseCommand { static hidden = true @@ -42,12 +40,7 @@ In interactive terminals, the command offers to copy the coding-agent instructio }, }), ...jsonFlag, - blocking: Flags.string({ - description: 'The minimum finding severity that causes a non-zero exit code.', - options: blockingLevels, - default: 'none', - env: 'SHOPIFY_FLAG_APP_SECURITY_BLOCKING', - }), + ...appSecurityBlockingFlag, yes: Flags.boolean({ description: 'Print coding-agent instructions without prompting.', default: false, @@ -70,7 +63,7 @@ In interactive terminals, the command offers to copy the coding-agent instructio configName: flags.config, json: flags.json, verbose: Boolean(flags.verbose), - blocking: flags.blocking as AppSecurityBlockingLevel, + blocking: flags.blocking, yes: flags.yes, skipInstructions: flags['skip-instructions'], ignorePatterns: flags.ignore ?? [], diff --git a/packages/app/src/cli/commands/app/security/review.test.ts b/packages/app/src/cli/commands/app/security/review.test.ts index 9a9c1becf32..e7f7956980b 100644 --- a/packages/app/src/cli/commands/app/security/review.test.ts +++ b/packages/app/src/cli/commands/app/security/review.test.ts @@ -1,4 +1,5 @@ import SecurityReview from './review.js' +import SecurityCheck from './check.js' import {appFlags} from '../../../flags.js' import securityReview from '../../../services/security-review.js' import {securityReviewJsonOutputSchema} from '../../../services/security-review-json.js' @@ -19,15 +20,52 @@ describe('app security review command', () => { expect(SecurityReview.jsonOutputSchema).toBe(securityReviewJsonOutputSchema) }) - test('reviews the current directory by default', async () => { + test('shares the --blocking flag with check', () => { + expect(SecurityReview.flags.blocking).toBe(SecurityCheck.flags.blocking) + expect(SecurityReview.flags.blocking.options).toEqual(['high', 'medium', 'low', 'none']) + expect(SecurityReview.flags.blocking.env).toBe('SHOPIFY_FLAG_APP_SECURITY_BLOCKING') + }) + + test('reads --check-id repeatedly and from SHOPIFY_FLAG_CHECK_ID', () => { + expect(SecurityReview.flags['check-id'].multiple).toBe(true) + expect(SecurityReview.flags['check-id'].env).toBe('SHOPIFY_FLAG_CHECK_ID') + }) + + test('reviews the current directory by default with no filter and no blocking', async () => { await SecurityReview.run([], import.meta.url) - expect(securityReview).toHaveBeenCalledWith({directory: cwd(), json: false}) + expect(securityReview).toHaveBeenCalledWith({ + directory: cwd(), + json: false, + verbose: false, + checkIds: [], + blocking: 'none', + }) }) - test('forwards --path and --json', async () => { - await SecurityReview.run(['--path', './fixtures/app', '--json'], import.meta.url) + test('forwards --path, --json, --verbose, every --check-id and --blocking', async () => { + await SecurityReview.run( + [ + '--path', + './fixtures/app', + '--json', + '--verbose', + '--check-id', + 'OPEN_REDIRECT', + '--check-id', + 'EOL_API_VERSION', + '--blocking', + 'medium', + ], + import.meta.url, + ) - expect(securityReview).toHaveBeenCalledWith({directory: resolvePath('./fixtures/app'), json: true}) + expect(securityReview).toHaveBeenCalledWith({ + directory: resolvePath('./fixtures/app'), + json: true, + verbose: true, + checkIds: ['OPEN_REDIRECT', 'EOL_API_VERSION'], + blocking: 'medium', + }) }) }) diff --git a/packages/app/src/cli/commands/app/security/review.ts b/packages/app/src/cli/commands/app/security/review.ts index c67f292ff62..2da23ba7b56 100644 --- a/packages/app/src/cli/commands/app/security/review.ts +++ b/packages/app/src/cli/commands/app/security/review.ts @@ -1,17 +1,19 @@ +import {appSecurityBlockingFlag} from './blocking-flag.js' import {appFlags} from '../../../flags.js' import securityReview from '../../../services/security-review.js' import {securityReviewJsonOutputSchema} from '../../../services/security-review-json.js' +import {Flags} from '@oclif/core' import BaseCommand from '@shopify/cli-kit/node/base-command' import {globalFlags, jsonFlag} from '@shopify/cli-kit/node/cli' export default class SecurityReview extends BaseCommand { static hidden = true - static summary = 'Show the stored App Security results.' + static summary = 'Show the combined App Security results.' - static descriptionWithMarkdown = `Prints the deterministic findings (\`.shopify/app-security/deterministic-findings.json\`) and the recorded agent findings (\`.shopify/app-security/agent-findings.json\`), each with its path and age. + static descriptionWithMarkdown = `Combines the deterministic results (\`.shopify/app-security/deterministic-findings.json\`, written by \`shopify app security check\`) with the recorded agent results (\`.shopify/app-security/agent-findings.json\`, written by \`shopify app security record\`) and shows one view of every check: its findings, status and source. -The two files are shown as they are stored. They aren't compared with each other or with the current source files.` +The agent results are optional. Use \`--check-id\` to narrow the review to specific checks, \`--verbose\` for full reasoning, evidence and suppressed findings, and \`--blocking\` to exit with code 1 when a check with findings is at or above a severity.` static get jsonOutputSchema() { return securityReviewJsonOutputSchema @@ -23,11 +25,23 @@ The two files are shown as they are stored. They aren't compared with each other ...globalFlags, path: appFlags.path, ...jsonFlag, + 'check-id': Flags.string({ + description: 'Show only this check. Repeat the flag to show several checks.', + env: 'SHOPIFY_FLAG_CHECK_ID', + multiple: true, + }), + ...appSecurityBlockingFlag, } public async run(): Promise { const {flags} = await this.parse(SecurityReview) - await securityReview({directory: flags.path, json: flags.json}) + await securityReview({ + directory: flags.path, + json: flags.json, + verbose: Boolean(flags.verbose), + checkIds: flags['check-id'] ?? [], + blocking: flags.blocking, + }) } } diff --git a/packages/app/src/cli/commands/app/security/submit.integration.test.ts b/packages/app/src/cli/commands/app/security/submit.integration.test.ts index c933fa0c26f..3ef4a320ec1 100644 --- a/packages/app/src/cli/commands/app/security/submit.integration.test.ts +++ b/packages/app/src/cli/commands/app/security/submit.integration.test.ts @@ -2,7 +2,10 @@ import SecuritySubmit from './submit.js' import {appSecurityArtifactPaths} from '../../../services/app-security-artifacts.js' import {resolveSecuritySubmitClientId} from '../../../services/app-security-submit-target.js' import {clearCachedAppInfo, setCachedAppInfo} from '../../../services/local-storage.js' -import {submissionScanFixture} from '../../../services/app-security-engine/tests/fixtures/submission-scan.js' +import { + agentFindingsDocument, + deterministicFindingsDocument, +} from '../../../services/app-security-engine/tests/fixtures/findings-documents.js' import {testDeveloperPlatformClient, testOrganizationApp} from '../../../models/app/app.test-data.js' import {defaultDeveloperPlatformClient} from '../../../utilities/developer-platform-client.js' import {Config} from '@oclif/core' @@ -78,14 +81,17 @@ function remoteClient() { return {appFromIdentifiers, accountInfo, generateSourceScanUploadUrl, createSourceScan} } -async function writeApp(directory: string) { +async function writeApp(directory: string, {agent = true}: {agent?: boolean} = {}) { const paths = appSecurityArtifactPaths(directory) await writeFile(joinPath(directory, 'shopify.app.toml'), 'client_id = "configured-client-id"\n') await mkdir(paths.artifactDirectory, {recursive: true}) - await writeFile(paths.deterministicFindingsPath, JSON.stringify(submissionScanFixture)) + await writeFile(paths.deterministicFindingsPath, JSON.stringify(deterministicFindingsDocument)) + if (agent) await writeFile(paths.agentFindingsPath, JSON.stringify(agentFindingsDocument)) return paths } +const resultFileNames = ['agent-findings.json', 'deterministic-findings.json'] + async function runCommand(argv: string[]) { let stdout = '' let stderr = '' @@ -141,11 +147,16 @@ describe('app security submit command boundary', () => { }) }) - test.each([false, true])('dry-run uses real root, scan and artifact I/O (json=%s)', async (json) => { + test.each([ + {json: false, agent: true}, + {json: true, agent: true}, + {json: false, agent: false}, + {json: true, agent: false}, + ])('dry-run uses real root, results and artifact I/O (json=$json, agent=$agent)', async ({json, agent}) => { await inTemporaryDirectory(async (directory) => { const client = remoteClient() - const paths = await writeApp(directory) - const scan = await readFile(paths.deterministicFindingsPath) + const paths = await writeApp(directory, {agent}) + const deterministic = await readFile(paths.deterministicFindingsPath) await writeFile(joinPath(directory, 'shopify.app.toml'), 'name = "Unlinked app"\n') const result = await runCommand(['--path', directory, '--dry-run', ...(json ? ['--json'] : [])]) @@ -155,26 +166,59 @@ describe('app security submit command boundary', () => { expect(JSON.parse(result.stdout)).toEqual({ operation: 'submit', dry_run: true, - payload: {path: paths.submissionPath, schema_version: 0}, + payload: {path: paths.submissionPath, schema_version: 2}, }) expect(result.stderr).toBe('') } else { expect(result.stdout).toBe('') - expect(result.stderr).toContain('Prepared the App Security submission without uploading it.') + expect(result.stderr).toContain('Prepared the App Security submission without sending it.') } const submission = JSON.parse(await readFile(paths.submissionPath, 'utf8')) - expect(submission.schemaVersion).toBe(0) + expect(submission.schemaVersion).toBe(2) expect(submission.report).not.toHaveProperty('attestation') expect(submission.report.metadata).toEqual({version_tag: null}) + expect(submission.report.sources.deterministic).toMatchObject({source: 'deterministic'}) + expect(submission.report.sources.agent).toEqual(agent ? expect.objectContaining({source: 'agent'}) : null) expect(resolveSecuritySubmitClientId).not.toHaveBeenCalled() expect(defaultDeveloperPlatformClient).not.toHaveBeenCalled() expect(client.appFromIdentifiers).not.toHaveBeenCalled() expect(fetch).not.toHaveBeenCalled() - await expect(readFile(paths.deterministicFindingsPath)).resolves.toEqual(scan) + await expect(readFile(paths.deterministicFindingsPath)).resolves.toEqual(deterministic) await expect(readdir(joinPath(directory, '.shopify'))).resolves.toEqual(['app-security']) }) }) + test.each([false, true])('missing results are an expected error with a next step (json=%s)', async (json) => { + await inTemporaryDirectory(async (directory) => { + remoteClient() + await writeFile(joinPath(directory, 'shopify.app.toml'), 'client_id = "configured-client-id"\n') + const paths = appSecurityArtifactPaths(directory) + const result = await runCommand(['--path', directory, '--force', ...(json ? ['--json'] : [])]) + + expect(result.exitCode).toBe(1) + if (json) { + expect(JSON.parse(result.stdout)).toEqual({ + operation: 'submit', + error: { + stage: 'preparation', + message: `No App Security results found in ${paths.artifactDirectory}.`, + next_steps: [expect.stringMatching(/^Run shopify app security check --path .* first, then submit\.$/)], + }, + }) + expect(result.stderr).toBe('') + } else { + expect(result.stdout).toBe('') + const messageText = unstyled(result.stderr).replaceAll('│', '').replace(/\s+/g, ' ') + expect(messageText).toContain('No App Security results found in') + expect(messageText).toContain('first, then submit.') + expect(result.stderr).not.toContain('To investigate the issue, examine this stack trace:') + } + expect(defaultDeveloperPlatformClient).not.toHaveBeenCalled() + expect(fetch).not.toHaveBeenCalled() + await expect(readdir(directory)).resolves.toEqual(['shopify.app.toml']) + }) + }) + test.each([ {flags: ['--config', 'staging'], clientId: undefined, configName: 'staging'}, {flags: ['--client-id', 'explicit-client-id'], clientId: 'explicit-client-id', configName: undefined}, @@ -191,10 +235,10 @@ describe('app security submit command boundary', () => { expect(JSON.parse(result.stdout)).toEqual({ operation: 'submit', dry_run: true, - payload: {path: paths.submissionPath, schema_version: 0}, + payload: {path: paths.submissionPath, schema_version: 2}, }) expect(resolveSecuritySubmitClientId).toHaveBeenCalledExactlyOnceWith({directory, clientId, configName}) - await expect(readFile(paths.submissionPath, 'utf8')).resolves.toContain('"schemaVersion": 0') + await expect(readFile(paths.submissionPath, 'utf8')).resolves.toContain('"schemaVersion": 2') expect(defaultDeveloperPlatformClient).not.toHaveBeenCalled() expect(client.appFromIdentifiers).not.toHaveBeenCalled() expect(client.generateSourceScanUploadUrl).not.toHaveBeenCalled() @@ -315,7 +359,7 @@ describe('app security submit command boundary', () => { expect(JSON.parse(result.stdout)).toEqual({ operation: 'submit', dry_run: false, - payload: {path: paths.submissionPath, schema_version: 0}, + payload: {path: paths.submissionPath, schema_version: 2}, submitted_at: submission.report.submitted_at, client_id: 'api-key', }) @@ -392,7 +436,7 @@ describe('app security submit command boundary', () => { expect(defaultDeveloperPlatformClient).not.toHaveBeenCalled() expect(client.appFromIdentifiers).not.toHaveBeenCalled() expect(fetch).not.toHaveBeenCalled() - await expect(readdir(paths.artifactDirectory)).resolves.toEqual(['deterministic-findings.json']) + await expect(readdir(paths.artifactDirectory)).resolves.toEqual(resultFileNames) }) }, ) @@ -431,7 +475,7 @@ describe('app security submit command boundary', () => { expect(client.generateSourceScanUploadUrl).not.toHaveBeenCalled() expect(client.createSourceScan).not.toHaveBeenCalled() expect(fetch).not.toHaveBeenCalled() - await expect(readdir(paths.artifactDirectory)).resolves.toEqual(['deterministic-findings.json']) + await expect(readdir(paths.artifactDirectory)).resolves.toEqual(resultFileNames) await expect(readFile(configPath, 'utf8')).resolves.toBe(configContent) } finally { if (selection === 'cached') clearCachedAppInfo(directory) diff --git a/packages/app/src/cli/commands/app/security/submit.test.ts b/packages/app/src/cli/commands/app/security/submit.test.ts index 9aa6ee94368..3d6f2f82a05 100644 --- a/packages/app/src/cli/commands/app/security/submit.test.ts +++ b/packages/app/src/cli/commands/app/security/submit.test.ts @@ -24,7 +24,7 @@ describe('app security submit command', () => { process.exitCode = previousExitCode }) - test('is hidden and lets the service link only after reading the scan', () => { + test('is hidden and lets the service link only after loading the results', () => { expect(SecuritySubmit.hidden).toBe(true) expect(SecuritySubmit.prototype).toBeInstanceOf(BaseCommand) expect(SecuritySubmit.prototype).not.toBeInstanceOf(AppLinkedCommand) @@ -32,13 +32,19 @@ describe('app security submit command', () => { expect(SecuritySubmit.flags.config).toBe(appFlags.config) expect(SecuritySubmit.flags['client-id']).toBe(appFlags['client-id']) expect(SecuritySubmit.args).not.toHaveProperty('directory') - expect(SecuritySubmit.descriptionWithMarkdown).toContain('`.shopify/app-security/deterministic-findings.json`') - expect(SecuritySubmit.descriptionWithMarkdown).toContain('Generated report fields exclude source code, file paths') - expect(SecuritySubmit.descriptionWithMarkdown).toContain('Optional feedback is included without redaction') - expect(SecuritySubmit.descriptionWithMarkdown).not.toContain( - 'No source code, file paths, snippets, or commit identifiers are sent', + }) + + test('frames the upload as sending the results review shows, plus feedback', () => { + expect(SecuritySubmit.summary).toBe('Send App Security results and feedback to Shopify.') + expect(SecuritySubmit.descriptionWithMarkdown).toBe( + 'Sends the App Security results that `shopify app security review` shows to Shopify, with your optional feedback. ' + + 'Reads `.shopify/app-security/deterministic-findings.json` and, when present, `agent-findings.json`, writes ' + + '`.shopify/app-security/submission.json` for inspection, and asks for confirmation before uploading.\n\n' + + 'The upload excludes source code, file paths, code snippets, evidence, finding messages, agent reasoning and ' + + 'reasons, suppression justifications, and commit identifiers. Feedback is sent without redaction. Optionally ' + + 'use `--version` to identify the app version these results came from. Use `--dry-run` to write and inspect ' + + 'the exact payload without uploading it.', ) - expect(SecuritySubmit.descriptionWithMarkdown).toContain('--version') expect(SecuritySubmit.descriptionWithMarkdown).not.toContain('--source-control-url') }) @@ -48,6 +54,12 @@ describe('app security submit command', () => { ) }) + test('describes feedback as optional and about the results or the tool', () => { + expect(SecuritySubmit.flags.feedback.description).toBe( + 'Optional feedback about these App Security results or this tool. Use - to read from stdin.', + ) + }) + test('does not offer a source-control URL or hash flag', () => { expect(SecuritySubmit.flags).not.toHaveProperty('source-control-url') expect(SecuritySubmit.flags).not.toHaveProperty('source-control-hash') diff --git a/packages/app/src/cli/commands/app/security/submit.ts b/packages/app/src/cli/commands/app/security/submit.ts index 9863ff2a4c0..0ce365090a7 100644 --- a/packages/app/src/cli/commands/app/security/submit.ts +++ b/packages/app/src/cli/commands/app/security/submit.ts @@ -12,11 +12,11 @@ import type {SecuritySubmitResult} from '../../../services/security-submit-resul export default class SecuritySubmit extends BaseCommand { static hidden = true - static summary = 'Submit App Security results to Shopify.' + static summary = 'Send App Security results and feedback to Shopify.' - static descriptionWithMarkdown = `Reads the most recent App Security scan (\`.shopify/app-security/deterministic-findings.json\`), writes a \`.shopify/app-security/submission.json\` file for inspection, asks for confirmation, and uploads the result to Shopify. + static descriptionWithMarkdown = `Sends the App Security results that \`shopify app security review\` shows to Shopify, with your optional feedback. Reads \`.shopify/app-security/deterministic-findings.json\` and, when present, \`agent-findings.json\`, writes \`.shopify/app-security/submission.json\` for inspection, and asks for confirmation before uploading. -Generated report fields exclude source code, file paths, code snippets, evidence, finding messages, and commit identifiers. Optional feedback is included without redaction. Optionally use \`--version\` to identify the app version corresponding to the scanned files. Use \`--dry-run\` to write and inspect the exact payload without uploading it.` +The upload excludes source code, file paths, code snippets, evidence, finding messages, agent reasoning and reasons, suppression justifications, and commit identifiers. Feedback is sent without redaction. Optionally use \`--version\` to identify the app version these results came from. Use \`--dry-run\` to write and inspect the exact payload without uploading it.` static description = this.descriptionWithoutMarkdown() @@ -32,7 +32,7 @@ Generated report fields exclude source code, file paths, code snippets, evidence env: 'SHOPIFY_FLAG_VERSION', }), feedback: Flags.string({ - description: 'Optional feedback about inaccurate or unhelpful App Security results. Use - to read from stdin.', + description: 'Optional feedback about these App Security results or this tool. Use - to read from stdin.', env: 'SHOPIFY_FLAG_APP_SECURITY_FEEDBACK', }), force: Flags.boolean({ diff --git a/packages/app/src/cli/services/app-security-api.test.ts b/packages/app/src/cli/services/app-security-api.test.ts index dcbac0e61ee..8b8632df144 100644 --- a/packages/app/src/cli/services/app-security-api.test.ts +++ b/packages/app/src/cli/services/app-security-api.test.ts @@ -55,17 +55,18 @@ describe('securityExitCode', () => { }) describe('App Security CLI integration', () => { - test('runs the in-tree engine and writes the scan and agent checks', async () => { + test('runs the in-tree engine and writes the deterministic findings and agent checks', async () => { await inTemporaryDirectory(async (directory) => { await createApp(directory) const result = await runSecurity({directory, blocking: 'none'}) - const scan = JSON.parse(await readFile(artifactPath(directory, 'deterministic-findings.json'))) + const deterministicFindings = JSON.parse(await readFile(artifactPath(directory, 'deterministic-findings.json'))) const agentChecks = JSON.parse(await readFile(artifactPath(directory, 'agent-checks.json'))) - expect(scan.schema_version).toBe(1) - expect(scan.engine.name).toBe('shopify-app-security') - expect(result.execution.engine).toEqual(scan.engine) + expect(deterministicFindings.schema_version).toBe(1) + expect(deterministicFindings.source).toBe('deterministic') + expect(deterministicFindings.engine.name).toBe('shopify-app-security') + expect(result.execution.engine).toEqual(deterministicFindings.engine) expect(result.execution.elapsedMilliseconds).toEqual(expect.any(Number)) expect(agentChecks.schema_version).toBe(1) expect(agentChecks.checks.length).toBeGreaterThan(0) @@ -102,7 +103,7 @@ describe('App Security CLI integration', () => { const result = await runSecurity({directory, blocking: 'high'}) - expect(JSON.stringify(result.execution.artifact)).not.toContain(testToken) + expect(JSON.stringify(result.execution.deterministicFindings)).not.toContain(testToken) await expect(readFile(artifactPath(directory, 'deterministic-findings.json'))).resolves.not.toContain(testToken) expect(result.exitCode).toBe(1) }) diff --git a/packages/app/src/cli/services/app-security-api.ts b/packages/app/src/cli/services/app-security-api.ts index 97f7e2aa899..47d2fbb2a82 100644 --- a/packages/app/src/cli/services/app-security-api.ts +++ b/packages/app/src/cli/services/app-security-api.ts @@ -2,6 +2,7 @@ import { AppRootDiscoveryError, findAppRoot, scanApp, + SEVERITY_RANK, type AppSecurityEngineMetadata, type AppSecurityScan, type Severity, @@ -14,15 +15,9 @@ export type AppSecurityBlockingLevel = Severity | 'none' export type AppSecurityExecution = AppSecurityScan & {elapsedMilliseconds: number} -const severityRank: Record = { - high: 3, - medium: 2, - low: 1, -} - export function securityExitCode(execution: AppSecurityExecution, blocking: AppSecurityBlockingLevel): number { if (blocking === 'none') return 0 - const blocks = execution.scan.issues.some((issue) => severityRank[issue.severity] >= severityRank[blocking]) + const blocks = execution.scan.issues.some((issue) => SEVERITY_RANK[issue.severity] >= SEVERITY_RANK[blocking]) return blocks ? 1 : 0 } diff --git a/packages/app/src/cli/services/app-security-artifacts.test.ts b/packages/app/src/cli/services/app-security-artifacts.test.ts index 71abc314563..749361a08e0 100644 --- a/packages/app/src/cli/services/app-security-artifacts.test.ts +++ b/packages/app/src/cli/services/app-security-artifacts.test.ts @@ -1,18 +1,13 @@ import { appSecurityArtifactPaths, cleanAppSecurityArtifacts, - readAgentFindings, - readDeterministicFindings, + readFindingsDocument, writeAgentFindings, writeCheckArtifacts, writeSubmission, } from './app-security-artifacts.js' -import { - scanApp, - SUBMISSION_SCHEMA_VERSION, - type AgentFindingsArtifact, - type AppSecuritySubmission, -} from './app-security-engine/index.js' +import {scanApp, SUBMISSION_SCHEMA_VERSION, type AppSecuritySubmission} from './app-security-engine/index.js' +import {agentFindingsDocument} from './app-security-engine/tests/fixtures/findings-documents.js' import {AbortError} from '@shopify/cli-kit/node/error' import {fileExists, inTemporaryDirectory, mkdir, readFile, writeFile} from '@shopify/cli-kit/node/fs' import {joinPath} from '@shopify/cli-kit/node/path' @@ -24,14 +19,6 @@ const submission = { report: {metadata: {}}, } as AppSecuritySubmission -const agentFindings: AgentFindingsArtifact = { - schema_version: 1, - engine: {name: 'shopify-app-security', version: '1.2.3'}, - recorded_at: '2026-08-24T00:00:00.000Z', - project: {commit: null, dirty: null}, - checks: [], -} - async function scanTestApp(directory: string) { await writeFile(joinPath(directory, 'shopify.app.toml'), 'name = "Test"\nclient_id = "test"\n') return scanApp(directory) @@ -70,126 +57,140 @@ describe('appSecurityArtifactPaths', () => { }) }) -describe('readDeterministicFindings', () => { - test('returns ok with deterministic findings written by a scan', async () => { +describe('readFindingsDocument', () => { + test('returns ok with the deterministic document written by check', async () => { await inTemporaryDirectory(async (directory) => { - const {artifact, agentChecks} = await scanTestApp(directory) - const {deterministicFindingsPath} = await writeCheckArtifacts(directory, {artifact, agentChecks}) + const {deterministicFindings, agentChecks} = await scanTestApp(directory) + const {deterministicFindingsPath} = await writeCheckArtifacts(directory, {deterministicFindings, agentChecks}) - await expect(readDeterministicFindings(deterministicFindingsPath)).resolves.toEqual({ + await expect(readFindingsDocument(deterministicFindingsPath, 'deterministic')).resolves.toEqual({ status: 'ok', - value: artifact, + value: deterministicFindings, }) }) }) - test('returns missing when the file does not exist', async () => { + test('returns ok with the agent document written by writeAgentFindings', async () => { await inTemporaryDirectory(async (directory) => { - await expect(readDeterministicFindings(joinPath(directory, 'deterministic-findings.json'))).resolves.toEqual({ - status: 'missing', - }) + const path = await writeAgentFindings(directory, agentFindingsDocument) + + expect(path).toBe(appSecurityArtifactPaths(directory).agentFindingsPath) + await expect(readFindingsDocument(path, 'agent')).resolves.toEqual({status: 'ok', value: agentFindingsDocument}) }) }) - test('returns invalid for malformed JSON', async () => { + test('narrows the document to the expected source so callers need no further checks', async () => { await inTemporaryDirectory(async (directory) => { - const path = joinPath(directory, 'deterministic-findings.json') - await writeFile(path, '{invalid') + const {deterministicFindings, agentChecks} = await scanTestApp(directory) + const {deterministicFindingsPath} = await writeCheckArtifacts(directory, {deterministicFindings, agentChecks}) + const agentFindingsPath = await writeAgentFindings(directory, agentFindingsDocument) - await expect(readDeterministicFindings(path)).resolves.toEqual({ - status: 'invalid', - message: expect.stringContaining('Could not parse JSON'), - }) + const deterministic = await readFindingsDocument(deterministicFindingsPath, 'deterministic') + const agent = await readFindingsDocument(agentFindingsPath, 'agent') + + // Source-specific fields compile without narrowing on `source`. + expect(deterministic.status === 'ok' && deterministic.value.detection).toEqual(deterministicFindings.detection) + expect(agent.status === 'ok' && agent.value.engine).toEqual(agentFindingsDocument.engine) }) }) - test('returns invalid with every schema error for an unrecognized artifact', async () => { + test('returns invalid when the document comes from the other source', async () => { await inTemporaryDirectory(async (directory) => { - const path = joinPath(directory, 'deterministic-findings.json') - await writeFile(path, '{"schema_version":3}') + const path = await writeAgentFindings(directory, agentFindingsDocument) - await expect(readDeterministicFindings(path)).resolves.toEqual({ + await expect(readFindingsDocument(path, 'deterministic')).resolves.toEqual({ status: 'invalid', - message: 'unsupported schema_version: 3 (expected 1); findings must be an array', + errors: ['source is "agent", but this file must hold "deterministic" findings.'], }) }) }) - test('returns invalid for an unreadable path', async () => { + test('returns missing when the file does not exist', async () => { await inTemporaryDirectory(async (directory) => { const path = joinPath(directory, 'deterministic-findings.json') - await mkdir(path) - await expect(readDeterministicFindings(path)).resolves.toEqual({ - status: 'invalid', - message: expect.stringContaining('Could not read the file'), - }) + await expect(readFindingsDocument(path, 'deterministic')).resolves.toEqual({status: 'missing'}) }) }) - test('rejects a file larger than 5 MB before parsing', async () => { + test('returns invalid for malformed JSON', async () => { await inTemporaryDirectory(async (directory) => { - const path = joinPath(directory, 'deterministic-findings.json') - await writeFile(path, 'x'.repeat(5_000_001)) + const path = joinPath(directory, 'agent-findings.json') + await writeFile(path, '{invalid') - await expect(readDeterministicFindings(path)).resolves.toEqual({ + await expect(readFindingsDocument(path, 'agent')).resolves.toEqual({ status: 'invalid', - message: 'The file is larger than 5 MB.', + errors: [expect.stringContaining('Could not parse JSON')], }) }) }) -}) -describe('readAgentFindings', () => { - test('returns ok with agent findings written by writeAgentFindings', async () => { + test('returns invalid with every translation error, without the path', async () => { await inTemporaryDirectory(async (directory) => { - const path = await writeAgentFindings(directory, agentFindings) - - expect(path).toBe(appSecurityArtifactPaths(directory).agentFindingsPath) - await expect(readAgentFindings(path)).resolves.toEqual({status: 'ok', value: agentFindings}) + const path = joinPath(directory, 'agent-findings.json') + const document = {...agentFindingsDocument, engine: {name: 'other'}, checks: [{id: 'X', status: 'skipped'}]} + await writeFile(path, JSON.stringify(document)) + + const result = await readFindingsDocument(path, 'agent') + + expect(result.status).toBe('invalid') + if (result.status !== 'invalid') return + expect(result.errors).toEqual( + expect.arrayContaining([ + expect.stringMatching(/^engine\.name: /), + expect.stringMatching(/^engine\.version: /), + expect.stringMatching(/^checks\[0\]\.status: /), + expect.stringMatching(/^checks\[0\]\.snapshot: /), + ]), + ) + for (const error of result.errors) expect(error).not.toContain(path) }) }) - test('returns missing when the file does not exist', async () => { + test('returns invalid for an unsupported schema version', async () => { await inTemporaryDirectory(async (directory) => { - await expect(readAgentFindings(joinPath(directory, 'agent-findings.json'))).resolves.toEqual({ - status: 'missing', + const path = joinPath(directory, 'deterministic-findings.json') + await writeFile(path, '{"schema_version":3}') + + await expect(readFindingsDocument(path, 'deterministic')).resolves.toEqual({ + status: 'invalid', + errors: ['unsupported schema_version: 3 (expected 1)'], }) }) }) - test('returns invalid for malformed JSON', async () => { + test('returns invalid for JSON that is not an object', async () => { await inTemporaryDirectory(async (directory) => { const path = joinPath(directory, 'agent-findings.json') - await writeFile(path, '{invalid') + await writeFile(path, '[]') - await expect(readAgentFindings(path)).resolves.toEqual({ + await expect(readFindingsDocument(path, 'agent')).resolves.toEqual({ status: 'invalid', - message: expect.stringContaining('Could not parse JSON'), + errors: ['expected a JSON object, received array'], }) }) }) - test('returns invalid for JSON that is not an object', async () => { + test('returns invalid for an unreadable path', async () => { await inTemporaryDirectory(async (directory) => { - const path = joinPath(directory, 'agent-findings.json') - await writeFile(path, '[]') + const path = joinPath(directory, 'deterministic-findings.json') + await mkdir(path) - await expect(readAgentFindings(path)).resolves.toEqual({ + await expect(readFindingsDocument(path, 'deterministic')).resolves.toEqual({ status: 'invalid', - message: 'agent findings must be a JSON object', + errors: [expect.stringContaining('Could not read the file')], }) }) }) - test('returns invalid for an unsupported schema version', async () => { + test('rejects a file larger than 5 MB before parsing', async () => { await inTemporaryDirectory(async (directory) => { - const path = joinPath(directory, 'agent-findings.json') - await writeFile(path, '{"schema_version":2,"checks":[]}') + const path = joinPath(directory, 'deterministic-findings.json') + await writeFile(path, 'x'.repeat(5_000_001)) - await expect(readAgentFindings(path)).resolves.toEqual({ + await expect(readFindingsDocument(path, 'deterministic')).resolves.toEqual({ status: 'invalid', - message: 'unsupported schema_version: 2 (expected 1)', + errors: ['The file is larger than 5 MB.'], }) }) }) @@ -198,31 +199,31 @@ describe('readAgentFindings', () => { describe('writeCheckArtifacts', () => { test('writes deterministic-findings.json and agent-checks.json', async () => { await inTemporaryDirectory(async (directory) => { - const {artifact, agentChecks} = await scanTestApp(directory) + const {deterministicFindings, agentChecks} = await scanTestApp(directory) const paths = appSecurityArtifactPaths(directory) - await expect(writeCheckArtifacts(directory, {artifact, agentChecks})).resolves.toEqual({ + await expect(writeCheckArtifacts(directory, {deterministicFindings, agentChecks})).resolves.toEqual({ deterministicFindingsPath: paths.deterministicFindingsPath, agentChecksPath: paths.agentChecksPath, }) - expect(JSON.parse(await readFile(paths.deterministicFindingsPath))).toEqual(artifact) + expect(JSON.parse(await readFile(paths.deterministicFindingsPath))).toEqual(deterministicFindings) expect(JSON.parse(await readFile(paths.agentChecksPath))).toEqual(agentChecks) }) }) test('overwrites earlier check artifacts and leaves agent-findings.json untouched', async () => { await inTemporaryDirectory(async (directory) => { - const {artifact, agentChecks} = await scanTestApp(directory) + const {deterministicFindings, agentChecks} = await scanTestApp(directory) const paths = appSecurityArtifactPaths(directory) await mkdir(paths.artifactDirectory) await writeFile(paths.deterministicFindingsPath, '{"stale":true}') await writeFile(paths.agentChecksPath, '{"stale":true}') await writeFile(paths.agentFindingsPath, '{"recorded":"by the agent"}') - await writeCheckArtifacts(directory, {artifact, agentChecks}) + await writeCheckArtifacts(directory, {deterministicFindings, agentChecks}) - expect(JSON.parse(await readFile(paths.deterministicFindingsPath))).toEqual(artifact) + expect(JSON.parse(await readFile(paths.deterministicFindingsPath))).toEqual(deterministicFindings) expect(JSON.parse(await readFile(paths.agentChecksPath))).toEqual(agentChecks) await expect(readFile(paths.agentFindingsPath)).resolves.toBe('{"recorded":"by the agent"}') }) @@ -230,11 +231,11 @@ describe('writeCheckArtifacts', () => { test('refuses to write through a .shopify symlink that targets outside the app', async () => { await inTemporaryDirectory(async (directory) => { - const {artifact, agentChecks} = await scanTestApp(directory) + const {deterministicFindings, agentChecks} = await scanTestApp(directory) await inTemporaryDirectory(async (externalDirectory) => { await symlink(externalDirectory, joinPath(directory, '.shopify'), 'dir') - await expect(writeCheckArtifacts(directory, {artifact, agentChecks})).rejects.toMatchObject({ + await expect(writeCheckArtifacts(directory, {deterministicFindings, agentChecks})).rejects.toMatchObject({ constructor: AbortError, message: expect.stringMatching(/outside the app/), }) @@ -293,16 +294,16 @@ describe('cleanAppSecurityArtifacts', () => { test('refuses a .shopify symlink that targets outside the app without deleting its files', async () => { await inTemporaryDirectory(async (directory) => { await inTemporaryDirectory(async (externalDirectory) => { - const externalScanPath = joinPath(externalDirectory, 'app-security', 'deterministic-findings.json') + const externalFindingsPath = joinPath(externalDirectory, 'app-security', 'deterministic-findings.json') await mkdir(joinPath(externalDirectory, 'app-security')) - await writeFile(externalScanPath, '{}') + await writeFile(externalFindingsPath, '{}') await symlink(externalDirectory, joinPath(directory, '.shopify'), 'dir') await expect(cleanAppSecurityArtifacts(directory)).rejects.toMatchObject({ constructor: AbortError, message: expect.stringMatching(/symbolic link/), }) - await expect(fileExists(externalScanPath)).resolves.toBe(true) + await expect(fileExists(externalFindingsPath)).resolves.toBe(true) }) }) }) diff --git a/packages/app/src/cli/services/app-security-artifacts.ts b/packages/app/src/cli/services/app-security-artifacts.ts index ea32267a156..151caac61b9 100644 --- a/packages/app/src/cli/services/app-security-artifacts.ts +++ b/packages/app/src/cli/services/app-security-artifacts.ts @@ -1,9 +1,10 @@ import { - AGENT_FINDINGS_SCHEMA_VERSION, - parseDeterministicFindings, + translateFindingsDocument, type AgentChecks, - type AgentFindingsArtifact, + type AgentFindingsDocument, type DeterministicFindingsDocument, + type FindingsDocument, + type FindingsSource, } from './app-security-engine/index.js' import {fileExists, fileSize, readFile} from '@shopify/cli-kit/node/fs' import {AbortError} from '@shopify/cli-kit/node/error' @@ -24,10 +25,11 @@ export interface AppSecurityArtifactPaths { legacyPaths: string[] } +/** An invalid artifact carries every problem the reader found, without the file's path: callers show that. */ export type ReadArtifactResult = | {status: 'ok'; value: T} | {status: 'missing'} - | {status: 'invalid'; message: string} + | {status: 'invalid'; errors: string[]} export function appSecurityArtifactPaths(appRoot: string): AppSecurityArtifactPaths { const artifactDirectory = joinPath(appRoot, '.shopify', 'app-security') @@ -41,21 +43,27 @@ export function appSecurityArtifactPaths(appRoot: string): AppSecurityArtifactPa } } +export type CheckArtifactPaths = Pick + +/** Writes the two files `check` produces: deterministic-findings.json and agent-checks.json. */ export async function writeCheckArtifacts( appRoot: string, - {artifact, agentChecks}: {artifact: DeterministicFindingsDocument; agentChecks: AgentChecks}, -): Promise> { + { + deterministicFindings, + agentChecks, + }: {deterministicFindings: DeterministicFindingsDocument; agentChecks: AgentChecks}, +): Promise { const paths = appSecurityArtifactPaths(appRoot) await ensureArtifactDirectory(appRoot, paths.artifactDirectory) - await writeAtomicArtifact(paths.deterministicFindingsPath, encodeArtifact(artifact)) + await writeAtomicArtifact(paths.deterministicFindingsPath, encodeArtifact(deterministicFindings)) await writeAtomicArtifact(paths.agentChecksPath, encodeArtifact(agentChecks)) return {deterministicFindingsPath: paths.deterministicFindingsPath, agentChecksPath: paths.agentChecksPath} } -export async function writeAgentFindings(appRoot: string, artifact: AgentFindingsArtifact): Promise { +export async function writeAgentFindings(appRoot: string, document: AgentFindingsDocument): Promise { const paths = appSecurityArtifactPaths(appRoot) await ensureArtifactDirectory(appRoot, paths.artifactDirectory) - await writeAtomicArtifact(paths.agentFindingsPath, encodeArtifact(artifact)) + await writeAtomicArtifact(paths.agentFindingsPath, encodeArtifact(document)) return paths.agentFindingsPath } @@ -84,34 +92,37 @@ export async function cleanAppSecurityArtifacts(appRoot: string): Promise removed[index]) } -export async function readDeterministicFindings( - path: string, -): Promise> { - const result = await readJsonArtifact(path) - if (result.status !== 'ok') return result - - const parsed = parseDeterministicFindings(result.value) - if (!parsed.ok) return {status: 'invalid', message: parsed.errors.join('; ')} - return {status: 'ok', value: parsed.artifact} -} +/** The findings document type for one source: DeterministicFindingsDocument or AgentFindingsDocument. */ +export type FindingsDocumentFor = Extract -/** Loosely identifies a stored agent-findings.json. Its contents are informational, so only the schema version is checked. */ -export async function readAgentFindings(path: string): Promise> { +/** + * Reads and translates a stored findings document. `expectedSource` is the source the file at `path` must + * hold, so a document copied into the wrong file is reported as invalid instead of being displayed as the + * other source's results. The result is typed for that source, so callers never re-narrow on `source`. + */ +export async function readFindingsDocument( + path: string, + expectedSource: TSource, +): Promise>> { const result = await readJsonArtifact(path) if (result.status !== 'ok') return result - const value = result.value - if (value === null || typeof value !== 'object' || Array.isArray(value)) { - return {status: 'invalid', message: 'agent findings must be a JSON object'} - } - const schemaVersion = (value as {schema_version?: unknown}).schema_version - if (schemaVersion !== AGENT_FINDINGS_SCHEMA_VERSION) { + const translated = translateFindingsDocument(result.value) + if (!translated.ok) return {status: 'invalid', errors: translated.errors} + if (!hasSource(translated.document, expectedSource)) { return { status: 'invalid', - message: `unsupported schema_version: ${String(schemaVersion)} (expected ${AGENT_FINDINGS_SCHEMA_VERSION})`, + errors: [`source is "${translated.document.source}", but this file must hold "${expectedSource}" findings.`], } } - return {status: 'ok', value: value as AgentFindingsArtifact} + return {status: 'ok', value: translated.document} +} + +function hasSource( + document: FindingsDocument, + source: TSource, +): document is FindingsDocumentFor { + return document.source === source } async function readJsonArtifact(path: string): Promise> { @@ -120,13 +131,13 @@ async function readJsonArtifact(path: string): Promise MAX_ARTIFACT_FILE_SIZE_BYTES) { - return {status: 'invalid', message: 'The file is larger than 5 MB.'} + return {status: 'invalid', errors: ['The file is larger than 5 MB.']} } content = await readFile(path) // Filesystem failures are returned for command-layer rendering. // eslint-disable-next-line no-catch-all/no-catch-all } catch (error) { - return {status: 'invalid', message: `Could not read the file: ${errorMessage(error)}`} + return {status: 'invalid', errors: [`Could not read the file: ${errorMessage(error)}`]} } try { @@ -134,7 +145,7 @@ async function readJsonArtifact(path: string): Promise { ]) expect(commands.record.args).toEqual(['app', 'security', 'record', {flag: '--path', value: '/tmp/app'}]) expect(commands.review.args).toEqual(['app', 'security', 'review', {flag: '--path', value: '/tmp/app'}]) + expect(commands.submit.args).toEqual(['app', 'security', 'submit', {flag: '--path', value: '/tmp/app'}]) expect(commands.clean.args).toEqual(['app', 'security', 'clean', {flag: '--path', value: '/tmp/app'}]) }) @@ -182,6 +183,7 @@ describe('resolveAppSecurityCommands', () => { ]) expect(commands.record.args).toEqual(['app', 'security', 'record', {flag: '--path', value: '/tmp/app'}]) expect(commands.review.args).toEqual(['app', 'security', 'review', {flag: '--path', value: '/tmp/app'}]) + expect(commands.submit.args).toEqual(['app', 'security', 'submit', {flag: '--path', value: '/tmp/app'}]) expect(commands.clean.args).toEqual(['app', 'security', 'clean', {flag: '--path', value: '/tmp/app'}]) }) @@ -207,8 +209,25 @@ describe('resolveAppSecurityCommands', () => { "Get-Content -Raw | shopify app security record --path '/tmp/app'", ) expect(formatAppSecurityCommand(commands.review, 'posix')).toBe("shopify app security review --path '/tmp/app'") + expect(formatAppSecurityCommand(commands.submit, 'posix')).toBe("shopify app security submit --path '/tmp/app'") expect(formatAppSecurityCommand(commands.clean, 'posix')).toBe("shopify app security clean --path '/tmp/app'") }) + + test('leaves the submit subcommand unquoted in each shell', () => { + const commands = resolveAppSecurityCommands(WINDOWS_APP_ROOT) + + for (const shell of ['posix', 'cmd', 'powershell'] as const) { + expect(splitQuotedCommand(formatAppSecurityCommand(commands.submit, shell), shell)).toEqual([ + 'shopify', + 'app', + 'security', + 'submit', + '--path', + WINDOWS_APP_ROOT, + ]) + expect(formatAppSecurityCommand(commands.submit, shell)).toMatch(/ submit --path /) + } + }) }) describe('formatAppSecurityCommand', () => { diff --git a/packages/app/src/cli/services/app-security-commands.ts b/packages/app/src/cli/services/app-security-commands.ts index 3e020111a22..4ec99016114 100644 --- a/packages/app/src/cli/services/app-security-commands.ts +++ b/packages/app/src/cli/services/app-security-commands.ts @@ -16,6 +16,7 @@ export interface AppSecurityCommands { scan: AppSecurityCommand record: AppSecurityCommand review: AppSecurityCommand + submit: AppSecurityCommand clean: AppSecurityCommand } @@ -46,6 +47,7 @@ export function resolveAppSecurityCommands( }, record: {command, args: subcommandArgs('record'), stdinPlaceholder: ''}, review: {command, args: subcommandArgs('review')}, + submit: {command, args: subcommandArgs('submit')}, clean: {command, args: subcommandArgs('clean')}, } } 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 5fb2852ae7f..0fb23f60f6b 100644 --- a/packages/app/src/cli/services/app-security-engine/INSTRUCTIONS.md +++ b/packages/app/src/cli/services/app-security-engine/INSTRUCTIONS.md @@ -101,16 +101,16 @@ When the document is accepted, `record` replaces {{AGENT_FINDINGS_PATH}} with it ### 6. Review, explain, and help fix -Show the recorded results: +Show the combined results: ```bash {{REVIEW_COMMAND}} ``` -It prints {{DETERMINISTIC_FINDINGS_PATH}} and {{AGENT_FINDINGS_PATH}} with their paths and ages. Report: +It combines {{DETERMINISTIC_FINDINGS_PATH}} with {{AGENT_FINDINGS_PATH}} into one result per check. Each check with findings gets its own box, most severe first, listing every finding with its file, line and source (deterministic or agent). A summary box follows with the checks with findings, the other checks (passed, not applicable or unresolved), deterministic coverage, the results files with their ages and versions, and next steps. Add `--json` for the machine-readable combined view, `--check-id ` (repeatable) to narrow the review to specific checks, and `--verbose` for full reasoning, evidence and suppressed findings. Report: - CLI and ruleset versions; -- deterministic and agent finding counts, grouped by severity; +- finding counts per check, grouped by severity and source; - each verified finding's impact and concise file/line evidence; - skipped or incomplete coverage and unresolved checks; - prioritized remediation steps. @@ -125,11 +125,11 @@ The results describe the source as it was when they were produced. Once source f {{SCAN_COMMAND}} ``` -It replaces the scan results and agent checks and never touches the recorded agent findings. Optionally repeat steps 2–5 to refresh the agent review. `record` replaces {{AGENT_FINDINGS_PATH}} wholesale. +It replaces the scan results and agent checks and never touches the recorded agent findings. Optionally repeat steps 2–5 to refresh the agent review. Until the agent records again, `review` shows both results for checks where the agent's result would otherwise take precedence, because the agent's result is now older than the deterministic one. `record` replaces {{AGENT_FINDINGS_PATH}} wholesale. ### 8. Submit only when explicitly authorized (optional) -Only after reviewing the results, submit only when the user explicitly requests or authorizes an upload to Shopify. Do not upload automatically; local results do not require submission. Submission reads the existing {{DETERMINISTIC_FINDINGS_PATH}}; agent findings aren't uploaded. +Only after reviewing the results, submit only when the user explicitly requests or authorizes an upload to Shopify. Do not upload automatically; local results do not require submission. Submission sends the results `review` shows: it reads the existing {{DETERMINISTIC_FINDINGS_PATH}} and, when present, {{AGENT_FINDINGS_PATH}}. The upload excludes source code, file paths, code snippets, evidence, finding messages, agent reasoning and reasons, suppression justifications, and commit identifiers. Run from the same app root used above (or pass `--path ` to each submit command). Inspect a dry run first: diff --git a/packages/app/src/cli/services/app-security-engine/checks/APP_PROXY_LIQUID_INJECTION.md b/packages/app/src/cli/services/app-security-engine/checks/APP_PROXY_LIQUID_INJECTION.md index 65ad5321bda..73a7de8d785 100644 --- a/packages/app/src/cli/services/app-security-engine/checks/APP_PROXY_LIQUID_INJECTION.md +++ b/packages/app/src/cli/services/app-security-engine/checks/APP_PROXY_LIQUID_INJECTION.md @@ -2,6 +2,7 @@ id: APP_PROXY_LIQUID_INJECTION version: 2 severity: high +precedence: prefer-agent --- # App Proxy Liquid Injection diff --git a/packages/app/src/cli/services/app-security-engine/checks/COMMITTED_SECRET.md b/packages/app/src/cli/services/app-security-engine/checks/COMMITTED_SECRET.md index 4c9af737b67..e0354ad16fd 100644 --- a/packages/app/src/cli/services/app-security-engine/checks/COMMITTED_SECRET.md +++ b/packages/app/src/cli/services/app-security-engine/checks/COMMITTED_SECRET.md @@ -2,6 +2,7 @@ id: COMMITTED_SECRET version: 3 severity: high +precedence: union --- # Committed Secret diff --git a/packages/app/src/cli/services/app-security-engine/checks/CREDENTIAL_BROWSER_LEAKAGE.md b/packages/app/src/cli/services/app-security-engine/checks/CREDENTIAL_BROWSER_LEAKAGE.md index df7fca1af7e..97f70c42599 100644 --- a/packages/app/src/cli/services/app-security-engine/checks/CREDENTIAL_BROWSER_LEAKAGE.md +++ b/packages/app/src/cli/services/app-security-engine/checks/CREDENTIAL_BROWSER_LEAKAGE.md @@ -2,6 +2,7 @@ id: CREDENTIAL_BROWSER_LEAKAGE version: 1 severity: high +precedence: prefer-agent --- # Credential Browser Leakage diff --git a/packages/app/src/cli/services/app-security-engine/checks/CREDENTIAL_LOG_LEAKAGE.md b/packages/app/src/cli/services/app-security-engine/checks/CREDENTIAL_LOG_LEAKAGE.md index eed527e5385..ff7b9bc0cef 100644 --- a/packages/app/src/cli/services/app-security-engine/checks/CREDENTIAL_LOG_LEAKAGE.md +++ b/packages/app/src/cli/services/app-security-engine/checks/CREDENTIAL_LOG_LEAKAGE.md @@ -2,6 +2,7 @@ id: CREDENTIAL_LOG_LEAKAGE version: 1 severity: high +precedence: prefer-agent --- # Credential Log Leakage diff --git a/packages/app/src/cli/services/app-security-engine/checks/DEPRECATED_SCRIPT_TAG_SCOPE.md b/packages/app/src/cli/services/app-security-engine/checks/DEPRECATED_SCRIPT_TAG_SCOPE.md index 7a0c55f5aa7..7f1e7d8d687 100644 --- a/packages/app/src/cli/services/app-security-engine/checks/DEPRECATED_SCRIPT_TAG_SCOPE.md +++ b/packages/app/src/cli/services/app-security-engine/checks/DEPRECATED_SCRIPT_TAG_SCOPE.md @@ -2,6 +2,7 @@ id: DEPRECATED_SCRIPT_TAG_SCOPE version: 1 severity: medium +precedence: prefer-agent --- # Deprecated Script Tag Scope diff --git a/packages/app/src/cli/services/app-security-engine/checks/EOL_API_VERSION.md b/packages/app/src/cli/services/app-security-engine/checks/EOL_API_VERSION.md index 770eb4838f3..8059404eecc 100644 --- a/packages/app/src/cli/services/app-security-engine/checks/EOL_API_VERSION.md +++ b/packages/app/src/cli/services/app-security-engine/checks/EOL_API_VERSION.md @@ -2,6 +2,7 @@ id: EOL_API_VERSION version: 1 severity: low +precedence: prefer-agent --- # Eol Api Version diff --git a/packages/app/src/cli/services/app-security-engine/checks/EXPIRING_OFFLINE_TOKEN.md b/packages/app/src/cli/services/app-security-engine/checks/EXPIRING_OFFLINE_TOKEN.md index 3582b7135c0..012aae0ee18 100644 --- a/packages/app/src/cli/services/app-security-engine/checks/EXPIRING_OFFLINE_TOKEN.md +++ b/packages/app/src/cli/services/app-security-engine/checks/EXPIRING_OFFLINE_TOKEN.md @@ -2,6 +2,7 @@ id: EXPIRING_OFFLINE_TOKEN version: 1 severity: medium +precedence: prefer-agent --- # Expiring Offline Token diff --git a/packages/app/src/cli/services/app-security-engine/checks/INSECURE_WEBHOOK_URL.md b/packages/app/src/cli/services/app-security-engine/checks/INSECURE_WEBHOOK_URL.md index b561218668a..886ef720360 100644 --- a/packages/app/src/cli/services/app-security-engine/checks/INSECURE_WEBHOOK_URL.md +++ b/packages/app/src/cli/services/app-security-engine/checks/INSECURE_WEBHOOK_URL.md @@ -2,6 +2,7 @@ id: INSECURE_WEBHOOK_URL version: 2 severity: high +precedence: prefer-agent --- # Insecure Configured Callback Url diff --git a/packages/app/src/cli/services/app-security-engine/checks/LIQUID_UNSAFE_RENDER.md b/packages/app/src/cli/services/app-security-engine/checks/LIQUID_UNSAFE_RENDER.md index 8911a8ee8ad..a322754caea 100644 --- a/packages/app/src/cli/services/app-security-engine/checks/LIQUID_UNSAFE_RENDER.md +++ b/packages/app/src/cli/services/app-security-engine/checks/LIQUID_UNSAFE_RENDER.md @@ -2,6 +2,7 @@ id: LIQUID_UNSAFE_RENDER version: 1 severity: medium +precedence: union --- # Liquid Unsafe Render diff --git a/packages/app/src/cli/services/app-security-engine/checks/MISSING_COMPLIANCE_WEBHOOKS.md b/packages/app/src/cli/services/app-security-engine/checks/MISSING_COMPLIANCE_WEBHOOKS.md index c88fe3faa64..d538e7d8a1b 100644 --- a/packages/app/src/cli/services/app-security-engine/checks/MISSING_COMPLIANCE_WEBHOOKS.md +++ b/packages/app/src/cli/services/app-security-engine/checks/MISSING_COMPLIANCE_WEBHOOKS.md @@ -2,6 +2,7 @@ id: MISSING_COMPLIANCE_WEBHOOKS version: 1 severity: medium +precedence: prefer-agent --- # Missing Compliance Webhooks diff --git a/packages/app/src/cli/services/app-security-engine/checks/REQUEST_CONTROLLED_ADMIN_CONTEXT.md b/packages/app/src/cli/services/app-security-engine/checks/REQUEST_CONTROLLED_ADMIN_CONTEXT.md index 8ed48d721e7..50e9fd105ce 100644 --- a/packages/app/src/cli/services/app-security-engine/checks/REQUEST_CONTROLLED_ADMIN_CONTEXT.md +++ b/packages/app/src/cli/services/app-security-engine/checks/REQUEST_CONTROLLED_ADMIN_CONTEXT.md @@ -2,6 +2,7 @@ id: REQUEST_CONTROLLED_ADMIN_CONTEXT version: 3 severity: high +precedence: prefer-agent --- # Request Controlled Admin Context 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 07d931a2194..074c8a1aefc 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 @@ -2,6 +2,7 @@ id: STATIC_FRAME_ANCESTORS version: 1 severity: high +precedence: prefer-agent --- # Static Frame Ancestors diff --git a/packages/app/src/cli/services/app-security-engine/checks/UNAUTHENTICATED_ENDPOINT.md b/packages/app/src/cli/services/app-security-engine/checks/UNAUTHENTICATED_ENDPOINT.md index a3b49b50027..d23a4da37e8 100644 --- a/packages/app/src/cli/services/app-security-engine/checks/UNAUTHENTICATED_ENDPOINT.md +++ b/packages/app/src/cli/services/app-security-engine/checks/UNAUTHENTICATED_ENDPOINT.md @@ -2,6 +2,7 @@ id: UNAUTHENTICATED_ENDPOINT version: 2 severity: high +precedence: union --- # Unauthenticated Endpoint diff --git a/packages/app/src/cli/services/app-security-engine/checks/UNSAFE_INNERHTML.md b/packages/app/src/cli/services/app-security-engine/checks/UNSAFE_INNERHTML.md index ec0d879715f..e60999d64d6 100644 --- a/packages/app/src/cli/services/app-security-engine/checks/UNSAFE_INNERHTML.md +++ b/packages/app/src/cli/services/app-security-engine/checks/UNSAFE_INNERHTML.md @@ -2,6 +2,7 @@ id: UNSAFE_INNERHTML version: 2 severity: high +precedence: union --- Find cases where user-controlled data is written to the DOM without 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 dfbc2424b4e..ef6ebecfeee 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 @@ -5,41 +5,41 @@ // prettier-ignore export const EMBEDDED_CHECK_SOURCES: ReadonlyArray = [ "---\nid: ACTIVE_UPLOADS_AND_PRIVILEGED_PREVIEWS\nversion: 1\nseverity: high\n---\n\n# Active Uploads And Privileged Previews\n\nFind cases where merchant-, customer-, webhook-, or external-service-supplied\nfiles become active content in a privileged origin. Trace uploads, imports,\npreviews, and generated assets from ingestion through storage and final render.\n\nThe risk is not the upload alone. The risk is an untrusted-upload-to-active-render\npath: SVG, HTML, XML, PDF, blob/data URL, or another active format is accepted and\nlater rendered in a storefront, embedded admin, customer-account, theme-editor,\nor operator/admin context where it can execute or leak protected data.\n\n## What to look for\n\n1. **Find upload and import entry points.** Search for file uploads, import jobs,\n webhook attachments, remote fetches, document parsers, blob/data URL handling,\n and generated preview endpoints.\n\n2. **Trace file metadata and validation.** Check size limits, extension checks,\n declared MIME type, magic-byte/file-signature verification, filename handling,\n generated storage names, antivirus/sanitization, and any image/PDF re-encoding.\n\n3. **Inspect storage and serving boundaries.** Determine whether the object is\n stored on a non-executable origin, served with explicit `Content-Type` and\n `Content-Disposition`, and prevented from inheriting privileged cookies or\n browser authority.\n\n4. **Follow every final renderer.** Check storefront/theme renderers, embedded\n admin previews, customer-account views, email/PDF previews, admin/operator\n tools, iframe/srcdoc/blob/data URL renderers, and any browser code that inserts\n the uploaded content into the DOM.\n\n5. **Check sandboxing and isolation.** Verify iframes, preview origins, CSP,\n download headers, SVG sanitization, PDF handling, and re-encoding before\n deciding the content is safe.\n\n## What to report\n\nReport a finding only for a complete untrusted-upload-to-active-render path where\nthe uploaded or imported object is actually rendered or served into a privileged\nexecutable context. Show:\n- who controls the uploaded/imported content;\n- which validation or isolation boundary is missing;\n- where the content becomes active or executable;\n- which privileged origin or user is affected; and\n- file/line evidence for both the ingest path and the renderer/serving path.\n\nExample:\n\n```json\n{\n \"file\": \"app/controllers/previews_controller.rb\",\n \"line\": 28,\n \"message\": \"Uploaded SVG is rendered inline in the admin preview without sanitization or origin isolation\",\n \"evidence\": [\n { \"file\": \"app/controllers/uploads_controller.rb\", \"line\": 14, \"quote\": \"params[:file]\" },\n { \"file\": \"app/controllers/previews_controller.rb\", \"line\": 28, \"quote\": \"render inline: blob.download\" }\n ],\n \"confidence\": \"high\",\n \"reasoning\": \"The merchant-controlled SVG is stored without re-encoding and later rendered inline in the embedded admin origin, so script-capable SVG content can execute with merchant authority.\"\n}\n```\n\nDo not report:\n- files that are forced to download and never rendered in an active origin;\n- images/PDFs that are re-encoded or sanitized before serving;\n- isolated preview origins with no privileged cookies, storage, or message bridge;\n- missing deployment details where you cannot establish executable rendering or unsafe serving.\n- permissive content types, inline disposition, or storage/header hygiene issues\n without a concrete privileged renderer or execution surface.\n", - "---\nid: APP_PROXY_LIQUID_INJECTION\nversion: 2\nseverity: high\n---\n\n# App Proxy Liquid Injection\n\nTrace verified app-proxy request values into active response bodies, including Liquid and HTML response types. Report only a request-controlled value that reaches an active response; static templates and inert JSON are not findings.\n", + "---\nid: APP_PROXY_LIQUID_INJECTION\nversion: 2\nseverity: high\nprecedence: prefer-agent\n---\n\n# App Proxy Liquid Injection\n\nTrace verified app-proxy request values into active response bodies, including Liquid and HTML response types. Report only a request-controlled value that reaches an active response; static templates and inert JSON are not findings.\n", "---\nid: APP_PROXY_UNVERIFIED_SIGNATURE\nversion: 2\nseverity: high\n---\n\nFind app proxy endpoints that read proxy parameters without verifying\nthe Shopify signature, allowing an attacker to impersonate Shopify and\nsend fake proxy requests.\n\nApp proxies let an app serve content directly on the merchant's store\nvia a URL like `https://shop.example.com/apps/my-app/proxy`. Shopify\nsigns every proxy request with an HMAC using the app's shared secret.\nIf the app doesn't verify this signature, anyone can send requests to\nthe proxy endpoint with forged parameters — including `shop`,\n`logged_in_customer_id`, and `path_prefix`.\n\n## What to look for\n\n1. **Find app proxy route handlers.** These are endpoints configured as\n app proxies in `shopify.app.toml` under `[app_proxy]` or in the app's\n routing config. They typically read parameters like:\n - `shop` or `shop_id`\n - `logged_in_customer_id`\n - `path_prefix`\n - `signature`\n - `timestamp`\n\n2. **Check for signature verification.** The handler must verify the\n HMAC signature before trusting any proxy parameter. Look for:\n - **Remix:** `authenticate.public.appProxy(request)` — the official\n verification function\n - **Rails:** `verified_request?` or manual HMAC verification using\n `ShopifyApp` utilities\n - **Express:** Manual HMAC verification using the app secret\n - **PHP:** `ShopifyUtils::verifyProxyRequest()` or equivalent\n\n3. **If no verification is present, check whether the handler:**\n - Reads `shop` from the query string and uses it to scope data\n - Reads `logged_in_customer_id` and uses it for authorisation\n - Returns any shop-specific data\n\n If any of these are true and there's no signature check, it's a real\n finding.\n\n4. **Check for the HMAC pattern even if the function name isn't obvious.**\n Some apps implement custom verification:\n - `crypto.createHmac('sha256', API_SECRET)`\n - `OpenSSL::HMAC.digest`\n - `hash_hmac('sha256', ...)`\n - Comparison with `timingSafeEqual` or `secure_compare`\n\n5. **Separate app-local findings from protocol hardening signals.** Missing\n verification is a finding when the handler trusts signed parameters without\n any verification boundary. Weak comparison, unusual canonicalization, or\n delimiterless concatenation is not automatically an app finding: keep it\n unresolved unless you can show a usable victim-signed request path or another\n concrete exploit condition in this app.\n\n## What to report\n\nFor each proxy handler that reads shop/customer parameters without\nsignature verification, or where you can demonstrate a usable victim-signed\nrequest path through a weak verification implementation:\n\n```json\n{\n \"file\": \"app/routes/proxy.ts\",\n \"line\": 15,\n \"message\": \"App proxy handler reads shop parameter without signature verification\",\n \"snippet\": \"const shop = url.searchParams.get('shop')\",\n \"evidence\": [\n {\n \"file\": \"app/routes/proxy.ts\",\n \"line\": 15,\n \"quote\": \"const shop = url.searchParams.get('shop')\"\n },\n {\n \"file\": \"app/routes/proxy.ts\",\n \"line\": 1,\n \"quote\": \"no authenticate.public.appProxy or HMAC verification found\"\n }\n ],\n \"confidence\": \"high\",\n \"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.\"\n}\n```\n\nDo not report:\n\n- Handlers that call `authenticate.public.appProxy(request)` (Remix)\n- Handlers with manual HMAC verification\n- Handlers that return only static content (no shop-specific data)\n- Protocol-only canonicalization concerns with no demonstrated app-local exploit path\n- Test handlers\n", - "---\nid: COMMITTED_SECRET\nversion: 3\nseverity: high\n---\n\n# Committed Secret\n\nInspect files skipped by deterministic secret scanning for committed credentials. Never quote or reproduce a secret; cite only the file and redacted credential kind, and recommend rotation.\n\nDo not report placeholders, public client identifiers (`SHOPIFY_API_KEY`, Stripe `pk_`), or files git confirms are untracked and ignored. Template env files (`.env.example`, `.sample`, `.template`, `.dist`) are findings only when they contain a known credential format.\n", - "---\nid: CREDENTIAL_BROWSER_LEAKAGE\nversion: 1\nseverity: high\n---\n\n# Credential Browser Leakage\n\nTrace credentials, access tokens, session tokens, and client secrets into loader/HTTP responses, browser globals, DOM values, client bundles, or external requests. Do not report server-only use or safe boolean/redacted/hash-derived values.\n", - "---\nid: CREDENTIAL_LOG_LEAKAGE\nversion: 1\nseverity: high\n---\n\n# Credential Log Leakage\n\nTrace credentials, access tokens, session tokens, and client secrets through aliases and helpers to console, logger, telemetry, or error-reporting sinks. Do not report boolean presence checks, deliberate redaction, or one-way hashes.\n", + "---\nid: COMMITTED_SECRET\nversion: 3\nseverity: high\nprecedence: union\n---\n\n# Committed Secret\n\nInspect files skipped by deterministic secret scanning for committed credentials. Never quote or reproduce a secret; cite only the file and redacted credential kind, and recommend rotation.\n\nDo not report placeholders, public client identifiers (`SHOPIFY_API_KEY`, Stripe `pk_`), or files git confirms are untracked and ignored. Template env files (`.env.example`, `.sample`, `.template`, `.dist`) are findings only when they contain a known credential format.\n", + "---\nid: CREDENTIAL_BROWSER_LEAKAGE\nversion: 1\nseverity: high\nprecedence: prefer-agent\n---\n\n# Credential Browser Leakage\n\nTrace credentials, access tokens, session tokens, and client secrets into loader/HTTP responses, browser globals, DOM values, client bundles, or external requests. Do not report server-only use or safe boolean/redacted/hash-derived values.\n", + "---\nid: CREDENTIAL_LOG_LEAKAGE\nversion: 1\nseverity: high\nprecedence: prefer-agent\n---\n\n# Credential Log Leakage\n\nTrace credentials, access tokens, session tokens, and client secrets through aliases and helpers to console, logger, telemetry, or error-reporting sinks. Do not report boolean presence checks, deliberate redaction, or one-way hashes.\n", "---\nid: CROSS_SITE_SCRIPTING\nversion: 1\nseverity: high\n---\n\n# Cross-Site Scripting\n\nFind reflected, stored, and client-side paths where lower-trust data becomes\nexecutable browser content across a trust boundary. Cover app-owned HTML pages,\nserver templates, embedded app views, customer-facing pages, and operator UIs,\nnot only theme extensions. Prove the source, rendering context, victim, and\nreachable execution path; an HTML-looking string or raw-rendering API alone is\nnot a finding.\n\n## What to look for\n\n1. **Map producers and consumers.** Identify the actual web frameworks, template\n engines, versions, escaping defaults, and rendering helpers. Trace URL/query\n and form/JSON input, stored customer/merchant content, product/metafield data,\n imports, webhook fields, and third-party API responses to their renderers.\n For client-side flows, include location fragments, storage, DOM attributes,\n and `postMessage` data after examining sender/origin validation. A database\n read, authenticated route, or Shopify API response does not by itself make\n the contained user-authored data trusted.\n\n2. **Inspect server-rendered escape hatches.** Follow response builders, layouts,\n partials, and component wrappers into final HTML, including:\n - Express/React Router HTML responses assembled with strings, and custom\n server-side rendering or hydration/bootstrap data.\n - Rails `raw`, `html_safe`, and HTML/inline rendering; EJS `<%- ... %>`,\n unescaped Handlebars output, Django/Jinja `safe` or disabled autoescaping,\n and Blade/Twig raw output.\n - Markdown/rich-text renderers with raw HTML or unsafe link handling.\n Read the real helper and framework behavior. A bypass API is only a lead;\n constant HTML and correctly escaped dynamic text are not findings.\n\n3. **Follow browser-side rendering and execution.** Inspect DOM HTML writes,\n React `dangerouslySetInnerHTML`, Vue `v-html`, Svelte `{@html}`, Lit\n `unsafeHTML`, Angular trust-bypass APIs, and wrappers around these sinks.\n Also inspect dynamic script URLs, event handlers, `srcdoc`, and strings\n passed to browser `eval`, `Function`, or timers. Follow stored content from\n its original write to later preview, support, or admin views; do not stop\n at a safe first renderer. Coordinate these paths with the existing checks\n listed below instead of creating duplicate findings.\n\n4. **Evaluate the exact output context.** Determine whether input lands in HTML\n text, a quoted/unquoted attribute, a URL, JavaScript data/code, CSS, or a nested\n context. HTML text escaping is not sufficient for an event handler or a\n script/URL context. URL encoding a parameter is not scheme validation for an\n entire `href` or `src`. For JSON embedded inside an HTML `` breakout; `JSON.stringify` alone does not do this. Do not\n invent the same breakout for JSON served as an inert `application/json`\n response. Trace any later consumer that reparses it as HTML or code.\n\n5. **Verify defenses at the final sink.** Follow escaping, HTML sanitization,\n URL allowlists, framework autoescaping, Trusted Types policies, and any\n decoding or mutations after sanitization. Check actual configuration and\n context, not just a sanitizer's name. A custom sanitizer is not automatically\n vulnerable, and a library call is not automatically safe in every context.\n To report a bypass, explain the concrete construct that survives the defense\n and can execute in that renderer. Account for enforced CSP and sandboxing;\n do not assume a bypass. Missing CSP alone is not XSS, and HttpOnly cookies\n do not prevent script from acting with the victim's browser authority.\n\n6. **Establish a victim and authority boundary.** Identify who can supply the\n input, how another principal encounters it, the document's actual origin,\n and the actions or data script could reach there. An embedded app iframe\n does not inherit the Shopify Admin parent's origin or authority. Content\n executing in an isolated or sandboxed origin must not be described as\n executing in the parent without evidence. Intentional author-controlled\n HTML, self-XSS requiring the victim to paste code, and a merchant editing\n their own permitted storefront code are not automatically privilege\n escalation. Show a lower-trust writer reaching a more privileged reader,\n another user/tenant, or a surface where executable content is not authorized.\n\n## Coordinate with existing checks\n\nFollow the complete path even when it crosses surfaces, but report the same\nsource-to-sink vulnerability only once, under the most specific owning check:\n\n- `UNSAFE_INNERHTML`: DOM HTML writes and browser code-evaluation sinks.\n- `THEME_EXTENSION_XSS` / `LIQUID_UNSAFE_RENDER`: theme-extension Liquid output.\n- `TEXT_SETTING_HTML_SMUGGLING`: merchant text settings becoming active content.\n- `APP_PROXY_LIQUID_INJECTION`: values from verified app-proxy requests reaching\n active responses.\n- `SCRIPT_TAG_URL_INJECTION`: Shopify ScriptTag source URLs.\n- `ACTIVE_UPLOADS_AND_PRIVILEGED_PREVIEWS`: uploaded/imported active files and\n their privileged previews.\n\nDelegate only when the specialized check covers the complete source-to-sink\npath, not merely the response surface, and use that check's own provenance.\nUse `CROSS_SITE_SCRIPTING` for remaining paths, such as reflected server HTML,\nstored customer content in an operator template, unsafe hydration data, or\nactive URL/attribute output outside those specialized paths. Stored buyer\nreviews rendered in app-proxy HTML/Liquid responses remain here when the\ncontent comes from a separate submission endpoint rather than the verified\nproxy request. Do not suppress a distinct vulnerable sink just because the\nsame input is used elsewhere.\n\n## What to report\n\nUse the finding and execution schemas in agent checks, including this check's\nID and version. Each finding must include:\n\n- The controlling principal, entry point, victim interaction, and required access.\n- File/line evidence for input or persistence, transformations, render call,\n and final sink/template, including any ineffective defense.\n- The exact parser context and a minimal inert marker/example showing how data\n becomes executable content. Explain the execution mechanism; do not assume\n a `