Add ability to extract custom claims from the JWT during authentication#431
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
|
| impersonator: session[:impersonator], | ||
| reason: expired ? 'INVALID_JWT' : nil, | ||
| } | ||
| result.merge!(claim_extractor.call(decoded)) if block_given? |
There was a problem hiding this 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.
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.
[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.
gjtorikian
left a comment
There was a problem hiding this comment.
LGTM -- there's a couple of other JWT PRs floating around so I'll get this released as soon as I can. Thanks!
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