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

Fix Expiry.Time() to return zero-value of time.Time{} when n is 0 #214

Merged
merged 1 commit into from Jan 13, 2019

Conversation

@dtuck9
Copy link
Contributor

commented Jan 11, 2019

time.Unix(0, 0) does not return time.Time{} as one might expect. Admittedly, this is a work around for what I'd perceive as a bug in golang.

For reference, this is required for validating JWT expiration only where the expiry is set. E.g.:

if !claims.Expiry.Time().IsZero() && claims.Expiry.Time().Before(time.Now()) {
     return errors.New("Expired token")
}

Please let me know if there's a more preferable way to accomplish this check.

See https://play.golang.org/p/sM4SZKzWvFw

@CLAassistant

This comment has been minimized.

Copy link

commented Jan 11, 2019

CLA assistant check
All committers have signed the CLA.

@dtuck9 dtuck9 force-pushed the dtuck9:fix-expiry-zero-time branch from 64da96a to 4470f3a Jan 11, 2019
@dtuck9 dtuck9 force-pushed the dtuck9:fix-expiry-zero-time branch from 4470f3a to cbf0fd6 Jan 11, 2019
@dtuck9 dtuck9 changed the title time.Unix(0,0) does not return time.Time{} as one would expect. This … Fix Expiry.Time() to return zero-value of time.Time{} when n is 0 Jan 11, 2019
@csstaub

This comment has been minimized.

Copy link
Collaborator

commented Jan 11, 2019

This is a nice catch, thank you @dtuck9. One thing I'm worried about here is the potential confusion between the value not being set in the claims (exp, iat and nbf are all optional) and the value being set explicitly to 0 in the JSON. It's unlikely someone would want to have a JWT that expires at UNIX epoch 0 of course, but I wouldn't want the code to get confused about it either way. So possibly a better fix is to make the NumericDate a *int64 (or make the fields *NumericDate) and update the marshal/unmarshal logic. I think I'll have to ponder this a bit over the weekend.

@csstaub

This comment has been minimized.

Copy link
Collaborator

commented Jan 11, 2019

Though I suppose if we change NumericDate to be an *int64 anyone not currently using NewNumericDate to construct NumericDate values will have their code broken. But on the other hand, it's also silly that exp:null and exp:0 deserialize to the same representation. Maybe we can just throw an error if we encounter exp:0 (or more generally, exp < 1) and just make those values unparseable.

@csstaub

This comment has been minimized.

Copy link
Collaborator

commented Jan 12, 2019

I added some changes on top of your branch in https://github.com/square/go-jose/compare/v2...cs/fix-expiry-zero-time?expand=1, please take a look and let me know what you think.

@dtuck9

This comment has been minimized.

Copy link
Contributor Author

commented Jan 12, 2019

@csstaub That looks like a pretty good compromise, and seems to be in line with common practice (and certainly solves the problem I was trying to solve where an unset exp came through as the zero-value).

Would you like me to close this PR in favor of your add-ons?

@csstaub

This comment has been minimized.

Copy link
Collaborator

commented Jan 12, 2019

I’ll merge my branch directly and then it should close this out as merged automatically, I think.

@csstaub csstaub merged commit cbf0fd6 into square:v2 Jan 13, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@dtuck9 dtuck9 deleted the dtuck9:fix-expiry-zero-time branch Jan 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.