Skip to content

Remove attestation from App Security #8692

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jek merged 7 commits into from
Oct 1, 2026
+4,474 −5,148
Merged

Remove attestation from App Security #8692

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cc27f9d
Remove the dead external findings merge
jek Sep 30, 2026
bdfc8c3
Remove attestation from the scan artifacts and check command
jek Sep 30, 2026
b425438
Replace trace.json with deterministic-findings.json
jek Sep 30, 2026
7f33f06
Replace review.json with agent-checks.json
jek Sep 30, 2026
9752174
Add record and agent-findings.json
jek Sep 30, 2026
04faebe
Replace check --clean with the clean command
jek Sep 30, 2026
245343a
Add the review command
jek Sep 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
178 changes: 64 additions & 114 deletions packages/app/src/cli/commands/app/security/check.integration.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {inTemporaryDirectory} from '@shopify/cli-kit/node/fs'
import {unstyled} from '@shopify/cli-kit/node/output'
import {joinPath} from '@shopify/cli-kit/node/path'
import {describe, expect, test, vi} from 'vitest'
import {mkdir, readFile, rm, writeFile} from 'node:fs/promises'
import {mkdir, readFile, writeFile} from 'node:fs/promises'
// eslint-disable-next-line n/prefer-global/console
import {Console} from 'node:console'

Expand All @@ -25,18 +25,6 @@ vi.mock('@shopify/cli-kit/node/session', async (importOriginal) => ({
setCurrentSessionAlias: vi.fn(),
}))

interface ReviewPack {
source_scan_id: string
checks: {id: string; version: number; prompt_hash: string}[]
}

interface Trace {
findings: {source: string; check_id?: string}[]
checks_executed: {kind: string; id: string; status: string}[]
}

const reviewedCheckId = 'MISSING_TENANT_ISOLATION'

async function createApp(directory: string): Promise<{nestedDirectory: string}> {
const routesDirectory = joinPath(directory, 'app', 'routes')
await mkdir(routesDirectory, {recursive: true})
Expand All @@ -50,39 +38,8 @@ async function createApp(directory: string): Promise<{nestedDirectory: string}>
return {nestedDirectory: routesDirectory}
}

async function readReviewPack(reviewPath: string): Promise<ReviewPack> {
return JSON.parse(await readFile(reviewPath, 'utf8')) as ReviewPack
}

function findingsDocument(reviewPack: ReviewPack): string {
const check = reviewPack.checks.find((entry) => entry.id === reviewedCheckId)
if (!check) throw new Error(`Missing review pack check ${reviewedCheckId}`)
const identity = {check_id: check.id, check_version: check.version, prompt_hash: check.prompt_hash}
return `${JSON.stringify({
schema_version: 1,
source_scan_id: reviewPack.source_scan_id,
checks_executed: [{...identity, status: 'executed', inspected_files: ['app/routes/index.ts']}],
findings: [
{
...identity,
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`
}

