When exp indicates the present, make it invalid.

[soranoba]

https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.4

The "exp" (expiration time) claim identifies the expiration time on
or after which the JWT MUST NOT be accepted for processing.

it is written on the reference.
So, it seems invalid when exp indicates the present. Isn't it right?

[lggomez]

That's a breaking change on the current claims structure (which we want to avoid for compatibility purposes despite standards adherence) but there's an ongoing PR adding an API compatible implementation of a RFC7519 compatible claim at #15

cc @oxisto

[lggomez]

@soranoba the PR was just merged into main, so you should be able to test this via go get github.com/golang-jwt/jwt/v4@main, there is a new claims type RegisteredClaims which performs the following RFC compliant validation:

jwt/claims.go

Lines 237 to 242 in 80625fb

	func verifyExp(exp *time.Time, now time.Time, required bool) bool {
	if exp == nil {
	return !required
	}
	return now.Before(*exp) || now.Equal(*exp)
	}

[oxisto]

@soranoba the PR was just merged into main, so you should be able to test this via go get github.com/golang-jwt/jwt/v4@main, there is a new claims type RegisteredClaims which performs the following RFC compliant validation:

jwt/claims.go

Lines 237 to 242 in 80625fb

	func verifyExp(exp *time.Time, now time.Time, required bool) bool {
	if exp == nil {
	return !required
	}
	return now.Before(*exp) || now.Equal(*exp)
	}

I fear the new claim is just replicating the old claim behaviour. However, since we did not do a release yet, we are somewhat free to change it. I need to double check the exact wording on the RFC again and will have a look at how other libraries handle this.

[soranoba]

Thank you for the information. I will check it.

[soranoba]

Now that the main branch uses time.Time (It is same dgrijalva/jwt-go v4.0.0-preview1), there are no real problems with RFC violations.

In the current v4, it checks expires in 1s units, so there was a problem that it is not regarded as expire even if several ms have passed in the test code.
I wanted this case to go away, so my worries have been resolved at the main branch.

However, I think the main branch is RFC incompliant either.

- return now.Before(*exp) || now.Equal(*exp) 
+ return now.Before(*exp)

I think it's correct to do this, but I'm not firmly reading the RFC, so I'll leave it to you.
Thanks.

[dunglas]

Your proposed patch looks correct to me @soranoba.

[mfridman]

There is a merge conflict on the PR. Should we fix it up?

[mfridman]

I also agree with this proposal. Likewise the time-based verification should just be:

func verifyExp(exp *time.Time, now time.Time, required bool) bool {
	if exp == nil {
		return !required
	}
- 	return now.Before(*exp) || now.Equal(*exp)
+	return now.Before(*exp)
}

jwt/claims.go

Line 241 in 205b3dc

return now.Before(*exp) || now.Equal(*exp)

[oxisto]

I also agree with this proposal. Likewise the time-based verification should just be:

func verifyExp(exp *time.Time, now time.Time, required bool) bool {
	if exp == nil {
		return !required
	}
- 	return now.Before(*exp) || now.Equal(*exp)
+	return now.Before(*exp)
}

jwt/claims.go

Line 241 in 205b3dc

return now.Before(*exp) || now.Equal(*exp)

Yes. Looks like this is correct. I fixed the merge conflict.

[mfridman]

lgtm.

note to self to consider rewriting these tests as table tests