Skip to content
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

Don't test that JWT's issued in future are invalid #4678

Merged
merged 1 commit into from
Nov 8, 2017

Conversation

seanh
Copy link
Contributor

@seanh seanh commented Nov 7, 2017

Remove two tests that JWT's whose iat value claims that they were
issued in the future fail validation.

These two tests fail on newer versions of PyJWT:

#4672

This is because PyJWT no longer raises an exception for future iat
times:

jpadilla/pyjwt#190

PyJWT removed this validation because:

  • Clock skew can cause one party to generate iat times a few seconds
    or minutes ahead of another's current time

  • The JWT spec (RFC 7519) doesn't say that a JWT with a future iat
    should be considered invalid, these JWTs are valid

  • Other JWT libraries don't do this check

Remove two tests that JWT's whose `iat` value claims that they were
issued in the future fail validation.

These two tests fail on newer versions of PyJWT:

#4672

This is because PyJWT no longer raises an exception for future `iat`
times:

jpadilla/pyjwt#190

PyJWT removed this validation because:

- Clock skew can cause one party to generate `iat` times a few seconds
or minutes ahead of another's current time

- The JWT spec (RFC 7519) doesn't say that a JWT with a future `iat`
should be considered invalid, these JWTs are valid

- Other JWT libraries don't do this check
@seanh seanh mentioned this pull request Nov 7, 2017
@codecov
Copy link

codecov bot commented Nov 7, 2017

Codecov Report

Merging #4678 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4678      +/-   ##
==========================================
- Coverage   96.56%   96.54%   -0.02%     
==========================================
  Files         367      367              
  Lines       20523    20517       -6     
  Branches     1165     1165              
==========================================
- Hits        19818    19809       -9     
- Misses        599      601       +2     
- Partials      106      107       +1
Impacted Files Coverage Δ
tests/h/oauth/jwt_grant_token_test.py 100% <ø> (ø) ⬆️
tests/h/auth/tokens_test.py 100% <ø> (ø) ⬆️
h/oauth/jwt_grant_token.py 92.3% <0%> (-4.62%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34438d3...ddec2c4. Read the comment docs.

@seanh seanh requested a review from dhwthompson November 7, 2017 15:20
Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me. I'll leave this until @fatbusinessman is back tomorrow in case he has any objections.

@robertknight robertknight merged commit bd8f6e5 into master Nov 8, 2017
@robertknight robertknight deleted the remove-future-token-verify-tests branch November 8, 2017 10:59
@dhwthompson
Copy link
Contributor

I'll leave this until @fatbusinessman is back tomorrow in case he has any objections.

Yup, this seems reasonable. I had a concern about whether there was some way to get long-lived tokens past our checks with this, but those are based on the nbf and exp claims, rather than iat, so I think we’re still good here.

@gobengo
Copy link

gobengo commented Nov 9, 2017

wow that pyjwt lib issue is from me like two years ago. small world

@seanh
Copy link
Contributor Author

seanh commented Nov 9, 2017

:)

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.

4 participants