Skip to content

fix: add documentation on sealing sessions#625

Merged
gjtorikian merged 2 commits intomainfrom
add-docs-on-sealing-sessions
Apr 16, 2026
Merged

fix: add documentation on sealing sessions#625
gjtorikian merged 2 commits intomainfrom
add-docs-on-sealing-sessions

Conversation

@gjtorikian
Copy link
Copy Markdown
Contributor

This PR adds documentation/guidance around applications that previously passed session= into authenticate_with_code() or authenticate_with_refresh_token().

user was also erroneously set to Optional in these helper methods.

@gjtorikian gjtorikian requested review from a team as code owners April 16, 2026 03:04
@gjtorikian gjtorikian requested a review from awolfden April 16, 2026 03:04
@gjtorikian gjtorikian merged commit 5ae90d0 into main Apr 16, 2026
10 checks passed
@gjtorikian gjtorikian deleted the add-docs-on-sealing-sessions branch April 16, 2026 03:05
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 16, 2026

Greptile Summary

This PR documents the v5→v6 migration path for users who passed session= to authenticate_with_code() / authenticate_with_refresh_token(), and correctly removes the erroneous Optional from the user parameter of seal_session_from_auth_response — ensuring sessions sealed via this helper always carry a user field (required by Session.refresh()).

The one pre-existing issue surfaced by touching session.py is that both Session.authenticate() and AsyncSession.authenticate() decode JWTs with verify_aud: False and no iss constraint, which conflicts with the team's JWT validation policy.

Confidence Score: 5/5

Safe to merge — the code change and tests are correct; the only finding is a pre-existing security hygiene gap.

The user parameter fix is correct and aligns with the existing Session.refresh() invariant. Documentation is accurate. The JWT claim-validation issue is pre-existing, not introduced by this PR, and is flagged as P2 — it does not block this change.

src/workos/session.py — pre-existing verify_aud: False and missing iss validation in both Session variants.

Security Review

  • JWT claim validation (session.py lines 227–233, 408–415): Both Session.authenticate() and AsyncSession.authenticate() decode access tokens with verify_aud: False and no iss constraint. A token signed by the correct JWKS key but destined for a different audience or issuer would be accepted. This was pre-existing but remains in the file touched by this PR.

Important Files Changed

Filename Overview
src/workos/session.py Makes user a required parameter in seal_session_from_auth_response, removes the conditional inclusion logic; correct fix, but pre-existing JWT validation gaps (no iss, aud disabled) remain in the touched file.
tests/test_session.py Tests updated to pass required user arg; test_seal_without_optional_fields correctly renamed to test_seal_without_impersonator and assertions updated to match new behavior.
docs/V6_MIGRATION_GUIDE.md Adds comprehensive migration section for session= removal from auth helpers, including examples for seal_session_from_auth_response and seal_data; checklist and search regex updated accordingly.

Sequence Diagram

sequenceDiagram
    participant App
    participant WorkOSClient
    participant seal_session_from_auth_response
    participant Cookie

    Note over App,Cookie: v6 explicit sealing flow
    App->>WorkOSClient: authenticate_with_code(code=code)
    WorkOSClient-->>App: AuthResponse(access_token, refresh_token, user, impersonator)
    App->>seal_session_from_auth_response: access_token, refresh_token, user.to_dict(), impersonator?, cookie_password
    seal_session_from_auth_response-->>App: sealed_session (Fernet encrypted)
    App->>Cookie: Set sealed_session cookie

    Note over App,Cookie: Later — session refresh
    App->>WorkOSClient: load_sealed_session(session_data, cookie_password)
    WorkOSClient-->>App: Session object
    App->>Session: refresh()
    Session-->>App: RefreshWithSessionCookieSuccessResponse(sealed_session, ...)
Loading

Comments Outside Diff (1)

  1. src/workos/session.py, line 227-233 (link)

    P2 security JWT iss and aud claims not validated

    Both Session.authenticate() (line 227) and AsyncSession.authenticate() (line 408) decode the access token with options={"verify_aud": False} and no iss constraint, leaving two of the three critical JWT claims unvalidated. A token signed by the correct JWKS key but issued for a different issuer or audience would be accepted. The team's security policy requires that both iss and aud are validated before use.

    decoded = jwt.decode(
        session["access_token"],
        signing_key.key,
        algorithms=self._JWK_ALGORITHMS,
        options={"verify_aud": False},  # aud skipped
        # no issuer= / options["verify_iss"] set
        leeway=self._client._jwt_leeway,
    )

    Consider passing the expected issuer and re-enabling audience verification (or explicitly setting the expected audience), or add a comment explaining why these validations are intentionally skipped for this endpoint.

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

Reviews (1): Last reviewed commit: "`user` is not optional" | Re-trigger Greptile

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