-
Notifications
You must be signed in to change notification settings - Fork 34
Add ability to extract custom claims from the JWT during authentication #431
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Via an optional block param. E.g.
```rb
result = session.authenticate do |jwt|
{ my_custom_claim: jwt['custom_claim'], my_other_claim: jwt['another_claim'] }
end
puts result[:my_custom_claim]
```
For use with JWT Templates [1] allowing additional properties to be included in the JWT.
Alternatively we could include the whole raw decoded token if an argument is passed with something like:
```rb
claims: include_raw_claims ? decoded : nil,
``
While simpler, it felt cleaner to allow extracting just what's needed.
1: https://workos.com/docs/authkit/jwt-templates
Greptile OverviewGreptile SummaryThis PR extends Main concern is that the merged hash is currently unvalidated and can (a) raise when the block returns Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant C as Caller
participant S as WorkOS::Session
participant Sealer as Session.unseal_data
participant J as JWT
participant B as claim_extractor block
C->>S: authenticate(include_expired:, &block)
alt session_data nil
S-->>C: {authenticated:false, reason:'NO_SESSION_COOKIE_PROVIDED'}
else
S->>Sealer: unseal_data(session_data, cookie_password)
alt invalid cookie
S-->>C: {authenticated:false, reason:'INVALID_SESSION_COOKIE'}
else
S->>J: decode(access_token, jwks, verify_expiration:false)
alt decode fails
S-->>C: {authenticated:false, reason:'INVALID_JWT'}
else
J-->>S: decoded claims
S->>S: expired? (decoded['exp'] < now)
alt expired && !include_expired
S-->>C: {authenticated:false, reason:'INVALID_JWT'}
else
S->>S: build result hash
opt block_given?
S->>B: call(decoded)
B-->>S: custom hash
S->>S: result.merge!(custom hash)
end
S-->>C: result
end
end
end
end
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, 2 comments
| impersonator: session[:impersonator], | ||
| reason: expired ? 'INVALID_JWT' : nil, | ||
| } | ||
| result.merge!(claim_extractor.call(decoded)) if block_given? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P0] merge! will raise if the block returns a non-Hash (including nil), which then gets caught by the generic rescue StandardError and turned into { authenticated: false, reason: e.message }. This makes the new block API brittle and can unexpectedly fail authentication for otherwise-valid sessions if a caller forgets to return a hash.
Consider treating nil as “no extra claims” and validating the return type before merging.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/workos/session.rb
Line: 76:76
Comment:
[P0] `merge!` will raise if the block returns a non-Hash (including `nil`), which then gets caught by the generic `rescue StandardError` and turned into `{ authenticated: false, reason: e.message }`. This makes the new block API brittle and can unexpectedly fail authentication for otherwise-valid sessions if a caller forgets to return a hash.
Consider treating `nil` as “no extra claims” and validating the return type before merging.
How can I resolve this? If you propose a fix, please make it concise.| impersonator: session[:impersonator], | ||
| reason: expired ? 'INVALID_JWT' : nil, | ||
| } | ||
| result.merge!(claim_extractor.call(decoded)) if block_given? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] The block’s returned hash can overwrite core fields like :authenticated, :session_id, :organization_id, and :reason because merge! prefers the block’s keys. If consumers pass through untrusted logic or make a mistake, this can produce inconsistent or misleading auth results.
A safer pattern is to merge under a dedicated key (e.g. :custom_claims) or explicitly reject/ignore collisions with reserved keys.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/workos/session.rb
Line: 76:76
Comment:
[P1] The block’s returned hash can overwrite core fields like `:authenticated`, `:session_id`, `:organization_id`, and `:reason` because `merge!` prefers the block’s keys. If consumers pass through untrusted logic or make a mistake, this can produce inconsistent or misleading auth results.
A safer pattern is to merge under a dedicated key (e.g. `:custom_claims`) or explicitly reject/ignore collisions with reserved keys.
How can I resolve this? If you propose a fix, please make it concise.
Via an optional block param. E.g.
For use with JWT Templates [1] allowing additional properties to be included in the JWT.
Alternatively we could include the whole raw decoded token if an argument is passed with something like:
[ ] Yes