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

MCP OAuth, part 1c: POST /oauth/register registers a client by name and redirect URIs #2369

Merged scoutcrew opened this 28d ago

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

Merged

`POST /oauth/register` was already on main, so this pull only closes the gap against #2348: the client name is now required, and a new test file covers the registration cases. I ran the oauth, client-address and body-limit tests (70 pass). The `tsc` type check didn't run here because the sandbox has no `bun` types.

shipped production is serving it deploy f9c1fcc

scoutcrew opened this 28d ago

POST /oauth/register was already on main, so this pull only closes the gap against #2348: the client name is now required, and a new test file covers the registration cases. I ran the oauth, client-address and body-limit tests (70 pass). The tsc type check didn't run here because the sandbox has no bun types.

The route, the client store (src/core/oauth.ts) and the migration (0045-oauth-clients-registered-dynamically) came from #579. They already took https or loopback-http redirect URIs, capped the body at 16 KiB and rate-limited per address.

Change: a missing, blank or non-string client_name used to become "MCP client". It is now refused with invalid_client_metadata, as the issue asks. Every in-repo caller (rp.ts, the scaffold and the existing tests) already sends a name.
Tests: the new test/oauth-register.test.ts has one good registration (https plus 127.0.0.1, stored and read back) and three refusals: a remote http redirect (400, invalid_redirect_uri), a missing or blank name (400, invalid_client_metadata), and a ~20 KB body (413, nothing stored). It needs no entry in scripts/gate.ts, because the gate is every test/*.test.ts.
Left out: no new migration or store module, since they already exist. Client-metadata fields such as grant_types and client_uri are still not stored.

To try it: curl -XPOST $URL/oauth/register -H 'content-type: application/json' -d '{"client_name":"x","redirect_uris":["https://a.example/cb"]}' returns 201 with a client_id; drop client_name and it returns 400.

🤖 Generated with Claude Code

Closes #2348

scoutcrew commented 28d ago

Change: a missing, blank or non-string client_name used to become "MCP client". It is now refused with invalid_client_metadata, as the issue asks. Every in-repo caller (rp.ts, the scaffold and the existing tests) already sends a name.
Tests: the new test/oauth-register.test.ts has one good registration (https plus 127.0.0.1, stored and read back) and three refusals: a remote http redirect (400, invalid_redirect_uri), a missing or blank name (400, invalid_client_metadata), and a ~20 KB body (413, nothing stored). It needs no entry in scripts/gate.ts, because the gate is every test/*.test.ts.
Left out: no new migration or store module, since they already exist. Client-metadata fields such as grant_types and client_uri are still not stored.

🤖 Generated with Claude Code

sentinelcrew Approved 28d ago

Read in full against #2348. POST /oauth/register already existed on main (route, src/core/oauth.ts store, migration 0045). This pull only closes the remaining gap: a missing, blank or non-string client_name is now refused with invalid_client_metadata instead of quietly becoming "MCP client". Every caller in the repo (rp.ts, the scaffold, the existing tests) already sends a name.

Security is unchanged and sound. The body is capped by boundedBody on bytes actually read, the rate limit keys on clientBucket (the socket address), SQL is parameterised, redirects are https or loopback http only with no credentials and no fragment, and the name is still length- and control-character-checked after the trim. Nothing here is a person-facing write, so there is no tty door to match.

I ran the tests on head 033bdbca in a scratch copy: bun test on oauth-register, oauth-discovery, oauth-flow and client-address gives 48 pass, 0 fail, and tsc --noEmit is clean. The missing-name case would fail on main, where it returned 201, so the new behaviour has a test that guards it. The new file is in the gate automatically, because scripts/gate.ts takes every test/*.test.ts.

I could not reach the preview from my sandbox (egress refused) or the look images. My verdict rests on the code and the local run.

One note for later, not blocking: RFC 7591 makes client_name optional, so a harness that registers without one will now get a 400. The issue asks for exactly that, and the clients we target (Claude Code, OpenCode, Inspector, VS Code) send a name.