Skip to content

Commit ea8e801

Browse files
darssenCopilot
darssen
and
Copilot
authored
JSON API: serializable_error() valid-int-only status hardening (#50077)
* JSON API: serializable_error() valid-int-only status hardening (Phase A for CONNECT-267 problem 1) * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Update comments to remove LINEAR references * Address Copilot review: clarify sub-400 comment, add non-numeric scalar test * Fix phpcs double-arrow alignment in data provider --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
1 parent fecf6e2 commit ea8e801

3 files changed

Lines changed: 180 additions & 6 deletions

File tree

‎projects/plugins/jetpack/changelog/update-json-api-serializable-error-valid-int‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
Significance: patch
2+
Type: bugfix
3+
4+
JSON API: Ensure error responses always serialize an HTTP error status (>= 400), never a non-integer or a 2xx that clients could interpret as success.

‎projects/plugins/jetpack/class.json-api.php‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -775,13 +775,12 @@ public static function wrap_http_envelope( $status_code, $response, $content_typ
775775
*/
776776
public static function serializable_error( $error ) {
777777

778-
$status_code = $error->get_error_data();
778+
// Always serialize a valid HTTP error status >= 400 -- never 1, a non-integer, or any sub-400 (2xx/3xx) an app could read as success.
779+
$data = $error->get_error_data();
780+
$status_code = ( is_array( $data ) && isset( $data['status_code'] ) ) ? $data['status_code'] : $data;
781+
$status_code = is_numeric( $status_code ) ? (int) $status_code : 0;
779782

780-
if ( is_array( $status_code ) && isset( $status_code['status_code'] ) ) {
781-
$status_code = $status_code['status_code'];
782-
}
783-
784-
if ( ! $status_code ) {
783+
if ( $status_code < 400 ) {
785784
$status_code = 400;
786785
}
787786
$response = array(

‎projects/plugins/jetpack/tests/php/json-api/WPCOM_JSON_API_Serializable_Error_Test.php‎

Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,171 @@
1+
<?php
2+
/**
3+
* WPCOM_JSON_API::serializable_error() unit tests.
4+
*
5+
* Run this test with command: jetpack docker phpunit jetpack -- --filter=WPCOM_JSON_API_Serializable_Error_Test
6+
*
7+
* @package automattic/jetpack
8+
*/
9+
10+
use Automattic\Jetpack\PHPUnit\WP_UnitTestCase_Fix;
11+
use PHPUnit\Framework\Attributes\CoversClass;
12+
use PHPUnit\Framework\Attributes\CoversMethod;
13+
use PHPUnit\Framework\Attributes\DataProvider;
14+
15+
require_once JETPACK__PLUGIN_DIR . 'class.json-api-endpoints.php';
16+
17+
/**
18+
* Tests that serializable_error() always serializes a valid HTTP error status: never
19+
* `1`, never a non-integer, and never a `< 400` status a client could read as success.
20+
*
21+
* @covers \WPCOM_JSON_API::serializable_error
22+
* @covers \WPCOM_JSON_API
23+
*/
24+
#[CoversClass( WPCOM_JSON_API::class )]
25+
#[CoversMethod( WPCOM_JSON_API::class, 'serializable_error' )]
26+
class WPCOM_JSON_API_Serializable_Error_Test extends WP_UnitTestCase {
27+
use WP_UnitTestCase_Fix;
28+
29+
/**
30+
* The rendered status_code for a given WP_Error.
31+
*
32+
* @param WP_Error $error Error.
33+
* @return int
34+
*/
35+
private function status_for( $error ): int {
36+
$serialized = WPCOM_JSON_API::serializable_error( $error );
37+
return $serialized['status_code'];
38+
}
39+
40+
/**
41+
* A valid status_code in the data passes through unchanged -- both the
42+
* canonical array key and a bare-integer data value.
43+
*/
44+
public function test_valid_status_passes_through() {
45+
$this->assertSame( 404, $this->status_for( new WP_Error( 'not_found', 'Nope', array( 'status_code' => 404 ) ) ) );
46+
$this->assertSame( 451, $this->status_for( new WP_Error( 'legal', 'Blocked', 451 ) ) );
47+
}
48+
49+
/**
50+
* The incident class: array error data WITHOUT a `status_code` key (e.g. the
51+
* WP-REST `status` shape) must never survive as a truthy array and `(int)`-cast
52+
* to `1`. It falls to the safe `400` default instead.
53+
*/
54+
public function test_array_without_status_code_is_safe_not_1() {
55+
foreach (
56+
array(
57+
new WP_Error( 'forbidden', 'No', array( 'status' => 403 ) ),
58+
new WP_Error( 'weird', 'Weird', array( 'foo' => 'bar' ) ),
59+
new WP_Error( 'empty', 'Empty', array() ),
60+
) as $error
61+
) {
62+
$status = $this->status_for( $error );
63+
$this->assertIsInt( $status );
64+
$this->assertNotSame( 1, $status );
65+
$this->assertSame( 400, $status );
66+
}
67+
}
68+
69+
/**
70+
* No data at all keeps the historical 400 default.
71+
*/
72+
public function test_no_data_defaults_to_400() {
73+
$this->assertSame( 400, $this->status_for( new WP_Error( 'generic', 'Generic' ) ) );
74+
}
75+
76+
/**
77+
* A non-numeric scalar (e.g. a string) error data value must never `(int)`-cast
78+
* to `0`/`1`; it falls to the safe `400` default.
79+
*
80+
* @param mixed $input Non-numeric scalar carried as the error data.
81+
* @dataProvider provide_non_numeric_scalars
82+
*/
83+
#[DataProvider( 'provide_non_numeric_scalars' )]
84+
public function test_non_numeric_scalar_is_safe_not_0_or_1( $input ) {
85+
$status = $this->status_for( new WP_Error( 'scalar', 'Scalar', $input ) );
86+
$this->assertIsInt( $status );
87+
$this->assertSame( 400, $status );
88+
}
89+
90+
/**
91+
* Data provider: non-numeric scalar error data values.
92+
*
93+
* @return array<string, array{mixed}>
94+
*/
95+
public static function provide_non_numeric_scalars(): array {
96+
return array(
97+
'plain string' => array( 'not a number' ),
98+
'mixed string' => array( 'error-42' ),
99+
'bool true' => array( true ),
100+
);
101+
}
102+
103+
/**
104+
* A success/redirect status paired with an error must never render as `< 400`
105+
* (the crash: an app reads a 2xx as a successful, URL-less site).
106+
*
107+
* @param int $input Non-error status carried on the error.
108+
* @dataProvider provide_non_error_statuses
109+
*/
110+
#[DataProvider( 'provide_non_error_statuses' )]
111+
public function test_non_error_status_coerced_to_400( $input ) {
112+
$this->assertSame( 400, $this->status_for( new WP_Error( 'oops', 'Oops', array( 'status_code' => $input ) ) ) );
113+
}
114+
115+
/**
116+
* Data provider: statuses a client could read as success.
117+
*
118+
* @return array<string, array{int}>
119+
*/
120+
public static function provide_non_error_statuses(): array {
121+
return array(
122+
'200 OK' => array( 200 ),
123+
'201 Created' => array( 201 ),
124+
'302 Found' => array( 302 ),
125+
);
126+
}
127+
128+
/**
129+
* Codes status_header() cannot render (Cloudflare 52x, other non-standard) are a valid
130+
* integer >= 400, so they pass through unchanged. Coercing them to a renderable status is
131+
* deliberately NOT this function's job -- that belongs at the status_header() call site;
132+
* here we only guarantee a sane integer.
133+
*
134+
* @param int $input Unknown-to-WP status carried on the error.
135+
* @dataProvider provide_unknown_statuses
136+
*/
137+
#[DataProvider( 'provide_unknown_statuses' )]
138+
public function test_unknown_status_passes_through_unchanged( $input ) {
139+
$this->assertSame( $input, $this->status_for( new WP_Error( 'upstream', 'Upstream', array( 'status_code' => $input ) ) ) );
140+
}
141+
142+
/**
143+
* Data provider: codes WP's get_status_header_desc() doesn't know.
144+
*
145+
* @return array<string, array{int}>
146+
*/
147+
public static function provide_unknown_statuses(): array {
148+
return array(
149+
'520 Cloudflare' => array( 520 ),
150+
'521 Cloudflare' => array( 521 ),
151+
'523 Cloudflare' => array( 523 ),
152+
'599 non-std' => array( 599 ),
153+
);
154+
}
155+
156+
/**
157+
* The error body shape (code + message + additional_data) is preserved
158+
* alongside the hardened status.
159+
*/
160+
public function test_error_shape_preserved() {
161+
$error = new WP_Error( 'my_code', 'My message', array( 'status_code' => 422 ) );
162+
$error->add_data( array( 'field' => 'name' ), 'additional_data' );
163+
164+
$serialized = WPCOM_JSON_API::serializable_error( $error );
165+
166+
$this->assertSame( 422, $serialized['status_code'] );
167+
$this->assertSame( 'my_code', $serialized['errors']['error'] );
168+
$this->assertSame( 'My message', $serialized['errors']['message'] );
169+
$this->assertSame( array( 'field' => 'name' ), $serialized['errors']['data'] );
170+
}
171+
}

0 commit comments

Comments
 (0)