Only issue invalid base64 warning on successful decode

[jeremyevans]

Description

To make the patch minimally invasive, this attempts to decode the given jwt in strict base64 mode only after a successful full decode and validation.

Most jwts are small, so this should have minimal performance impact, though it does require decoding twice. It is possible to fix the issue and only decode once, but it would be more invasive.

A big advantage of this approach is the use of :uplevel when calling Kernel#warn, so that the calling location that triggers the warning is included in the warning message, greatly simplifying debugging.

Checklist

Before the PR can be merged be sure the following are checked:

• There are tests for the fix or feature added/changed
• A description of the changes and a reference to the PR has been added to CHANGELOG.md. More details in the CONTRIBUTING.md

[anakinj]

Thanks for taking the time thinking about this. In this approach the concern is that now it's doing this for every successful decode "just in case".

[jeremyevans]

Thanks for taking the time thinking about this. In this approach the concern is that now it's doing this for every successful decode "just in case".

Correct. It's a simpler approach. I'm not sure what the performance difference is, but I expect it to be small unless the JWTs are quite large. Would you like me to benchmark both approaches?

[anakinj]

My only argument against this is that it will affect all the requests that have no issue with base64 encoding, the assumption that JWTs are small is a bit dangerous :)

Benchmarking would be interesting. On the other hand one could focus on trying to get a new major version out there that would just remove all this complexity of being nice to the user.

[jeremyevans]

Here's a benchmark:

require 'benchmark/ips'
require 'jwt'

h = {}
ARGV[0].to_i.times{|i| h[i.to_s] = i}
key = '1'*32
jwt = JWT.encode(h, key)

Benchmark.ips do |x|
  x.report('decode'){JWT.decode(jwt, key)}
end

Performance of #601 is about 5-10% worse than #600 depending on the size of the JWT:

#601:

0 entries:
              decode     18.119k (_ 2.8%) i/s -     91.884k in   5.075099s
10000 entries:
              decode    199.303 (_ 3.0%) i/s -      1.008k in   5.061691s

#600:

0 entries:
              decode     18.959k (_ 1.6%) i/s -     94.770k in   5.000008s
10000 entries:
              decode    220.187 (_ 3.6%) i/s -      1.113k in   5.060928

Parsing JSON is much slower than decoding base64, which is why even for large JWTs, this will never result in a major slowdown. Up to you whether the simpler implementation and easier debugging (via uplevel) in this approach is worth the slight decrease in performance.

[jeremyevans]

Closing as fixed by #600.