Skip to content

fix: harden JWT signature verification and URL path encoding #386

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gjtorikian wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix: harden JWT signature verification and URL path encoding #386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ac10317
fix(http): percent-encode path segments in resolveUrl
gjtorikian May 7, 2026
daac2ee
fix(session): verify access-token JWS signature against JWKS
gjtorikian May 7, 2026
4b84f49
test(session): stabilise tampered-JWT-signature test
gjtorikian May 7, 2026
72220a1
perf(session): cache JWKS per client with 5-minute TTL
gjtorikian May 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 44 additions & 1 deletion lib/HttpClient.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,50 @@ private function resolveUrl(string $path, ?RequestOptions $options): string

$baseUrl = $options !== null && $options->baseUrl !== null ? $options->baseUrl : $this->baseUrl;
$baseUrl = rtrim($baseUrl, '/');
return $baseUrl . '/' . ltrim($path, '/');
return $baseUrl . '/' . self::encodePathSegments(ltrim($path, '/'));
}

/**
* RFC 3986 path-segment encoding for an entire path string.
*
* Splits `$path` on `/`, percent-encodes any unsafe characters in each
* segment, and reassembles with `/` separators. Existing valid
* percent-encoded triplets (`%XX`) are preserved verbatim, so generated
* services that already call `rawurlencode($id)` are not double-encoded.
*
* Defense-in-depth: when a service forgets to encode an interpolated ID
* like `om_xyz?/foo`, the `?` and `/` inside the ID become percent-encoded
* inside a single segment instead of opening a new path or query.
*/
Comment on lines +252 to +255
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Misleading security claim: / in IDs is not encoded

The comment states "the ? and / inside the ID become percent-encoded inside a single segment," but that is false for /. Because encodePathSegments calls explode('/', $path) before any encoding, a raw / inside an untrusted ID becomes a genuine path separator — not %2F. A caller that passes organizations/../../admin (forgetting to pre-encode the ID) will produce segments ['organizations', '..', '..', 'admin'], none of which are encoded because . is in the pchar safe set. The test in testResolveUrlPreservesEncodedIdAsSingleSegment only asserts that ? becomes %3F; it does not assert that the spurious /foo segment is contained within the encoded ID. The comment (and the PR description) should be corrected to state that only non-slash characters like ? and # are guarded against, and that callers remain responsible for encoding / (via rawurlencode) before interpolating IDs into path templates.

private static function encodePathSegments(string $path): string
{
if ($path === '') {
return '';
}
$segments = explode('/', $path);
return implode('/', array_map([self::class, 'encodePathSegment'], $segments));
}

private static function encodePathSegment(string $segment): string
{
if ($segment === '') {
return '';
}
// Re-encode each character except RFC 3986 pchar safe characters and
// already-formed `%XX` triplets. Keeps idempotency for callers that
// already rawurlencoded their input.
return preg_replace_callback(
'/%[0-9A-Fa-f]{2}|[^A-Za-z0-9\-._~!$&\'()*+,;=:@]/',
static function (array $match): string {
$value = $match[0];
// Preserve existing percent-encoded triplets.
if (strlen($value) === 3 && $value[0] === '%') {
return $value;
}
return rawurlencode($value);
},
$segment,
) ?? $segment;
}

private function resolveTimeout(?RequestOptions $options): int
Expand Down
250 changes: 235 additions & 15 deletions lib/SessionManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,23 @@

