Repository navigation
Conversation
…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.
|
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 F2 (P2), the response could advertise an unusable URI. The response now returns F3 (P3), a reserved query parameter caused a 500. Owner decision, redirects. 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 |
|
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 Both P3s stay here as known limits, not fixes:
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.
|
The owner asked for Migration. EF configuration. Clock. The cutoff used to come from the API's clock, while the database stamped the row. #819's guard. Verification at d4dcefd.
|
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/registeris 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 advertisesregistration_endpoint.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 thancode, and anytoken_endpoint_auth_methodother thannonegetinvalid_client_metadata.https, orhttponlocalhost,127.0.0.1or[::1]. A fragment, user info or a custom scheme such ascursor://is refused. The limits are 1 to 10 URIs of at most 2048 characters each. Anything else getsinvalid_redirect_uri, and so does a URI OpenIddict's own validator refuses, such as one with a reservedissquery parameter.httploopback 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 anhttpsURI stays a web client and every URI matches exactly.logo_uriorclient_uri./auth/logindoes, so a stray JWT cannot pull it into the idempotency protocol.Rate limit. The new
oauth-registerpolicy allows 10 registrations per client IP per hour. It runs on the sharedIFixedWindowCounterthroughDistributedIpFixedWindowPolicy, 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.
OAuthPurgeSweepruns from theDurableJobWorkerpoll 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:IOpenIddictTokenManager.PruneAsyncdeletes tokens older than 14 days (OpenIddict's default) that are redeemed, revoked, expired, or under an authorization that is no longer valid.IOpenIddictAuthorizationManager.PruneAsyncdeletes authorizations older than 14 days that are not valid, or ad hoc, and have no tokens left.DELETEremoves 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
AddOAuthApplicationCreatedAtaddsCreatedAtUtcand 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 existingStampCreatedBusinessRecordfunction asTR_OpenIddictApplications_BusinessRecordTimestamps. The trigger overwrites any supplied value on insert and keeps the old value on update. The EF shadow property is configured throughBusinessRecordModel.ConfigureCreatedTimestamp, which is nowinternalso 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'snow(), 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 mappedCreatedAtUtccolumn, framework entities included, instead of countingICreatedRecordtypes.Guard updates. The adapter ledger has a row for the sweep.
FilterFreeSetSitesclassifies the newdb.OAuthApplicationsdelete. The coupling matrix anddocs/schema/are regenerated.src/AGENTS.mdhas the rule, anddocs/decisions/797-oauth-client-registration.mdhas 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.
client_id_metadata_document_supported, then DCR if it advertisesregistration_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 accepthttp://localhost:<any port>as a native loopback redirect, next to127.0.0.1and[::1]. RFC 8252 §8.3 prefers the literal addresses but does not forbidlocalhost, 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.shcarries 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.OAuthClientRegistrationTestscovers these cases:127.0.0.1,[::1]andlocalhost, with and without a registered port;issparameter becomesinvalid_redirect_uri;OAuthApplicationCreatedAtTestschecks 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 leaveCreatedAtUtcunchanged.OAuthPurgeTestsages 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.OAuthServerProductionTestsasserts that registration answers 404 in Production and thatIOAuthPurgedoes 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) andredirect-fragment.redirect-any-http-hosthttpallowed on any hostredirect-fragmentlocalhost-refusedlocalhostdropped from the loopback hostsloopback-not-nativeloopback-port-keptloopback-query-droppedloopback-host-mergedlocalhostresponse-normalizedAbsoluteUrireserved-parameter-500grant-anyauth-method-anyname-keeps-bidiname-uncappeddiscovery-silentregistration_endpointnot advertisedregister-unlimitedregister-process-localregister-reads-bearertokens-unprunedauthorizations-unprunedretention-ignoredapproved-app-deletedunapproved-keptwindow-ignoredsweep-window-droppednowas the windowfollower-sweepssweep-unwiredstamp-trigger-missingstamp-insert-onlystamp-sent-by-efThe 12 #795 mutants gave the same results as before: 11 killed and
account-id-in-tokenheld.LoopbackClient_OnAnotherPort_StillMatchesTheResthas 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:
BusinessRecord*,TableOwner*,TenantBypass*, migrations, schema docs, jobs, sales products and image pins;docs/schema/is regenerated, andhas-pending-model-changesreports none.