MCP 2026-07-28 readiness: Waves 0-2 + adversarial review round (16 defects found and fixed)

.github/workflows/ci.yml

@@ -161,7 +161,18 @@ jobs:
       # in lexical (file-name) order — which matches the numbered
       # migration convention. Newline-separated so the shell loop can
       # iterate cleanly.
+      #
+      # `middleware/migrations` is the core runtime domain — the shared
+      # schema for the main app's features. It is self-contained: no
+      # cross-domain FKs and no extension dependencies, so its position
+      # in this list is not load-bearing.
+      #
+      # Still uncovered (each needs its own audit before enabling):
+      # middleware/src/conductor/migrations,
+      # middleware/src/services/graph/migrations,
+      # middleware/packages/harness-memory-postgres/src/migrations.
       MIGRATION_DOMAINS: |
+        middleware/migrations
         middleware/packages/harness-knowledge-graph-neon/src/migrations
         middleware/src/auth/migrations
         middleware/src/plugins/routines/migrations

docs/adr/0006-mcp-client-id-metadata-documents.md

@@ -0,0 +1,172 @@
+# 0006 — Client ID Metadata Documents coexist permanently with manual OAuth clients
+
+## Status
+
+Accepted
+
+- **Date:** 2026-07-30
+- **Deciders:** omadia maintainers
+- **Supersedes:** —
+
+## Context and Problem Statement
+
+omadia has shipped a provider-agnostic MCP OAuth 2.1 + PKCE stack since epic
+#459 W9. To obtain the OAuth client it presents to an authorization server it
+had two paths: RFC 7591 Dynamic Client Registration (DCR), and a client an
+operator registers once by hand. The MCP authorization spec now deprecates DCR
+in favour of **Client ID Metadata Documents (CIMD)**, where the `client_id` is
+an https URL the authorization server *dereferences* to read `redirect_uris`
+and `client_name`.
+
+Issue #546 framed CIMD as "the enterprise answer" and implied the manual client
+was a stopgap. Should omadia adopt CIMD as the new default and sunset the manual
+path?
+
+## Decision Drivers
+
+- **The two real IdPs in enterprise deployments — Microsoft Entra ID and Okta —
+  do not support CIMD.** They use pre-registered app registrations. For them the
+  manual client is not a workaround; it is the protocol-correct path.
+- **CIMD inverts the network direction.** Every other mode only requires omadia
+  to reach *out*: a redirect the operator's browser follows, an outbound token
+  POST. CIMD requires the identity provider to reach *in* and GET a URL on
+  omadia's own host. That is a strictly stronger requirement.
+- Many omadia installs are on-premises or behind a corporate firewall and have
+  **no inbound HTTPS route at all**. A design that needs one cannot be the only
+  path.
+- DCR's deprecation is on a 12-month clock, and brokers that only offer DCR must
+  keep working throughout.
+- Single tenancy: **no table in any migration carries a tenant column.** One
+  omadia install serves one organization, so a CIMD `client_id` identifies the
+  whole install.
+
+## Considered Options
+
+- **A — CIMD as one link in an explicit chain; manual stays permanent.**
+- **B — CIMD as the default, manual deprecated behind a compatibility flag.**
+- **C — byte5 hosts a metadata relay so every install gets a working
+  `client_id` regardless of inbound reachability.**
+
+## Decision Outcome
+
+Chosen option: **A**.
+
+The client-acquisition chain is explicit and ordered:
+
+```
+stored → cimd → dcr (deprecated, warns) → manual → McpOAuthNeedsClientError
+```
+
+CIMD is attempted **only** when both sides agree it can work: the authorization
+server advertises `client_id_metadata_document_supported`, a metadata URL is
+configured, and the document is verifiably reachable. Any of those failing makes
+the chain fall through — never fail.
+
+**Option C was rejected outright.** A byte5-hosted relay would mean every
+customer's `client_id` is a URL on a byte5 domain, so every customer's OAuth
+client would *identify byte5* to that customer's identity provider. That is
+wrong on identity grounds before it is wrong on availability grounds, and it
+inserts byte5 into a customer's authorization path. It is not offered by
+default, and it is not planned.
+
+**Option B was rejected** because it inverts which path is load-bearing: it
+would deprecate the only mode Entra ID and Okta can use.
+
+### Consequences
+
+- 🟢 **Good:** CIMD removes the app-registration step at MCP-native brokers
+  (Smithery-class) without changing anything for IdP-backed servers.
+- 🟢 **Good:** A firewalled install degrades cleanly. The metadata endpoint
+  answers **501 with an actionable message**, not 500, and the manual path keeps
+  working untouched.
+- 🟢 **Good:** DCR keeps working and merely logs a deprecation notice, so no
+  broker breaks on our timeline.
+- 🔴 **Bad:** Three acquisition modes is more surface than two. Mitigated by
+  making the chain a single ordered function and surfacing the resolved mode in
+  the UI, so an operator can always see which one applies and why.
+- 🔴 **Bad:** Reachability cannot be *proven* from inside the process — only the
+  IdP's own network can answer it. We check the conditions that make the answer
+  definitively "no" (see below) and treat a "yes" as a strong necessary
+  condition, not a guarantee.
+- ⚪ **Neutral:** `mcp_oauth_clients.registered_via` gains `'cimd'` and a
+  `client_metadata_url` column (migration `0032`).
+
+## Deployment note
+
+**CIMD requires inbound HTTPS reachability.** Set `FLOW_PUBLIC_BASE_URL` to an
+https origin this deployment is reachable at **from the internet**. The identity
+provider fetches `GET {FLOW_PUBLIC_BASE_URL}/.well-known/omadia-mcp-client`
+itself; an origin that only resolves inside your network will not do.
+
+- The metadata URL is derived from `FLOW_PUBLIC_BASE_URL` **alone** —
+  deliberately not the `?? PUBLIC_BASE_URL` fallback the redirect URI uses,
+  because `PUBLIC_BASE_URL` defaults to `http://localhost:3979`, exactly the
+  shape that is not inbound reachable. Requiring an explicit declaration means an
+  unconfigured install lands in the clean degraded state rather than publishing a
+  `client_id` no provider can fetch.
+- It is derived from **config, never from the inbound `Host` header**, so it is
+  stable across restarts and proxy hops. The URL *is* the `client_id` and stored
+  `mcp_oauth_clients` rows reference it.
+- The served `redirect_uris` must equal `McpOAuthService.redirectUri` exactly. If
+  they diverge, the authorization server matches the authorize request's
+  `redirect_uri` against the document, finds no match, and every code exchange
+  fails — at the provider, far from the cause. Both are wired from one variable
+  in `index.ts`, and `middleware/test/mcpOAuth.test.ts` asserts the equality.
+- **If inbound access is not possible, nothing is broken.** Register a one-time
+  OAuth client per issuer in the MCP Control Center. That path is fully
+  supported, permanently, and is the correct path for Entra ID and Okta.
+- **A byte5-hosted metadata relay is not offered by default** — see Option C
+  above.
+- **Single tenancy is the reality.** No migration defines a tenant column. One
+  install serves one organization, `mcp_oauth_clients` is keyed by issuer alone,
+  and the CIMD `client_id` identifies the install as a whole. Do not read
+  multi-tenancy into this schema.
+
+### Reachability check
+
+The probe (`services/mcpCimd.ts`) reuses `assertPublicHttpsUrl` from
+`services/ssrfGuard.ts` — the same guard the RFC 9728 / RFC 8414 discovery chain
+uses, deliberately not a second validator. It rejects:
+
+1. no configured public base origin;
+2. plain http, and RFC 1918 / loopback / link-local / CGNAT literals,
+   `.internal` / `.local` names, and hostnames that DNS-resolve into those
+   ranges;
+3. a URL that does not serve *our* document — the fetched document's `client_id`
+   must equal the URL fetched, so a catch-all proxy route answering `200` with
+   something else is caught.
+
+The verdict is cached (5 min, single-flight) because it runs on every MCP Control
+Center status poll.
+
+## Security properties preserved
+
+- A CIMD client is **public by construction**: the document is world-readable, so
+  there is no client secret and PKCE alone protects the exchange.
+  `token_endpoint_auth_method` is `"none"` — an accurate claim, not a shortcut.
+- The metadata document carries **no secret**: only the redirect URI and a
+  display name, both of which the IdP already sees during the authorize
+  round-trip.
+- Tokens stay in the vault namespace; `mcp_oauth_tokens` holds refs only.
+- No token, `code`, or `code_verifier` reaches a log line — OAuth error text goes
+  through `services/secretRedaction.ts`.
+- `mcp_oauth_flows` TTL is enforced in two places (verified, not assumed): an
+  opportunistic prune of rows older than 15 minutes on every flow create, and an
+  age-bounded `DELETE … RETURNING` on consume, so a leaked stale `state` cannot
+  be redeemed later even if no prune has run.
+- The flow-bound endpoint pinning and the RFC 9207 `iss` validation added in W0-1
+  are untouched.
+
+## More Information
+
+- Issue #546 (CIMD half, W2-4). The issue body's premise that the registry
+  "supports only static headers with `secretRef`" is **incorrect** — the full
+  OAuth 2.1 + PKCE stack shipped in epic #459 W9.
+- Implementation: `middleware/src/services/mcpCimd.ts`,
+  `middleware/src/services/mcpOAuthService.ts` (`ensureClient`),
+  `middleware/src/routes/mcpClientMetadata.ts`,
+  `middleware/src/auth/publicPaths.ts`,
+  `middleware/migrations/0032_mcp_oauth_cimd.sql`.
+- Tests: `middleware/test/mcpOAuth.test.ts`,
+  `middleware/test/publicPaths.test.ts`,
+  `middleware/test/mcpOAuthCimdMigration.pg.test.ts`.

