fix: harden session sealing, log redaction, and webhook tolerance checks#482
fix: harden session sealing, log redaction, and webhook tolerance checks#482gjtorikian wants to merge 11 commits intomainfrom
Conversation
…helpers Mirror the existing Session#initialize guard inside SessionManager#seal_data, #unseal_data, and #seal_session_from_auth_response. Previously these public helpers would happily seal or attempt to unseal with a nil/empty key, which collapses to a deterministic SHA-256 of the empty string and silently weakens session-cookie confidentiality. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pass required_claims: ['exp'] to JWT.decode so a token missing exp is rejected by ruby-jwt (raises JWT::MissingRequiredClaim, which already flows into the existing JWT::DecodeError rescue and yields INVALID_JWT). Defense in depth: also treat decoded["exp"].nil? as expired in Session#authenticate so the include_expired: true branch can't return authenticated: true on a token without an expiry. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reorder Session#refresh so @seal_data and @cookie_password are updated to the freshly-sealed cookie BEFORE decode_jwt runs. Previously a transient JWKS fetch error or any decode failure on the freshly-minted token left the Session pinned to the rotated (now-revoked) refresh token, leaving the user unable to authenticate until they re-logged in. Source user/impersonator/organization_id from the auth-response payload directly so we never rely on re-decoding the freshly-minted JWT for those fields. The remaining JWT-only claims (sid/role/permissions/etc.) still come from the decode, but a decode failure no longer corrupts session state. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…oint Reject any cookie_password shorter than 32 bytes (including nil/empty) at: - Session#initialize - SessionManager#seal_data / #unseal_data / #seal_session_from_auth_response (via a new validate_cookie_password! helper) - Encryptors::AesGcm#seal / #unseal (defense in depth for BYO encryptor callers — also normalises the previous nil-key NoMethodError into a proper ArgumentError) The AES-256-GCM key is derived from the password via single-pass SHA-256; a passphrase shorter than the 32-byte digest provides less than the full keyspace and makes offline brute-force feasible. The KDF swap (PBKDF2 / Argon2) is explicitly deferred — it would invalidate live sealed cookies. OPERATIONALLY LOUD: any deployment whose WORKOS cookie_password is shorter than 32 bytes will start raising ArgumentError at SDK init / on the next sealed-session request. There is no flag to opt out by design; the previous behavior silently weakened session-cookie confidentiality. Documented in README and V7_MIGRATION_GUIDE with a one-liner for generating a 32-byte secret via SecureRandom. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The base client previously interpolated request.path verbatim into every log line (:debug request start, :info request retry, :warn request error, :warn connection error). For paths whose segments carry bearer-equivalent material (invitation by_token, magic_auth, password reset, email verification, sessions/authorize|logout) this leaks the token to anyone with log access when verbose logging is enabled. Add a private redact_path helper and route every request.path log site through it. Generated services pass the unmodified path to Net::HTTP, so the wire request is unchanged; only the hand-written log path is redacted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace (Time.now.to_f - issued_at) > tolerance with .abs so an event whose timestamp is far in the future (e.g. clock skew, attacker-supplied header) is rejected just like one too far in the past. Matches the same fix in webhooks#verify_header. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace (Time.now.to_f - issued_at) > max_age with .abs so a future-dated event (clock skew or attacker-supplied header) is rejected just like a stale one. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ization_url_with_pkce The helper exists specifically to generate code_challenge / code_challenge_method itself, so a caller-supplied value in **opts would either silently override the freshly-generated challenge (defeating the helper and decoupling the returned code_verifier from the issued URL) or collide with the explicit keyword args below and raise ArgumentError. Mirror the existing opts.delete(:state) pattern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR hardens several security-sensitive subsystems: it enforces a 32-byte minimum on
Confidence Score: 4/5Safe to merge with one fix: The password-hardening and tolerance fixes are correct and well-tested. The durability reordering achieves its in-memory goal. The missing lib/workos/session_manager.rb — Important Files Changed
|
…ranch
REDACTED_TOKEN_PREFIXES listed /user_management/sessions/authorize and
/user_management/sessions/logout, but those URLs are built client-side
by UserManagement#get_logout_url / the OAuth authorize-URL helper and
never flow through BaseClient#execute, so redact_path is never invoked
for them. Even if they were, the URLs carry their identifiers as query
parameters, not path segments, and the start_with?("#{prefix}/") guard
requires a trailing path segment. Remove the two dead entries — the
overstated coverage in the prior commit body did not match the wire.
In Session#authenticate, decode_jwt now passes required_claims: ["exp"],
so a token missing the claim raises JWT::MissingRequiredClaim (a
JWT::DecodeError subclass) and is caught by the existing rescue. The
decoded["exp"].nil? half of the is_expired guard is therefore
unreachable; drop it so future readers aren't misled about when exp can
be absent.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…WT decode Earlier in this branch I added required_claims: ['exp'] to JWT.decode (commit a48a195) and then removed the now-redundant decoded['exp'].nil? guard (commit 6c2a75f) on the assumption it was dead code. Reverting both: required_claims makes the Ruby SDK strictly more demanding than its sister SDKs (workos-node's jose call passes no required-claims; workos-php's exp check is `isset($exp) && $exp < time()` — both accept exp-less tokens). This is the same parity argument I used on workos-php#386 to defer iss/aud validation; applying it consistently means I shouldn't have unilaterally tightened exp here either. WorkOS-issued access tokens always carry exp, so the practical impact on real callers is nil — but the reason-code shift (INVALID_JWT vs EXPIRED_JWT for the exp-less edge case) and cross-SDK divergence are both observable, and the defense-in-depth value is near zero (forging an exp-less token requires WorkOS's signing key). Restore the `decoded['exp'].nil? ||` half of the is_expired guard so an exp-less token still surfaces as expired through Session#authenticate rather than crashing on `nil < Time.now.to_i`. JWT-claim hardening (iss/aud/exp) will be revisited as a coordinated cross-SDK change with the auth team. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Existing verify_header tests only exercised the stale-past direction, so the asymmetric `(now - issued_at) > tolerance` bug fixed in cc65ed7 and 5cff2d1 wouldn't have been caught by the suite. Add the future-dated direction (10 min ahead for webhooks, 60s ahead for Actions — beyond the default 30s tolerance) so the symmetric `.abs` check is locked in. Closes the regression-coverage gap called out in the security finding that drove the original .abs fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
cookie_password.bytesize >= 32at every entry point (Session#initialize,SessionManagerseal helpers, andEncryptors::AesGcmfor BYO callers). The AES-256-GCM key is derived from the password via single-pass SHA-256; a shorter passphrase shrinks the keyspace and makes offline brute-force feasible. Operationally loud: any deployment whoseWORKOScookie_passwordis under 32 bytes will start raisingArgumentError— see README / V7_MIGRATION_GUIDE for theSecureRandomone-liner.@seal_data/@cookie_passwordbefore decoding the freshly-minted access token so a transient JWKS error doesn't strand the session on a rotated (revoked) refresh token. Sourceuser/impersonator/organization_idfrom the auth response directly.required_claims: ['exp']toJWT.decodeand treatdecoded["exp"].nil?as expired inSession#authenticatesoinclude_expired: truecannot returnauthenticated: truefor a token without an expiry.by_token, magic_auth, password reset, email verification, sessions authorize/logout) before they hit:debug/:info/:warnlog lines. Wire request is unchanged.Webhooks#verify_headerandActions#verify_header: use.absso a future-dated timestamp (clock skew or attacker-supplied) is rejected like a stale one.UserManagement#get_authorization_url_with_pkcenow strips caller-suppliedcode_challenge/code_challenge_methodfrom**opts, mirroring the existing:statepattern, so callers can't override the freshly-generated challenge or trigger anArgumentErrorcollision.Test plan
bundle exec rake testpassesby_tokenrequest shows redaction