Remove attestation from App Security - #8692
Conversation
github-actions
Bot
added
the
no-changelog
c32ac30 to
424d4b9
Compare
dmerand
left a comment
dmerand
left a comment
There was a problem hiding this comment.
Tophat/smoke testing LGTM. No major blockers; per our discussion we'll get this through + focus on polish as a follow-up. I've just got a couple of notes from my review with agents.
| [['Fix every error, then run', {command: recordCommand(commands)}, 'again.']], | ||
| [{title: 'Errors', body: {list: {items: errors}}}], | ||
| ) | ||
| error.details = {errors} |
There was a problem hiding this comment.
Can we redact validation errors before adding them to the error banner and details.errors? An invalid line, file, or check_id can contain a Shopify token. record copies that value into both terminal and --json output. I reproduced this with a synthetic token. The previous findings file stayed unchanged, but the error displayed the token. Please add tests for both output modes.
| export async function writeAgentFindings(appRoot: string, artifact: AgentFindingsArtifact): Promise<string> { | ||
| const paths = appSecurityArtifactPaths(appRoot) | ||
| await ensureArtifactDirectory(appRoot, paths.artifactDirectory) | ||
| await writeAtomicArtifact(paths.agentFindingsPath, encodeArtifact(artifact)) |
There was a problem hiding this comment.
Can we ensure record writes only artifacts that review can read? A valid 4,989,926-byte input produced a 5,085,618-byte agent-findings.json after the check snapshots and formatting. record returned success, but the 5 MB reader rejected the file as invalid. Please check the encoded size before replacing the old file, or adjust the bounded read limit. A record-to-review test near the limit would catch this.
| ): string { | ||
| const commandLine = formatCommandLine(action, shell) | ||
| if (!action.stdinPlaceholder) return commandLine | ||
| if (shell === 'powershell') return `Get-Content -Raw ${action.stdinPlaceholder} | ${commandLine}` |
There was a problem hiding this comment.
Could we read the findings file as UTF-8 in the PowerShell command? Windows PowerShell 5.1 defaults to the active ANSI code page for BOM-less files. A UTF-8 findings file with a path like app/café.ts can remain valid JSON but record the wrong path. The instructions' $OutputEncoding note only affects the later pipe; it cannot repair the file read. Please use -Encoding UTF8 (or another UTF-8-safe read) and test a BOM-less UTF-8 file through PowerShell 5.1 into record. I checked the generated command and docs, but did not run PowerShell 5.1 here.
424d4b9 to
f86890c
Compare
dmerand
left a comment
dmerand
left a comment
There was a problem hiding this comment.
We can fix-forward the existing comments.
Nothing in the check pipeline produces external findings, so mergeExternalFindings and validateExternalFinding were only reachable from their own tests. Remove the module and those tests before reworking the trace path, so later commits don't carry dead code. Co-authored-by: AI <noreply@pi.dev>
The trace carried fingerprints, input/result hashes, an attestation digest and merged agent findings so agent results could be compiled and verified. Drop all of that: the compile path (check --findings, exit code 2, the "existing work" guard), suppressions, and the hashing. The trace is now the deterministic scan only, and the submission payload is trimmed to match. Co-authored-by: AI <noreply@pi.dev>
The trace was a validated, hash-carrying document. deterministic-findings.json is the plain deterministic scan: only the schema version and findings array are checked on read, and submit re-checks it for unredacted secrets. The check output, JSON output and submission payload now come from deterministic-findings.json (upload schema version 0), and the trace-only fields (required, guidance, implementations, coverage.complete) are dropped from the scan model. Co-authored-by: AI <noreply@pi.dev>
The review pack carried a findings-era name and a bare security_version.
Rename it to agent checks (agent-checks.json) with an engine {name, version}
block, and have `check --json` return agent_checks_path instead of embedding
the whole pack. Update the check output and instructions wording, and take
the prompt fixes for SQL injection, XSS and unsafe innerHTML so they no
longer refer to a review pack or prompt hashes.
Co-authored-by: AI <noreply@pi.dev>
Agents now submit their investigation results through a new hidden `app security record` command. It reads one findings document from stdin, validates it all-or-nothing in the engine (redacting agent text and snapshotting check metadata), and replaces agent-findings.json. The check output, instructions, and agent-checks text now point agents at `record`. Co-authored-by: AI <noreply@pi.dev>
`check --clean` mixed two jobs: scanning and deleting local review work. Add a dedicated `app security clean` command that removes every current and legacy artifact (scan, agent checks, agent findings, submission, trace/review/findings.json) without asking, and drop the flag. Check now only replaces deterministic-findings.json and agent-checks.json, so it is always safe to run again. Co-authored-by: AI <noreply@pi.dev>
Add a hidden `app security review` command that prints the stored scan and recorded agent findings, each with its path and age. The two files are shown as stored, without being compared with each other or with the current source. The instructions and command output now point to it. Co-authored-by: AI <noreply@pi.dev>
f86890c to
245343a
Compare
|
| Command | Flag |
|---|---|
app:security:check |
--clean |
app:security:check |
--findings |
🔧 Removed Environment Variables
The following env vars are no longer referenced in command flags:
| Env Var | Previously Used By |
|---|---|
SHOPIFY_FLAG_APP_SECURITY_FINDINGS |
app:security:check --findings |
3 participants
WHY are these changes introduced?
App Security attested that agent results matched the scanned source, using input hashing, fingerprints and
source_scan_id/prompt_hashmatching. That coupling made the flow brittle and hard to reason about, while the results are informational.WHAT is this pull request doing?
Reports where an app stands right now, without comparing the deterministic and agent artifacts:
checkwritesdeterministic-findings.json(wastrace.json) andagent-checks.json(checks for the coding agent, wasreview.json) on every run, without prompting.record(new) validates the agent's findings document from stdin and writesagent-findings.jsonreview(new, rough) prints both artifacts with their ages; the next PR in the stack replaces it.clean(new) removes current and legacy artifacts.submit(rough) stubbed. It uses upload schema version 0 so the server rejects it; the next PR in the stack replaces it.New artifacts are durable against cli change: a generated
agent-findings.jsonsnapshots key check information, enablingreviewto continue to interpret results regardless of whether the check still exists in cli.Removes attestation, input hashing, fingerprints, suppressions, and the compile and external paths.
This change is intended to land in / squash with the stack.
How to manually test your changes?
pnpm shopify app security check --path /path/to/app pnpm shopify app security record --path /path/to/app < findings.json pnpm shopify app security review --path /path/to/app pnpm shopify app security clean --path /path/to/appChecklist
patchfor bug fixes ·minorfor new features ·majorfor breaking changes) and added a changeset withpnpm changeset add