diff --git a/packages/app/src/cli/services/app-security-artifacts.ts b/packages/app/src/cli/services/app-security-artifacts.ts index 7dc5d143f49..0dfc10dfd01 100644 --- a/packages/app/src/cli/services/app-security-artifacts.ts +++ b/packages/app/src/cli/services/app-security-artifacts.ts @@ -13,7 +13,8 @@ import {randomBytes} from 'node:crypto' import {lstat, mkdir, realpath, rename, unlink, writeFile} from 'node:fs/promises' import type {Stats} from 'node:fs' -const MAX_ARTIFACT_FILE_SIZE_BYTES = 5_000_000 +/** Readers reject an artifact larger than this, so writers check `encodedArtifactSize` against it first. */ +export const MAX_ARTIFACT_FILE_SIZE_BYTES = 5_000_000 export interface AppSecurityArtifactPaths { artifactDirectory: string @@ -140,6 +141,11 @@ async function readJsonArtifact(path: string): Promise { 'shopify app security record --path "/tmp/app" < ', ) expect(formatAppSecurityCommand(commands.record, 'powershell')).toBe( - "Get-Content -Raw | shopify app security record --path '/tmp/app'", + "Get-Content -Raw -Encoding UTF8 | shopify app security record --path '/tmp/app'", ) expect(formatAppSecurityCommand(commands.review, 'posix')).toBe("shopify app security review --path '/tmp/app'") expect(formatAppSecurityCommand(commands.clean, 'posix')).toBe("shopify app security clean --path '/tmp/app'") @@ -321,7 +322,7 @@ describe('formatAppSecurityCommand', () => { const recordArguments = ['shopify', 'app', 'security', 'record', '--path', WINDOWS_APP_ROOT] expect(splitQuotedCommand(formatAppSecurityCommand(commands.record, shell), shell)).toEqual( shell === 'powershell' - ? ['Get-Content', '-Raw', '', '|', ...recordArguments] + ? ['Get-Content', '-Raw', '-Encoding', 'UTF8', '', '|', ...recordArguments] : [...recordArguments, '<', ''], ) expect(splitQuotedCommand(formatAppSecurityCommand(commands.clean, shell), shell)).toEqual([ @@ -388,6 +389,40 @@ describe('formatAppSecurityCommand', () => { expect(result.stdout).not.toContain('EXPANDED') }) }) + + test.skipIf(process.platform !== 'win32')( + 'the PowerShell record command reads a findings file without a byte order mark as UTF-8 in Windows PowerShell 5.1', + async () => { + await inTemporaryDirectory(async (directory) => { + const findingsPath = joinPath(directory, 'findings.json') + const receivedPath = joinPath(directory, 'received.json') + const saveStdin = joinPath(directory, 'save-stdin.js') + // writeFile writes UTF-8 without a byte order mark. + await writeFile(findingsPath, '{"file": "app/café.ts"}') + // Saves stdin's bytes to a file, so PowerShell's console encoding can't change what the test reads back. + await writeFile( + saveStdin, + "const chunks = []\nprocess.stdin.on('data', (chunk) => chunks.push(chunk))\nprocess.stdin.on('end', () => require('fs').writeFileSync(process.argv[2], Buffer.concat(chunks)))\n", + ) + // The record command's stdin form, with node in place of shopify. + const record: AppSecurityCommand = { + command: `& ${quoteShellArgument(process.execPath, 'powershell')}`, + args: [quoteShellArgument(saveStdin, 'powershell'), quoteShellArgument(receivedPath, 'powershell')], + stdinPlaceholder: quoteShellArgument(findingsPath, 'powershell'), + } + // As the instructions say: without this, Windows PowerShell 5.1 pipes text to node as ASCII. + const script = `$OutputEncoding = [System.Text.UTF8Encoding]::new(); ${formatAppSecurityCommand(record, 'powershell')}` + const result = spawnSync( + 'powershell.exe', + ['-NoProfile', '-NonInteractive', '-EncodedCommand', Buffer.from(script, 'utf16le').toString('base64')], + {encoding: 'utf8', windowsHide: true}, + ) + + expect(result.status, result.stderr).toBe(0) + expect(JSON.parse(await readFile(receivedPath))).toStrictEqual({file: 'app/café.ts'}) + }) + }, + ) }) describe('formatAppSecurityInlineStdinCommand', () => { diff --git a/packages/app/src/cli/services/app-security-commands.ts b/packages/app/src/cli/services/app-security-commands.ts index 3e020111a22..6e02f06e7dd 100644 --- a/packages/app/src/cli/services/app-security-commands.ts +++ b/packages/app/src/cli/services/app-security-commands.ts @@ -97,7 +97,8 @@ function quoteCmdSegment(part: string): string { /** * Renders a command for the given shell. A command that reads stdin is shown reading its placeholder file: * redirected with `<` in POSIX shells and cmd.exe, and piped from `Get-Content -Raw` in PowerShell, - * which has no `<` redirection. + * which has no `<` redirection. `-Encoding UTF8` because Windows PowerShell 5.1 otherwise reads a file + * without a byte order mark in the ANSI code page. */ export function formatAppSecurityCommand( action: AppSecurityCommand, @@ -105,7 +106,7 @@ export function formatAppSecurityCommand( ): string { const commandLine = formatCommandLine(action, shell) if (!action.stdinPlaceholder) return commandLine - if (shell === 'powershell') return `Get-Content -Raw ${action.stdinPlaceholder} | ${commandLine}` + if (shell === 'powershell') return `Get-Content -Raw -Encoding UTF8 ${action.stdinPlaceholder} | ${commandLine}` return `${commandLine} < ${action.stdinPlaceholder}` } diff --git a/packages/app/src/cli/services/app-security-engine/checks/index.ts b/packages/app/src/cli/services/app-security-engine/checks/index.ts index 05da2ff88da..1f120fc2759 100644 --- a/packages/app/src/cli/services/app-security-engine/checks/index.ts +++ b/packages/app/src/cli/services/app-security-engine/checks/index.ts @@ -507,7 +507,9 @@ export function recordAgentFindings(document: unknown, options: RecordAgentFindi const grouped = groupFindingsByCheck(executed.reports, findings) errors.push(...grouped.errors) - if (errors.length > 0) return {ok: false, errors} + // Errors quote agent input (check IDs, paths, line values), which can hold secrets. They're shown in the + // terminal and in --json output, so they're redacted like everything that's stored. + if (errors.length > 0) return {ok: false, errors: errors.map(redactText)} const storedChecks = grouped.groups .map( diff --git a/packages/app/src/cli/services/app-security-instructions.test.ts b/packages/app/src/cli/services/app-security-instructions.test.ts index c62556a7dfe..5c4df8575ca 100644 --- a/packages/app/src/cli/services/app-security-instructions.test.ts +++ b/packages/app/src/cli/services/app-security-instructions.test.ts @@ -156,7 +156,11 @@ describe('appSecurityInstructions', () => { expect(instructions).toContain( codeBlock('powershell', "@'", '', `'@ | ${record}`), ) - expect(instructions).toContain(codeBlock('powershell', `Get-Content -Raw | ${record}`)) + // The encoding note follows both forms, since both pipe text to record. + expect(instructions).toContain( + `${codeBlock('powershell', `Get-Content -Raw -Encoding UTF8 | ${record}`)}\n\nWindows PowerShell 5.1 pipes text to \`record\` as ASCII by default.`, + ) + expect(instructions).toContain('`$OutputEncoding = [System.Text.UTF8Encoding]::new()` before either command.') expect(instructions).toContain("The closing `'@` must start its line.") expect(instructions).not.toContain("<<'EOF'") expect(instructions).not.toContain(`${record} <`) diff --git a/packages/app/src/cli/services/app-security-instructions.ts b/packages/app/src/cli/services/app-security-instructions.ts index 44b22c1281e..70fc1d7c0f6 100644 --- a/packages/app/src/cli/services/app-security-instructions.ts +++ b/packages/app/src/cli/services/app-security-instructions.ts @@ -69,8 +69,13 @@ ${fileCommand}` const shellNotes = shell === 'powershell' - ? `The closing \`'@\` must start its line. The single-quoted here-string keeps PowerShell from expanding \`$\` in the document. Windows PowerShell 5.1 pipes text as ASCII by default, so if the document contains non-ASCII characters, run \`$OutputEncoding = [System.Text.UTF8Encoding]::new()\` first.` + ? `The closing \`'@\` must start its line. The single-quoted here-string keeps PowerShell from expanding \`$\` in the document.` : `The quoted \`'EOF'\` keeps the shell from expanding \`$\` and backticks in the document.` + // Both PowerShell forms pipe text to a native command, which Windows PowerShell 5.1 encodes as ASCII. + const encodingNote = + shell === 'powershell' + ? `\n\nWindows PowerShell 5.1 pipes text to \`record\` as ASCII by default. If the document contains non-ASCII characters, run \`$OutputEncoding = [System.Text.UTF8Encoding]::new()\` before either command.` + : '' return `${codeBlock(inlineCommand, shell)} @@ -78,7 +83,7 @@ Replace \`${RECORD_DOCUMENT_PLACEHOLDER}\` with the document itself; you don't n If you'd rather write the document to a file, pipe the file instead, ${replaceFilePlaceholder}: -${fileCommand}` +${fileCommand}${encodingNote}` } function instructionPaths( diff --git a/packages/app/src/cli/services/security-record.test.ts b/packages/app/src/cli/services/security-record.test.ts index 18beab8382e..fbc6e8f40f5 100644 --- a/packages/app/src/cli/services/security-record.test.ts +++ b/packages/app/src/cli/services/security-record.test.ts @@ -1,9 +1,9 @@ import securityRecord, {renderSecurityRecordResult} from './security-record.js' import {securityRecordJsonOutputSchema} from './security-record-json.js' import {resolveAppSecurityRoot} from './app-security-api.js' -import {appSecurityArtifactPaths, writeAgentFindings} from './app-security-artifacts.js' +import {appSecurityArtifactPaths, readFindingsDocument, writeAgentFindings} from './app-security-artifacts.js' import {formatAppSecurityCommand, resolveAppSecurityCommands} from './app-security-commands.js' -import {fileExists, inTemporaryDirectory, mkdir, readFile, writeFile} from '@shopify/cli-kit/node/fs' +import {fileExists, fileSize, inTemporaryDirectory, mkdir, readFile, writeFile} from '@shopify/cli-kit/node/fs' import {AbortError, handler} from '@shopify/cli-kit/node/error' import {joinPath} from '@shopify/cli-kit/node/path' import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output' @@ -274,6 +274,64 @@ describe('securityRecord', () => { }) }) + describe('redacts secrets quoted in validation errors', () => { + const documentWithSecrets = JSON.stringify({ + schema_version: 1, + checks_executed: [{check_id: FAKE_SHOPIFY_TOKEN, check_version: 1}], + findings: [ + finding({check_id: FAKE_SHOPIFY_TOKEN}), + finding({file: `../${FAKE_SHOPIFY_TOKEN}`}), + finding({line: FAKE_SHOPIFY_TOKEN}), + ], + }) + const redactedErrors = [ + expect.stringMatching(/^checks_executed\[0\] \(.*REDACTED.*\): unknown check_id$/), + expect.stringMatching(/^findings\[0\] \(.*REDACTED.*\): unknown check_id$/), + expect.stringMatching(/^findings\[1\] \(MISSING_TENANT_ISOLATION\): unsafe file path .*REDACTED/), + expect.stringMatching(/^findings\[2\] \(MISSING_TENANT_ISOLATION\): invalid line number: .*REDACTED/), + ] + + test('in the terminal', async () => { + await inTemporaryDirectory(async (directory) => { + const appRoot = await createApp(directory) + const error = await recordError(appRoot, testDependencies(documentWithSecrets)) + expect(error.details).toStrictEqual({errors: redactedErrors}) + + const output = mockAndCaptureOutput() + output.clear() + try { + await handler(error) + + expect(output.error()).toContain('unknown check_id') + expect(output.error()).toContain('REDACTED') + expect(output.error()).not.toContain(FAKE_SHOPIFY_TOKEN) + } finally { + output.clear() + } + }) + }) + + test('in JSON mode', async () => { + await inTemporaryDirectory(async (directory) => { + const appRoot = await createApp(directory) + const error = await recordError(appRoot, testDependencies(documentWithSecrets)) + + const output = mockAndCaptureOutput() + output.clear() + vi.stubEnv('SHOPIFY_FLAG_JSON', '1') + try { + await handler(error) + + expect(JSON.parse(output.info()).error.details).toStrictEqual({errors: redactedErrors}) + expect(output.info()).not.toContain(FAKE_SHOPIFY_TOKEN) + } finally { + vi.unstubAllEnvs() + output.clear() + } + }) + }) + }) + test('fails when nothing is piped on stdin', async () => { await inTemporaryDirectory(async (directory) => { const appRoot = await createApp(directory) @@ -315,6 +373,40 @@ describe('securityRecord', () => { await expectRejected(document, [expect.stringContaining('the limit is 5 MB')]) }) + describe('near the 5 MB limit', () => { + // 1,000 findings of about 5 KB each. The stored document adds about 180 KB of snapshots and indentation, + // so the reasoning length decides which side of the limit the stored file lands on. + function nearLimitDocument(reasoningLength: number): string { + return JSON.stringify({ + schema_version: 1, + findings: Array.from({length: 1_000}, (_, index) => + finding({line: index + 1, message: 'm'.repeat(4_000), reasoning: 'r'.repeat(reasoningLength)}), + ), + }) + } + + test('rejects a document under the input limit whose stored form review could not read', async () => { + const document = nearLimitDocument(700) + expect(Buffer.byteLength(document)).toBeLessThan(5_000_000) + + await expectRejected(document, [ + expect.stringMatching(/^The recorded findings would be stored as \d+ bytes; the limit is 5 MB\./), + ]) + }) + + test('records a document that review can read', async () => { + await inTemporaryDirectory(async (directory) => { + const appRoot = await createApp(directory) + const {agentFindingsPath} = appSecurityArtifactPaths(appRoot) + + await securityRecord({appRoot}, testDependencies(nearLimitDocument(500))) + + await expect(fileSize(agentFindingsPath)).resolves.toBeGreaterThan(4_900_000) + await expect(readFindingsDocument(agentFindingsPath, 'agent')).resolves.toMatchObject({status: 'ok'}) + }) + }) + }) + test('rejects invalid JSON', async () => { await expectRejected('{"schema_version": 1,', [expect.stringContaining('The findings document is not valid JSON')]) }) diff --git a/packages/app/src/cli/services/security-record.ts b/packages/app/src/cli/services/security-record.ts index 52ee7b58c33..a1cd779ce3f 100644 --- a/packages/app/src/cli/services/security-record.ts +++ b/packages/app/src/cli/services/security-record.ts @@ -1,4 +1,4 @@ -import {writeAgentFindings} from './app-security-artifacts.js' +import {encodedArtifactSize, MAX_ARTIFACT_FILE_SIZE_BYTES, writeAgentFindings} from './app-security-artifacts.js' import {formatAppSecurityCommand, resolveAppSecurityCommands} from './app-security-commands.js' import {countLabel} from './app-security-format.js' import { @@ -110,6 +110,16 @@ export default async function securityRecord( }) if (!recorded.ok) throw rejectedDocumentError(recorded.errors, commands) + // The stored document adds check snapshots and indentation, so an input under the limit can still be stored + // over it. `review` can't read a file that large, so refuse to replace the existing one with it. + const storedSize = encodedArtifactSize(recorded.document) + if (storedSize > MAX_ARTIFACT_FILE_SIZE_BYTES) { + throw rejectedDocumentError( + [`The recorded findings would be stored as ${storedSize} bytes; the limit is 5 MB. Shorten or remove findings.`], + commands, + ) + } + const path = await dependencies.writeAgentFindings(options.appRoot, recorded.document) return { path, diff --git a/packages/app/src/cli/services/security-review-output.test.ts b/packages/app/src/cli/services/security-review-output.test.ts index a8b1a9bb53c..0a7535246f7 100644 --- a/packages/app/src/cli/services/security-review-output.test.ts +++ b/packages/app/src/cli/services/security-review-output.test.ts @@ -623,6 +623,60 @@ describe('buildSecurityReviewAlerts', () => { ]) }) + test('lists the suppressed and superseded findings of unresolved checks in full with --verbose', () => { + // The agent's prefer-agent result supersedes both deterministic findings, and its own finding is suppressed, + // so the unresolved check has no active findings. + const agent: AgentFindingsDocument = { + ...agentFindingsDocument, + checks: agentFindingsDocument.checks.map((check) => + check.id === 'CREDENTIAL_LOG_LEAKAGE' + ? { + ...check, + status: 'unresolved', + reason: {code: 'dynamic_logger', message: 'The logger is configured at runtime.'}, + findings: check.findings.map((finding) => ({ + ...finding, + suppression: {justification: 'The token is redacted by the logger.'}, + })), + } + : check, + ), + } + const sources: Sources = {deterministic: deterministicFindingsDocument, agent} + + const [concise] = buildSecurityReviewAlerts(presenterInput(sources, {checkIds: ['CREDENTIAL_LOG_LEAKAGE']})) + const [verbose] = buildSecurityReviewAlerts( + presenterInput(sources, {checkIds: ['CREDENTIAL_LOG_LEAKAGE'], verbose: true}), + ) + + expect(concise!.options.headline).toBe('1 check unresolved.') + expect(concise!.options.customSections).toHaveLength(1) + expect(verbose!.options.headline).toBe('1 check unresolved.') + const sections = verbose!.options.customSections! + expect(sections[0]).toEqual(concise!.options.customSections![0]) + expect(sections.slice(1).map((section) => section.title)).toEqual([ + 'CREDENTIAL_LOG_LEAKAGE \u00b7 deterministic \u00b7 app/routes/orders.tsx:12 (superseded)', + 'CREDENTIAL_LOG_LEAKAGE \u00b7 agent \u00b7 app/routes/orders.tsx:12 (suppressed)', + 'CREDENTIAL_LOG_LEAKAGE \u00b7 deterministic \u00b7 app/shopify.server.ts:40 (superseded)', + ]) + expect(sections[1]!.body).toContainEqual({ + subdued: '\nEvidence: app/routes/orders.tsx:12 \u2014 console.log(session.accessToken)', + }) + expect(sections[2]).toEqual({ + title: 'CREDENTIAL_LOG_LEAKAGE \u00b7 agent \u00b7 app/routes/orders.tsx:12 (suppressed)', + body: [ + 'The session access token is written to the server log.', + {subdued: '\nConfidence: high'}, + { + subdued: + '\nReasoning: The logged object is the authenticated session, whose accessToken is a live credential.', + }, + {subdued: '\nEvidence: app/routes/orders.tsx:12 \u2014 console.log(session.accessToken)'}, + {subdued: '\nSuppressed: The token is redacted by the logger.'}, + ], + }) + }) + test('lists passed and not applicable checks with their disposition counts', () => { // With no active findings, MISSING_TENANT_ISOLATION passes; its one finding is suppressed. const agent: AgentFindingsDocument = { diff --git a/packages/app/src/cli/services/security-review-output.ts b/packages/app/src/cli/services/security-review-output.ts index ae7655028de..8e55811c5aa 100644 --- a/packages/app/src/cli/services/security-review-output.ts +++ b/packages/app/src/cli/services/security-review-output.ts @@ -119,7 +119,7 @@ export function buildSecurityReviewAlerts(input: SecurityReviewPresenterInput): const withFindings = checks.filter((check) => activeFindings(check).length > 0) return [ - ...(unresolved.length > 0 ? [unresolvedChecksAlert(unresolved)] : []), + ...(unresolved.length > 0 ? [unresolvedChecksAlert(unresolved, input.verbose)] : []), ...(passedOrNotApplicable.length > 0 ? [passedChecksAlert(passedOrNotApplicable, input.verbose)] : []), ...withFindings.map((check) => checkWithFindingsAlert(check, input.verbose, input.now)), summaryAlert(buildSecurityReviewSummary(input)), @@ -331,15 +331,22 @@ function resultsFileCells(row: ResultsFileRow): InlineToken[] { return [row.name, row.updated, row.commit, row.engine] } -function unresolvedChecksAlert(checks: CombinedCheck[]): SecurityReviewAlert { +/** + * Unresolved checks with no active findings. With `--verbose`, each check's suppressed and superseded findings + * follow it in full, prefixed by the check ID, as in the passed checks box. + */ +function unresolvedChecksAlert(checks: CombinedCheck[], verbose: boolean): SecurityReviewAlert { return { type: 'warning', options: { headline: `${countLabel(checks.length, 'check')} unresolved.`, - customSections: checks.map((check) => ({ - title: `${check.id} · ${check.title}`, - body: {tabularData: sourceStatusRows(check, {withDispositions: true}), firstColumnSubdued: true}, - })), + customSections: checks.flatMap((check) => [ + { + title: `${check.id} · ${check.title}`, + body: {tabularData: sourceStatusRows(check, {withDispositions: true}), firstColumnSubdued: true}, + }, + ...(verbose ? check.findings.map((finding) => findingSection(finding, verbose, `${check.id} · `)) : []), + ]), }, } }