docs/adr/README.md

@@ -33,6 +33,7 @@ decision, write a new ADR and mark the old one **Superseded by …**.
 | 0003 | [Capability-based, multi-provider middleware](0003-capability-based-multi-provider-middleware.md) | Accepted | 2026-06-03 |
 | 0004 | [Knowledge graph as the agent memory substrate](0004-knowledge-graph-as-memory-substrate.md) | Accepted | 2026-06-03 |
 | 0005 | [Two-phase confirmation for write-capable connectors](0005-two-phase-confirmation-for-writes.md) | Accepted | 2026-06-03 |
+| 0006 | [Client ID Metadata Documents coexist permanently with manual OAuth clients](0006-mcp-client-id-metadata-documents.md) | Accepted | 2026-07-30 |
 
 > These first records are written *retroactively* — they document decisions that
 > were already implemented and proven in the product. New decisions should be

middleware/migrations/0031_mcp_oauth_iss_delegation.sql

@@ -0,0 +1,145 @@
+-- ── MCP OAuth: RFC 9207 `iss` binding + explicit delegation mode (W0-1) ─────
+-- Three live defects in the MCP OAuth path are closed here:
+--
+--  D1  The callback trusted `state` alone and never validated the RFC 9207
+--      `iss` authorization-response parameter against the issuer recorded for
+--      the flow. `mcp_oauth_flows.issuer` already exists; what was missing is
+--      knowing whether the authorization server ADVERTISED iss support, so an
+--      absent `iss` from an AS that promised one can be rejected. That flag is
+--      captured at authorize time (never re-discovered at callback — same
+--      reasoning as migration 0016).
+--
+--  D2  `oauthUserKey()` silently fell back to the shared literal 'operator',
+--      so a channel turn (Teams/Telegram) with no mapped identity inherited
+--      the operator's authority at the customer's MCP server — a confused
+--      deputy. `mcp_servers.delegation` makes the choice explicit per server:
+--        per_user → the acting identity must resolve, or the call fails closed
+--        service  → one shared identity is the deliberate, opted-in design
+--
+--  D3  Concurrent refreshes for the same (server, user) raced each other. Not
+--      a schema concern, but `mcp_oauth_tokens.issuer` lands here so a stored
+--      token can be invalidated when its issuer rotates.
+--
+-- ⚠️ OPERATOR-VISIBLE BEHAVIOUR CHANGE — read before deploying.
+-- A fail-closed `per_user` default for EVERY row would break installed systems
+-- whose channel users reach MCP servers today precisely BECAUSE of the
+-- 'operator' fallback. So this migration is deliberately asymmetric:
+--   • when `mcp_servers.delegation` is introduced, pre-existing rows that
+--     already hold an operator token are grandfathered to today's shared
+--     behaviour (`delegation = 'service'`),
+--   • operators who later opt an existing server into `per_user` stay there on
+--     every re-apply, because that grandfathering runs only on the first
+--     application that adds the column, and
+--   • only NEWLY created servers get the safe `per_user` default.
+-- That one-time asymmetry is intentional. A new server that later acquires an
+-- operator token is still `per_user` unless an operator deliberately chooses
+-- shared delegation in the MCP Control Center (or UPDATEs the column directly).
+
+-- ── D2: explicit delegation mode per MCP server ─────────────────────────────
+-- The column introduction and the compatibility backfill are one decision and
+-- must happen exactly once. Re-applying 0031 after an operator has opted an
+-- existing server into `per_user` must preserve that operator decision:
+-- changing `delegation` does NOT delete the stored operator token row, so a
+-- standing backfill would silently flip the server back to `service` on the
+-- next apply and reopen D2 for unmapped channel users.
+--
+-- Backward compatibility on that first apply only: every EXISTING server that
+-- already holds an OPERATOR token keeps the shared identity it is working with
+-- today. Guarded by `to_regclass` so the migration is safe on a database where
+-- `mcp_oauth_tokens` has not been created yet. The argument is UNQUALIFIED on
+-- purpose: it must resolve through `search_path` like every other reference in
+-- this file. A hardcoded `public.` would probe the wrong schema wherever the
+-- domain is applied outside `public` — the guard would then answer about a
+-- table this statement does not touch, and the backfill would be skipped (or
+-- run) for a reason unrelated to the data in front of it.
+--
+-- The predicate is `user_key = 'operator'`, NOT "has any token row". This
+-- backfill exists solely to preserve the behaviour the 'operator' fallback was
+-- producing (see D2 above), and that fallback only ever applied where an
+-- operator token existed to borrow. A server holding only per-user tokens —
+-- `user_key = 'alice@corp.com'` and nothing else — was never using a shared
+-- identity, so flipping it to `service` would be a silent identity change no
+-- operator decided on: `resolveMcpUserKey` would hand every caller the shared
+-- `operator` key, and once anyone completed a re-auth the minted operator token
+-- would be shared by every caller, including unmapped channel users. The
+-- narrow predicate leaves such a server on the safe `per_user` default, which
+-- is the choice its stored tokens already imply.
+--
+-- The literal must stay in sync with `SERVICE_USER_KEY` in
+-- `src/services/mcpDelegation.ts` (a migration cannot import it).
+DO $$
+DECLARE
+  delegation_exists BOOLEAN;
+BEGIN
+  SELECT EXISTS (
+    SELECT 1
+      FROM pg_attribute
+     WHERE attrelid = 'mcp_servers'::regclass
+       AND attname = 'delegation'
+       AND NOT attisdropped
+  )
+    INTO delegation_exists;
+
+  IF NOT delegation_exists THEN
+    -- Plain ADD COLUMN, not IF NOT EXISTS: the catalog gate above already
+    -- proved absence, and masking a broken gate here would turn a logic error
+    -- into silent drift instead of failing loudly at the statement that broke.
+    ALTER TABLE mcp_servers
+      ADD COLUMN delegation TEXT NOT NULL DEFAULT 'per_user';
+
+    IF to_regclass('mcp_oauth_tokens') IS NOT NULL THEN
+      UPDATE mcp_servers s
+         SET delegation = 'service'
+       WHERE EXISTS (
+         SELECT 1 FROM mcp_oauth_tokens t
+          WHERE t.server_id = s.id
+            AND t.user_key = 'operator'
+       );
+    END IF;
+  END IF;
+END $$;
+
+-- `conname` is unique per (connamespace, conrelid), NOT cluster-wide, so an
+-- unanchored lookup reports "exists" for a same-named constraint sitting in any
+-- other schema and the ALTER below is silently skipped. Anchoring on
+-- `conrelid = 'mcp_servers'::regclass` resolves the relation through
+-- `search_path`, matching every other unqualified reference in this file. The
+-- cast cannot raise here for any new reason: this migration still requires
+-- `mcp_servers` to resolve, the gate block above already resolved the same
+-- regclass while checking whether `delegation` exists, and the old `ALTER TABLE
+-- ... ADD COLUMN IF NOT EXISTS` would likewise have failed on a missing table
+-- because `IF NOT EXISTS` guards the column name, not the relation.
+DO $$
+BEGIN
+  IF NOT EXISTS (
+    SELECT 1 FROM pg_constraint
+     WHERE conname = 'mcp_servers_delegation_chk'
+       AND conrelid = 'mcp_servers'::regclass
+  ) THEN
+    ALTER TABLE mcp_servers
+      ADD CONSTRAINT mcp_servers_delegation_chk
+      CHECK (delegation IN ('per_user', 'service'));
+  END IF;
+END $$;
+
+-- ── D1: remember whether the AS advertised RFC 9207 at authorize time ───────
+-- NULL on pre-0031 in-flight flows → treated as "not advertised", so a flow
+-- started before this migration is not retroactively rejected for a missing
+-- `iss`. A mismatched `iss` is rejected regardless of this flag.
+ALTER TABLE mcp_oauth_flows
+  ADD COLUMN IF NOT EXISTS iss_required BOOLEAN NOT NULL DEFAULT false;
+
+-- ── D3 companion: bind a stored token to the issuer that minted it ──────────
+-- Lets a token be invalidated when the server's issuer rotates instead of
+-- being replayed against a different authorization server.
+ALTER TABLE mcp_oauth_tokens
+  ADD COLUMN IF NOT EXISTS issuer TEXT;
+
+-- ── Audit: record the acting identity on every MCP call ─────────────────────
+-- `caller_agent` is the orchestrator/sub-agent slug, not WHO the call acted
+-- as. Without this an operator cannot answer "whose credentials touched that
+-- server?" — the exact question the confused-deputy bug raises.
+ALTER TABLE mcp_call_log
+  ADD COLUMN IF NOT EXISTS acting_identity TEXT;
+
+-- rollback: ALTER TABLE mcp_call_log DROP COLUMN acting_identity; ALTER TABLE mcp_oauth_tokens DROP COLUMN issuer; ALTER TABLE mcp_oauth_flows DROP COLUMN iss_required; ALTER TABLE mcp_servers DROP CONSTRAINT mcp_servers_delegation_chk, DROP COLUMN delegation;