Skip to content

Commit 8a27745

Browse files
pfefferlepfefferle
authored
Harden AT Protocol HTTP redirects (#171)
1 parent 8aa9b9d commit 8a27745

7 files changed

Lines changed: 332 additions & 36 deletions

File tree

‎.github/changelog/harden-atproto-http-redirects‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
Significance: patch
2+
Type: fixed
3+
4+
Hardened the security of connections to your Bluesky account.

‎includes/class-api.php‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -78,16 +78,18 @@ public static function request( string $method, string $endpoint, array $args =
7878
}
7979

8080
$defaults = array(
81-
'method' => $method,
82-
'headers' => array(
81+
'method' => $method,
82+
'headers' => array(
8383
'Authorization' => 'DPoP ' . $access_token,
8484
'Content-Type' => $content_type,
8585
'DPoP' => $dpop_proof,
8686
),
87-
'timeout' => 30,
87+
'timeout' => 30,
88+
'redirection' => 0,
8889
);
8990

90-
$args = \wp_parse_args( $args, $defaults );
91+
$args = \wp_parse_args( $args, $defaults );
92+
$args['redirection'] = 0;
9193

9294
if ( ! empty( $args['body'] ) && \is_array( $args['body'] ) ) {
9395
$args['body'] = \wp_json_encode( $args['body'] );
@@ -194,7 +196,7 @@ public static function request( string $method, string $endpoint, array $args =
194196
return self::request( $method, $endpoint, $original_args, null, true );
195197
}
196198

197-
if ( $status >= 400 ) {
199+
if ( ! is_success_status( $status ) ) {
198200
$msg = $body['message'] ?? ( $body['error'] ?? \__( 'PDS request failed.', 'atmosphere' ) );
199201
return new \WP_Error( 'atmosphere_pds', $msg, array( 'status' => $status ) );
200202
}

‎includes/functions.php‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -650,3 +650,23 @@ function debug_log( string $message ): void {
650650
// phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_error_log
651651
\error_log( '[atmosphere] ' . $message );
652652
}
653+
654+
/**
655+
* Whether an HTTP status code is in the Success (2xx) class.
656+
*
657+
* "Success" is the IANA registry name for the 2xx range
658+
* ({@link https://www.iana.org/assignments/http-status-codes/http-status-codes.xhtml}).
659+
* AT Protocol OAuth and PDS requests disable redirects, so any non-2xx
660+
* status (including a 3xx the server would have redirected) is treated
661+
* as a failure. Centralizes that check for the OAuth and API callers.
662+
*
663+
* @since unreleased
664+
*
665+
* @param mixed $status HTTP status code (int, or '' when the request failed).
666+
* @return bool True for 200-299, false otherwise.
667+
*/
668+
function is_success_status( $status ): bool {
669+
$status = (int) $status;
670+
671+
return $status >= 200 && $status < 300;
672+
}

‎includes/oauth/class-client.php‎

Lines changed: 40 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
use function Atmosphere\clear_scheduled_hooks;
1717
use function Atmosphere\debug_log;
1818
use function Atmosphere\get_connection;
19+
use function Atmosphere\is_success_status;
1920

2021
/**
2122
* OAuth client that manages the authorization lifecycle.
@@ -275,12 +276,13 @@ private static function authorize_via_par(
275276
$response = \wp_safe_remote_post(
276277
$par_url,
277278
array(
278-
'headers' => array(
279+
'headers' => array(
279280
'Content-Type' => 'application/x-www-form-urlencoded',
280281
'DPoP' => $dpop_proof,
281282
),
282-
'body' => $body,
283-
'timeout' => 15,
283+
'body' => $body,
284+
'timeout' => 15,
285+
'redirection' => 0,
284286
)
285287
);
286288

@@ -314,12 +316,13 @@ private static function authorize_via_par(
314316
$response = \wp_safe_remote_post(
315317
$par_url,
316318
array(
317-
'headers' => array(
319+
'headers' => array(
318320
'Content-Type' => 'application/x-www-form-urlencoded',
319321
'DPoP' => $dpop_proof,
320322
),
321-
'body' => $body,
322-
'timeout' => 15,
323+
'body' => $body,
324+
'timeout' => 15,
325+
'redirection' => 0,
323326
)
324327
);
325328

@@ -339,9 +342,9 @@ private static function authorize_via_par(
339342
}
340343
}
341344

342-
if ( $status >= 400 || empty( $data['request_uri'] ) ) {
345+
if ( ! is_success_status( $status ) || empty( $data['request_uri'] ) ) {
343346
$msg = $data['error_description'] ?? ( $data['error'] ?? \__( 'PAR request failed.', 'atmosphere' ) );
344-
return new \WP_Error( 'atmosphere_par', $msg );
347+
return new \WP_Error( 'atmosphere_par', $msg, array( 'status' => $status ) );
345348
}
346349

347350
$params = array(
@@ -433,12 +436,13 @@ public static function handle_callback( string $code, string $state ): true|\WP_
433436
$response = \wp_safe_remote_post(
434437
$token_endpoint,
435438
array(
436-
'headers' => array(
439+
'headers' => array(
437440
'Content-Type' => 'application/x-www-form-urlencoded',
438441
'DPoP' => $dpop_proof,
439442
),
440-
'body' => $token_body,
441-
'timeout' => 15,
443+
'body' => $token_body,
444+
'timeout' => 15,
445+
'redirection' => 0,
442446
)
443447
);
444448

@@ -470,12 +474,13 @@ public static function handle_callback( string $code, string $state ): true|\WP_
470474
$response = \wp_safe_remote_post(
471475
$token_endpoint,
472476
array(
473-
'headers' => array(
477+
'headers' => array(
474478
'Content-Type' => 'application/x-www-form-urlencoded',
475479
'DPoP' => $dpop_proof,
476480
),
477-
'body' => $token_body,
478-
'timeout' => 15,
481+
'body' => $token_body,
482+
'timeout' => 15,
483+
'redirection' => 0,
479484
)
480485
);
481486

@@ -502,9 +507,9 @@ public static function handle_callback( string $code, string $state ): true|\WP_
502507
}
503508
}
504509

505-
if ( $status >= 400 || empty( $data['access_token'] ) ) {
510+
if ( ! is_success_status( $status ) || empty( $data['access_token'] ) ) {
506511
$msg = $data['error_description'] ?? ( $data['error'] ?? \__( 'Token exchange failed.', 'atmosphere' ) );
507-
return new \WP_Error( 'atmosphere_token', $msg );
512+
return new \WP_Error( 'atmosphere_token', $msg, array( 'status' => $status ) );
508513
}
509514

510515
/*
@@ -712,12 +717,13 @@ private static function refresh_locked( array $conn ): true|\WP_Error {
712717
$response = \wp_safe_remote_post(
713718
$token_endpoint,
714719
array(
715-
'headers' => array(
720+
'headers' => array(
716721
'Content-Type' => 'application/x-www-form-urlencoded',
717722
'DPoP' => $dpop_proof,
718723
),
719-
'body' => $body,
720-
'timeout' => 15,
724+
'body' => $body,
725+
'timeout' => 15,
726+
'redirection' => 0,
721727
)
722728
);
723729

@@ -749,12 +755,13 @@ private static function refresh_locked( array $conn ): true|\WP_Error {
749755
$response = \wp_safe_remote_post(
750756
$token_endpoint,
751757
array(
752-
'headers' => array(
758+
'headers' => array(
753759
'Content-Type' => 'application/x-www-form-urlencoded',
754760
'DPoP' => $dpop_proof,
755761
),
756-
'body' => $body,
757-
'timeout' => 15,
762+
'body' => $body,
763+
'timeout' => 15,
764+
'redirection' => 0,
758765
)
759766
);
760767

@@ -781,7 +788,7 @@ private static function refresh_locked( array $conn ): true|\WP_Error {
781788
}
782789
}
783790

784-
if ( $status >= 400 || empty( $data['access_token'] ) ) {
791+
if ( ! is_success_status( $status ) || empty( $data['access_token'] ) ) {
785792
$msg = $data['error_description'] ?? ( $data['error'] ?? \__( 'Token refresh failed.', 'atmosphere' ) );
786793

787794
/*
@@ -829,7 +836,7 @@ private static function refresh_locked( array $conn ): true|\WP_Error {
829836
}
830837
}
831838

832-
return new \WP_Error( 'atmosphere_refresh', $msg );
839+
return new \WP_Error( 'atmosphere_refresh', $msg, array( 'status' => $status ) );
833840
}
834841

835842
/*
@@ -1371,12 +1378,13 @@ public static function revoke_refresh_token(
13711378
$response = \wp_safe_remote_post(
13721379
$revocation_endpoint,
13731380
array(
1374-
'headers' => array(
1381+
'headers' => array(
13751382
'Content-Type' => 'application/x-www-form-urlencoded',
13761383
'DPoP' => $dpop_proof,
13771384
),
1378-
'body' => $body,
1379-
'timeout' => 10,
1385+
'body' => $body,
1386+
'timeout' => 10,
1387+
'redirection' => 0,
13801388
)
13811389
);
13821390

@@ -1413,12 +1421,13 @@ public static function revoke_refresh_token(
14131421
$response = \wp_safe_remote_post(
14141422
$revocation_endpoint,
14151423
array(
1416-
'headers' => array(
1424+
'headers' => array(
14171425
'Content-Type' => 'application/x-www-form-urlencoded',
14181426
'DPoP' => $dpop_proof,
14191427
),
1420-
'body' => $body,
1421-
'timeout' => 10,
1428+
'body' => $body,
1429+
'timeout' => 10,
1430+
'redirection' => 0,
14221431
)
14231432
);
14241433

@@ -1446,7 +1455,7 @@ public static function revoke_refresh_token(
14461455
* indicates a misconfigured client or a server outage; either
14471456
* way disconnect proceeds.
14481457
*/
1449-
if ( $status >= 400 ) {
1458+
if ( ! is_success_status( $status ) ) {
14501459
debug_log(
14511460
\sprintf(
14521461
'refresh-token revocation returned status %d',

‎tests/phpunit/tests/class-test-api.php‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,56 @@ private function http_response( int $status, array $body ): array {
131131
);
132132
}
133133

134+
/**
135+
* PDS API requests must not follow redirects.
136+
*/
137+
public function test_request_disables_redirection() {
138+
$captured_args = null;
139+
140+
\add_filter(
141+
'pre_http_request',
142+
static function ( $response, $args, $url ) use ( &$captured_args ) {
143+
if ( false !== \strpos( $url, '/xrpc/' ) ) {
144+
$captured_args = $args;
145+
return array(
146+
'response' => array( 'code' => 200 ),
147+
'headers' => new \WpOrg\Requests\Utility\CaseInsensitiveDictionary( array() ),
148+
'body' => (string) \wp_json_encode( array( 'ok' => true ) ),
149+
);
150+
}
151+
152+
return $response;
153+
},
154+
10,
155+
3
156+
);
157+
158+
$result = API::get( '/xrpc/com.atproto.repo.getRecord' );
159+
160+
$this->assertIsArray( $result );
161+
$this->assertSame( 0, $captured_args['redirection'] ?? null );
162+
}
163+
164+
/**
165+
* Redirect responses are surfaced as failed PDS requests.
166+
*/
167+
public function test_request_rejects_redirect_response() {
168+
$this->stub_http(
169+
function () {
170+
$this->fail( 'Token endpoint should not be called for a PDS redirect response.' );
171+
},
172+
function () {
173+
return $this->http_response( 307, array() );
174+
}
175+
);
176+
177+
$result = API::get( '/xrpc/com.atproto.repo.getRecord' );
178+
179+
$this->assertWPError( $result );
180+
$this->assertSame( 'atmosphere_pds', $result->get_error_code() );
181+
$this->assertSame( 307, $result->get_error_data()['status'] ?? null );
182+
}
183+
134184
/**
135185
* When the PDS rejects the access token with a 401 `InvalidToken`,
136186
* `API::request()` must call `Client::refresh()` exactly once and

0 commit comments

Comments
 (0)