Repository navigation
Add OAuth Client ID Metadata Document (CIMD) support - #20
Open
roborourke wants to merge 66 commits into
Open
roborourke wants to merge 66 commits into
roborourke wants to merge 66 commits into
Conversation
Client ID Metadata Documents let a client use an https URL as its client_id, with the URL serving its own metadata. This adds the standalone building blocks. They are not wired into the authorize or token flow yet. The server fetches attacker-chosen URLs, so the fetcher is built around SSRF defence. The host is resolved once, every address must be public according to an explicit special-use CIDR blocklist (filter_var alone misses ranges such as 100.64.0.0/10 and the IPv6 transition prefixes), and the connection is pinned to the first vetted address with CURLOPT_RESOLVE so a second DNS answer cannot swap it. The pin is a one-shot http_api_curl action that is removed in a finally block and ignores any other URL. Redirects are refused, the body is capped at 5120 bytes, and only JSON media types are accepted. Client URL policy is stricter than the draft: queries are rejected as well as fragments, and the site's own issuer URL is refused. The cache lifetime comes from Cache-Control max-age, clamped to 300-3600 seconds, with no-store and no-cache falling back to the minimum so a client cannot force a refetch on every request. validate_document only accepts public clients using authorization_code with the code response type. Shared-secret auth, client_secret fields, jwks plus jwks_uri, and private key material in jwks are all rejected. Hosts resolve through the oauth2.cimd.pre_resolve_host filter so tests never touch DNS. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Covers every rejection rule for client ID URLs, one address in each blocked IPv4 and IPv6 range plus public addresses and the loopback filter, the fetcher's error and caching paths, and each validate_document rule. HTTP is mocked with pre_http_request and DNS with oauth2.cimd.pre_resolve_host, so no test touches the network. pre_http_request fires before wp_http_validate_url in WP_Http::request(), so the mock also short-circuits the unsafe-URL check. The tests also check that the http_api_curl pin is gone after success, transport error, bad response and an exception. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review found that the connection pin could be skipped or could fail open, and that wrongly typed document fields fell through to defaults. Pinning: the CURLOPT_RESOLVE entry only helps if curl sees the same host string we vetted. Percent-encoded and IDN hosts are normalised by Requests first, so client ID URLs now need a plain ASCII host (letters, digits, dots, hyphens). That also drops IPv6 literals. Ports are limited to the http_allowed_safe_ports list, with 443 as the default, because wp_safe_remote_get used to do this and we no longer use it. fetch_document now refuses to run when HTTPS would not go through the curl transport (only curl fires http_api_curl) or when WP_HTTP_Proxy would send the request through a proxy, since curl would then resolve nothing locally. WordPress cannot force a transport for a single wp_remote_get call, so the check reads the transport that Requests has cached for SSL requests. The http_api_curl callback now throws a Requests exception on a URL mismatch or a failed curl_setopt, and fetch_document turns that into oauth2.cimd.unsafe_transport. reject_unsafe_urls is off: our checks are stricter, and its gethostbyname lookup is IPv4 only, which broke AAAA-only hosts. Responses: 429 and 5xx from the document server are transient 503 instead of terminal. Documents: redirect_uris, grant_types, response_types, token_endpoint_auth_methods_supported and jwks.keys must be lists, and token_endpoint_auth_method a string, whenever the key is present. Only an absent key gets a default. jwks must be an object with a keys list of objects, and any private or symmetric member (d, p, q, dp, dq, qi, k, kty oct) is rejected. jwks_uri must be an https URL and is never fetched. Loopback http redirects accept all of 127.0.0.0/8. The IPv6 blocklist gains ::/96, fec0::/10, 3fff::/20 and 5f00::/16. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Covers the host regex (percent-encoded, IDN and IPv6 hosts), the port allowlist and its filter, the pin callback driven with a real curl handle, the unsafe_transport mapping, the non-curl and missing transport cases, and 429 and 5xx handling. The proxy cases define WP_PROXY_HOST and WP_PROXY_PORT, so they run in separate processes to keep the constants out of the other tests. Document tests add wrong-type cases for every list field and the auth method properties, JWKS private and symmetric members, jwks_uri, the 127.0.0.0/8 loopback boundaries, and the four new IPv6 ranges. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The pre-request transport check reads Requests' internal transport cache, and a requests.before_request hook can still swap the transport after that check. Nothing then guarantees that the CURLOPT_RESOLVE pin was applied. The http_api_curl callback now records success once curl_setopt has set the pin. After the request, fetch_document discards the response and returns terminal oauth2.cimd.unsafe_transport (500) if the pin never ran, whatever the response was. This replaces the failure flag that the callback used to set before throwing, since a callback that throws also leaves the pin unset. Tests: the HTTP mock now fires http_api_curl with a real curl handle and the request URL, as the curl transport does, through one shared helper. A new test returns a valid 200 document without firing the action and expects unsafe_transport. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The CURLOPT_RESOLVE pin only helps if nothing else can route the request. Proxy environment variables (https_proxy, HTTPS_PROXY, ALL_PROXY) are read by curl itself, so the WP_HTTP_Proxy check cannot see them. A manual check confirmed that an unpinned request goes over the env proxy. The http_api_curl callback now runs at PHP_INT_MAX and sets every option the pin relies on, checking each result: an empty CURLOPT_PROXY, CURLOPT_NOPROXY of *, no FOLLOWLOCATION, HTTPS-only PROTOCOLS and REDIR_PROTOCOLS, and CURLOPT_RESOLVE last. Any failed option, URL mismatch or second call for the same fetch throws a Requests exception, and fetch_document maps a throw to terminal unsafe_transport. The pinned flag is set only after all options succeed. fetch_document also returns unsafe_transport when the Requests class is missing. Client ID hosts must now end in a label that starts with a letter. That rejects every IP literal and numeric host form (0x7f000001, 2130706433, 0177.0.0.1, 127.1) that curl or Requests might interpret differently from the resolver. IP literal client IDs are not supported. jwks_uri is parsed and needs an https scheme and a host. Loopback redirect hosts compare localhost case-insensitively and detect IPv6 loopback by packed address, so [0:0:0:0:0:0:0:1] counts. The document checks use a local is_list function instead of the array_is_list polyfill. Tests cover the numeric hosts, the callback priority, a second callback call, jwks_uri forms and the loopback spellings. The mock HTTP helper clears earlier mocks so one fetch fires the pin once. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The http_api_curl action is not the last hook before curl_exec, so another callback could still change curl options after the pin was set. The URL check, and the throw on a mismatch or second call, stay on http_api_curl and record that it ran for our URL. The connection options and CURLOPT_RESOLVE entry are now set from a one-shot requests-curl.before_send callback at PHP_INT_MAX, which refuses to act unless the first callback ran. The pinned flag is set only after every option succeeds, and both callbacks are removed after the request. Add a regression test that fetches over real curl with https_proxy and HTTPS_PROXY set to a dead local port. It checks that the connection goes straight to the vetted address and does not go through the proxy. Without the proxy options the underlying curl error mentions the proxy, so the test fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The shadow client and authorize tests need the same pre_http_request, DNS and pin helpers that the fetcher tests use. Moving them into a trait lets both test files use them without copying. The mocks are cleared through one reset method that each tear_down calls. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A client ID metadata document client has no registration, but authorization codes and tokens are stored as meta on the client post and the token flow loads clients through Client. A shadow post per URL keeps that working. The post slug is cimd- plus the SHA-256 of the URL, so lookup is by slug and the stored URL must match exactly, which stops a hash collision or a hand-made post from standing in for another URL. For these clients get_id returns the URL, the type is always public, no secret is required, PKCE is required (still passed through the existing filter), client_credentials is off, and the redirect URIs come from the stored metadata. Redirect matching is exact. The one relaxation is the loopback rule from RFC 8252: a registered http URI on localhost, 127.0.0.0/8 or [::1] also matches the same URI with any port. The candidate is rebuilt from the registered URI and compared as a string, so userinfo or fragment tricks cannot slip through. Registered clients keep the existing component match and filter. save_cimd creates or refreshes a shadow post. An existing post keeps its status so a refreshed document never approves or unapproves a client, and a post that is not a shadow post for the same URL is never touched. A shadow post is never returned for a plain slug id, and a registered post is never returned for a URL id. Add CIMD\is_enabled for the feature flag. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
get_client now takes include_unapproved and discover arguments. With the defaults it stays a database lookup, so the token endpoint and Access_Token::get_client never make a request. With discover set and the feature enabled, a CIMD URL is fetched when the stored client is missing or expired. Discovery is guarded in three ways. A per-user transient counter limits real fetches to 10 per 10 minutes by default, filterable through oauth2.cimd.rate_limit; cache hits, busy locks and URL validation failures do not count. A per-URL option lock stops concurrent fetches of one URL, with a 30 second timeout so a crashed request cannot block it, and it is always released. When either guard blocks a fetch, or the fetch fails in a way that may pass, the stored client is returned even if stale, because the document it was built from was valid when fetched. Terminal failures such as an invalid document are returned as errors so a client that now publishes bad metadata stops working. The oauth2.cimd.policy filter decides what happens to a fetched document: auto publishes a new client, manual saves it as a draft, deny refuses it. An existing client keeps its status whatever the policy says, so a refresh never approves or unapproves a record. A draft that discovery touches is only returned to callers that asked for unapproved clients. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
handle_authorisation now resolves the client with get_client, asking for unapproved clients and turning on discovery only when the user is logged in. A logged-out request never fetches or writes anything: if the id is a CIMD URL that is not stored, it goes straight to the login redirect and the document is fetched after login. The login redirect moves into a small helper because it is now reached from two places. A WP_Error from discovery is returned as it is, so the existing error page shows its message and status, for example 503 for a transient fetch failure or 403 when the policy denies the client. The user is never redirected to the client for these errors, because a redirect URI that came from an unfetched document cannot be trusted. A draft client from the manual policy still gets the existing unapproved 403. get_client also returns the personal access token client for its id. That object is not a Client, so it is treated as not found here, as it was before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A client ID metadata document client authenticates with the none method, and its document may not hold a shared secret. If the token request carries a client secret for one, either in the body or in an HTTP Basic header, the endpoint now returns invalid_client instead of ignoring it. Accepting a secret that nothing can check would imply the client was authenticated when it was not. A Basic header with an empty secret is still accepted, since that is how a public client names itself. The token endpoint keeps using the database-only get_client, so it never fetches a document. Add an end-to-end test that authorizes a new CIMD client, then exchanges the code with a PKCE verifier and checks that no HTTP request was made. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Clients and MCP hosts read this flag from the authorization server metadata to learn that they can use a metadata document URL as a client ID. It is set while oauth2.cimd.enabled is true, before the existing metadata filter so that filter can still change it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Older curl versions word the connection error as "host port N" rather than "host:N", so matching the port made the test depend on the curl version. The host name is enough to show the connection went to the pinned host rather than through the proxy, and the check that the message does not mention a proxy is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The oauth2.pkce.required filter ran after the CIMD override, so a filter returning false made a public client with no secret skip PKCE. A CIMD client has nothing else to bind the code to the requester, so PKCE is always required for it and the filter is not consulted. Registered clients keep the filter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A token stores only the client ID, which for a CIMD client is the URL. If the shadow record was deleted and rediscovered, or the same URL was discovered on another site of a network, an old token would be accepted for what is really a new record with possibly different redirect URIs. Each shadow record now has a random generation. Access tokens and authorization codes are stamped with it when they are created, and a token or code whose generation is missing or different is refused. save_cimd also stores a hash of the redirect URIs, grant types, response types and authentication method. When a refresh changes it, the record gets a new generation, which ends every older token and code, and under the manual policy the record goes back to draft so an admin has to approve the new settings. A refresh that changes only things like the name keeps the generation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A CIMD client name comes from a document the client wrote, so any site can call itself anything. The consent screen now shows the host of the client ID under the app name, and the host the user will be sent to after approving. When that host is loopback (localhost, 127.0.0.0/8 or [::1]) it adds a warning that the app runs on this device and that the user should only continue if they started it. render_form takes the validated redirect URI as a third argument, with a default so existing callers and overriding themes are unaffected, and the template receives it as a variable. The markup comes from a helper that escapes both hosts, so it can be tested without wp-login.php. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The enabled flag, the admit_url filter and the deny policy were only checked when a document was fetched. A client already stored kept working at the token endpoint and for bearer tokens after an admin turned it off. get_client now returns null for a CIMD ID when the feature is off, when validate_client_id_url refuses the URL, or when the oauth2.cimd.policy filter returns deny for the stored metadata. These checks need no network or writes, so they run on authorize, the token endpoint and bearer authentication alike. The policy lookup moves into get_policy so the fetch path and the lookup path use the same filter call. An invalid or refused URL now gives a plain not-found at authorize instead of a CIMD error, because the lookup stops before discovery. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The stale record was a fallback for any failure, so a client whose document had become invalid or gone for good (a 404, a client_id mismatch, a document that now breaks the rules) kept working through rate limits, busy locks and transient errors. A terminal failure on a refresh now sets _oauth2_cimd_failed on the stored record. While it is set, discovery returns the error instead of the record in every case, and the record is not treated as fresh. A later successful refresh clears the marker. If no error is at hand, oauth2.cimd.client_failed (400) is returned. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The lock was an add_option row guarded by a read-then-delete takeover, so two requests could both take over a stale lock, and the options were per site. The rate limit counter was a per-site transient that was written before the lock was held. Both now live in inc/cimd/limits.php. With an external object cache the lock is wp_cache_add with a random token in a global group, and it is released only if the cache still holds our token. Without one the lock is a row in the main site's options table, shared by every site of a network. The row is inserted with INSERT IGNORE on the unique option_name, holds a token and a timestamp, and an expired lock is taken over with an UPDATE that matches the old value and must change exactly one row. Release deletes the row only if it still holds our value. The per-user limit (10 per 600 seconds) is joined by a network-wide cap (60 per 600 seconds). Both go through oauth2.cimd.rate_limit, which now also receives the scope. A slot is reserved only after the lock is held, right before a real fetch. With an external cache the counters use wp_cache_incr, which is atomic. Without one they are site transients that are read and then written, so the limit is soft under concurrency; a refused global slot gives the user slot back. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every refresh rewrote the post title and five meta values even when the document was identical, which is the common case. A refresh whose normalised metadata equals the stored metadata now updates only _oauth2_cimd_expires, and only if it differs, and clears the failure marker if one is set. The post update and the other meta writes are skipped, so an unchanged refresh costs one write instead of seven and creates no revision. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
For a hierarchical post type WordPress also checks attachments when it makes a slug unique. If an attachment already used the hashed slug, wp_insert_post quietly saved the shadow post under a suffixed slug, and the by-slug lookup would then never find it, so every request would create another post. save_cimd now checks the slug after the insert. On a mismatch it force-deletes the new post and returns oauth2.cimd.slug_conflict. The trash status is removed from the existing-post lookup, because WordPress renames the slug of a trashed post and it never matched. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sanitize_text_field strips percent-encoded octets such as %20, so a client ID URL or a registered redirect URI containing one could never match what the document says. At authorize, a client_id that starts with https:// is now read with wp_unslash only, and other ids still go through sanitize_text_field. For a CIMD client the redirect_uri is compared as sent, because its registered URIs are matched exactly. Registered clients keep the sanitised value, so their behaviour is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Explain what client ID metadata documents are, that they are on by default, and how the client behaves. Describe each oauth2.cimd.* filter with a short example, how to block a client (a filter, not deleting the record, which is recreated on the next use), and the plugin's limits with where they are stored. Add a section on limiting requests at the web server, with nginx and Apache examples, because the plugin limits are a backstop and not the first line of defence. Link the page from the table of contents. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The %i identifier placeholder needs WordPress 6.2, which is older than some supported branches. The lock queries now put the trusted base_prefix table name in the query string, with a phpcs ignore that says why, and keep %s for the values. The external-cache release read the entry and then deleted it. If our entry had already expired and another request had taken the lock between the read and the delete, the delete would remove the new owner's lock. The handle now carries the time the lock was taken, for the cache path as well. Release deletes only if less than LOCK_TIMEOUT minus 5 seconds have passed and the stored value still matches our handle. Past that age the entry may be gone already, so it is left to expire. Cache backends offer no compare-and-delete, so this narrows the window rather than closing it, and the cost of leaving a lock is at most five more seconds of a busy answer. The tests pass handles with an old timestamp to release_lock, which needs no time override. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When a refresh changed the redirect URIs, the new generation, the metadata, the demotion and the hash were written one after the other with no check that the generation had been stored. If the generation write failed but the hash write worked, the next refresh saw an unchanged hash and never tried the rotation again, so old tokens and codes stayed valid for the new redirect URIs. save_cimd now writes the new generation first and reads it back. If it was not stored, it returns oauth2.cimd.save_failed (500), writes nothing else, and deletes a record it had just created, so the next refresh tries again. The demotion and the other meta follow, and the hash that records the change is written last. This puts the demotion before the hash, not after it: if the demotion failed after the hash was stored, a retry would see no change and leave a record published that the manual policy meant to hold. The failure marker write is checked too. If it cannot be stored, the current request still returns the error. The security hash now sorts copies of the redirect URIs, grant types and response types, so reordering them in the document does not rotate the generation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The token endpoint loaded a code, which checked the code's generation against the client, and then issued the token stamped with the client's generation at that moment. If the generation rotated in between, the token was stamped with the new value and so was valid for a client the code was never issued for. The endpoint now reads the generation stored on the code and passes it through the token meta as client_generation. Access_Token::create takes it out of the meta and uses it for the stamp, and uses the client's current generation when none is given. A rotation between loading the code and issuing the token therefore gives a token that is already dead. The generation is read before the code is deleted, because the delete removes the value. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The notice was printed by a direct call in the bundled template, so a theme that overrides oauth2-authorize.php would drop it, and with it the only place the user sees where a CIMD client comes from. render_form now adds the notice to the oauth2_authorize_form action at priority 0 for the length of the include and removes it afterwards. Any template that fires the action shows the notice, once, before the callbacks added at the default priority, and the direct call is removed from the bundled template. The redirect URI reaches the callback through a closure created in render_form. That is less invasive than the alternatives: adding an argument to the action would be missed by an overriding theme that still passes only the client, and a global would outlive the render. The action's signature does not change, so existing callbacks are unaffected. The tests render the bundled template with stand-ins for the two wp-login.php functions it calls. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The page said a permanently failed client is no longer used. Only new authorizations and the fallback to the stored record are blocked. Codes already issued and existing tokens keep working until a later refresh changes the generation, or an admin deletes the client, and the text now says so. The web server examples keyed on the action query argument, which can be repeated, written in another case, encoded or sent in a POST body. They now match on the path only, and say that this limits all of wp-login.php so the rate should be generous. The nginx example puts limit_req in the server block or the existing PHP location instead of a new location that would replace the PHP handler. The mod_security rule uses REQUEST_FILENAME with path normalisation and lowercasing, the mod_evasive example is removed because it cannot be scoped to a few paths, and a section covers setting real client addresses behind a proxy or CDN. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The per-URL fetch lock had two implementations: an object cache entry that expired on its own, and an atomic SQL row. The cache variant could not tell whether its entry had expired and been taken by another request when releasing, so it guessed from the handle's age, and an expiring entry could be deleted under its new owner. Two lock kinds also doubled the cases to test and to reason about. The lock is now always the SQL row on the base-prefix options table: INSERT IGNORE takes it, an owned takeover of a stale lock is an UPDATE matching the old value that must change exactly one row, and an owned release is a DELETE matching our handle. The rate-limit counters keep their object cache branch, since a lost count there only makes a limit slightly soft. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
column_description() printed the stored post content as it was. The edit form saves the description through wp_kses_post(), so HTML is allowed, but a client created through Client::create() skips that filter. The column now runs the content through wp_kses_post() on output. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Describe the oauth2.issuer filter and that iss always matches the published metadata issuer, with a note that RFC 8414 and RFC 9207 expect an https issuer. Rewrite the limits section for per-site scoping on a network, say where each counter lives with and without an object cache, and show how to change the user and site-wide limits and windows with oauth2.cimd.rate_limit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
validate_client_id_url() refused only home_url() and its trailing-slash form as the site's own issuer. With the oauth2.issuer filter the published issuer can differ from home_url(), so a client ID equal to it was admitted. The check now rejects the issuer from Well_Known\get_issuer() as well, with and without a trailing slash, and still rejects home_url(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The oauth2.well_known_authorization_server_metadata filter can change the issuer a client reads from the RFC 8414 document. The RFC 9728 authorization_servers entry and the check that refuses the site's own issuer as a client ID still read the unfiltered value, so they could disagree with the published issuer and with iss. Both now also use Well_Known\get_published_issuer(). The metadata builder does not call the protected resource builder, so this does not recurse. validate_client_id_url() rejects home_url(), get_issuer() and the published issuer, each with and without a trailing slash. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The per-user counter is stored with update_user_option() and was never removed, so every user who fetched a document kept a user meta row for good. Reading an expired counter now deletes it before a new window starts, so the row exists only while a window is active or recently used. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Tell readers to set the issuer with the oauth2.issuer filter, and say that a change made through the metadata filter also reaches iss, the protected resource metadata and the client ID check but is not the supported route. Note that the per-user counter row is deleted once its window has expired. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
RFC 9700 section 4.8.2 says the authorization server MUST reject a token request that carries a code_verifier when the authorization request had no code_challenge. Without that check, an attacker who strips the challenge from the authorization request can downgrade the flow and redeem an intercepted code. The oauth2.pkce.reject_unexpected_verifier filter let a site turn that check off. PKCE has not shipped yet, so no client depends on the lenient path, and there is no legitimate case for accepting the verifier. Drop the filter and its test; the existing rejection tests at the code and token-endpoint level cover the behaviour. Raised in review on WP-API#85. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WP-CLI registers a method under its own name and treats @alias as an extra name, so the command answered to both generate_code_challenge and generate-code-challenge. @subcommand replaces the method name, leaving generate-code-challenge (the form the README and examples use) as the only way to call it. Raised in review on WP-API#85. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The filter's only use was to let plain satisfy a client's "PKCE required" setting. RFC 9700 section 2.1.1 explains that plain gives no protection against an app on the same device reading the authorization request, which is the threat a PKCE requirement exists to stop. A site that opts a client into PKCE should not be able to quietly weaken it back to plain, so "required" now always means S256. The weak_method error drops its sprintf now that the accepted method is fixed. The two tests that exercised the filter go with it; test_pkce_required_client_with_plain_is_rejected still covers the rejection. Found while sweeping the PR for hooks with no clear use, after review on WP-API#85. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
oauth2.pkce.supported_methods took an array, so a site could remove S256 as well as plain. RFC 7636 section 4.2 makes S256 mandatory to implement on the server, so dropping it would leave clients that do the right thing with no method to use. The array also invited values the plugin cannot derive a challenge for, which supported_methods() then had to strip out. The only useful choice a site has is whether to accept plain, so make that the whole filter: oauth2.pkce.allow_plain, a boolean that defaults to true. S256 is always supported. The tests that used the array filter now turn plain off instead, and the unsupported-method test sends plain and checks the error lists only S256. A new test covers an omitted method, which RFC 7636 section 4.3 treats as plain, so it is refused too when plain is off. Found while sweeping the PR for hooks with no clear use, after review on WP-API#85. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The CIMD draft (section 10.7) says a server must not fetch from special-use addresses, and PHP's NO_PRIV_RANGE and NO_RES_RANGE flags miss several of them. Each CIDR now carries a comment naming the range and its RFC so a reader can check the list against the registry without looking each one up. The fetch lock writes SQL to the options table directly, which looks odd next to add_option(). The docblocks now say why: add_option() checks and then upserts so two requests can both win, add_site_option() writes to sitemeta which has no unique key on meta_key, and an object cache lock cannot release atomically. They also describe the three statements and why the rows use base_prefix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The filter result was cast and clamped, so a negative limit became 0 and a string such as "many" became 0 too, which silently blocks every fetch. A site that returns junk now gets the default for the scope instead. The result must be an array with an integer-like limit of 0 or more and a window of 1 or more, and both are cast to int. Booleans, fractions and missing keys are rejected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Two filters could set the issuer, and only the metadata filter reaches every place that publishes it. get_issuer() now returns home_url(). The RFC 8414 document, the RFC 9207 iss parameter, the protected resource authorization_servers entry and the client ID self-check all read get_published_issuer(), which is the issuer after oauth2.well_known_authorization_server_metadata. The docs and tests now use that filter to change the issuer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The policy filter had three values, and deny let a site stop a stored client in two more places than admit_url does. The only real choice left is whether a new client publishes or waits for an admin, so that is now a boolean that defaults to true. When it is false a new record is saved as a draft and a security-hash change demotes a published record to draft, as the manual policy did. The filter receives the metadata and the URL. Blocking a client is done with oauth2.cimd.admit_url, which already applies at authorize, at the token endpoint and for bearer tokens. The deny tests are replaced by admit_url tests for an unknown URL, a stale stored record and a fresh one at authorize, plus the existing token and bearer checks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
None has a production use case, and allow_loopback and pre_resolve_host let a site switch off the rule that the server must not fetch from special-use addresses. The 300 to 3600 second clamp on the cache lifetime stays as fixed behaviour, and loopback addresses are always blocked. Tests still need to fake DNS, so resolve_host() keeps a static override that set_host_resolver() sets. It is marked @internal and exists for tests, and the test trait resets it in tear_down. Tests that needed a short cache lifetime now expire the stored record after discovery. The proxy environment regression test used allow_loopback to reach 127.0.0.1 through fetch_document(). The pin callbacks now come from create_pin_callbacks(), marked @internal, and the test runs them on a real curl handle with https_proxy set, then checks that the curl error does not mention a proxy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add PKCE support (RFC 7636)
The filter result was cast with (bool), so a string such as 'false', 'manual' or 'deny', a non-empty array or the int 1 published the client. That is the wrong way round for an approval switch, and a site that ported an old policy callback would approve everything. Only a return value of true now publishes. Any other value saves the record as a draft, which is what the docs and docblock now say. The docs also name the fields whose change demotes a published client: redirect URIs, grant types, response types and the authentication method, the same four the security hash covers. A data-provider test covers true, false, 'false', 'manual', 'deny', 1, 0, null, [] and ['x']. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
resolve_host() had grown two parameters only so set_host_resolver() could reach its static variable. The static now lives in a small @internal function, host_resolver_override(), that returns the override when called with no argument and stores one when called with an argument. set_host_resolver() writes through it, and resolve_host() takes one argument again and only reads it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A non-zero errno also passes when curl fails to resolve the host, which is what happens if the pin is not applied. CURLE_COULDNT_CONNECT only occurs when CURLOPT_RESOLVE sent curl to 127.0.0.1 and the connection was refused, so the test now proves the resolve entry was used as well as that no proxy was. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
willmot
reviewed
Nov 9, 2026
| * @return string Issuer identifier. | ||
| */ | ||
| function get_published_issuer() { | ||
| $metadata = get_authorization_server_metadata(); |
Member
There was a problem hiding this comment.
[P1] Keep issuer lookup safe during early bearer authentication
I reproduced a startup failure with this on a local hmn.md installation: native browser login, consent and the PKCE token exchange succeed, but the first authenticated MCP request returns HTTP 500.
The existing Try H2 plugin calls
current_user_can()duringplugins_loaded, which triggers bearer authentication before WordPress has initialized$wp_rewrite. CIMD client lookup reaches this function, which builds the full discovery document:
Access_Token::get_client() → get_client() → validate_client_id_url() → get_published_issuer() → get_authorization_server_metadata() → get_token_url() → rest_url()That ends with
Call to a member function using_index_permalinks() on null. The registered-client journey works on the same installation. I also reproduced it in a standalone WordPress regression test: with pretty permalinks and an uninitialized rewrite global, registered-token lookup succeeds and CIMD-token lookup throws.Could we resolve the published issuer independently of constructing endpoint URLs, and have discovery and authorization responses use that same value? The existing issuer override through
oauth2.well_known_authorization_server_metadataneeds to remain respected; simply returninghome_url()here would lose that behavior.Please add coverage for CIMD bearer lookup before rewrite initialization. With a QA-only early rewrite-object initialization, the remaining native browser → consent → token → MCP → draft journey passes, but that workaround should not be needed in production.
- Written by Codex gpt-6.1-sol medium
Collaborator
Author
There was a problem hiding this comment.
Thanks, fixed in 2a719f6.
Claude: Stored-client lookups (token endpoint and bearer auth) skip the issuer comparison now, so they never build the metadata document or touch
rest_url(). Only discovery at authorize compares the client ID with the issuer, and a stored client can only come from discovery. Theoauth2.well_known_authorization_server_metadataoverride still applies there. The new tests set pretty permalinks and null out$wp_rewrite, and they fail with your fatal without the fix.
Member
|
This is great and will mean |
willmot
approved these changes
Nov 9, 2026
Brings in the final PKCE commits merged upstream, so the CIMD branch can be opened against WP-API/OAuth2 with a single merge base. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WordPress 6.7 and 6.8 predate PHP 8.5 support. Their IXR classes use the (double) and (boolean) casts that PHP 8.5 deprecates at compile time. In the main test process the warnings only print, but the CIMD proxy tests run in a separate process, and PHPUnit fails any isolated test whose child writes to stderr. Excluding the unsupported pairs keeps the matrix to combinations WordPress supports. Silencing deprecation output in phpunit.xml would also hide deprecations from the plugin's own code, so it was not used. The versions list is resolved dynamically, so these entries stop matching once 6.7 and 6.8 leave the newest five. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bearer authentication can run before WordPress sets up $wp_rewrite, for example when a plugin calls current_user_can() on plugins_loaded. For a CIMD client, OAuth2\get_client() validated the client ID URL, and that validation compared it with the published issuer. Getting the published issuer builds the whole authorization server metadata, including rest_url() for the endpoints, and rest_url() fatals while $wp_rewrite is null. Registered clients were not affected. validate_client_id_url() gets a $check_issuer parameter, and get_client() passes its discover flag. Only discovery, which runs at the authorize step, compares the URL with the issuer. A stored client can only come from discovery, so it has already passed that check. Database-only lookups (token endpoint, bearer auth) still run the structural checks and the oauth2.cimd.admit_url filter. They also no longer build the metadata document on every authenticated request. The reviewer suggested resolving the issuer without building endpoint URLs. That would need a second path that matches the oauth2.well_known_authorization_server_metadata filter, which receives and can rewrite the full document. Skipping the check where it is redundant keeps a single issuer source. The new tests set a permalink structure and null out $wp_rewrite. Without the fix, both resolver tests fail with the reported fatal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
joehoyle
reviewed
Nov 12, 2026
| if ( is_loopback_host( $redirect_host ) ) { | ||
| printf( | ||
| '<p class="login-origin login-origin-warning" style="margin-bottom: 16px;"><strong>%s</strong></p>', | ||
| esc_html__( 'This app will run on this device. Only continue if you started it yourself.', 'oauth2' ) |
Member
There was a problem hiding this comment.
What is "this device"? Is it "this website" or "this browser"? also app -> application
Collaborator
Author
There was a problem hiding this comment.
It means the computer or phone the browser is running on. The warning only shows for a loopback redirect, where a local program like a CLI receives the code. I've reworded it to "This application is running on the device you are using now, not on a website." and switched to "application" in 3940091.
Posted on my behalf by Claude.
A reviewer asked whether "this device" meant the website or the browser. The warning shows when the redirect URI is a loopback address, so the authorization code goes to a program listening on the computer or phone the browser runs on. The new text says that directly and contrasts it with a website. The consent strings also say "application" rather than "app", to match the rest of the plugin's UI. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds support for OAuth Client ID Metadata Documents (CIMD), so an app can use an https URL as its
client_idwith no manual registration.CIMD lets the server fetch a small JSON document from the client_id URL. The MCP authorization spec now prefers CIMD (servers SHOULD support it) and marks Dynamic Client Registration as deprecated. This follows draft-ietf-oauth-client-id-metadata-document-03.
CIMD is on by default, with auto-approve. Three filters control it:
oauth2.cimd.enabled(on or off),oauth2.cimd.admit_url(allow or block a client URL, which is the way to block a client) andoauth2.cimd.auto_approve.auto_approveis a boolean. Only a realtruepublishes a new client. Anything else leaves it as a draft until an admin approves it.oauth2.cimd.rate_limitreturns the limit and window. Invalid values fall back to the defaults.When the server fetches
Safe fetching
Strict validation
client_idmust match its URL.Storage
oauth2_clientrecord keyed by the URL. Its state lives in one checked meta value.Limits
Consent screen. It shows the host the app is published at and the redirect host. Loopback redirects get a warning.
Admin. CIMD apps get a read-only view. They can still be approved and deleted.
Related specs
client_id_metadata_document_supported.issparameter is added to all authorization responses, andauthorization_response_iss_parameter_supportedis advertised. The issuer ishome_url. A site can change it with the existingoauth2.well_known_authorization_server_metadatafilter. Theissparameter, the protected-resource metadata and the client-ID check all follow the published value.Small fixes to existing code
Testing. Both the single-site and multisite PHPUnit suites pass with 900 tests. phpcs is clean. Every stage had two independent code reviews.
Docs. See
docs/cimd.md.The applications list, with CIMD apps labelled "Metadata document":
The read-only edit view of an approved CIMD app:
A pending CIMD app with the Approve button:
The consent screen, showing the app's host, the redirect host and the loopback warning:
Known limits
Also open upstream as WP-API#92.
🤖 Posted on my behalf by Claude Code.