Skip to content

fix: harden session sealing, log redaction, and webhook tolerance checks#482

Open
gjtorikian wants to merge 11 commits intomainfrom
updates
Open

fix: harden session sealing, log redaction, and webhook tolerance checks#482
gjtorikian wants to merge 11 commits intomainfrom
updates

Conversation

@gjtorikian
Copy link
Copy Markdown
Contributor

Summary

  • Session cookie_password hardening: require cookie_password.bytesize >= 32 at every entry point (Session#initialize, SessionManager seal helpers, and Encryptors::AesGcm for 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 whose WORKOS cookie_password is under 32 bytes will start raising ArgumentError — see README / V7_MIGRATION_GUIDE for the SecureRandom one-liner.
  • Session refresh durability: persist @seal_data / @cookie_password before decoding the freshly-minted access token so a transient JWKS error doesn't strand the session on a rotated (revoked) refresh token. Source user / impersonator / organization_id from the auth response directly.
  • JWT exp required: pass required_claims: ['exp'] to JWT.decode and treat decoded["exp"].nil? as expired in Session#authenticate so include_expired: true cannot return authenticated: true for a token without an expiry.
  • Base client log redaction: redact bearer-token path segments (invitation by_token, magic_auth, password reset, email verification, sessions authorize/logout) before they hit :debug / :info / :warn log lines. Wire request is unchanged.
  • Symmetric tolerance in Webhooks#verify_header and Actions#verify_header: use .abs so a future-dated timestamp (clock skew or attacker-supplied) is rejected like a stale one.
  • PKCE helper hygiene: UserManagement#get_authorization_url_with_pkce now strips caller-supplied code_challenge / code_challenge_method from **opts, mirroring the existing :state pattern, so callers can't override the freshly-generated challenge or trigger an ArgumentError collision.

Test plan

  • bundle exec rake test passes
  • Existing session/cookie_password tests still pass; new tests cover the < 32-byte rejection paths
  • Webhook + Actions verify_header reject future-dated timestamps
  • Spot-check: log output for an invitation by_token request shows redaction
  • Manual: a session refresh whose JWKS fetch fails leaves the previously-good session unchanged

gjtorikian and others added 8 commits May 7, 2026 14:35
…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>
@gjtorikian gjtorikian requested review from a team as code owners May 8, 2026 18:27
@gjtorikian gjtorikian requested a review from faroceann May 8, 2026 18:27
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 8, 2026

Greptile Summary

This PR hardens several security-sensitive subsystems: it enforces a 32-byte minimum on cookie_password at every entry point, flips the session-durability order so @seal_data is persisted before JWT decode, adds symmetric timestamp rejection for webhook/action tolerance checks, redacts token-bearing path segments from log output, and strips caller-supplied PKCE params from get_authorization_url_with_pkce.

  • Cookie-password hardening: Session#initialize, SessionManager#validate_cookie_password!, and Encryptors::AesGcm#validate_key! all now raise ArgumentError for keys under 32 bytes; documented in README and V7_MIGRATION_GUIDE.
  • Refresh durability: @seal_data / @cookie_password are written before decode_jwt is called so a transient JWKS failure no longer strands the session — though RefreshError still doesn't expose sealed_session, limiting how much a typical caller can benefit.
  • Log redaction: BaseClient#redact_path replaces token-bearing path segments with [REDACTED]; sessions/logout and sessions/authorize tokens remain unredacted because they appear as query parameters.

Confidence Score: 4/5

Safe to merge with one fix: decode_jwt should add required_claims: ['exp'] to match the stated intent and close the gap in the refresh path.

The password-hardening and tolerance fixes are correct and well-tested. The durability reordering achieves its in-memory goal. The missing required_claims: ['exp'] in decode_jwt means Session#refresh can return RefreshSuccess(authenticated: true) for a token that immediately appears expired on the next authenticate call, an inconsistency between the two code paths.

lib/workos/session_manager.rb — decode_jwt is missing required_claims: ['exp'], creating an inconsistency between the authenticate and refresh code paths.

Important Files Changed

Filename Overview
lib/workos/session.rb Adds cookie_password minimum-length guard, fixes is_expired nil check, and reorders @seal_data/@cookie_password assignment to persist state before JWT decode.
lib/workos/session_manager.rb Adds validate_cookie_password! helper wired into seal/unseal entry points. decode_jwt still omits required_claims: ['exp'], iss validation, and aud validation.
lib/workos/base_client.rb Introduces redact_path for token-bearing path segments. Sessions logout/authorize tokens in query params remain unredacted.
lib/workos/encryptors/aes_gcm.rb Adds minimum key-length validation and refactors v7/legacy fallback into nested begin/rescue. Changes are correct and safe.
lib/workos/webhooks.rb Adds .abs for symmetric tolerance check. Low-risk one-line fix.
lib/workos/actions.rb Same symmetric tolerance fix as webhooks.rb.
lib/workos/user_management.rb Strips caller-supplied code_challenge/code_challenge_method from opts in get_authorization_url_with_pkce.

Comments Outside Diff (1)

  1. lib/workos/session_manager.rb, line 175-186 (link)

    P1 required_claims: ['exp'] described in PR but not implemented

    The PR description states "pass required_claims: ['exp'] to JWT.decode", yet decode_jwt does not include that option. Without it, a server-issued token that omits the exp claim is decoded successfully by the JWT gem (with verify_expiration: true enabled, the gem simply skips the check when the claim is absent rather than raising). The decoded["exp"].nil? guard added to Session#authenticate catches this on the authentication path, but Session#refresh calls decode_jwt at line 123 with no subsequent nil check — a token without exp would produce RefreshSuccess(authenticated: true) even though the follow-on authenticate call would immediately report authenticated: false (expired). Adding required_claims: ['exp'] to decode_jwt would enforce the invariant on both code paths and make the intent explicit.

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

Reviews (4): Last reviewed commit: "test(webhooks,actions): cover future-dat..." | Re-trigger Greptile

Comment thread lib/workos/base_client.rb
Comment thread lib/workos/session.rb
gjtorikian and others added 3 commits May 8, 2026 12:24
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant