Skip to content

fix(config): reject a non-numeric exp or nbf in default claims - #458

Open
agustin2606 wants to merge 1 commit into
joken-elixir:mainfrom
agustin2606:fix/non-numeric-exp
Open

agustin2606 wants to merge 1 commit into
joken-elixir:mainfrom
agustin2606:fix/non-numeric-exp

Conversation

@agustin2606

Copy link
Copy Markdown

Fixes #452

@mattmikolay offered to send a PR for this; picking it up since it's been a month without one. Happy to defer if you had something in progress.

The default exp validation was &(&1 > current_time()). Under Erlang term ordering any non-number compares greater than a number, so a token whose exp is a string never expired. The same happens with "exp": null: JOSE decodes JSON null as the atom :null, which also sorts above every number.

This PR:

  • Guarded the default exp and nbf validations with is_number/1, through two private predicates (valid_exp?/1, valid_nbf?/1). RFC 7519 defines both as a NumericDate, which may be non-integer, so floats are still accepted.
  • Hardened nbf the same way. It already rejected non-numbers, but only because current_time() >= "string" happens to be false; the guard makes that explicit.
  • Added property tests that exp and nbf reject every non-numeric shape a decoded claim can take (string, boolean, nil, :null, list, map), and a test that a float exp is honoured.
  • Added an [Unreleased] CHANGELOG entry.

With the snippet from the issue, "pizza time" and null now return {:error, [message: "Invalid token", claim: "exp", ...]}, and a valid numeric exp still passes. mix test, mix format --check-formatted, mix credo --strict and mix compile --warnings-as-errors pass locally (Elixir 1.20.4 / OTP 29). I didn't run dialyzer.

The default exp validation was `&1 > current_time()`. Under Erlang term
ordering every non-number compares greater than a number, so a token
whose exp is a string, or null (decoded by JOSE as :null), never expired.

Guard both exp and nbf with is_number/1. RFC 7519 defines them as a
NumericDate, which may be non-integer, so floats are still accepted.

Fixes joken-elixir#452

@mattmikolay mattmikolay left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great! Thanks for implementing this fix. It would be great if this can ship soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Malformed exp considered valid by default claims function

2 participants