Skip to content

feat(api): let OAuth clients register themselves, and sweep expired OAuth rows - #1137

Open
mforce wants to merge 6 commits into
mainfrom
feat/797-oauth-client-registration
Open

mforce wants to merge 6 commits into
mainfrom
feat/797-oauth-client-registration

Conversation

@mforce

@mforce mforce commented Oct 8, 2026 •

Copy link
Copy Markdown
Owner

An assistant that has never seen this server can now register itself and send a user to approve it. The server also deletes OAuth rows nobody needs any more. Both stay behind #795's Production gate, so Production behaviour does not change until #798.

Closes #797

What changed

Registration (RFC 7591). POST /api/v1/oauth/register is anonymous on purpose. A registered client gets nothing until a user approves it on the consent screen (#798), so the risk is junk rows. The endpoint comment says so. Discovery now advertises registration_endpoint.

  • The server registers public clients with the authorization-code grant only. PKCE with S256 was already required server-wide (OAuth slice 1: stand up OpenIddict — tables, migration, token issuance #795).
  • A client may ask for refresh_token. The server accepts the request and does not grant it, because access tokens last until revoked (Build an OAuth 2.1 authorization server (OpenIddict) so MCP clients can authenticate #788). RFC 7591 §3.2.1 allows this, and the response lists what was registered. Every other grant, any response type other than code, and any token_endpoint_auth_method other than none get invalid_client_metadata.
  • Redirect URIs must be https, or http on localhost, 127.0.0.1 or [::1]. A fragment, user info or a custom scheme such as cursor:// is refused. The limits are 1 to 10 URIs of at most 2048 characters each. Anything else gets invalid_redirect_uri, and so does a URI OpenIddict's own validator refuses, such as one with a reserved iss query parameter.
  • A client whose redirect URIs are all http loopback is registered as native, and its URIs are stored without a port. It can then authorize on any port, as RFC 8252 §7.3 requires, while scheme, host, path and query must still match. A client that also lists an https URI stays a web client and every URI matches exactly.
  • The 201 response lists the redirect URI strings OpenIddict stored and compares, so a client can send them back unchanged.
  • The client's name is untrusted. Every control, format, private-use or unassigned code point becomes a space, which covers bidi overrides and isolates. Runs of spaces collapse. The name is capped at 100 UTF-16 units and cut between graphemes. The server does not store any other client metadata, such as logo_uri or client_uri.
  • The endpoint ignores an ambient session bearer, the same way /auth/login does, so a stray JWT cannot pull it into the idempotency protocol.

Rate limit. The new oauth-register policy allows 10 registrations per client IP per hour. It runs on the shared IFixedWindowCounter through DistributedIpFixedWindowPolicy, keyed behind the trusted-proxy rules (#260). #796 owns the limits on token, authorize and OAuth-authenticated calls. This PR only adds this one policy.

Sweep. OAuthPurgeSweep runs from the DurableJobWorker poll under the leader gate (#271). It does not use Quartz. Each run makes three calls, in an order the foreign keys force, because they do not cascade:

  1. IOpenIddictTokenManager.PruneAsync deletes tokens older than 14 days (OpenIddict's default) that are redeemed, revoked, expired, or under an authorization that is no longer valid.
  2. IOpenIddictAuthorizationManager.PruneAsync deletes authorizations older than 14 days that are not valid, or ad hoc, and have no tokens left.
  3. One DELETE removes applications older than one day that have no authorization and no token.

Approval is an authorization row. A live connection holds a valid access token with no expiry, so the sweep leaves the token, its authorization and its application alone. The sweep resolves IOAuthPurge, which is registered only with the OAuth server, so in Production the sweep does nothing.

Application creation time. OpenIddict has no creation time on applications, so migration AddOAuthApplicationCreatedAt adds CreatedAtUtc and stamps it the way #819 stamps every created-only record. It adds the column, backfills existing rows with #819's unknown sentinel (1970-01-01), makes the column NOT NULL, and attaches the existing StampCreatedBusinessRecord function as TR_OpenIddictApplications_BusinessRecordTimestamps. The trigger overwrites any supplied value on insert and keeps the old value on update. The EF shadow property is configured through BusinessRecordModel.ConfigureCreatedTimestamp, which is now internal so this configuration can call it. As a result, EF never sends a value and reads back the database's. The one-day cutoff compares that column with the database's now(), so both sides come from one clock. Token and authorization pruning stays on the API's clock, because OpenIddict stamps those rows from the API. #819's upgrade guard now matches stamping triggers to every mapped CreatedAtUtc column, framework entities included, instead of counting ICreatedRecord types.

Guard updates. The adapter ledger has a row for the sweep. FilterFreeSetSites classifies the new db.OAuthApplications delete. The coupling matrix and docs/schema/ are regenerated. src/AGENTS.md has the rule, and docs/decisions/797-oauth-client-registration.md has the rationale.

Decisions

1. Registration mechanisms. This PR supports Dynamic Client Registration only. As a reference, not a protocol-version commitment, I checked it against the MCP authorization specification revision 2026-07-28, the current one on 2026-10-08. Choosing the MCP version stays with #789 and #806.

  • That revision says servers and clients SHOULD support Client ID Metadata Documents.
  • It makes DCR a MAY and marks it deprecated, kept for compatibility with servers that lack metadata documents. The 2025-11-25 revision cited in the issue correction called DCR optional but did not deprecate it.
  • Clients try pre-registration first, then metadata documents if the server advertises client_id_metadata_document_supported, then DCR if it advertises registration_endpoint.

For context only, the clients I expect to register are desktop and CLI assistants with MCP support, such as Claude Desktop and Claude Code, IDE agents such as VS Code and Cursor, hosted connectors such as claude.ai and ChatGPT, and the MCP Inspector.

2. Client ID Metadata Documents are deferred. Discovery does not advertise them, so a client that only implements metadata documents cannot connect. The clients we expect still support DCR in their current first-party documentation: Claude Code, Claude's hosted connectors, VS Code and ChatGPT. The owner has put metadata-document support on the follow-up batch. It means fetching the document over HTTPS with SSRF protection, caching it, and checking redirect URIs against it.

3. No farm opt-out, and I do not think this slice needs one. Registration happens before anyone signs in, so it has no farm to check. A switch could only refuse consent, which is #798's screen. Every connection already needs a user to approve it with their password, and #799 lets an Owner see and revoke any connection. If a switch is wanted, it belongs at consent and in account data, as #788 says.

Redirect decision (owner, round 1)

The first version refused localhost. The owner chose to accept http://localhost:<any port> as a native loopback redirect, next to 127.0.0.1 and [::1]. RFC 8252 §8.3 prefers the literal addresses but does not forbid localhost, and Claude Code, the MCP Inspector and Cursor register it. Custom URI schemes stay refused. Admitting one would need a deliberate exception for that scheme. The decision record has the reasoning.

Proof

Each rule has a test, and tools/oauth/mutation-check.sh carries one mutant per claim next to #795's. The runner builds each mutant, runs the named test, and judges it from the TRX, including theory tests, which report one result per case.

  • OAuthClientRegistrationTests covers these cases:
    • a self-registered client completes the flow, and so does each redirect URI the registration response returned;
    • a loopback client authorizes on a port it did not register, on 127.0.0.1, [::1] and localhost, with and without a registered port;
    • a change of path, host, query or scheme on another port is refused;
    • a reserved iss parameter becomes invalid_redirect_uri;
    • the discovery entry is present;
    • allowed and refused redirect URIs, grants, response types and auth methods behave as specified;
    • the name sanitiser turns literal inputs into literal outputs;
    • a session bearer is ignored;
    • the per-IP limit runs on the shared counter.
  • OAuthApplicationCreatedAtTests checks that the database replaces a creation time supplied by raw SQL, that an EF insert supplying one reads back the database's value, and that an update through OpenIddict and a raw update both leave CreatedAtUtc unchanged.
  • OAuthPurgeTests ages rows by rewriting their creation times. For applications it switches the trigger off inside one transaction and measures age on the database's clock, as Standardize business-record timestamps and chronological list ordering #819's tests do. Dead rows past retention are pruned while a live connection survives, dead rows inside retention stay, unapproved applications go after the window but not before, and only the leader runs the sweep.
  • OAuthServerProductionTests asserts that registration answers 404 in Production and that IOAuthPurge does not resolve.

The full run of 41 mutants ran at d4dcefd. The baseline and the restored tree each passed 61 of 61 OAuth tests. 39 mutants were killed with their declared message, and two held by design: account-id-in-token (#795) and redirect-fragment.

Mutant What it breaks Verdict
redirect-any-http-host plain http allowed on any host killed
redirect-fragment my fragment check removed held: OpenIddict's validator still refuses it, now as a 400
localhost-refused localhost dropped from the loopback hosts killed
loopback-not-native loopback clients not marked native killed
loopback-port-kept loopback URIs stored with their port killed
loopback-query-dropped stored loopback URI loses its query killed
loopback-host-merged every loopback host stored as localhost killed
response-normalized response returns AbsoluteUri killed
reserved-parameter-500 OpenIddict's validation refusal not caught killed (500 instead of 400)
grant-any any grant accepted killed
auth-method-any any client auth method accepted killed
name-keeps-bidi format characters kept in the name killed
name-uncapped name length cap removed killed
discovery-silent registration_endpoint not advertised killed
register-unlimited rate limit removed killed
register-process-local per-IP process-local limiter instead of the shared counter killed
register-reads-bearer ambient session bearer honoured killed
tokens-unpruned token prune does nothing killed
authorizations-unpruned authorization prune does nothing killed
retention-ignored tokens pruned without the 14-day retention killed
approved-app-deleted application delete ignores authorizations and tokens killed (the foreign key refuses it)
unapproved-kept unapproved applications counted, not deleted killed
window-ignored one-day window ignored killed
sweep-window-dropped sweep passes now as the window killed
follower-sweeps a follower runs the sweep killed
sweep-unwired the worker never calls the sweep killed
stamp-trigger-missing migration does not attach the #819 trigger killed
stamp-insert-only trigger fires on insert only killed
stamp-sent-by-ef EF configured without the #819 helper, so it sends its own value killed

The 12 #795 mutants gave the same results as before: 11 killed and account-id-in-token held.

LoopbackClient_OnAnotherPort_StillMatchesTheRest has one product mutant, loopback-host-merged. Its path, query and scheme rows pin OpenIddict's own comparison, which this PR does not change, so no mutation of this PR's code can make them fail.

No mutant switches the cutoff back to the API's clock. In the test environment the API and the database share a clock, so that change cannot be observed there. The generated SQL, "CreatedAtUtc" < now() - @window, was checked directly.

Local runs at d4dcefd, filtered as the brief asks, all passed:

  • 147 integration tests covering OAuth, BusinessRecord*, TableOwner*, TenantBypass*, migrations, schema docs, jobs, sales products and image pins;
  • 467 application tests covering architecture, the business-record census, table owners, tenant bypass and documentation.

docs/schema/ is regenerated, and has-pending-model-changes reports none.

mforce added 5 commits October 8, 2026 06:12
…Auth rows

Add RFC 7591 registration at POST /api/v1/oauth/register for public
authorization-code clients redirecting to https or loopback, with a
sanitised display name and a per-IP limit on the shared counter.
Discovery advertises the endpoint.

OAuthPurgeSweep runs under the DurableJobWorker leader gate. It prunes
dead tokens and authorizations past 14 days through OpenIddict, then
deletes applications nobody approved within a day, keyed on a new
Postgres-stamped CreatedAtUtc column.

Both stay behind the #795 Production gate.
…t URIs

A client whose redirect URIs are all http loopback is now registered as
native with port-less URIs, so it can authorize on any port as RFC 8252
7.3 requires; localhost joins 127.0.0.1 and [::1]. The registration
response returns the stored strings OpenIddict compares, and an OpenIddict
validation refusal, such as a reserved iss parameter, is a 400
invalid_redirect_uri instead of a 500.
@mforce

mforce commented Oct 8, 2026

Copy link
Copy Markdown
Owner Author

Round 1 review by Codex GPT-6 Astra (head 4475894): 0 P1, 2 P2, 1 P3. All three findings are fixed in ae4b0a2. e84f28b corrects five mutant rows in the runner. The owner decided both open questions.

F1 (P2), loopback clients could not change port. A client whose redirect URIs are all http loopback is now registered with ApplicationType = native, and its URIs are stored without a port. That is the form OpenIddict's relaxed comparison needs (RFC 8252 §7.3). Scheme, host, path and query must still match. A client that also lists an https URI stays a web client, and every URI matches exactly. LoopbackClient_AuthorizesOnAnotherPort registers on one port and completes the flow on another for 127.0.0.1, [::1] and localhost, plus a port-less registration authorized on 49152 and a URI with a query. LoopbackClient_OnAnotherPort_StillMatchesTheRest refuses a changed path, host, query or scheme. These mutants are killed: loopback-not-native, loopback-port-kept, loopback-query-dropped and loopback-host-merged. The path, query and scheme rows pin OpenIddict's own comparison, so no mutation of this PR's code turns them red.

F2 (P2), the response could advertise an unusable URI. The response now returns OriginalString, which is the string OpenIddict stores and compares ordinally. ReturnedRedirectUri_IsUsable completes the flow with the returned URI for https://client.example, HTTPS://Client.Example:443/cb and a loopback URI. The response-normalized mutant, which goes back to AbsoluteUri, is killed.

F3 (P3), a reserved query parameter caused a 500. Register now catches OpenIddictExceptions.ValidationException and returns 400 invalid_redirect_uri with OpenIddict's messages. ReservedRedirectParameter_IsARegistrationError covers ?iss=x. The reserved-parameter-500 mutant is killed, because the response becomes 500 instead of 400. As a side effect, redirect-fragment now holds, because OpenIddict's validator refuses a fragment with the same 400 when my check is removed.

Owner decision, redirects. http://localhost:<any port> is accepted as a native loopback redirect, alongside 127.0.0.1 and [::1]. Custom schemes such as cursor:// stay refused. The decision record gives the reasons. RFC 8252 §8.3 prefers the literal addresses but does not forbid localhost, and Claude Code, the MCP Inspector and Cursor register it.

Owner decision, Client ID Metadata Documents. They stay deferred. Current first-party documentation for Claude Code, Claude's hosted connectors, VS Code and ChatGPT still describes DCR support. The PR body says so, and the owner has put metadata documents on the follow-up batch.

Verification. The full run of 38 mutants at ae4b0a2 had baseline and restore each at 58 of 58. Five rows failed on table errors, not test gaps: xUnit prints Redirect as Found, and one filter did not compile. After e84f28b, a rerun of those five killed all of them. The PR body has the full table. Locally, 58 OAuth integration tests and 447 architecture and tenant-bypass tests pass. CI was green at ae4b0a2.

@mforce

mforce commented Oct 8, 2026

Copy link
Copy Markdown
Owner Author

Review round 2 by Codex GPT-6 Astra, run locally on head e84f28b, found 0 P1, 0 P2 and 2 P3. Round 1's F2 and F3 are confirmed fixed, and the mutation-table corrections hold. Across localhost, 127.0.0.1 and [::1], all six cross-host substitutions fail, and changed scheme, path, case and query values are refused.

Both P3s stay here as known limits, not fixes:

  1. An explicit default port is refused (ClientRegistration.cs:66). Native redirects are stored without a port. http://localhost:80/cb therefore fails to authorize even when the client sends the string it registered; the port-less spelling works. Loopback callbacks do not use port 80 in practice.
  2. Native matching canonicalizes more than the port (ClientRegistration.cs:64). OpenIddict's native comparison treats canonical-equivalent paths and queries as equal. Examples are /%63b for /cb, /a/../cb, tab=%31 for tab=1, and an IPv6 zone id on [::1]. They all still need the same host and an equivalent path, so they give no cross-host or cross-path access.

That is the end of the review loop for this PR. Round 1 found two real interoperability defects, round 2 found only these two P3 edges, and no further round is planned.

The migration now attaches the existing StampCreatedBusinessRecord function
to OpenIddictApplications instead of a now() column default, after adding
and backfilling the column with the unknown sentinel. EF configures the
shadow property through BusinessRecordModel.ConfigureCreatedTimestamp, and
the unapproved-application cutoff uses the database clock that stamped it.
The #819 upgrade guard now matches stamping triggers to every mapped
CreatedAtUtc column, framework entities included.
@mforce

mforce commented Oct 8, 2026

Copy link
Copy Markdown
Owner Author

The owner asked for OpenIddictApplications.CreatedAtUtc to be stamped by #819's trigger instead of a now() column default. That change is in d4dcefd.

Migration. AddOAuthApplicationCreatedAt follows #819's order. It adds the column as nullable, backfills existing rows with #819's unknown sentinel (1970-01-01), makes the column NOT NULL, and then attaches the existing "StampCreatedBusinessRecord"() as TR_OpenIddictApplications_BusinessRecordTimestamps. It creates no new function. The function and trigger names are literal strings in the migration, not C# constants. Down drops the trigger before the column. The migration is unmerged, so I edited it in place and kept its ID. has-pending-model-changes reports no changes.

EF configuration. HasDefaultValueSql("now()") is gone, and no SQL string remains in the EF configuration. The shadow property now goes through BusinessRecordModel.ConfigureCreatedTimestamp. I changed only that helper's visibility, from private to internal, so the Access configuration in the same assembly can call it instead of copying its three lines.

Clock. The cutoff used to come from the API's clock, while the database stamped the row. IOAuthPurge now takes the window rather than a cutoff, and the delete compares against the database's own clock. The generated SQL is "CreatedAtUtc" < now() - @window. Token and authorization pruning stays on the API's clock, because OpenIddict stamps those rows from the API.

#819's guard. BusinessRecordChronologyMigrationTests used to compare the number of timestamp triggers with the number of ICreatedRecord types. OpenIddict's application cannot implement that interface, so the guard now compares the set of trigger tables with every mapped CreatedAtUtc column. That check is stricter, and it covers framework entities.

Verification at d4dcefd.

  • New tests in OAuthApplicationCreatedAtTests, each with a killed mutant:
    • a creation time supplied by raw SQL is replaced (stamp-trigger-missing);
    • an EF insert that supplies a value reads back the database's (stamp-sent-by-ef);
    • an update through OpenIddict and a raw update both leave the value unchanged (stamp-insert-only).
  • The expiry tests now age rows on the database clock. window-ignored, unapproved-kept and sweep-window-dropped are still killed.
  • The full mutation run of 41 mutants had baseline and restore each at 61 of 61, with 39 killed and 2 held by design.
  • Locally, 147 integration tests passed (OAuth, BusinessRecord*, TableOwner*, TenantBypass*, migrations, schema docs, jobs), and so did 467 application tests (architecture, census, table owners, tenant bypass, documentation).
  • docs/schema/ is regenerated and now lists the trigger.
  • CI is green: 18 passed and 1 skipped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OAuth slice 3: dynamic client registration with rate limiting and unapproved-app expiry

1 participant