Skip to content

Commit ac10317

Browse files
committed
fix(http): percent-encode path segments in resolveUrl
Centralize RFC 3986 path-segment encoding in HttpClient::resolveUrl so an untrusted ID interpolated into a path template (e.g. om_xyz?/foo) stays inside a single segment instead of opening a new path or query. Existing services that already call rawurlencode are preserved (percent-encoded triplets are not double-encoded).
1 parent c5d2495 commit ac10317

2 files changed

Lines changed: 96 additions & 1 deletion

File tree

lib/HttpClient.php

Lines changed: 44 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -238,7 +238,50 @@ private function resolveUrl(string $path, ?RequestOptions $options): string
238238

239239
$baseUrl = $options !== null && $options->baseUrl !== null ? $options->baseUrl : $this->baseUrl;
240240
$baseUrl = rtrim($baseUrl, '/');
241-
return $baseUrl . '/' . ltrim($path, '/');
241+
return $baseUrl . '/' . self::encodePathSegments(ltrim($path, '/'));
242+
}
243+
244+
/**
245+
* RFC 3986 path-segment encoding for an entire path string.
246+
*
247+
* Splits `$path` on `/`, percent-encodes any unsafe characters in each
248+
* segment, and reassembles with `/` separators. Existing valid
249+
* percent-encoded triplets (`%XX`) are preserved verbatim, so generated
250+
* services that already call `rawurlencode($id)` are not double-encoded.
251+
*
252+
* Defense-in-depth: when a service forgets to encode an interpolated ID
253+
* like `om_xyz?/foo`, the `?` and `/` inside the ID become percent-encoded
254+
* inside a single segment instead of opening a new path or query.
255+
*/
256+
private static function encodePathSegments(string $path): string
257+
{
258+
if ($path === '') {
259+
return '';
260+
}
261+
$segments = explode('/', $path);
262+
return implode('/', array_map([self::class, 'encodePathSegment'], $segments));
263+
}
264+
265+
private static function encodePathSegment(string $segment): string
266+
{
267+
if ($segment === '') {
268+
return '';
269+
}
270+
// Re-encode each character except RFC 3986 pchar safe characters and
271+
// already-formed `%XX` triplets. Keeps idempotency for callers that
272+
// already rawurlencoded their input.
273+
return preg_replace_callback(
274+
'/%[0-9A-Fa-f]{2}|[^A-Za-z0-9\-._~!$&\'()*+,;=:@]/',
275+
static function (array $match): string {
276+
$value = $match[0];
277+
// Preserve existing percent-encoded triplets.
278+
if (strlen($value) === 3 && $value[0] === '%') {
279+
return $value;
280+
}
281+
return rawurlencode($value);
282+
},
283+
$segment,
284+
) ?? $segment;
242285
}
243286

244287
private function resolveTimeout(?RequestOptions $options): int

tests/HttpClientTest.php

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -252,6 +252,58 @@ public function testEmptyErrorBodySetsNullCodeAndError(): void
252252
}
253253
}
254254

255+
public function testResolveUrlPreservesEncodedIdAsSingleSegment(): void
256+
{
257+
// security-fix-plan.md finding #61: an untrusted ID like `om_xyz?/foo`
258+
// must remain a single percent-encoded path segment instead of
259+
// opening a new path/query when interpolated into a path template.
260+
// Generated services already call `rawurlencode($id)`; the centralized
261+
// fix must preserve that encoding (no double-encoding) and not re-split
262+
// the encoded slash.
263+
$mock = new MockHandler([
264+
new Response(200, ['Content-Type' => 'application/json'], '{}'),
265+
]);
266+
$history = [];
267+
$handler = HandlerStack::create($mock);
268+
$handler->push(\GuzzleHttp\Middleware::history($history));
269+
270+
$client = new HttpClient(
271+
apiKey: 'test_key',
272+
clientId: null,
273+
baseUrl: 'https://api.workos.com',
274+
timeout: 10,
275+
maxRetries: 0,
276+
handler: $handler,
277+
);
278+
279+
$id = 'om_xyz?/foo';
280+
$client->request('GET', 'organizations/' . rawurlencode($id));
281+
282+
$request = $history[array_key_last($history)]['request'];
283+
$uri = $request->getUri();
284+
285+
// No query string — `?` inside the ID stayed percent-encoded.
286+
$this->assertSame('', $uri->getQuery());
287+
// The whole ID (including its `/`) stayed inside a single segment.
288+
$this->assertSame('/organizations/om_xyz%3F%2Ffoo', $uri->getPath());
289+
290+
// Defense-in-depth: a path with the raw ID also has its `?` encoded
291+
// and does not produce a stray query string.
292+
$client2 = new HttpClient(
293+
apiKey: 'test_key',
294+
clientId: null,
295+
baseUrl: 'https://api.workos.com',
296+
timeout: 10,
297+
maxRetries: 0,
298+
handler: $handler,
299+
);
300+
$mock->append(new Response(200, ['Content-Type' => 'application/json'], '{}'));
301+
$client2->request('GET', 'organizations/om_xyz?/foo');
302+
$rawRequest = $history[array_key_last($history)]['request'];
303+
$this->assertSame('', $rawRequest->getUri()->getQuery());
304+
$this->assertStringContainsString('om_xyz%3F', $rawRequest->getUri()->getPath());
305+
}
306+
255307
public function testNonStringCodeFieldIsIgnored(): void
256308
{
257309
$body = json_encode([

0 commit comments

Comments
 (0)