jjwt
jjwt copied to clipboard
Add SpotBugs / Find Security Bugs
Sets the default encoding in GsonDeserializer
to UTF_8
Previously this was unset, and used the system default
Coverage remained the same at 100.0% when pulling a620bf735e77b0d21b6d6a71c2044b200aabbf11 on gson-utf8 into eadf0ce4fc252341f04c50a45aac6f9c670283e7 on master.
Wow @lhazlewood, I had convinced myself 0.11 was already out. 🤦♂
I changed the note to say that Gson support was added.
As for the test, I added SpotBugs and Find Security Bugs (which is how I found this initially) Added a few fixes for those issues as well, a few of them are less critical but were simple (and easier to fix than exclude)
Thoughts? (spotbugs changes are in a08c1fba109aa3335f38fd782d2f9c78ff5d7e75)
I meant an actual unit test that inspects the String
instance and ensures the encoding is UTF-8
.
Probably just change the name, but let's wrap up the other open questions/issues first and then figure out the best option
@lhazlewood have a suggestion on a good way to do this?
I meant an actual unit test that inspects the String instance and ensures the encoding is UTF-8.
The only option I could think of was to create a byte array with characters known to render differently, but IMHO, it's easier to scan the source tree for these types of issues 🤔
@bdemers just create a mock gson instance and mock gson.fromJson
, and assert that the argument is UTF-8 compatible?
I found this: https://www.turro.org/publications/?item=114&page=0
But if you don't want to mess with that, no objection from me. I'm just paranoid 😅
I was messing with creating keys today with BC (via JCA), and it's really really easy to get a RuntimeException
to be thrown. I think if we just tack on a | RuntimeException
we should be good for backward compatibility.
I think if we just tack on a
| RuntimeException
we should be good for backward compatibility.
If that's the case, why change the code at all? Just curious.
Valid point, I was just thinking it would be easier to change in the future, but it's probably just noise
@lhazlewood back to the topic of the test, we actually do have tests that fail when the system's default encoding is NOT UTF-8
we just don't see this because that is usually what Linux and MacOS is set to.
Adding a test for this is challenging because it would require forking a JVM to change the file.encoding
property.
I tried this from my IDE, but you should be able to reproduce it by running the tests against master
with following arg-Dfile.encoding=ISO-8859-1
I updated the PR name to better reflect the changes here, and I've moved the Gson change log note to: #547
@bdemers what do you want to do with this? Thoughts?
I'll take another pass at this in a different PR, based off the latest code