// Byte snapshot of every local artifact so a refused scan can be proven to leave them untouched.
async function artifactBytes(paths: Record<'tracePath' | 'reviewPath' | 'findingsPath' | 'submissionPath', string>) {
const readOrMissing = async (path: string) => readFile(path).catch(() => 'missing')
return {
trace: await readOrMissing(paths.tracePath),
review: await readOrMissing(paths.reviewPath),
findings: await readOrMissing(paths.findingsPath),
submission: await readOrMissing(paths.submissionPath),
}
async function readJson(path: string): Promise<unknown> {
return JSON.parse(await readFile(path, 'utf8'))
}

function errorText(stderr: string): string {
Expand Down Expand Up @@ -130,90 +87,83 @@ async function runCommand(argv: string[]) {
}

describe('app security check command boundary', () => {
test('refuses a plain scan once findings from a custom path are compiled, even after that file is gone', async () => {
test('scans an app from a nested directory and writes deterministic-findings.json and agent-checks.json', async () => {
await inTemporaryDirectory(async (directory) => {
await inTemporaryDirectory(async (findingsDirectory) => {
const {nestedDirectory} = await createApp(directory)
const paths = appSecurityArtifactPaths(directory)

const scan = await runCommand(['--path', directory, '--json', '--skip-instructions'])
expect(scan.exitCode).toBe(0)
expect(JSON.parse(scan.stdout)).toMatchObject({operation: 'scan'})

const customFindingsPath = joinPath(findingsDirectory, 'agent-findings.json')
await writeFile(customFindingsPath, findingsDocument(await readReviewPack(paths.reviewPath)))
const compile = await runCommand([
'--path',
directory,
'--findings',
customFindingsPath,
'--json',
'--skip-instructions',
])
expect(compile.exitCode).toBe(0)
expect(JSON.parse(compile.stdout)).toMatchObject({operation: 'compile'})
const trace = JSON.parse(await readFile(paths.tracePath, 'utf8')) as Trace
expect(trace.findings).toContainEqual(expect.objectContaining({source: 'agent', check_id: reviewedCheckId}))
expect(trace.checks_executed).toContainEqual(
expect.objectContaining({kind: 'agent', id: reviewedCheckId, status: 'executed'}),
)

await rm(customFindingsPath)
await expect(readFile(paths.findingsPath)).rejects.toMatchObject({code: 'ENOENT'})
await writeFile(paths.submissionPath, '{"sentinel":"submission"}\n')
const before = await artifactBytes(paths)

const refused = await runCommand(['--path', nestedDirectory, '--skip-instructions'])
expect(refused.exitCode).toBe(1)
expect(refused.stdout).toBe('')
const message = errorText(refused.stderr)
expect(message).toContain('App Security did not start a new scan.')
expect(message).toContain('The existing trace contains agent review results:')
expectMentionsPath(message, paths.tracePath)
expect(message).toContain('--clean')
await expect(artifactBytes(paths)).resolves.toEqual(before)
expect(before.findings).toBe('missing')
const {nestedDirectory} = await createApp(directory)
const paths = appSecurityArtifactPaths(directory)

const result = await runCommand(['--path', nestedDirectory, '--json', '--skip-instructions'])

expect(result.exitCode).toBe(0)
const output = JSON.parse(result.stdout)
expect(Object.keys(output).sort()).toEqual(['agent_checks_path', 'deterministic_findings', 'engine'])
expect(output.agent_checks_path).toBe(paths.agentChecksPath)
expect(output.engine).toMatchObject({name: 'shopify-app-security'})
await expect(readJson(paths.deterministicFindingsPath)).resolves.toEqual(output.deterministic_findings)
await expect(readJson(paths.deterministicFindingsPath)).resolves.toMatchObject({
schema_version: 1,
engine: {name: 'shopify-app-security'},
findings: expect.any(Array),
})
await expect(readJson(paths.agentChecksPath)).resolves.toMatchObject({
schema_version: 1,
checks: expect.arrayContaining([
expect.objectContaining({id: expect.any(String), version: expect.any(Number)}),
]),
})
await expect(readFile(paths.agentFindingsPath)).rejects.toMatchObject({code: 'ENOENT'})
})
})

test('refuses a plain scan while default agent findings are pending', async () => {
test('re-scanning replaces the check artifacts without prompting and leaves agent findings untouched', async () => {
await inTemporaryDirectory(async (directory) => {
const {nestedDirectory} = await createApp(directory)
const paths = appSecurityArtifactPaths(directory)

const scan = await runCommand(['--path', directory, '--json', '--skip-instructions'])
expect(scan.exitCode).toBe(0)
expect(JSON.parse(scan.stdout)).toMatchObject({operation: 'scan'})

await writeFile(paths.findingsPath, findingsDocument(await readReviewPack(paths.reviewPath)))
await writeFile(paths.submissionPath, '{"sentinel":"submission"}\n')
const before = await artifactBytes(paths)

const refused = await runCommand(['--path', nestedDirectory, '--skip-instructions'])
expect(refused.exitCode).toBe(1)
expect(refused.stdout).toBe('')
const message = errorText(refused.stderr)
expect(message).toContain('App Security did not start a new scan.')
expect(message).toContain('Agent findings exist at:')
expectMentionsPath(message, paths.findingsPath)
expect(message).toContain('--findings')
expect(message).toContain('--clean')
await expect(artifactBytes(paths)).resolves.toEqual(before)
const firstScan = await runCommand(['--path', directory, '--json', '--skip-instructions'])
expect(firstScan.exitCode).toBe(0)

// Check must not read, validate, or rewrite the agent's findings, so any bytes survive a re-scan.
const agentFindings = '{"recorded": "by the agent", "kept": "byte for byte"}'
await writeFile(paths.agentFindingsPath, agentFindings)
await writeFile(paths.deterministicFindingsPath, '{"sentinel": "previous scan"}\n')
await writeFile(paths.agentChecksPath, '{"sentinel": "previous agent checks"}\n')
await writeFile(joinPath(nestedDirectory, 'index.ts'), 'export const loader = () => ({changed: true})')

const rescan = await runCommand(['--path', directory, '--skip-instructions'])

expect(rescan.exitCode).toBe(0)
expect(unstyled(rescan.stdout)).not.toMatch(/discard/i)
await expect(readJson(paths.deterministicFindingsPath)).resolves.toMatchObject({
schema_version: 1,
findings: expect.any(Array),
})
await expect(readJson(paths.agentChecksPath)).resolves.toMatchObject({
schema_version: 1,
checks: expect.any(Array),
})
await expect(readFile(paths.agentFindingsPath, 'utf8')).resolves.toBe(agentFindings)
})
})

test('rejects --clean together with --findings before touching the app', async () => {
test('rejects a missing config', async () => {
await inTemporaryDirectory(async (directory) => {
await createApp(directory)
const paths = appSecurityArtifactPaths(directory)

const result = await runCommand(['--path', directory, '--clean', '--findings', 'findings.json'])

expect(result.exitCode).toBe(2)
expect(result.stderr).toContain('--findings')
expect(result.stderr).toContain('--clean')
await expect(readFile(paths.tracePath)).rejects.toMatchObject({code: 'ENOENT'})
const result = await runCommand([
'--path',
directory,
'--config',
'shopify.app.dev-dashboard.json',
'--skip-instructions',
])

expect(result.exitCode).toBe(1)
const message = errorText(result.stderr)
expect(message).toContain("Couldn't find app configuration at")
expectMentionsPath(message, joinPath(directory, 'shopify.app.shopifyappdev-dashboardjson.toml'))
await expect(readFile(paths.deterministicFindingsPath)).rejects.toMatchObject({code: 'ENOENT'})
})
})
})
60 changes: 20 additions & 40 deletions packages/app/src/cli/commands/app/security/check.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {appFlags} from '../../../flags.js'
import securityCheck from '../../../services/security-check.js'
import AppLinkedCommand from '../../../utilities/app-linked-command.js'
import BaseCommand from '@shopify/cli-kit/node/base-command'
import {globalFlags} from '@shopify/cli-kit/node/cli'
import {resolvePath} from '@shopify/cli-kit/node/path'
import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output'
import {describe, expect, test, vi} from 'vitest'
Expand All @@ -19,6 +20,12 @@ describe('app security check command', () => {
expect(SecurityCheck.args).not.toHaveProperty('directory')
})

test('accepts only the scan flags, with no findings or clean flags', () => {
const commandFlags = Object.keys(SecurityCheck.flags).filter((name) => !(name in globalFlags))

expect(commandFlags.sort()).toEqual(['blocking', 'config', 'ignore', 'json', 'path', 'skip-instructions', 'yes'])
})

test('forwards --path and flags to the service', async () => {
await SecurityCheck.run(
['--path', './fixtures/unlinked-app', '--json', '--verbose', '--blocking', 'high', '--skip-instructions'],
Expand All @@ -33,8 +40,6 @@ describe('app security check command', () => {
blocking: 'high',
yes: false,
skipInstructions: true,
findingsPath: undefined,
clean: false,
ignorePatterns: [],
})
})
Expand Down Expand Up @@ -85,8 +90,6 @@ describe('app security check command', () => {
blocking: 'none',
yes: true,
skipInstructions: false,
findingsPath: undefined,
clean: false,
ignorePatterns: [],
})
})
Expand All @@ -100,53 +103,27 @@ describe('app security check command', () => {
expect(securityCheck).toHaveBeenCalledWith(expect.objectContaining({configName: 'staging', skipInstructions: true}))
})

test('forwards --clean and keeps it mutually exclusive with --findings', async () => {
await SecurityCheck.run(['--clean', '--skip-instructions'], import.meta.url)

expect(securityCheck).toHaveBeenCalledWith(expect.objectContaining({clean: true, findingsPath: undefined}))
expect(SecurityCheck.flags.clean.exclusive).toEqual(['findings'])
test.each(['--findings', '--clean'])('rejects the removed %s flag', async (removedFlag) => {
await expect(SecurityCheck.run([removedFlag, '--skip-instructions'], import.meta.url)).rejects.toThrow()
})

test.each(['true', 'false'])(
'ignores an inherited SHOPIFY_FLAG_APP_SECURITY_CLEAN=%s so a plain scan stays non-destructive',
async (inheritedValue) => {
vi.stubEnv('SHOPIFY_FLAG_APP_SECURITY_CLEAN', inheritedValue)
try {
expect(SecurityCheck.flags.clean).not.toHaveProperty('env')

await SecurityCheck.run(['--skip-instructions'], import.meta.url)
expect(securityCheck).toHaveBeenLastCalledWith(expect.objectContaining({clean: false}))

await SecurityCheck.run(['--findings', './findings.json', '--skip-instructions'], import.meta.url)
expect(securityCheck).toHaveBeenLastCalledWith(
expect.objectContaining({clean: false, findingsPath: resolvePath('./findings.json')}),
)
} finally {
vi.unstubAllEnvs()
}
},
)

test('resolves and forwards an agent findings file', async () => {
await SecurityCheck.run(['--findings', './findings.json', '--skip-instructions'], import.meta.url)

expect(securityCheck).toHaveBeenCalledWith(expect.objectContaining({findingsPath: resolvePath('./findings.json')}))
})

test('describes --yes as printing instructions and keeps it mutually exclusive with --skip-instructions', () => {
test('describes the artifacts it writes and how agent results are recorded', () => {
expect(SecurityCheck.flags.yes.description).toBe('Print coding-agent instructions without prompting.')
expect(SecurityCheck.flags['skip-instructions'].description).toBe("Don't offer to show coding-agent instructions.")
expect(SecurityCheck.flags.clean.description).toBe('Discard the current local review and start a new scan.')
expect(SecurityCheck.flags.yes.exclusive).toEqual(['skip-instructions'])
expect(SecurityCheck.flags['skip-instructions'].exclusive).toEqual(['yes'])
expect(SecurityCheck.summary).toContain('deterministic-findings.json')
expect(SecurityCheck.summary).toContain('agent-checks.json')
expect(SecurityCheck.descriptionWithMarkdown).toContain('`deterministic-findings.json` and `agent-checks.json`')
expect(SecurityCheck.descriptionWithMarkdown).toContain('`shopify app security record`')
expect(SecurityCheck.descriptionWithMarkdown).toContain('copy the coding-agent instructions')
expect(SecurityCheck.descriptionWithMarkdown).toContain('`--config`')
expect(SecurityCheck.descriptionWithMarkdown).toContain('copying is the default')
expect(SecurityCheck.descriptionWithMarkdown).toContain('shopify app security instructions')
expect(SecurityCheck.descriptionWithMarkdown).toContain('Pass `--clean` to discard that work and start over')
expect(SecurityCheck.descriptionWithMarkdown).not.toMatch(/--findings|--clean|compile|trace/)
})

test('documents --ignore as ordered .gitignore patterns that follow-up commands repeat', () => {
test('documents --ignore as ordered .gitignore patterns that the coding-agent instructions repeat', () => {
expect(SecurityCheck.flags.ignore.multiple).toBe(true)
expect(SecurityCheck.flags.ignore.description).toBe(
'Ignore files that match this .gitignore pattern, relative to the app directory. Start the pattern with ! to include matching files again. Repeat the flag to add patterns; later patterns take precedence.',
Expand All @@ -156,7 +133,10 @@ describe('app security check command', () => {
expect(SecurityCheck.descriptionWithMarkdown).toContain('later patterns take precedence')
expect(SecurityCheck.descriptionWithMarkdown).toContain("--ignore '!build/'")
expect(SecurityCheck.descriptionWithMarkdown).toContain('single quotes in POSIX shells and PowerShell')
expect(SecurityCheck.descriptionWithMarkdown).toContain('`--findings`')
expect(SecurityCheck.descriptionWithMarkdown).toContain(
'The coding-agent instructions this check offers repeat the patterns.',
)
expect(SecurityCheck.descriptionWithMarkdown).toContain("Other `app security` commands don't take `--ignore`")
})

test('allows --yes in JSON mode while preserving non-interactive output behavior', async () => {
Expand Down
Loading
Loading