Automattic / Automattic/jetpack

My Jetpack: chat authentication caches failed WordPress.com responses and emits an invalid status

Open
#50,902 0 comments 0 reactions 0 assignees View on GitHub
Bug
Dominant language
PHP
Stars
1.8k
Forks
898
Avg merge
1d 18h
Merged PRs (30d)
774

Description

`REST_Zendesk_Chat` in `projects/packages/my-jetpack` treats any non-empty response body from WordPress.com as a success, and emits an invalid HTTP status when the request fails.

### Failed responses are cached

`get_chat_authentication()` guards only on `is_wp_error()` and an empty body:

```php
if ( is_wp_error( $response ) || empty( $response['body'] ) ) {
return new WP_Error( 'chat_authentication_failed', … );
}

set_transient( $transient_key, $body, self::TRANSIENT_EXPIRY );
```

A 4xx or 5xx from WordPress.com that returns a body passes that guard and is stored in the transient cache, so a single failed upstream response can be served from cache for the full one-week TTL rather than being retried.

### The emitted status is not a valid HTTP status

`wp_remote_retrieve_response_code()` is called *before* the `is_wp_error()` check, so a transport error (for example a missing user token) yields `''`, and the REST server emits status `0`. `WP_REST_Response::is_error()` returns `false` for that, so a failed request can read as a success to a client.

`get_chat_availability()` has the identical shape in its `chat_config_data_fetch_failed` path.

### What the fix should entail

Fail on any non-200, don't cache it, and emit a valid status. Both handlers already compute `$response_code`, so it is a matter of including it in the existing guard and falling back when it is empty:

```php
if ( is_wp_error( $response ) || empty( $response['body'] ) || 200 !== $response_code ) {
return new WP_Error( '…', '…', array( 'status' => $response_code ? $response_code : 400 ) );
}
```

`is_wp_error()` must stay first so the `$response['body']` access remains short-circuited.

This matches the convention already used elsewhere in the same package — `class-rest-purchases.php` and `class-rest-recommendations-evaluation.php` both gate on `200 !== $response_code`.

Worth covering with tests, for both handlers, that a non-200 is not cached, that a caller recovers once the upstream is healthy again, and that the emitted status is valid.

### Worth deciding first

Both `chat/*` routes look orphaned. No JS in the monorepo calls `chat/authentication` or `chat/availability` — the `ZendeskChat` component and its queries were removed in #42477 — and the endpoint constants at `projects/packages/my-jetpack/_inc/data/constants.ts` (`REST_API_CHAT_AVAILABILITY_ENDPOINT`, `REST_API_CHAT_AUTHENTICATION_ENDPOINT`) are declared but never imported. The routes are still registered in `Initializer` and callable by any logged-in user.

If the intent is to remove the routes rather than keep them, that would make this moot, so it is probably worth settling that before fixing the caching.

Contributor guide

Open the contributing guide

Research direction

First settle whether the orphaned chat routes should remain, since removing them would make this fix unnecessary. If they remain, inspect REST_Zendesk_Chat in projects/packages/my-jetpack and compare the response-code checks in class-rest-purchases.php and class-rest-recommendations-evaluation.php. Locate the tests for both handlers, then verify failed non-200 responses are not cached, healthy retries recover, and emitted statuses are valid.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
api, backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.