From 776deb565b3ba8c5d15faf74157034c1f0faa88a Mon Sep 17 00:00:00 2001 From: Mar Lopez Date: Fri, 18 Sep 2026 17:11:16 -0400 Subject: [PATCH] Move App Doctor route authentication review to the agent Assisted-By: devx/6a87267f-002b-45a7-a964-2b150210ff87 --- .changeset/agentic-route-auth-review.md | 5 + .../app-doctor-engine/INSTRUCTIONS.md | 2 + .../checks/UNAUTHENTICATED_ENDPOINT.md | 194 ++++++++++-------- .../app-doctor-engine/checks/embedded.ts | 4 +- .../app-doctor-engine/rules/catalog.ts | 6 +- .../app-doctor-engine/rules/js-rules.ts | 38 ---- .../app-doctor-engine/scanners/index.ts | 5 - .../tests/deterministic-rules.test.ts | 33 +-- .../tests/scan-contract.test.ts | 31 +++ 9 files changed, 151 insertions(+), 167 deletions(-) create mode 100644 .changeset/agentic-route-auth-review.md diff --git a/.changeset/agentic-route-auth-review.md b/.changeset/agentic-route-auth-review.md new file mode 100644 index 00000000000..ecd5327032f --- /dev/null +++ b/.changeset/agentic-route-auth-review.md @@ -0,0 +1,5 @@ +--- +'@shopify/app': patch +--- + +Move App Doctor route authentication review to agentic analysis to avoid false positives for non-template authentication. diff --git a/packages/app/src/cli/services/app-doctor-engine/INSTRUCTIONS.md b/packages/app/src/cli/services/app-doctor-engine/INSTRUCTIONS.md index a7f7a20b9cc..c22edbf822e 100644 --- a/packages/app/src/cli/services/app-doctor-engine/INSTRUCTIONS.md +++ b/packages/app/src/cli/services/app-doctor-engine/INSTRUCTIONS.md @@ -136,3 +136,5 @@ When the user explicitly wants a fast local or CI scan without semantic investig ``` Honor the installed CLI's documented JSON and blocking flags when requested. Do not describe a deterministic-only scan as the full App Doctor review. + +Route authentication is reviewed by the agentic `UNAUTHENTICATED_ENDPOINT` check. A deterministic-only scan does not verify route authentication; an unreported agent review remains unresolved in the trace. diff --git a/packages/app/src/cli/services/app-doctor-engine/checks/UNAUTHENTICATED_ENDPOINT.md b/packages/app/src/cli/services/app-doctor-engine/checks/UNAUTHENTICATED_ENDPOINT.md index 81e58aece5a..7342c9ee4dd 100644 --- a/packages/app/src/cli/services/app-doctor-engine/checks/UNAUTHENTICATED_ENDPOINT.md +++ b/packages/app/src/cli/services/app-doctor-engine/checks/UNAUTHENTICATED_ENDPOINT.md @@ -1,92 +1,112 @@ --- id: UNAUTHENTICATED_ENDPOINT -version: 1 +version: 2 severity: high --- -Find controller actions or route handlers that process requests without -verifying the caller's identity or authority. - -An unauthenticated endpoint in a Shopify app is one where a request from -outside the merchant's session — another app, a scraper, an attacker — can -reach a handler that reads or modifies shop data. Authentication in Shopify -apps comes in several forms, and the right one depends on the entry point: - -- **Embedded app pages** use `authenticate.admin(request)` (Remix) or - `ShopifyApp::EnsureHasSession` (Rails). The session is validated by a - JWT or session cookie. -- **Webhooks** are verified by HMAC signature - (`ShopifyApp::WebhookVerification`, `@shopify/webhook-processer`). -- **App proxies** are verified by signature query parameters. -- **Service-to-service** endpoints use bearer tokens or mTLS. - -A handler is unauthenticated if none of these apply AND the handler -accesses shop-scoped data. A public endpoint that returns static content -is not a finding. - -## What to look for - -1. **Find every route handler.** In Rails, these are controller actions - (methods in a controller class). In Remix/Express, these are loader - and action exports, or route handlers. In PHP, these are controller - methods or route closures. - -2. **For each handler, check whether authentication is applied.** Look for: - - A `before_action` / `beforeAction` / middleware that validates the - session (often inherited from a parent controller — follow the chain) - - An `authenticate.admin(request)` call in a Remix loader/action - - An HMAC verification for webhook/proxy endpoints - - A bearer token check for service endpoints - -3. **Follow inheritance.** A controller with no visible `before_action` - may inherit one from `ApplicationController` or a parent. Read the - parent class before flagging. - -4. **Check for `skip_idor_protection` or `protect_from_forgery` exceptions.** - These are explicit opt-outs — the author chose to skip a protection. - Determine whether the remaining auth (if any) is sufficient. - -5. **Check for the try/catch fallback pattern:** - ```js - try { - authenticate.admin(request); - } catch { - url.searchParams.get("shop"); - } - ``` - This looks authenticated but falls back to user input on failure — the - catch block bypasses auth entirely. This is a real finding. - -## What to report - -For each genuinely unauthenticated handler that accesses shop data: - -```json -{ - "file": "app/controllers/...", - "line": 42, - "message": "Controller action processes shop data with no auth verification", - "snippet": "def export\n Order.all.to_csv\nend", - "evidence": [ - { "file": "path", "line": 12, "quote": "the line showing no auth" }, - { - "file": "path", - "line": 5, - "quote": "class FooController < ApplicationController (no before_action in parent)" - } - ], - "confidence": "high", - "reasoning": "why this handler is reachable without authentication" -} -``` - -Do not report: - -- Public endpoints that return static content (health checks, app metadata) -- Handlers protected by inherited `before_action` (verify the parent first) -- Webhook/proxy handlers with HMAC verification -- Test controllers or development-only routes - -Every finding must cite the file and line where you determined auth is -absent. If you couldn't read the parent controller, say so — do not assume -auth is missing. +# Unauthenticated Endpoint + +Find route handlers and controller actions that reach a sensitive read or +write of shop-scoped data without a verified request on every path that +reaches it. + +A Shopify endpoint is unauthenticated when an outside request — another +app, a scraper, an attacker — can reach a handler that reads or modifies +shop data and no verification binds that request to an authenticated +principal before the sensitive action. The correct verification depends on +the entry point, and you must resolve the one actually in use rather than +recognize a spelling. + +## Resolve actual verification + +Do not match a token string. Trace the real authentication path for each +handler, including everything that hides it: + +- **Embedded app pages** may use `authenticate.admin(request)`, + `context.shopify.authenticate.admin(request)` (Remix/React Router), + `ShopifyApp::EnsureHasSession` (Rails), or an equivalent session validator. +- **Webhooks and app proxies** verify their signed request, for example + through `authenticate.webhook(request)` or + `authenticate.public.appProxy(request)`. Follow the framework's actual + signature verification before trusting request-derived shop context. +- **Service-to-service and staff/app endpoints** may use bearer tokens, + a staff session, or mTLS rather than merchant-session authentication. +- **Intentionally public routes** can return public data without merchant + authentication. Confirm that they do not expose credentials or protected + operations; a public-looking route name alone proves nothing. + +Follow every indirection before deciding a handler is verified or not: + +- Resolve the loaded context — `context.shopify.authenticate` may be + imported, destructured, injected, or built by a factory/DI container. +- Follow aliases, wrappers, re-exports, and helpers that call the real + authenticator on the caller's behalf. +- In Rails, follow `before_action` / `before_action :authenticate` up the + inheritance chain to the parent controller before flagging. +- In Express/Connect, follow middleware mounted above the route. +- `skip_idor_protection` and `protect_from_forgery` opt-outs are explicit + exceptions — determine whether the remaining verification is sufficient. + +## Verification must precede every sensitive action + +Verification must dominate the sensitive action on every path that reaches +it, not merely appear somewhere in the handler: + +- Asynchronous verification must be **awaited** or otherwise complete + before the sensitive read/write. Synchronous middleware can enforce the + same boundary without an `await` keyword. +- Verification failure must stop the protected operation. Propagating a + rejection or exception is valid; an explicit `try/catch` is not required. + A catch that swallows failure and continues to a sensitive sink is a bypass. +- An authenticator in an unrelated helper, an unexecuted branch, or after + the protected operation is not a barrier. Resolve receiver identity too: + a local no-op named `authenticate` does not verify anything. +- Bind verification to the actual incoming request and principal. A + separately supplied shop selector is not made trustworthy by authenticating + an unrelated session; investigate that under the tenant/authorization checks. + +## Evidence and standards + +Report a finding only when you can demonstrate all of: + +1. **Reachability** — the handler is reachable from an outside request on + a deployment path. Test/dev naming or a `test/` path is not proof of + unreachability in production; do not treat naming as either safe or + vulnerable. +2. **No verified request** — trace the actual authentication path, including + inheritance, middleware, aliases, and factories, and show a reachable + path to the sensitive action without completed verification that rejects + invalid requests. +3. **A sensitive sink** — show a protected read or write of shop-scoped + data, not merely a call named `graphql`, `prisma`, or `session`. Identify + the concrete data or authority exposed to an unauthenticated caller. +4. **Principal, source, guard, sink, impact** — cite the file/line of the + untrusted source, the missing or weak guard, the sensitive sink, and + the affected authority or data. + +Object authorization — acting on a shop other than the authenticated one — +is a distinct concern. Do not fold it into this check and do not report it +here unless the request itself is also unverified. + +## What is not a finding + +- A missing canonical import or helper name on its own — resolve the real + path before concluding. +- Code that is genuinely unreachable on deployment paths — but + unreachability asserted by naming convention alone is not a barrier; + prove the deployment boundary. +- An intentionally public endpoint that exposes no protected data or action. +- A webhook/proxy handler whose signature verification you confirmed. + +## When to stay unresolved + +If you could not read the parent controller, the mounted middleware, the +DI factory, or the dependency that supplies the authenticator, you have +not established verification and you have not established a finding — +record the check as unresolved with the specific missing input. A missing +dependency or out-of-scope middleware means unresolved, not "safe" and not +"vulnerable". + +Report every confirmed finding with file/line evidence for the source, +guard, sink, and impact, and record the check execution per the review +pack instructions. diff --git a/packages/app/src/cli/services/app-doctor-engine/checks/embedded.ts b/packages/app/src/cli/services/app-doctor-engine/checks/embedded.ts index e2ae6adcdb0..f16b73d9df0 100644 --- a/packages/app/src/cli/services/app-doctor-engine/checks/embedded.ts +++ b/packages/app/src/cli/services/app-doctor-engine/checks/embedded.ts @@ -33,11 +33,11 @@ export const EMBEDDED_CHECK_SOURCES: ReadonlyArray = [ "---\nid: STATIC_FRAME_ANCESTORS\nversion: 1\nseverity: high\n---\n\n# Static Frame Ancestors\n\nInspect embedded-app Content-Security-Policy construction. Report wildcard or static cross-shop frame-ancestors policies; accept a policy derived safely for the authenticated shop plus Shopify Admin.\n", "---\nid: TEXT_SETTING_HTML_SMUGGLING\nversion: 2\nseverity: high\n---\n\nFind cases where merchant-configurable text settings are rendered as HTML\nwithout sanitisation, allowing HTML to be smuggled past custom-JS detection.\n\nMany apps have settings fields where merchants can enter custom text —\nheader text, banner content, widget titles, custom CSS, \"additional\nHTML\" fields. If the app renders these settings as raw HTML server-side\nor in Liquid, the merchant can inject arbitrary HTML including script\ntags, iframes, or event handlers.\n\nThis is distinct from `UNSAFE_INNERHTML` (which targets JavaScript\nDOM writes). HTML smuggling via text settings bypasses custom-JS detection\nbecause the HTML is rendered server-side or in Liquid, not via\n`innerHTML` in JavaScript. A scanner looking for `innerHTML` will miss it.\n\n## What to look for\n\n1. **Find text/settings fields that accept arbitrary input.** Search for:\n - Rails: settings models, `ShopSetting`, `AppConfig`, preference columns\n that store text/HTML\n - Remix: settings mutations that store text in metafields or app data\n - Liquid: theme settings schema with text fields (`\"type\": \"text\"`,\n `\"type\": \"richtext\"`)\n - Any field named `header_html`, `custom_html`, `banner_text`,\n `additional_script`, `tracking_code`\n\n2. **Trace where the setting value is rendered.** For each setting:\n - Rails: `render html: setting.value`, `render(inline: setting.value)`,\n `<%= raw setting.value %>`, `setting.value.html_safe`\n - Liquid: `{{ setting.value }}` in HTML or executable contexts without context-appropriate escaping or structured rendering\n - JavaScript: if the setting value flows into `innerHTML` — this\n overlaps with `UNSAFE_INNERHTML` but the entry point is a settings\n field, not a URL param\n - React: `dangerouslySetInnerHTML` where the HTML comes from a\n settings/metafield value\n - Generated JavaScript/service workers, email or PDF rendering, operator/admin\n UIs, and previews that later treat the stored field as executable content\n - App Proxy HTML/Liquid/JavaScript responses or script/iframe URL builders fed\n by the same persisted setting\n\n3. **Check whether the setting is merchant-configurable.** The key\n question is: can the merchant (or an attacker who compromised the\n merchant account) control this value?\n - If the value is set by the app developer in code — not user-controlled\n - If the value is set by the merchant through a settings UI — user-controlled\n - If the value is stored in a metafield that the merchant can edit — user-controlled\n - If the value comes from a product/customer metafield — user-controlled\n\n4. **Check for sanitisation.** Is the setting value sanitised before\n rendering?\n - `sanitize_html(setting.value)` or equivalent\n - Content type set to `text/plain` instead of `text/html`\n - Context-appropriate Liquid filters such as `escape`, `json`, or `metafield_tag`\n - Allowlist of permitted HTML tags\n\n5. **Look for richtext settings.** Theme extension settings with\n `\"type\": \"richtext\"` are designed to accept HTML — but if the app\n renders them without sanitisation in a context where script execution\n is possible, it's still a vulnerability.\n\n6. **Trace every persisted field to every renderer.** A settings write that looks\n harmless in one route can become executable later in a preview, email, PDF,\n service worker, or operator/admin UI. Do not stop at the first renderer.\n\n## What to report\n\nFor each setting rendered as HTML without sanitisation:\n\n```json\n{\n \"file\": \"app/controllers/settings_controller.rb\",\n \"line\": 20,\n \"message\": \"Merchant-configurable header_text setting rendered as raw HTML\",\n \"snippet\": \"render html: shop_setting.header_text\",\n \"evidence\": [\n {\n \"file\": \"app/controllers/settings_controller.rb\",\n \"line\": 20,\n \"quote\": \"render html: shop_setting.header_text\"\n },\n {\n \"file\": \"app/models/shop_setting.rb\",\n \"line\": 5,\n \"quote\": \"field :header_text, :text (merchant-configurable)\"\n }\n ],\n \"confidence\": \"high\",\n \"reasoning\": \"The header_text setting is merchant-configurable and rendered as raw HTML without sanitisation. A merchant (or attacker who compromised the merchant account) can inject arbitrary HTML including script tags.\"\n}\n```\n\nDo not report:\n\n- Settings rendered as `text/plain` (no HTML parsing)\n- Settings that are developer-configured constants (not merchant-editable)\n- Settings with explicit HTML sanitisation (`sanitize_html`, allowlist)\n- Liquid output with context-appropriate escaping/serialization for its destination\n- `{% raw %}` blocks that contain no dynamic output\n- Test files\n", "---\nid: THEME_EXTENSION_XSS\nversion: 2\nseverity: high\n---\n\nFind cases where theme app extensions render user-controlled data\nwithout escaping, creating XSS vulnerabilities in the merchant's\nstorefront.\n\nTheme app extensions render Liquid in the merchant's storefront. If\nthe Liquid renders user-controlled data (metafields, product data,\ncustomer input, URL parameters) without proper escaping, an attacker\ncan inject script that executes on the merchant's store — affecting\nevery visitor to that store.\n\nThis is distinct from `UNSAFE_INNERHTML` (which targets JavaScript\nDOM writes). Theme extension XSS happens in Liquid, server-side,\nthrough Shopify's rendering pipeline. The entry point is a Liquid\ntemplate, not a JavaScript file.\n\n## What to look for\n\n1. **Find all Liquid files in theme app extensions.** Search for:\n - `*.liquid` files under `extensions/` or `theme-app-extension/`\n - Liquid blocks, snippets, sections\n - Theme extension entry points\n\n2. **Inspect every dynamic output context.** Shopify Liquid output is not automatically HTML-escaped, and `{% raw %}` suppresses Liquid parsing so apparent output inside it is literal text. Determine whether each value uses context-appropriate handling such as `escape`/`escape_once` in HTML text or ordinary attributes, `json` when embedding JavaScript data, or `metafield_tag` for supported rich metafield rendering. HTML escaping is not sufficient for event handlers, `srcdoc`, or `