fix: harden JWT signature verification and URL path encoding

[gjtorikian]

Summary

• Verify access-token JWS signatures against the JWKS in SessionManager::decodeAccessToken, with an RS256 allow-list that rejects alg: none, missing/unknown kid, tampered signatures, and tokens lacking a matching JWK. Expiration is now checked only after signature verification.
• Issuer/audience enforcement is intentionally deferred (TODO) until the documented WorkOS values are confirmed; mirrors current Ruby/Python SDK behavior.
• Centralize RFC 3986 path-segment encoding in HttpClient::resolveUrl so an untrusted ID interpolated into a path template (e.g. om_xyz?/foo) cannot escape its segment into a new path or query. Pre-encoded triplets from existing rawurlencode callers are preserved (no double-encoding).
• Stabilize the tampered-signature test by flipping a middle byte of the JWS signature instead of the last base64url char, which could canonicalise back to the original bytes.

Test plan

• composer test passes (HttpClient + SessionManager suites)
• Confirm existing services that already rawurlencode IDs are not double-encoded
• Manually verify a tampered access token is rejected end-to-end
• Confirm tokens with alg: none or unknown kid are rejected

[greptile-apps]

Greptile Summary

This PR hardens JWT access-token verification in SessionManager (adds RS256 signature check against JWKS, algorithm allow-list, kid matching, and a 300 s in-process cache) and centralizes RFC 3986 path-segment encoding in HttpClient::resolveUrl to prevent untrusted IDs from injecting query parameters or extra path segments.

• SessionManager: decodeAccessToken is rewritten to perform full JWS signature verification via a hand-rolled DER/PEM builder, rejecting alg:none, missing/unknown kid, tampered signatures, and expired tokens; a getCachedJwks layer with TTL + forced-refresh-on-miss replaces the previous per-call fetch.
• HttpClient: encodePathSegments / encodePathSegment encode unsafe characters in each path segment while preserving already-encoded %XX triplets, so callers that already call rawurlencode are not double-encoded.
• Tests: Six new SessionManagerTest cases cover the happy path, tampered signature, alg:none, unknown kid, cache hit, and key-rotation refresh; one new HttpClientTest case validates ?-encoding and no double-encoding.

Confidence Score: 3/5

The JWT verification core is sound but the path-encoding comment misstates what is actually protected, and the unknown-kid test passes due to a mock-queue exception rather than the intended guard code.

The encodePathSegments docblock explicitly claims a raw / in an unencoded ID gets percent-encoded inside a single segment — this is false, explode('/', $path) runs before encoding so / still becomes a real path separator. Additionally, testAuthenticateRejectsUnknownKid never reaches the 'No JWKS key matches JWT kid' code path because the empty MockHandler queue throws first. The security comment in HttpClient.php is the higher concern because it describes protection that does not exist.

lib/HttpClient.php (misleading security comment about / encoding) and tests/SessionManagerTest.php (unknown-kid test coverage gap).

Important Files Changed

Filename	Overview
lib/HttpClient.php	Adds centralized RFC 3986 path-segment encoding in resolveUrl; the implementation correctly handles ?/# and preserves pre-encoded triplets, but the docblock falsely claims / inside unencoded IDs is also protected.
lib/SessionManager.php	Adds RS256 JWS signature verification via manual DER/PEM construction, an algorithm allow-list (rejects alg:none), kid matching, and a 300 s in-process JWKS cache with forced refresh on kid miss; logic is sound.
tests/HttpClientTest.php	New testResolveUrlPreservesEncodedIdAsSingleSegment test validates ? encoding and no double-encoding; does not cover the / path-separator behaviour that the comment claims to protect against.
tests/SessionManagerTest.php	Adds six new JWT-verification tests covering valid signature, tampered signature, alg:none, unknown kid, JWKS caching, and key-rotation refresh; testAuthenticateRejectsUnknownKid passes for the wrong reason (empty mock queue exception rather than the intended kid-miss guard).

_{Reviews (3): Last reviewed commit: "perf(session): cache JWKS per client wit..." | Re-trigger Greptile}

[greptile-apps]

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)

[gjtorikian]

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.

[greptile-apps]

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.

[greptile-apps]

testAuthenticateRejectsUnknownKid exercises the wrong failure path

Only one JWKS response is queued in the mock. The initial getCachedJwks call consumes it; when the kid miss triggers a forced refresh (getCachedJwks($clientId, forceRefresh: true)), the MockHandler queue is empty and throws OutOfBoundsException. That exception propagates through fetchJwks and decodeAccessToken and is caught by authenticate()'s catch (\Exception $e) block, which returns invalid_jwt — so the assertion passes. However, the actual code path that throws new \InvalidArgumentException('No JWKS key matches JWT kid') is never reached. If a future refactor removes or breaks that guard, this test will still pass because the mock queue exhaustion covers for it. To test the intended behaviour, a second response (also advertising only kid_other) should be queued so the forced refresh succeeds but still cannot find kid_signed_with.