Skip to content

Adds UTF-8 encoding in Util base64 encode/decode#93

Merged
pitbulk merged 1 commit intoSAML-Toolkits:masterfrom
rembjo0:topic-encoding
Feb 10, 2017
Merged

Adds UTF-8 encoding in Util base64 encode/decode#93
pitbulk merged 1 commit intoSAML-Toolkits:masterfrom
rembjo0:topic-encoding

Conversation

@rembjo0
Copy link
Copy Markdown

@rembjo0 rembjo0 commented Feb 4, 2017

Default encoding used in Util.base64encoder() methods.
This commits adds UTF-8 encoding in string <-> byte conversions.

Added Util.toBytesUtf8(String) and Util.toStringUtf8(byte[]).

No tests added for this one. It's a bit messy to test
things that require the JVM defaults to change. We usually
use code analysis tools to detect byte<->string conversion
issues.

Default encoding used in Util.base64encoder() methods.
This commits adds UTF-8 encoding in string <-> byte conversions.

Added Util.toBytesUtf8(String) and Util.toStringUtf8(byte[]).

No tests added for this one. It's a bit messy to test
things that require the JVM defaults to change. We usually
use code analysis tools to detect byte<->string conversion
issues.
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.08%) to 95.863% when pulling bbe081b on rembjo0:topic-encoding into 560d41d on onelogin:master.

@pitbulk
Copy link
Copy Markdown
Contributor

pitbulk commented Feb 6, 2017

@miszobi are you ok with that change?

@miszobi
Copy link
Copy Markdown
Contributor

miszobi commented Feb 6, 2017

Yup, looks okay to me. Was trying to figure out a way to break the tests by running using a different encoding over the weekend, but didn't get to it. Will submit a separate PR if I do that.

@rembjo0 what encoding you're using that you run into issues, out of curiosity?

@rembjo0
Copy link
Copy Markdown
Author

rembjo0 commented Feb 6, 2017

@miszobi, CP1252, running on windows. Note that we suspected that this was causing the logout response signature verification to fail. But it turned out that the message contained only ascii and thus it made no difference. I do hope these methods will use a fixed encoding in the future as it removes the uncertainty. We do send the NameId in the LogoutRequest and this will be encoded in CP1252 without this patch. Everything has sensible names in this environment, but it would not surprise me if somebody figures out a way to add non-ascii chars to the NameId.

@pitbulk pitbulk merged commit 39d5dac into SAML-Toolkits:master Feb 10, 2017
@pitbulk
Copy link
Copy Markdown
Contributor

pitbulk commented Feb 10, 2017

Merged that, @miszobi your test on that will be welcomed.

@rembjo0 rembjo0 deleted the topic-encoding branch February 15, 2017 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants