-
Notifications
You must be signed in to change notification settings - Fork 372
Add duplicate claim name detection per RFC 7519 Section 4 #713
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
Implements duplicate claim name detection as specified in RFC 7519 Section 4, which states: > The Claim Names within a JWT Claims Set MUST be unique; JWT parsers MUST either reject JWTs with duplicate Claim Names or use a JSON parser that returns only the lexically last duplicate member name. This feature allows users to reject JWTs that contain duplicate keys in the header or payload, which is recommended for security-sensitive applications to prevent claim confusion attacks.
0838041 to
bf095f9
Compare
lib/jwt/json.rb
Outdated
|
|
||
| # @api private | ||
| # Checks for duplicate keys in a JSON string using a StringScanner-based tokenizer | ||
| # rubocop:disable Style/RedundantRegexpArgument |
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.
lib/jwt/encoded_token.rb
Outdated
| # @raise [ArgumentError] if the provided JWT is not a String. | ||
| def initialize(jwt) | ||
| # @raise [JWT::DuplicateKeyError] if allow_duplicate_keys is false and duplicate keys are found. | ||
| def initialize(jwt, allow_duplicate_keys: true) |
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.
Would like to avoid bloating the initializer with feature parameters, the original JWT.encode and JWT.decode methods became parameter monsters over time.
A few random options, not sure if Im a fan of either of my own suggestions :)
token = EncodedToken.new(jwt)
token.raise_on_duplicate_keys!
token = EncodedToken.new(jwt)
token.decode_payload!(raise_on_duplicate_keys:true)
Or allowing changing the default JSON decoder with a stricter one, token.raise_on_duplicate_keys! could internally do something like this.
token = EncodedToken.new(jwt)
token.parser = JWT::JSON.new(raise_on_duplicate_keys:true)
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.
I've organized it. What do you think?
lib/jwt/json.rb
Outdated
| # JWT::JSON.parse('{"a":1,"a":2}', allow_duplicate_keys: false) | ||
| # # => raises JWT::DuplicateKeyError | ||
| def parse(data, allow_duplicate_keys: true) | ||
| DuplicateKeyChecker.check!(data) unless allow_duplicate_keys |
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.
Should we try to use the built-in-soon-to-be-default ::JSON.parse(data, allow_duplicate_key: false) to deal with this issue. Feels like this gem is not supposed to be dealing with parsing concerns.
Also the new default will kick in in json 3.0. Think if the user has not taken a stance the default behavior of the json gem should be honored.
So adding as little logic as possible related to this feature will make future maintenance easier.
lib/jwt/decode.rb
Outdated
| token.header['alg'] | ||
| end | ||
|
|
||
| def allow_duplicate_keys?(options) |
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.
I have the urge not to add features to the old API anymore. Could we try that and document how to use this feature using the new EncodedToken class.
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.
I've added instructions for use. How does that look?
|
Overall I support this feature and think it should be the default behavior. My biggest concern is adding a lot of logic to this now it will stay and live in the gem in the future. Would almost just like to require |
1acb9e3 to
11450ac
Compare
| os: | ||
| - ubuntu-latest | ||
| ruby: | ||
| - "2.5" |
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.
MEMO: We added json gem >= 2.13.0 to the dependencies, but this version requires Ruby 2.7+. Since ruby-jwt has required_ruby_version = ‘>= 2.5’, dependency resolution fails during bundle install in a Ruby 2.5 environment.
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.
Would like to support as many versions as possible, what about dynamically figure out if its possible to enable this feature or not.
|
I updated this PR. WDYT? |
lib/jwt/json.rb
Outdated
| # @api private | ||
| class JSON | ||
| class << self | ||
| # Generates a JSON string from the given data |
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.
As these apis are marked private and pretty self-explanatory I would not bother having detailed comments on them.
lib/jwt/json.rb
Outdated
| # JWT::JSON.parse('{"a":1,"a":2}', allow_duplicate_keys: false) | ||
| # # => raises JWT::DuplicateKeyError | ||
| def parse(data, allow_duplicate_keys: true) | ||
| ::JSON.parse(data, allow_duplicate_key: allow_duplicate_keys) |
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.
This is problematic in a way that when JSON 3.0 ships with the switched default we still are allowing duplicate keys by default. Would rather at that point enforce the new default for everyone.
Could we pass the parameter to the JSON lib only if the parameter is false,
| # @return [self] | ||
| # @raise [JWT::DuplicateKeyError] if duplicate keys are found during subsequent parsing. | ||
| def raise_on_duplicate_keys! | ||
| @allow_duplicate_keys = false |
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.
Could we here detect if the JSON gem is supporting the feature and raise if we are not compatible? Would allow us to support older Ruby versions still.
| # encoded_token.verify_signature!(algorithm: 'HS256', key: 'secret') | ||
| # encoded_token.payload # => {'pay' => 'load'} | ||
| class EncodedToken | ||
| class EncodedToken # rubocop:disable Metrics/ClassLength |
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.
Wonder if we could do something to not need to disable cops 🤔
…on a runtime feature check instead of a hard dependency on json >= 2.13.0
Description
Implements duplicate claim name detection as specified in RFC 7519 Section 4, which states:
see: https://datatracker.ietf.org/doc/html/rfc7519#section-4
This feature allows users to reject JWTs that contain duplicate keys in the header or payload, which is recommended for security-sensitive applications to prevent claim confusion attacks.
Checklist
Before the PR can be merged be sure the following are checked: