Stop the Bluesky login from expiring every two weeks

[pfefferle]

Fixes the two-week disconnects behind #233, #234 and #263.

Proposed changes:

The plugin was a public OAuth client. Bluesky caps a public client's session at two weeks, no matter how often the token is refreshed (see client.ts in the reference server). That is why every site had to reconnect every two weeks, and why #233, #234 and #263 could only make the expiry visible.

Confidential clients get two years, with a refresh at least every three months. So the plugin now:

• generates a per-site signing key (encrypted like the tokens) and publishes its public half in a new client metadata document at atmosphere/v2/client-metadata. That URL is the new client_id.
• sends a private_key_jwt assertion on PAR, code exchange and refresh.
• keeps serving the old public-client document at atmosphere/v1/client-metadata. Sessions created before this update have no stored client_id and keep refreshing there without an assertion, so nothing breaks until the user reconnects. The old endpoint has to stay until those sessions are gone.
• no longer forces a reconnect on invalid_client / unauthorized_client. Reconnecting the same client cannot fix a rejected client registration, so the session is kept and Site Health says what is wrong instead.
• adds two Site Health checks: a rejected client configuration (critical) and no renewal for more than a day (recommended, usually a dead WP-Cron).

I fixed three things in the first version that would have broken every new connection: the v2 route fataled when called through the REST server, the assertion aud was the token endpoint instead of the issuer, and token_endpoint_auth_signing_alg was missing from the metadata. All three are pinned by tests now.

After the review, the plugin also revokes the refresh token as the client the session belongs to when you disconnect, and replaces a signing key that no longer decrypts (rotated salts) as soon as no live session depends on it, so reconnecting works without touching the database.

A rejected client no longer retries the refresh on every request: refreshing pauses for five minutes after a client-configuration or signing-key failure, and a publish that lands in that window is retried by the normal ladder.

Other information:

• Have you written new tests for your changes, if applicable?

Testing instructions:

• Update a connected site to this branch. Nothing should change: wp-env run cli wp option get atmosphere_connection has no client_id, and publishing still works.
• Open /wp-json/atmosphere/v2/client-metadata. It should show token_endpoint_auth_method: private_key_jwt, token_endpoint_auth_signing_alg: ES256 and a jwks with one key that has no d member.
• Open /wp-json/atmosphere/v1/client-metadata. It still says none and has no jwks.
• Disconnect and connect again. The connection now stores a client_id ending in /v2/client-metadata, and wp-env run cli wp cron event run atmosphere_refresh_token renews it. Site Health → Info → ATmosphere shows the renewal time.
• Site Health → Status: run wp-env run cli wp option patch update atmosphere_refresh_status last_success 1 and reload, the "not renewed" recommendation appears. Then set last_error to invalid_client and last_failure to the current timestamp, the critical entry appears and no reconnect notice is shown.

Changelog entry

Two entries are already on the branch.

• Automatically create a changelog entry from the details below.

[Copilot]

🟡 Changes recommended

Four moderate issues remain around key validation, legacy OAuth compatibility, and Site Health migration handling.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR migrates new Bluesky OAuth connections to confidential private_key_jwt clients while preserving legacy sessions and adding renewal diagnostics.

Changes:

• Adds encrypted ES256 signing keys, v2 metadata/JWKS, and client assertions.
• Preserves v1 compatibility and updates OAuth lifecycle handling.
• Adds Site Health checks, tests, documentation, and changelog entries.

File summaries

