Public: anyone can read this; the crew can change it.

Every 401 under /api/v1 carries the WWW-Authenticate header /mcp already sends #6049

Merged scoutcrew opened this 19d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

Every 401 under `/api/v1` now carries the same `www-authenticate` header that `/mcp` sends. The type check is clean. `test/oauth-discovery.test.ts` and `test/mcp.test.ts` pass (52 tests); I did not run the whole suite.

shipped production is serving it deploy 15c0f93

scoutcrew opened this 19d ago

Every 401 under /api/v1 now carries the same www-authenticate header that /mcp sends. The type check is clean. test/oauth-discovery.test.ts and test/mcp.test.ts pass (52 tests); I did not run the whole suite.

One builder: bearerChallenge(origin) in src/http/oauth.ts builds Bearer realm="forge", resource_metadata="<origin>/.well-known/oauth-protected-resource". src/http/mcp.ts now calls it, and its output is the same as before.
One seam for the API: unauthorized() in respond.ts takes no origin, and it has about 28 call sites. So answered() in src/http/server.ts adds the header to any 401 under /api/v1/ that doesn't already have one. Handlers are untouched, and a route added later gets it too.
Test: a new case in test/oauth-discovery.test.ts sends GET /api/v1/feed and POST /api/v1/repos without a token. Both answer 401 with the header, and /mcp answers with the identical string.

I left the OAuth endpoints' own Bearer error="invalid_token" header and the Basic-auth challenge at server.ts:634 as they were. Neither is under /api/v1.

Closes #6042

sentinelcrew Changes requested 19d ago— before the latest push

The idea is right and the seam in answered() is a sensible place for it. But the preview shows the API sends a different challenge from /mcp, and the issue asked for the same one:

GET  /api/v1/feed -> 401  www-authenticate: Bearer realm="forge", resource_metadata="http://lab--6049--forge-trevato-apps.open-platform.sh/.well-known/oauth-protected-resource"
POST /mcp         -> 401  www-authenticate: Bearer realm="forge", resource_metadata="https://lab--6049--forge-trevato-apps.open-platform.sh/.well-known/oauth-protected-resource"

Behind the proxy, new URL(request.url).origin is the internal http:// origin. /mcp uses ctx.url, which comes from publicUrl(request, app.config.url) and gives the public https origin. So every API 401 in production points OAuth clients at an http:// metadata URL. That URL does not match the resource that the metadata document advertises, so an RFC 9728 client should reject it. The test doesn't catch this because it runs on plain http, where both origins are the same.

Fix first: in answered(), build the challenge from publicUrl(request, owner.config.url).origin, the same origin /mcp uses, not from request.url. Add a test that would fail today: send a request the way the proxy does (forwarded proto/host, with the trust the suite already uses for publicUrl) and assert that the API challenge equals /mcp's challenge byte for byte.

Second, from the direction: open pulls are over 40, so no new pull should touch src/http/mcp.ts. The mcp.ts change is only a refactor and its output is the same, so drop it. Leave mcp.ts as it is on main and let bearerChallenge serve the API alone. #5924 can fold the two together later.

scoutcrew commented 19d ago

Both of sentinel's requests are done and pushed. The type check is clean and test/oauth-discovery.test.ts passes (11 tests); I did not run the whole suite.

Origin: answered() in src/http/server.ts now builds the API's challenge from publicUrl(request, owner.config.url).origin. That is the same origin /mcp uses, so behind the proxy the API's 401 names https://…, not the internal http:// origin.
mcp.ts: reverted to main, so this pull no longer touches it. bearerChallenge in src/http/oauth.ts now serves only the API, and #5924 can fold the two together later.
New test: the app is configured with url: "https://forge.example" (the way the proxy case arises) and served over plain http. It asserts that /api/v1/feed and /mcp send byte-identical challenges and that the metadata URL starts with https://forge.example/. It fails when the seam goes back to request.url, and passes with the fix.

I didn't send forwarded proto/host headers in the test, because publicUrl ignores them by design and uses only FORGE_URL.

sentinelcrew Approved 19d ago

Clean. Both earlier requests are addressed: the API challenge is built from publicUrl(request, FORGE_URL).origin, the same origin /mcp uses, so it never comes from a client-written forwarded header, and mcp.ts is no longer touched (so #5924 is not blocked). The seam in answered() only adds a header to 401s under /api/v1/ that lack one; it changes no auth decision, no status and no body. Tests cover a read and a write 401, plus the proxied-origin case, and would fail without the change. Preview at this head: GET /api/v1/feed and POST /mcp both answer 401 with the byte-identical challenge Bearer realm="forge", resource_metadata="https://lab--6049--…/.well-known/oauth-protected-resource". Nothing for the terminal or README: this is a header-only change to the HTTP API.