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

AsymmetricSignatureVerifier should not use Charset.defaultCharset() #485

Closed
Marcono1234 opened this issue Jun 11, 2021 · 3 comments · Fixed by #491
Closed

AsymmetricSignatureVerifier should not use Charset.defaultCharset() #485

Marcono1234 opened this issue Jun 11, 2021 · 3 comments · Fixed by #491
Labels
waiting for customer This issue is waiting for a response from the issue or PR author

Comments

@Marcono1234
Copy link
Contributor

Describe the problem

AsymmetricSignatureVerifier uses the default charset during token validation:

byte[] contentBytes = content.getBytes(Charset.defaultCharset());

It should probably instead specify an explicit charset, most likely either StandardCharsets.ISO_8859_1 or StandardCharsets.US_ASCII.

@lbalmaceda
Copy link
Contributor

@Marcono1234 thanks for raising this. We use it to extract UTF-8 content. As per the Android docs, this value is always "UTF-8". A change from the Android OS side would be a breaking change for everyone who depends on it, so it's highly unlikely
image

Can you expand a bit on the reasons why we shouldn't use the default charset?

@lbalmaceda lbalmaceda added the waiting for customer This issue is waiting for a response from the issue or PR author label Jun 17, 2021
@Marcono1234
Copy link
Contributor Author

Ah you are right. This might not actually a problem then. I created the issue with the Java implementation in mind which does not make such guarantee and therefore using the default charset there is in most cases not a good idea.
Though if your intention is to use UTF-8, then it might be good to make that more explicit by using StandardCharsets.UTF_8. You are targeting API level 21 and that constant exists since API level 19 so you could use it without any issues.

@lbalmaceda
Copy link
Contributor

Sounds good. We used to pass the string name of the charset and that made us try/catch or ignore a runtime exception that would never be thrown (on android at least).

Would you like to send a PR with that change? Note that it might be used in more than one place. e.g. #484

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for customer This issue is waiting for a response from the issue or PR author
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants