Move App Doctor route authentication review to the agent

.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.

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.

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.