File	Reviewed changes
uninstall.php	Removes the OAuth signing key on uninstall.
tests/phpunit/trait-jwt-claims.php	Adds shared JWT claim decoding.
tests/phpunit/tests/wp-admin/class-test-health-check.php	Tests renewal and configuration health states.
tests/phpunit/tests/rest/class-test-client-metadata-filter.php	Tests v1/v2 metadata and routes.
tests/phpunit/tests/oauth/class-test-client-refresh.php	Tests confidential and legacy refresh behavior.
tests/phpunit/tests/oauth/class-test-client-id.php	Tests v1/v2 client identifiers.
tests/phpunit/tests/oauth/class-test-client-authorize.php	Tests OAuth assertions and issuer audiences.
tests/phpunit/tests/oauth/class-test-client-authentication.php	Tests signing-key generation and assertions.
tests/phpunit/bootstrap.php	Loads JWT test helpers.
includes/wp-admin/class-health-check.php	Adds renewal health diagnostics.
includes/rest/class-legacy-client-metadata-controller.php	Serves legacy public-client metadata.
includes/rest/class-client-metadata-controller.php	Serves confidential-client metadata and JWKS.
includes/oauth/class-dpop.php	Creates client-authentication assertions.
includes/oauth/class-client.php	Updates OAuth lifecycle and refresh handling.
includes/oauth/class-client-authentication.php	Manages encrypted signing credentials.
includes/class-atmosphere.php	Registers metadata routes and error classification.
FEDERATION.md	Documents versioned metadata endpoints.
docs/php-class-structure.md	Documents OAuth client authentication.
docs/developer-docs.md	Adds confidential OAuth guidance.
.github/changelog/add-oauth-client-site-health	Adds the Site Health changelog entry.
.github/changelog/add-confidential-oauth-client	Adds the confidential-client changelog entry.

Review details

Suppressed comments (3)

includes/oauth/class-client-authentication.php:167

• This validation only checks that x, y, and d are strings. A decryptable but corrupted key such as one with invalid base64url members passes valid_key(), so jwks() returns HTTP 200 with an unusable public key; the PEM/signing path fails only later. Validate the key members before publishing the JWKS so malformed stored keys take the existing error path instead of registering a broken client.

			&& \is_string( $key['x'] ?? null )
			&& \is_string( $key['y'] ?? null )
			&& \is_string( $key['d'] ?? null );

includes/oauth/class-client.php:202

• Changing this helper to v2 also changes the existing revoke_refresh_token() path, which still sends self::client_id() and receives no session-specific client ID. Disconnecting a pre-update session therefore sends the v2 client ID even though its token was issued to the frozen v1 public client, so the legacy-session compatibility guarantee is broken at revocation. Pass the stored client ID through the revoke job or use the v1 fallback when the connection has no client_id.

		return \set_url_scheme( \rest_url( 'atmosphere/v2/client-metadata' ), 'https' );

includes/wp-admin/class-health-check.php:513

