feat(mt#1655): refine OAuth discovery stubs to RFC 8414 200-with-empty-flows

[minsky-ai]

Summary

mt#1635 (DONE) made the /mcp dialog badges go green by returning JSON 404s on the OAuth discovery endpoints instead of Express's HTML 404. But a residual SDK auth failed: ... line persisted because Claude Code's MCP SDK frames any non-2xx OAuth probe as failure, even when static-bearer auth is already working.

This task refines the stub: return 200 with RFC 8414 (Authorization Server Metadata) / RFC 9728 (Protected Resource Metadata) minimal documents that advertise the server's existence as an OAuth authority but declare zero usable flows (response_types_supported: [], no authorization_endpoint, no token_endpoint, no registration_endpoint). Spec-conformant SDKs treat this as "OAuth advertised but no flow usable → fall through to static-bearer," which should suppress the dialog cosmetic.

The metadata is a strict-subset of what mt#1634 (umbrella full-OAuth implementation) will produce. Not throw-away work — mt#1634 extends this foundation with real flow advertisements (authorization_endpoint, token_endpoint, registration_endpoint, grant_types_supported, etc.). /register stays at 400 since DCR is genuinely unsupported in the stub tier; mt#1634 will replace it.

Key changes

• src/commands/mcp/start-command.ts:
  • New exported builder functions buildAuthorizationServerMetadata(issuer) and buildProtectedResourceMetadata(resource) returning Object.freeze'd minimal-metadata documents.
  • Removed OAUTH_DISCOVERY_NOT_SUPPORTED_BODY (the 404+error-body constant from mt#1635).
  • OAUTH_REGISTER_NOT_SUPPORTED_BODY unchanged.
  • Added app.set("trust proxy", true) so Railway's X-Forwarded-Proto / X-Forwarded-Host populate req.protocol and req.get("host") correctly. Without this, req.protocol returns "http" (Railway terminates TLS at the edge), producing a wrong issuer URL the SDK would reject.
  • The two .well-known handlers now compute issuer/resource URL per-request from request headers, call the builders, and respond 200 (not 404).
• src/commands/mcp/start-command.test.ts:
  • 14 new tests covering both builders + the unchanged OAUTH_REGISTER_NOT_SUPPORTED_BODY.
  • Strict-subset assertions: builders explicitly do NOT include authorization_endpoint, token_endpoint, registration_endpoint, grant_types_supported, scopes_supported (the whole point of mt#1655).
  • Frozen-state and JSON round-trip pinning.
  • 26/26 tests pass.

Why a 200 with empty-flows works (when 404 didn't)

Response	What the SDK sees	What the SDK does
HTML 404 (pre-mt#1635)	Unparseable	JSON parse error shown literally; dialog badges red
JSON 404 + not_supported (mt#1635)	"OAuth probe failed"	Badges green (other paths working), but bottom line shows SDK auth failed: <our error_description>
JSON 200 + RFC 8414 with empty flows (mt#1655)	"OAuth metadata exists, no flows usable"	Spec-conformant: silently fall through to static-bearer; bottom line clears

Acceptance test coverage

#	Spec criterion	How tested
1	/.well-known/oauth-authorization-server returns 200 with issuer + response_types_supported: []	unit (start-command.test.ts) — builder shape pinned
2	Strict subset: NO flow endpoints advertised	unit — explicit toBeUndefined assertions for each excluded field
3	/.well-known/oauth-protected-resource returns 200 with resource only	unit
4	Both bodies frozen + JSON-serializable	unit
5	POST /register continues to return 400	unchanged from mt#1635; existing tests cover
6	curl https://...up.railway.app/.well-known/oauth-authorization-server returns 200 with the expected metadata	manual post-deploy
7	Claude Code /mcp dialog shows NO SDK auth failed line	manual post-deploy — this is the load-bearing acceptance test

Empirical-verification dependency

Criterion #7 is an SDK behavioral assertion — verifiable only by deploying and observing the dialog. If a 200-with-empty-flows still produces a misleading line, this task may need a second round to tweak the body shape (e.g., omit response_types_supported entirely, add a stub service_documentation field, etc.). The unit tests pin the JSON shape; the dialog observation is the load-bearing acceptance test.

Out of scope

• Wiring real OAuth flows (mt#1634).
• Identity model, PKCE, token issuance (mt#1634).
• ADR-006 amendments (mt#1634).
• Replacing /register's 400 response (DCR is genuinely unsupported until mt#1634).

Concurrency analysis

(N/A — no check-then-act pattern introduced. The builders are pure functions; the handlers are stateless responders that compute URL from per-request headers and return constant-shape JSON.)

Verification artifact

(N/A per implement-task §7a — pure-function builders with full unit-test coverage. The empirical SDK-behavior verification is operator-driven post-deploy: probe the live endpoints + observe the dialog.)

Test plan

• bun test src/commands/mcp/start-command.test.ts — 26/26 pass
• mcp__minsky__validate_typecheck — 0 errors
• mcp__minsky__validate_lint — 0 errors, 0 warnings
• Post-deploy: curl https://minsky-mcp-production.up.railway.app/.well-known/oauth-authorization-server returns 200 + RFC 8414 metadata
• Post-deploy: curl https://minsky-mcp-production.up.railway.app/.well-known/oauth-protected-resource returns 200 + RFC 9728 metadata
• Post-deploy: open Claude Code /mcp; minsky-hosted shows no SDK auth failed line

Origin

Discovered 2026-05-08, post-deploy of mt#1635: the dialog status went from red-failed to green-authenticated, but a residual SDK auth failed: Dynamic Client Registration is not implemented; ... line persisted. The user is implementing full OAuth in mt#1634 soon, so option B (refine to RFC 8414 200-with-empty-flows, which converges with mt#1634's foundation) was chosen over option A (accept residual line).

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

The PR moves OAuth discovery stubs from 404 to 200 with minimal RFC 8414/9728 metadata and introduces per-request URL composition plus trust proxy. While the builder functions are well-covered, there are gaps and risks in the route-level behavior. Blocking: (1) no tests exercise the actual .well-known HTTP handlers to assert 200 status, body shape, and header-derived URLs; (2) app.set("trust proxy", true) is a global behavior change without tests or scoping, which can affect security-relevant properties; (3) composing resource by blindly appending options.endpoint can generate invalid URLs when the option lacks a leading slash. Non-blocking: startup logs always print http:// which may mislead in TLS-fronted deployments. Please add route tests, scope/justify trust proxy, normalize endpoint, and then this will be in good shape.

Findings

• [BLOCKING] src/commands/mcp/start-command.test.ts:118 — No tests exercise the new HTTP .well-known handlers returning 200; only the pure builders are covered
  This PR changes the route behavior for /.well-known/oauth-authorization-server and /.well-known/oauth-protected-resource from 404 to 200 and computes issuer/resource per-request, but the test suite only validates the pure-function builders. There are no integration tests that actually start the HTTP server in --http mode and GET these endpoints to assert:

• status code is 200 (regression from prior 404)

• body shape matches the builders

• issuer/resource are computed from request headers as intended (including trust-proxy effects)

Given the per-request composition and the newly introduced app.set("trust proxy", true), this is a behavior change with real risk that isn't pinned by tests. Please add route-level tests (similar to the existing spawn-based shutdown tests) that hit both .well-known endpoints and assert 200 + expected JSON.

• [BLOCKING] src/commands/mcp/start-command.ts:231 — app.set("trust proxy", true) is a global behavior change that can impact req.ip/security logic; no tests or docs cover side effects
  Enabling Express's trust proxy globally affects multiple request properties (req.protocol, req.ip, req.secure, etc.). While it's motivated here to correct issuer/resource under Railway TLS termination, this change applies to all routes, including the auth-gated MCP endpoint. There are no tests validating that req.ip-dependent logic (present or future) isn't affected, nor any commentary limiting scope to proto/host parsing only.

At minimum:

• Constrain trust proxy to the known proxy hops (e.g., a function or numeric hop count) or document the security posture in code comments.
• Add a test that simulates X-Forwarded-Proto/X-Forwarded-Host and verifies the .well-known routes use https while unrelated logic (e.g., checkBearerAuth) remains unaffected.

Right now this is an unbounded, app-wide behavior change with no coverage.

• [BLOCKING] src/commands/mcp/start-command.ts:246 — resource URL generation blindly concatenates options.endpoint, which may be missing a leading slash and produces an invalid URL
  In /.well-known/oauth-protected-resource you build resource as:

const resource = `${req.protocol}://${req.get("host") ?? "localhost"}${options.endpoint}`;

options.endpoint is user-configurable via --endpoint and is not validated/normalized to ensure it starts with /. If a caller passes --endpoint mcp (no leading slash), this will emit https://example.commcp instead of https://example.com/mcp. Previously, the endpoint value only affected the Express route binding; now it's embedded into a public metadata document, so the impact of a malformed value is greater.

Please normalize options.endpoint once (e.g., ensure it starts with a single leading slash) or use path.posix.join("/", options.endpoint) when composing the URL, and add a test covering both with/without leading slash to pin the behavior.

• [NON-BLOCKING] src/commands/mcp/start-command.ts:303 — Hard-coded http:// in startup logs contradicts trust proxy/TLS reality and may confuse operators
  Startup logs always print MCP endpoint: http://${options.host}:${httpPort}${options.endpoint} and Health check: http://... even when the service is behind TLS termination and .well-known uses req.protocol with trust proxy to advertise https. This can mislead operators copying URLs from logs. Consider reflecting the externally observed protocol or adding a note that URLs are internal (plain HTTP) and may be fronted by TLS in production.

Inline comments

• src/commands/mcp/start-command.ts:241 — Minor: the issuer builder uses req.get("host") ?? "localhost". If Host is absent/malformed, the fallback yields http(s)://localhost, which may not be correct in production. Consider using req.hostname (honors trust proxy) or validating Host and returning a 500 to avoid emitting misleading metadata.
• src/commands/mcp/start-command.ts:246 — Edge case: when options.endpoint is /, the current concatenation yields a trailing / (fine). But when it’s an empty string or lacks the leading slash (e.g., mcp), the composed resource becomes invalid (https://example.commcp). Recommend normalizing options.endpoint once at option-parse time and reusing the normalized value everywhere.
• src/commands/mcp/start-command.test.ts:15 — Nice builder coverage. To fully exercise the route behavior you just changed to 200 + minimal metadata, consider starting the server in --http mode on a random port and fetching /.well-known/oauth-authorization-server and /.well-known/oauth-protected-resource to assert status 200 and the computed issuer/resource respecting X-Forwarded-Proto/Host.

Stale R1 review on commit bd0744c. All three BLOCKING findings + the NON-BLOCKING + inline comments addressed in commit 7994cd9: (1) added 4 HTTP-level integration tests spawning the server in --http mode and asserting 200 + body shape + X-Forwarded-Proto behavior, (2) tightened trust proxy from true to 1 (single-hop scope) with code comment documenting rationale, (3) extracted normalizeEndpointPath helper and applied to resource URL composition (closes the --endpoint mcp → https://example.commcp latent bug), and (4) added comment block above startup logs explaining the printed http:// is the internal listener vs the public TLS-fronted URL. Pure-function builder unit tests retained. Reviewer-bot silent 10+ min on the post-fix commit despite fixes being substantive — webhook miss class per CLAUDE.md feedback_self_authored_pr_merge_constraints. CI green on 7994cd9.