Repository navigation
Stop app security agent checks leaving the React Router template unresolved - #8830
Merged
Merged
Conversation
A fresh React Router app came back with METAFIELD_OFFLINE_TOKEN, MISSING_AUTHORIZATION_CHECK and STATIC_FRAME_ANCESTORS unresolved for some agents, though the template code is safe. The instructions told the agent to record unresolved whenever it couldn't prove exploitability, and the three prompts didn't describe the React Router SDK defaults. - Record executed when the code establishes the boundary a check asks about, and unresolved only for an incomplete investigation or a named candidate. - METAFIELD_OFFLINE_TOKEN v2 checks token provenance, so an offline session from authenticate.admin is no longer a finding by itself, and its catalog text no longer contradicts its fix. - MISSING_AUTHORIZATION_CHECK v3 treats authenticate.admin as Shopify's standard model, while still reporting skipped app-defined roles, other users' records and online-to-offline fallbacks. - STATIC_FRAME_ANCESTORS v2 accepts addDocumentResponseHeaders and still inspects headers set after or instead of it, including values built from variables. The deterministic version moves with it. - Add a deterministic fixture test of the template at a pinned commit, and a dev smoke script that runs codex and claude against the template and a planted-bug negative control. Co-authored-by: Qlaw <noreply@qlaw.quick.shopify.io>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The smoke runner can falsely succeed without executing agents and unsafely interpolates workspace paths into shell commands.
1 open finding
What changed in this PR
Calibrates App Security checks to correctly pass safe React Router template patterns while preserving concrete findings.
Changes:
- Refines unresolved-status guidance and three agent checks.
- Adds pinned-template regression coverage.
- Adds a reusable agent smoke-test harness and changeset.
| File | Description |
|---|---|
.changeset/app-security-unresolved-calibration.md |
Records the user-facing fix. |
bin/app-security-smoke/README.md |
Documents smoke-test usage. |
bin/app-security-smoke/run.js |
Adds template and negative-control agent runs. |
packages/app/src/cli/services/app-security-engine/INSTRUCTIONS.md |
Clarifies executed versus unresolved status. |
packages/app/src/cli/services/app-security-engine/checks/METAFIELD_OFFLINE_TOKEN.md |
Refines offline-token provenance rules. |
packages/app/src/cli/services/app-security-engine/checks/MISSING_AUTHORIZATION_CHECK.md |
Documents React Router authorization defaults. |
packages/app/src/cli/services/app-security-engine/checks/STATIC_FRAME_ANCESTORS.md |
Accepts SDK-managed CSP headers. |
packages/app/src/cli/services/app-security-engine/checks/embedded.ts |
Regenerates embedded prompts and instructions. |
packages/app/src/cli/services/app-security-engine/checks/index.ts |
Aligns generated agent guidance. |
packages/app/src/cli/services/app-security-engine/rules/catalog.ts |
Updates metafield check metadata. |
packages/app/src/cli/services/app-security-engine/scanners/index.ts |
Bumps the deterministic CSP check version. |
packages/app/src/cli/services/app-security-engine/tests/checks.test.ts |
Tests revised prompt guidance. |
packages/app/src/cli/services/app-security-engine/tests/deterministic-rules.test.ts |
Verifies the CSP version bump. |
packages/app/src/cli/services/app-security-engine/tests/fixtures/react-router-template.ts |
Adds the generated template fixture. |
packages/app/src/cli/services/app-security-engine/tests/react-router-template.test.ts |
Verifies expected deterministic results. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The default React Router template will be covered by the upcoming eval suite, so this change keeps only the prompt, rule and catalog updates and their unit tests. Add a unit test for the METAFIELD_OFFLINE_TOKEN catalog text. Co-authored-by: Qlaw <noreply@qlaw.quick.shopify.io>
STATIC_FRAME_ANCESTORS v2 covers policies built from variables as well as literal ones, and agent snapshots take their description from the catalog. Co-authored-by: Qlaw <noreply@qlaw.quick.shopify.io>
jek
approved these changes
Oct 7, 2026
jek
left a comment
jek
left a comment
Contributor
There was a problem hiding this comment.
Worked on my template:
shopify app security review --check-id METAFIELD_OFFLINE_TOKEN --check-id MISSING_AUTHORIZATION_CHECK --check-id STATIC_FRAME_ANCESTORS
│ 3 checks passed. │
│ │
│ METAFIELD_OFFLINE_TOKEN passed │
│ MISSING_AUTHORIZATION_CHECK passed │
│ STATIC_FRAME_ANCESTORS passed │
│ │
Contributor
Author
|
/snapit |
Contributor
|
🫰✨ Thanks @jplhomer! Your snapshot has been published to npm. Built from Test the snapshot by installing your package globally: pnpm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20261008143951Caution After installing, validate the version by running |
dmerand
approved these changes
Oct 8, 2026
dmerand
left a comment
dmerand
left a comment
Contributor
There was a problem hiding this comment.
LGTM, small LLM suggestion.
The glossary still said unresolved meant you couldn't finish the check or prove the issue, which contradicted the executed-versus-unresolved rule. It now means an incomplete investigation or a named candidate whose boundary is still unclear. Co-authored-by: Qlaw <noreply@qlaw.quick.shopify.io>
…ssary # Conflicts: # packages/app/src/cli/services/app-security-engine/tests/deterministic-rules.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WHY are these changes introduced?
A fresh
shopify app initReact Router app (main-clitemplate) can come back fromshopify app security reviewwith a warning: "3 checks unresolved" (METAFIELD_OFFLINE_TOKEN, MISSING_AUTHORIZATION_CHECK, STATIC_FRAME_ANCESTORS). Context: https://shopify.slack.com/archives/C0BT7EGQZ32/p1791391260952729The template code is safe for all three, and the result depends on the agent. Against the unmodified template on
main, codex recorded all three as unresolved in 5 of 7 passes, while claude recorded them clean in 3 of 3. The CLI never calls a model, so these statuses come only from the agent. Two things in the CLI's text push agents there:unresolvedwhenever the agent can't prove exploitability or affected authority. Agents read that as "unresolved unless I can prove a negative", even after they've verified the boundary.authenticate.admin(request), which uses an offline session by default (useOnlineTokens ?? false). STATIC_FRAME_ANCESTORS's agent result overrides its deterministic pass (prefer-agent).WHAT is this pull request doing?
INSTRUCTIONS.md,checks/index.ts): recordexecutedwhen the code establishes the boundary a check asks about. Useunresolvedonly when the investigation couldn't be completed, or for a named candidate the agent could neither settle nor show to be broken.unauthenticated.admin(shop)or a stored offline session reached from unverified input. An offline session fromauthenticate.admin(request)isn't a finding by itself. It's still reported when the write bypasses an app-defined per-user permission, or when request-controlled values reach$appstate that merchants can't edit. The$appnamespace alone isn't treated as safe. The catalog title, description and fix now say the same thing.authenticate.adminwith offline scopes is Shopify's standard model and not a finding by itself. Three cases are still reported: a skipped app-defined role or allowlist (comparing loader, action and sibling routes), another app user's records, and privileged paths that fall back from online to offline sessions.addDocumentResponseHeaders. The agent still inspects headers that app code sets after or instead of it, including values built from variables, which the deterministic regex can't evaluate. The deterministic version moves to 2 with it, as the registry requires.checks.test.ts,deterministic-rules.test.ts): cover the new rule wording, the prompt versions and key guidance, the METAFIELD_OFFLINE_TOKEN catalog text, and the STATIC_FRAME_ANCESTORS deterministic version.An eval of the default React Router template is deferred to the upcoming eval suite. In a manual pre-change comparison against the unmodified template, codex recorded all three checks as unresolved in 5 of 7 passes. With these prompts, codex and claude recorded them clean in 6 of 6 passes. Against a copy with one planted bug per check, they caught all three bugs in 6 of 6 passes.
How to manually test your changes?
shopify app init.shopify app security check, ask your coding agent to followshopify app security instructions, then runshopify app security review.Checklist
patchfor bug fixes ·minorfor new features ·majorfor breaking changes) and added a changeset withpnpm changeset addPR authored by Qlaw