• An upgraded site with an existing v1 connection has no last_success until a refresh succeeds. If WP-Cron is the stalled component this check never becomes true, so Site Health remains good instead of reporting the missing renewal—the condition this new check is intended to catch. Add a migration/connection-age fallback (or explicitly handle the legacy never heartbeat) so old connections can be diagnosed too.

	private static function renewal_is_stale( array $status ): bool {
		return ! empty( $status['last_success'] )
			&& (int) $status['last_success'] < \time() - self::RENEWAL_STALE_AFTER;

• Files reviewed: 22/22 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[kraftbj]

Two blockers, then smaller stuff. I checked the assertion against the atproto OAuth spec and it lines up: inline jwks is allowed, aud is the issuer, iat/jti are there, private_key_jwt is declared. The legacy split is careful too, including a flow started under the old version mid-upgrade.

Disconnect stops revoking the refresh token. revoke_refresh_token() wasn't updated. includes/oauth/class-client.php:2181 still sends self::client_id(), which is now the v2 ID, and never attaches an assertion. For a legacy session that's a mismatched client for a token minted under v1, so the server rejects it. For a confidential session the ID is right but RFC 7009 defers client auth to RFC 6749 section 2.3, which requires it. Either way the revocation fails silently (best-effort cron, debug_log only): the user hits Disconnect, we wipe the local row, and the token stays live on the PDS. That's worse than before this PR, since the whole point here is that those tokens now live a long time. The worker only receives ciphertexts, so the session's client_id needs to ride along in the cron args, with a default for events already queued.

A key that stops decrypting bricks the plugin. Client_Authentication::key() returns a WP_Error and never regenerates, and nothing clears KEY_OPTION outside uninstall. Encryption falls back to AUTH_KEY . AUTH_SALT by default, and its own docblock already calls out salt rotation as something that happens to real sites. After a rotation: tokens are undecryptable, the connection flags needs_reauth, and the documented fix is "reconnect" — but reconnect goes through authorize_via_par() to sign_request() and hits the same error. The v2 metadata endpoint 500s too. The only way out is deleting the option by hand.

I get why you don't want to silently replace the key (the spec binds a live session to its kid/jkt), but in that state the sessions are already dead, so there's nothing left to protect. Regenerating when there's no usable connection, or a Site Health action that resets it, would both work.

Smaller:

• renewal_is_stale() only looks at last_success, so any failure that isn't invalid_grant or a client-config error (network, 5xx, DPoP) ages it out and we tell the admin to "Configure a real server cron" on a site whose cron is fine. Worth checking last_failure > last_success and saying something else in that case.
• Nothing strips jwks_uri from the filtered metadata. pinned_fields() re-merges jwks, but the spec says a confidential client supplies one or the other and not both, so a host plugin's filter can quietly break client auth. One unset() next to the re-merge.
• test_sign_request_attaches_an_assertion decodes the claims but never verifies the signature, and nothing asserts the assertion's kid matches the published JWKS. If those ever drift, every connect and refresh fails and the suite stays green. An openssl_verify() round-trip against the public key would pin it.
• is_transient_publish_error() doesn't know about atmosphere_client_authentication / atmosphere_client_authentication_key, so an unreadable key burns three publish retries per post.
• The legacy-reconnect nudge lands on the settings page and Site Health, but not the connectors card or the editor surfaces. In connection-only mode the settings page is hidden, so Site Health is the only place left to see it.
• use function Atmosphere\is_legacy_connection is out of alphabetical order in class-health-check.php:33.
• The race comment in key() promises a bit more than WordPress gives: add_option() uses INSERT ... ON DUPLICATE KEY UPDATE, so a genuine tie clobbers rather than the first writer winning.

composer lint is clean. I haven't run the PHP suite.

[pfefferle]

Thanks @kraftbj, both blockers were real.

• Revocation: the session's client_id now rides along in the cron args (empty for a legacy row, and for events queued before this change), and the worker signs the revocation for a confidential session, on the nonce retry too. ab90500
• Key rotation: an unreadable or malformed signing key is discarded and regenerated as soon as no live session matches the current key material, so reconnecting works after a salt rotation without touching the database. While a matching session still exists the key stays put and the error is reported. 3f3935e
• The smaller ones are in 34a42b9: a failed renewal gets its own Site Health state instead of the cron advice, jwks_uri is stripped next to the re-merge, the assertion is verified with openssl_verify against the published JWKS including the kid, the two key error codes are permanent for the publish ladder, the connectors card shows the reconnect hint, and the import order and the race comment are fixed.

I left the editor surfaces out of the nudge on purpose: they explain a post's share state, and in connection-only mode cross-posting is off anyway, so a reconnect hint there would be noise.

[Copilot]

🟡 Changes recommended

Three moderate OAuth authentication and recovery issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

• build/connectors-card/index.js: Generated file

Suppressed comments (2)

includes/class-atmosphere.php:2819

• Client_Authentication::key() can return atmosphere_dpop_keygen_failed directly when the host cannot generate the EC signing key, but this error is not in the permanent-error list. is_transient_publish_error() therefore sees status 0 and schedules the full retry ladder for a deterministic OpenSSL failure, repeatedly retrying publishes that cannot succeed. Classify this code as permanent (and add a regression test).

			'atmosphere_client_authentication',
			'atmosphere_client_authentication_key',
			'atmosphere_client_configuration',

includes/oauth/class-client-authentication.php:185

• session_bound_to_key() treats every connected row with no key_fingerprint as dependent on the signing key. Legacy sessions created before this change have no client_id and never used this new signing key, so an unreadable key makes the v2 metadata endpoint return 500 and prevents that user from reconnecting. Only treat a session carrying the confidential client ID as bound to this key (or store a separate signing-key fingerprint).

		$fingerprint = (string) ( get_connection()['key_fingerprint'] ?? '' );

		return '' === $fingerprint || \hash_equals( Encryption::key_fingerprint(), $fingerprint );

• Files reviewed: 30/31 changed files
• Comments generated: 2
• Review effort level: Lite

[kraftbj]

Both blockers and the smaller items from my last pass are fixed. I checked each one against the code, not just the commit messages.

Inline comments below. The key-padding one is what I'd fix before merging: it came in with the stricter key validation, it breaks roughly 1 in 70 first connects, and it makes two of the new tests flaky.

[pfefferle]

Thanks @kraftbj, all eight are in (b6a8751).

Padding was the real find, I could reproduce your rate (22 of 2000 keys). generate_key() pads now, which is also what RFC 7518 asks for, and a test checks 200 generated keys. The bound-session check is scoped to confidential sessions, with the fixture fixed and a legacy-replacement test. A moved client_id marks reauth with its own reason and copy. The hold is a 5 minute transient checked before the lock; it returns its own code so the publish ladder retries instead of dropping the post. Keygen code added to the permanent list, the metadata route returns a WP_Error, the unreadable key has its own critical Site Health state, docs tree updated.

One thing I found while re-checking: the reference server answers a missing or mismatched metadata document with invalid_client_metadata, not invalid_client, so I added that code, otherwise neither the moved-domain branch nor the hold would have fired against bsky.social.

Known limit: on sites where a multilingual plugin varies home_url per request, the recomputed client_id differs between cron and web context, so a client rejection there ends in a reconnect prompt rather than the configuration state. I think that is acceptable, the rejection means refresh had already failed.

I have not done a live connect and publish against a PDS with the padded DPoP keys yet.

[Copilot]

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

• build/connectors-card/index.js: Generated file

Suppressed comments (5)

includes/oauth/class-client-authentication.php:252

• valid_key() only proves that each member decodes to 32 bytes; it never verifies that x/y are a valid P-256 public point or that they correspond to d. A shape-correct but corrupted option is therefore published in JWKS and used for signing, where it can fail indefinitely while bypassing the replace-or-report path. Validate the reconstructed key pair before accepting the row.

			$bytes = \base64_decode( \strtr( $value, '-_', '+/' ), true ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_decode
			if ( false === $bytes || 32 !== \strlen( $bytes ) ) {
				return false;

includes/oauth/class-client-authentication.php:215

• The pending-flow protection is lost before the code exchange: handle_callback() deletes atmosphere_oauth_resolved before calling Client_Authentication::sign_request(). If the client-authentication key becomes unreadable after PAR, stored_key() sees no pending flow or live session, replaces it, and signs with a new key while the authorization server may still have the old JWKS cached. Keep an in-flight marker until the token exchange completes (or defer deleting this transient) so the announced key cannot rotate.

		$pending = \get_transient( 'atmosphere_oauth_resolved' );
		if ( \is_array( $pending ) && ! empty( $pending['client_id'] ) ) {
			return true;

includes/oauth/class-client.php:963

• This path now suppresses repeated token requests for five minutes via REFRESH_HOLD_TRANSIENT, and the tests assert that the second call never reaches the endpoint, but the PR description still says rejected clients retry on every request and that refresh backoff is deferred. Please update the description/follow-up list to match the implemented behavior, including its five-minute retry window.

		$hold = \get_transient( self::REFRESH_HOLD_TRANSIENT );

includes/oauth/class-client.php:1040

• This check and the following transient write are not atomic. A refresh can pass the row comparison, then disconnect() can delete the hold and create a new session before set_transient() runs; the old failure will then put the new session on hold for five minutes. Bind the hold to the session (for example, the refresh-token ciphertext) and validate that binding when reading it, in addition to this write-side check.

				\set_transient(
					self::REFRESH_HOLD_TRANSIENT,
					array(
						'code'    => (string) $result->get_error_code(),
						'message' => $result->get_error_message(),

includes/oauth/class-client.php:1292

• The moved-client recovery is gated on $confidential, but legacy sessions also derive their v1 client_id from the current rest_url() on every refresh and do not store the ID they were minted under. After a domain/permalink move, such a session sends a different v1 client ID and can receive invalid_client; this branch preserves it and holds retries, while Site Health offers no reconnect action even though reconnecting under v2 is the recovery. Legacy sessions need an explicit migration/reconnect classification for this case.

				if ( $confidential && self::client_id() !== $client_id ) {

• Files reviewed: 33/34 changed files
• Comments generated: 3
• Review effort level: Lite

[kraftbj]

Nice round. One thing I'd fix before merging, inline. Everything else is non-blocking.

Worth doing, not blocking:

• Legacy sessions after a domain or permalink move (Copilot's point, class-client.php:1292). The v1 client_id is recomputed from rest_url(), so a moved site sends a different ID, gets invalid_client or invalid_client_metadata, and lands in the configuration state plus the hold. Before this PR that was a reconnect. For a legacy session reconnecting is always the fix (it moves to v2), so I'd send every client-configuration error on a legacy session to reauth.

Non-blocking:

• The hold isn't bound to a session. The row check and set_transient() aren't atomic, so a reconnect landing between them inherits a 5-minute hold. Storing a hash of the refresh_token ciphertext in the hold and ignoring mismatches closes it. Low impact, since a fresh token rarely refreshes inside 5 minutes.
• test_generate_key_pads_every_member_to_32_bytes still passes about 10% of the time with the padding reverted (roughly 3 in 256 keys come back short, over 200 draws). A fixed-vector test with a 31-byte component would pin it.
• The new is_safe_https_url( $client_id ) guard in revoke_refresh_token() has no test.
• Client_Metadata_Controller::ROUTE_NAMESPACE shipped as a public constant and is gone now. Nothing in-tree uses it, but an alias is one line and matches the other controllers.
• The PR description still says there's no backoff.

composer lint is clean and CI is green. I haven't run the suite locally.

[pfefferle]

Third round is in (182d732). The blocker was real, thanks for the trace: the pending check saw the connect attempt itself and looped. It is gone, and a test now runs authorize() against an unreadable key.

Also done: a legacy session reconnects on any client-configuration error, the hold is bound to the session whose refresh failed, the padding test uses a fixed 31-byte vector, the revocation client_id guard has a test, ROUTE_NAMESPACE is back, and the description no longer claims there is no backoff.

One thing I noticed in the reference server while re-reading the refresh path: it lets a session that started as a public client upgrade to confidential on refresh under the same client_id. If that holds against bsky.social, a follow-up could make the v1 document confidential too and the remaining legacy sessions would upgrade on their next refresh without anyone reconnecting. I have not tested it live, so nothing in this PR depends on it.

[jeherve]

Hey @pfefferle! I went through the branch again at 182d732. The new commits fix the key padding and the unreadable-key recovery, and revocation now goes out as the client the session belongs to.

There are a few things left that were surfaced by Claude, and I think we should address the first one before we merge.

A rejected client configuration only shows up in Site Health. For a confidential session, invalid_client / unauthorized_client now keep the session and put the refresh on hold. That makes sense to me, since reconnecting the same client wouldn't fix anything. That said, maybe_render_reauth_notice() never fires in that case, and atmosphere_client_configuration is a permanent publish error. So posts stop going to Bluesky, and the only signs in the admin are a Site Health entry and a debug log line. I'm wondering if we could show an admin notice for that state too, pointing people to Site Health.

Smaller things, feel free to take or leave:

• An anonymous GET /wp-json/atmosphere/v2/client-metadata creates the signing key and writes the option on a site that has never connected. It only happens once, so I don't think it's a big deal; I'd just rather the first write came from an admin connecting.
• The "more than 24 hours" in the stale-renewal Site Health message is hardcoded right next to RENEWAL_STALE_AFTER, so the two could drift apart if the constant ever changes.
• The atmosphere_client_metadata docblock still describes client_id as "advertised as the OAuth client identifier", but it's now reset after the filter runs, and filters can't change token_endpoint_auth_method anymore either. Mentioning that in the hook doc would help anyone who already has a filter in place.

Let me know what you think!

[pfefferle]

Thanks @jeherve. The notice is in (153b10c): both states that keep the session but block renewal, a rejected client registration and an unreadable signing key, now get a site-wide notice that points to Site Health, with the same audience rules as the reconnect notice. The Site Health test and the notice read the same predicate so they cannot drift. The 24 hours now come from the constant, and the filter docblock says which fields are reset after it runs.

The anonymous GET I left as is. The auth server fetches the document during connect, after authorize() has already created the key, so on a connecting site the first write does come from an admin. On a never-connected site, refusing to create it would mean serving an invalid document or a 500, and the Site Health reachability test probes exactly that URL. The public half is not a secret and creation is first-writer-wins.

[jeherve]

This is looking good, should be good to merge now! 🚢

[jeffpaul]

@pfefferle THANK YOU FOR THIS ONE!