Skip to content

fix: prevent partial state mutation in Session#refresh#471

Merged
gjtorikian merged 1 commit intoseven-sealsfrom
seven-seals-hardening
Apr 29, 2026
Merged

fix: prevent partial state mutation in Session#refresh#471
gjtorikian merged 1 commit intoseven-sealsfrom
seven-seals-hardening

Conversation

@gjtorikian
Copy link
Copy Markdown
Contributor

Summary

Follow-up to #470, addressing Greptile's P2 finding about state mutation ordering.

  • Moves decode_jwt call before @seal_data/@cookie_password assignment, so a malformed access_token from the API doesn't leave the Session object half-updated.
  • Adds rescue JWT::DecodeError so callers get a RefreshError instead of an unhandled exception.
  • Adds a test that verifies seal_data is unchanged after a failed decode.

Re: Greptile's P2 about iss/aud validation — intentionally not addressed here. Every WorkOS SDK (Ruby, Python, Node, Go, .NET) deliberately skips iss/aud verification because the JWT lives inside an AES-256-GCM sealed cookie; the unseal step already authenticates the data. Adding claim verification would break cross-SDK consistency without meaningful security benefit.

Test plan

  • test_refresh_returns_error_on_malformed_access_token_without_mutating_state — API returns garbage access_token, session state is preserved, caller gets RefreshError
  • Full suite passes (858 tests, 0 failures)

🤖 Generated with Claude Code

Move `decode_jwt` before `@seal_data`/`@cookie_password` assignment so
that a malformed access_token from the API doesn't leave the Session
object half-updated. Also rescue `JWT::DecodeError` so callers get a
`RefreshError` instead of an unhandled exception.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@gjtorikian gjtorikian requested review from a team as code owners April 29, 2026 12:54
@gjtorikian gjtorikian requested review from dandorman and removed request for a team April 29, 2026 12:54
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 29, 2026

Greptile Summary

This PR fixes a state-mutation ordering bug in Session#refresh: decode_jwt is now called before @seal_data/@cookie_password are assigned, so a malformed access_token returned by the API can no longer leave the Session object in a half-updated state. A matching rescue JWT::DecodeError clause converts the previously unhandled exception into a RefreshError, and a new test verifies the state-preservation guarantee.

Confidence Score: 4/5

Safe to merge; the fix is correct and well-tested, with only a pre-existing P2 rule deviation acknowledged in the PR description.

The core fix is logically correct — moving decode_jwt before state mutation prevents half-updated state, and the new rescue prevents an unhandled exception from leaking. The single P2 finding (missing iss/aud claim validation) is a pre-existing, intentional design choice documented in the PR description. No P0 or P1 issues are present.

No files require special attention beyond the noted iss/aud validation caveat in lib/workos/session.rb.

Important Files Changed

Filename Overview
lib/workos/session.rb Moves decode_jwt before @seal_data/@cookie_password assignment to prevent half-updated state on malformed token, and adds JWT::DecodeError rescue; iss/aud claims still not validated (intentional per PR description)
test/workos/test_session.rb Adds test verifying that a malformed access_token from the API returns RefreshError and leaves session.seal_data unmodified; assertion is correct and sufficient

Reviews (1): Last reviewed commit: "fix: prevent partial state mutation in S..." | Re-trigger Greptile

Comment thread lib/workos/session.rb

# Decode before mutating session state so a malformed access_token
# doesn't leave the Session half-updated.
decoded = @manager.decode_jwt(auth_response["access_token"])
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 iss/aud claims not validated on decode

decode_jwt is called here without validating the iss or aud claims. The PR description explicitly notes this is intentional (sealed cookie already authenticates the data, and all WorkOS SDKs share this behaviour), but the team's engineering rule requires both claims to be verified. If the cross-SDK consistency constraint ever relaxes, the missing validation here would allow a JWT signed for a different audience or issuer to be accepted.

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

@gjtorikian gjtorikian merged commit 7d6a0d6 into seven-seals Apr 29, 2026
7 checks passed
@gjtorikian gjtorikian deleted the seven-seals-hardening branch April 29, 2026 14:59
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