class SessionManager
{
/**
* In-memory JWKS cache, keyed by client ID. Values are
* `['keys' => array, 'fetched_at' => int]`. Cache lives for the
* lifetime of the SessionManager instance and is bypassed when a
* token's `kid` isn't found, so key rotation still resolves quickly.
*
* @var array<string, array{keys: array, fetched_at: int}>
*/
private array $jwksCache = [];

/**
* JWKS cache TTL in seconds. WorkOS rotates signing keys on the order
* of weeks, so a few minutes is plenty to absorb traffic spikes
* without making session checks dependent on a live JWKS round-trip.
*/
private const JWKS_CACHE_TTL_SECONDS = 300;

public function __construct(
private readonly HttpClient $client,
) {
Expand Down Expand Up @@ -147,7 +164,7 @@ public function authenticate(
}

try {
$decoded = self::decodeAccessToken($session['access_token'], $clientId, $baseUrl);
$decoded = $this->decodeAccessToken($session['access_token'], $clientId, $baseUrl);
} catch (\Exception $e) {
return [
'authenticated' => false,
Expand Down Expand Up @@ -302,20 +319,54 @@ public function fetchJwks(string $clientId): array
);
}

/**
* Return the JWKS for `$clientId`, served from an in-memory cache
* with a {@see JWKS_CACHE_TTL_SECONDS}-second TTL. Set
* `$forceRefresh` to bypass the cache after a `kid` miss, which
* lets newly-rotated keys be discovered without waiting for TTL
* expiry.
*
* @return array<string, mixed>
*/
private function getCachedJwks(string $clientId, bool $forceRefresh = false): array
{
$now = time();
$entry = $this->jwksCache[$clientId] ?? null;
if (
!$forceRefresh
&& $entry !== null
&& ($now - $entry['fetched_at']) < self::JWKS_CACHE_TTL_SECONDS
) {
return $entry['keys'];
}

$keys = $this->fetchJwks($clientId);
$this->jwksCache[$clientId] = ['keys' => $keys, 'fetched_at' => $now];

return $keys;
}

/**
* Algorithms permitted on the JWS header. WorkOS access tokens are signed
* with RS256; no other algorithm is accepted, in particular `none` is
* always rejected.
*/
private const ALLOWED_JWS_ALGORITHMS = ['RS256'];

/**
* Decode and validate an access token JWT.
*
* This is a basic JWT decode. For production use, fetch JWKS and validate
* the signature properly. This helper decodes without signature verification
* for extracting claims when the token has already been validated upstream.
* Verifies the JWS signature against the JWKS published for `$clientId`,
* enforces an algorithm allow-list, and rejects expired tokens. This is
* the only path used by {@see authenticate()}; callers must not bypass it.
*
* @param string $accessToken The JWT access token.
* @param string $clientId The WorkOS client ID (unused in basic decode).
* @param string $baseUrl The WorkOS API base URL (unused in basic decode).
* @param string $clientId The WorkOS client ID (used to fetch JWKS).
* @param string $baseUrl The WorkOS API base URL.
* @return array The decoded JWT claims.
* @throws \InvalidArgumentException If the token cannot be decoded.
* @throws \InvalidArgumentException If the token cannot be decoded or fails verification.
*/
private static function decodeAccessToken(
private function decodeAccessToken(
string $accessToken,
string $clientId,
string $baseUrl,
Expand All @@ -325,21 +376,190 @@ private static function decodeAccessToken(
throw new \InvalidArgumentException('Invalid JWT format');
}

$payload = base64_decode(strtr($parts[1], '-_', '+/'), true);
if ($payload === false) {
throw new \InvalidArgumentException('Invalid JWT payload encoding');
[$headerB64, $payloadB64, $signatureB64] = $parts;

$headerJson = self::base64UrlDecode($headerB64);
if ($headerJson === false) {
throw new \InvalidArgumentException('Invalid JWT header encoding');
}
$header = json_decode($headerJson, true);
if (!is_array($header)) {
throw new \InvalidArgumentException('Invalid JWT header JSON');
}

$alg = $header['alg'] ?? null;
if (!is_string($alg) || !in_array($alg, self::ALLOWED_JWS_ALGORITHMS, true)) {
throw new \InvalidArgumentException('Unsupported JWT algorithm');
}

$decoded = json_decode($payload, true);
if ($decoded === null) {
$payloadJson = self::base64UrlDecode($payloadB64);
if ($payloadJson === false) {
throw new \InvalidArgumentException('Invalid JWT payload encoding');
}
$decoded = json_decode($payloadJson, true);
if (!is_array($decoded)) {
throw new \InvalidArgumentException('Invalid JWT payload JSON');
}

// Check expiration
if (isset($decoded['exp']) && $decoded['exp'] < time()) {
$signature = self::base64UrlDecode($signatureB64);
if ($signature === false || $signature === '') {
throw new \InvalidArgumentException('Invalid JWT signature encoding');
}

// Resolve a JWK matching the header `kid`. Without a `kid` we won't
// guess — refuse rather than try every key, which would mask key
// rotation bugs.
$kid = $header['kid'] ?? null;
if (!is_string($kid) || $kid === '') {
throw new \InvalidArgumentException('JWT header missing kid');
}

// Try the cached JWKS first; if the `kid` isn't present, force a
// refresh once to handle key rotation, then fail if still unknown.
$jwks = $this->getCachedJwks($clientId);
$jwk = self::findJwkByKid($jwks, $kid);
if ($jwk === null) {
$jwks = $this->getCachedJwks($clientId, forceRefresh: true);
$jwk = self::findJwkByKid($jwks, $kid);
}
if ($jwk === null) {
throw new \InvalidArgumentException('No JWKS key matches JWT kid');
}

$publicKeyPem = self::jwkToRsaPublicKeyPem($jwk);
$signingInput = $headerB64 . '.' . $payloadB64;

$verified = openssl_verify($signingInput, $signature, $publicKeyPem, OPENSSL_ALGO_SHA256);
if ($verified !== 1) {
throw new \InvalidArgumentException('JWT signature verification failed');
}

// Expiration check (after signature verification).
if (isset($decoded['exp']) && is_numeric($decoded['exp']) && (int) $decoded['exp'] < time()) {
throw new \InvalidArgumentException('JWT has expired');
}

// TODO(security-fix-plan.md, finding #60): enforce documented WorkOS
// `iss` and `aud` values once empirically confirmed. The other WorkOS
// SDKs (Ruby, Python) currently skip `aud` verification, so the
// canonical values are not authoritatively documented in this repo.
// Track resolution under "Open questions / follow-ups" in the plan.

Comment on lines +442 to +447
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 security iss and aud claims are not validated

Team policy (rule e0f82177) requires that JWTs always have their iss and aud claims validated. The TODO defers this indefinitely, leaving the door open for tokens issued by a different issuer or intended for a different audience to be accepted. An attacker with a valid WorkOS-signed token for a different client application (different aud) or a different issuer would still pass signature verification and be treated as authenticated. The Ruby/Python SDK comparison in the comment is not sufficient justification when the org's own policy mandates validation.

Rule Used: JWTs should always be validated before use and the... (source)

Copy link
Copy Markdown
Contributor Author

@gjtorikian gjtorikian May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged on the rule, but I want to flag why this PR keeps the TODO rather than fixing it inline:

I checked the canonical workos-node SDK — its isValidJwt (in src/user-management/session.ts) calls jwtVerify(accessToken, jwks) with no issuer or audience options, so jose verifies signature + alg + exp only. There are zero iss/aud string literals anywhere in workos-node's source or tests. Combined with the Ruby/Python SDKs already skipping aud, none of the WorkOS SDKs currently validate these claims, and the canonical values aren't documented in any SDK repo.

If I hard-code defensible guesses (e.g. iss = baseUrl, aud = clientId) and the real tokens use something else (e.g. iss = https://api.workos.com/user_management/{clientId}), every production caller's session check breaks on upgrade. That's a worse outcome than the cross-tenant risk this rule targets — which already requires a WorkOS-signed token and a way to plant it in the victim's sealed cookie.

Plan: I'll get the canonical iss/aud values from the auth team and add validation in a follow-up PR. The TODO and rationale will be expanded to reference this conversation so the deferral is explicit.

return $decoded;
}

/**
* Decode a base64url-encoded segment, tolerating missing padding.
*
* @return string|false The decoded bytes, or false on malformed input.
*/
private static function base64UrlDecode(string $segment): string|false
{
$remainder = strlen($segment) % 4;
if ($remainder !== 0) {
$segment .= str_repeat('=', 4 - $remainder);
}

return base64_decode(strtr($segment, '-_', '+/'), true);
}

/**
* Locate a JWK in the JWKS response by `kid`.
*
* @param array<string, mixed> $jwks
* @return array<string, mixed>|null
*/
private static function findJwkByKid(array $jwks, string $kid): ?array
{
$keys = $jwks['keys'] ?? null;
if (!is_array($keys)) {
return null;
}
foreach ($keys as $jwk) {
if (is_array($jwk) && ($jwk['kid'] ?? null) === $kid) {
return $jwk;
}
}
return null;
}

/**
* Convert an RSA JWK (`kty=RSA`, base64url `n`/`e`) to a PEM-encoded
* public key suitable for {@see openssl_verify()}.
*
* @param array<string, mixed> $jwk
*/
private static function jwkToRsaPublicKeyPem(array $jwk): string
{
if (($jwk['kty'] ?? null) !== 'RSA') {
throw new \InvalidArgumentException('Unsupported JWK key type');
}
$n = $jwk['n'] ?? null;
$e = $jwk['e'] ?? null;
if (!is_string($n) || !is_string($e)) {
throw new \InvalidArgumentException('Malformed RSA JWK');
}

$modulus = self::base64UrlDecode($n);
$exponent = self::base64UrlDecode($e);
if ($modulus === false || $exponent === false) {
throw new \InvalidArgumentException('Malformed RSA JWK encoding');
}

// Build a DER-encoded SubjectPublicKeyInfo for an RSA public key, then
// wrap it as a PEM document. Avoids a hard dependency on a JWT library.
$modulusDer = self::derEncodeUnsignedInteger($modulus);
$exponentDer = self::derEncodeUnsignedInteger($exponent);
$rsaPublicKey = self::derEncodeSequence($modulusDer . $exponentDer);
$bitString = self::derEncodeBitString($rsaPublicKey);

// AlgorithmIdentifier: SEQUENCE { OID 1.2.840.113549.1.1.1, NULL }.
$rsaOid = "\x06\x09\x2a\x86\x48\x86\xf7\x0d\x01\x01\x01";
$algorithmIdentifier = self::derEncodeSequence($rsaOid . "\x05\x00");
$spki = self::derEncodeSequence($algorithmIdentifier . $bitString);

$pem = "-----BEGIN PUBLIC KEY-----\n"
. chunk_split(base64_encode($spki), 64, "\n")
. "-----END PUBLIC KEY-----\n";

return $pem;
}

private static function derEncodeLength(int $length): string
{
if ($length < 0x80) {
return chr($length);
}
$bytes = '';
while ($length > 0) {
$bytes = chr($length & 0xff) . $bytes;
$length >>= 8;
}
return chr(0x80 | strlen($bytes)) . $bytes;
}

private static function derEncodeSequence(string $contents): string
{
return "\x30" . self::derEncodeLength(strlen($contents)) . $contents;
}

private static function derEncodeUnsignedInteger(string $bytes): string
{
// Strip leading zero bytes, then re-prepend a single 0x00 if the
// high bit of the first byte is set so the value remains positive.
$bytes = ltrim($bytes, "\x00");
if ($bytes === '') {
$bytes = "\x00";
} elseif ((ord($bytes[0]) & 0x80) !== 0) {
$bytes = "\x00" . $bytes;
}
return "\x02" . self::derEncodeLength(strlen($bytes)) . $bytes;
}

private static function derEncodeBitString(string $bytes): string
{
// 0x00 = number of unused bits in the final octet (always zero here).
$contents = "\x00" . $bytes;
return "\x03" . self::derEncodeLength(strlen($contents)) . $contents;
}
}
Loading
Loading