Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions .changeset/remove-app-doctor-cve-audit.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
'@shopify/app': patch
---
Remove the dependency CVE audit check from `shopify app doctor`.
2 changes: 0 additions & 2 deletions packages/app/src/cli/commands/app/doctor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,6 @@ describe('app doctor command', () => {
expect(Doctor.descriptionWithMarkdown).toContain('copy the coding-agent instructions')
expect(Doctor.descriptionWithMarkdown).toContain('copying is the default')
expect(Doctor.descriptionWithMarkdown).toContain('shopify app doctor instructions')
expect(Doctor.descriptionWithMarkdown).toContain('npm audit')
expect(Doctor.descriptionWithMarkdown).toContain('https://registry.npmjs.org/')
})

test('allows --yes in JSON mode while preserving non-interactive output behavior', async () => {
Expand Down
4 changes: 1 addition & 3 deletions packages/app/src/cli/commands/app/doctor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@ export default class Doctor extends BaseCommand {

static descriptionWithMarkdown = `Runs Shopify App Doctor locally and creates its review pack and trace.

Pass \`--findings\` after completing the review pack to validate agent findings and compile them into the trace. In interactive terminals, the command offers to copy the coding-agent instructions, print them, or choose nothing; copying is the default. In CI and other non-interactive environments, instructions aren't offered unless you pass \`--yes\`, which prints them. JSON output never prompts or prints those instructions. You can also run \`shopify app doctor instructions\` to print, copy, or write them later.

CVE detection runs \`npm audit\` (or the pnpm/yarn equivalent) in an isolated sandbox and sends package names and versions to the public npm registry at https://registry.npmjs.org/.`
Pass \`--findings\` after completing the review pack to validate agent findings and compile them into the trace. In interactive terminals, the command offers to copy the coding-agent instructions, print them, or choose nothing; copying is the default. In CI and other non-interactive environments, instructions aren't offered unless you pass \`--yes\`, which prints them. JSON output never prompts or prints those instructions. You can also run \`shopify app doctor instructions\` to print, copy, or write them later.`

static description = this.descriptionWithoutMarkdown()

Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ export const EMBEDDED_CHECK_SOURCES: ReadonlyArray<string> = [
"---\nid: EOL_API_VERSION\nversion: 1\nseverity: low\n---\n\n# Eol Api Version\n\nInspect every unresolved `shopify.app*.toml` plus React Router `app/shopify.server.*` declarations. Shopify publishes quarterly versions in January, April, July, and October and supports each stable version for 12 months; App Doctor allows a documented 30-day extension grace period before reporting it as end-of-life. Cite the exact declaration. For malformed config, computed `ApiVersion` values, or a Shopify-announced exceptional extension, inspect the source and current lifecycle policy rather than inferring from unrelated constants.\n",
"---\nid: EXPIRING_OFFLINE_TOKEN\nversion: 1\nseverity: medium\n---\n\n# Expiring Offline Token\n\nFor supported React Router apps, verify `expiringOfflineAccessTokens` is enabled and the selected session storage persists `expires`, `refreshToken`, and `refreshTokenExpires` metadata needed for refresh and rotation. `isOnline: false` selects an offline session; it does not disable token expiry and is not a finding. Report an explicit `expiringOfflineAccessTokens: false`. Treat absent or computed flags, custom storage, and ambiguous Prisma schemas as unresolved investigation: inspect storage adapters, migrations, and serialization before returning a clean result. Config-only and unsupported frameworks are handled by the runtime applicability boundary.\n",
"---\nid: INSECURE_WEBHOOK_URL\nversion: 2\nseverity: high\n---\n\n# Insecure Configured Callback Url\n\nInspect webhook destinations and OAuth redirect URLs in every unresolved Shopify app configuration. Relative Shopify paths and valid pubsub/eventbridge webhook destinations are allowed. Report HTTP, malformed, credential-bearing, wildcard-host, wildcard-path, or otherwise unsafe configured callback URLs.\n",
"---\nid: KNOWN_CVE_IN_DEPENDENCY\nversion: 3\nseverity: medium\n---\n\n# Known Cve In Dependency\n\nWhen deterministic package-manager audit is unavailable, inspect the JavaScript manifest and lockfile statically for known vulnerable dependency versions. Do not execute the repository's package manager, scripts, plugins, binaries, or configuration. Distinguish version exposure from reachable impact: when source evidence allows it, check whether the app actually uses the vulnerable API or helper and whether the relevant configuration is enabled, especially for Shopify SDK behavior that depends on how a helper is called. If static evidence cannot confirm either the vulnerable version or the vulnerable API/helper reachability, mark the check unresolved instead of running repository-controlled code.\n",
"---\nid: LIQUID_UNSAFE_RENDER\nversion: 1\nseverity: medium\n---\n\n# Liquid Unsafe Render\n\nInspect only theme-extension Liquid/HTML files the parser could not analyze. Liquid output is not automatically HTML-escaped. Check the destination: use `escape`/`escape_once` for HTML text and ordinary attributes, `json` when embedding a value as JavaScript data, and `metafield_tag` only for supported rich metafield rendering in HTML content. HTML escaping is not sufficient for event handlers, `srcdoc`, or a `<script src>` URL; avoid merchant-controlled output there and validate any dynamic URL. An ordinary non-script `src` attribute is not itself an executable sink. `{% raw %}` suppresses Liquid parsing, so apparent output inside it is literal text. Follow rendered snippets and inspect ambiguous filters or parser failures before reporting.\n",
"---\nid: METAFIELD_OFFLINE_TOKEN\nversion: 1\nseverity: high\n---\n\n# Metafield Offline Token\n\nTrace metafield writes and determine whether they use an offline Admin API context where an authenticated online context is required. Confirm token provenance and compensating authorization before reporting.\n",
"---\nid: MISSING_AUTHORIZATION_CHECK\nversion: 2\nseverity: high\n---\n\nFind controller actions or route handlers that access resources without\nchecking whether the current user is authorized to access them, beyond\nauthentication. Authentication verifies WHO you are; authorization verifies\nWHAT you can do. An app can be authenticated but still access resources\nbelonging to another merchant if authorization checks are missing.\n\nThis is distinct from `MISSING_TENANT_ISOLATION` (database query\nscoping) — this check looks for missing policy/permission checks on\nactions, even when the data access is scoped. For example, an app might\nscope queries by shop but not check whether the merchant has the right to\ndelete a resource, or whether a staff member can access admin-only\nactions.\n\n## What to look for\n\n1. **Find authorization frameworks.** Check what the app uses:\n - Rails: Pundit (`authorize`, `policy`, `Pundit`), CanCanCan\n (`can?`, `ability`), action_access filters\n - Remix/Express: middleware that checks roles/permissions\n - Custom: `before_action :check_admin`, `if current_user.can?`\n\n2. **Find actions without authorization checks.** For each controller\n action or route handler, determine:\n - Is there a `before_action` that checks authorization (not just\n authentication)?\n - Is there a Pundit `authorize` call?\n - Is there a CanCanCan `authorize!` or `can?` check?\n - Is there a custom permission check?\n\n3. **Check for `skip_idor_protection` or equivalent opt-outs.** These\n disable IDOR/authorization checks. For each, determine:\n - Is the skip justified? (e.g., public endpoint, webhook, health check)\n - Does the skip expose a state-changing action to unauthorised users?\n - Is there a compensating control (HMAC, session token, etc.)?\n\n4. **Check for admin-only functionality reachable by merchants.** Look for:\n - Controllers under `admin/` namespace that don't check staff vs merchant\n - Actions that modify app configuration without checking the caller's role\n - Staff-only operations accessible through the merchant-facing UI\n\n5. **Check for missing object-level authorization.** Even if the query\n is scoped by shop, does the handler verify that the specific resource\n belongs to the current merchant?\n - `Order.find(params[:id])` scoped by shop — but does it check the\n merchant can access this specific order?\n - `Product.find(params[:id])` — is there a policy check, or just\n tenant scoping?\n\n6. **Compare authorization across state transitions.** Do not stop at one\n handler. Compare create/read/update/delete paths, replay after role or UI\n changes, direct URLs that survive permission downgrades, and backend actions\n that remain callable after a feature is hidden or disabled in the UI.\n\n## What to report\n\nFor each action that exposes a concrete unauthorized operation beyond authentication:\n\n```json\n{\n \"file\": \"app/controllers/orders_controller.rb\",\n \"line\": 15,\n \"message\": \"Destroy action has no authorization check beyond authentication\",\n \"snippet\": \"def destroy\\n Order.find(params[:id]).destroy\\nend\",\n \"evidence\": [\n {\n \"file\": \"app/controllers/orders_controller.rb\",\n \"line\": 15,\n \"quote\": \"def destroy\"\n },\n {\n \"file\": \"app/controllers/orders_controller.rb\",\n \"line\": 5,\n \"quote\": \"before_action :authenticate_user (no authorize check)\"\n }\n ],\n \"confidence\": \"medium\",\n \"reasoning\": \"The destroy action authenticates the user but does not call authorize or check a policy. Any authenticated merchant can delete any order within their shop, even if they shouldn't have delete permissions.\"\n}\n```\n\nOnly report when you can show an unauthorized operation is actually reachable;\ndifferent helper names or inconsistent UI affordances are not enough by themselves.\n\nDo not report:\n\n- Actions with explicit `authorize` / `can?` / policy checks\n- Actions protected by a `before_action` that checks authorization\n- Public endpoints (health checks, static content)\n- Webhook handlers (HMAC is the authorization)\n- Actions that only read data the merchant owns (scoped by session.shop\n AND no object-level access control needed)\n- Internal/staff-only controllers (under `Internal::` namespace, behind\n employee SSO like `EmployeeIdentity`, `IdentityClient`, etc.)\n- Test files (under test/ or \\*\\_test.rb)\n- Test controllers\n",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,15 +105,6 @@ export const RULE_CATALOG: RuleCatalogEntry[] = [
description: 'Detects direct credential flows into responses, loaders, globals, DOM, and external requests.',
fix: 'Keep credentials server-side and return only non-sensitive derived data.',
},
{
id: 'KNOWN_CVE_IN_DEPENDENCY',
title: 'Known CVE in dependency',
severity: 'high',
points: -20,
description:
'Runs the selected JavaScript package manager audit against production dependencies in the committed lockfile.',
fix: 'Upgrade the dependency to a patched version.',
},

{
id: 'EXPIRING_OFFLINE_TOKEN',
Expand Down
Loading
Loading