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

cognitoauth: Fix "Unexpected char 0x0a at 82 in header value" Error When Using Client Secret #327

Closed
wants to merge 1 commit into from

Conversation

kyarosh
Copy link

@kyarosh kyarosh commented Aug 11, 2017

This request fixes issue #315, which occurs whenever a client secret is used.

Pkce.encodeBase64() currently uses Base64.DEFAULT when creating the base64-encoded string to be used for the PKCE Authorization header. This breaks the ability to use a client secret, as Base64.DEFAULT adds a newline (0x0a) character at the end of the encoded string, which results in the call to httpsURLConnection.addRequestProperty() in AuthHttpClient.httpPost() to throw an exception with a stack trace similar to the following:

java.lang.IllegalArgumentException: Unexpected char 0x0a at 82 in header value: Basic M20zbG5majcxaDljYzZqamk5ajdmZDVndWM6NGxqaXZjdWYzMDlvNWM4NGhrNnYzc28zZDFnajdxNGZpN2lnM2R2aDdzdnY2MWp1Y3Iy
   at com.android.okhttp.Headers$Builder.checkNameAndValue(Headers.java:313)
   at com.android.okhttp.Headers$Builder.add(Headers.java:245)
   at com.android.okhttp.internal.huc.HttpURLConnectionImpl.addRequestProperty(HttpURLConnectionImpl.java:579)
   at com.android.okhttp.internal.huc.DelegatingHttpsURLConnection.addRequestProperty(DelegatingHttpsURLConnection.java:186)
   at com.android.okhttp.internal.huc.HttpsURLConnectionImpl.addRequestProperty(HttpsURLConnectionImpl.java)
   at com.amazonaws.mobileconnectors.cognitoauth.util.AuthHttpClient.httpPost(AuthHttpClient.java:56)
   at com.amazonaws.mobileconnectors.cognitoauth.AuthClient$1.run(AuthClient.java:290)
   at java.lang.Thread.run(Thread.java:761)

@codecov-io
Copy link

Codecov Report

Merging #327 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #327   +/-   ##
=======================================
  Coverage    6.68%    6.68%           
=======================================
  Files        5114     5114           
  Lines      198418   198418           
  Branches    47401    47401           
=======================================
  Hits        13267    13267           
  Misses     184022   184022           
  Partials     1129     1129
Impacted Files Coverage Δ
...zonaws/mobileconnectors/cognitoauth/util/Pkce.java 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6591694...cde1273. Read the comment docs.

@lexmakali
Copy link
Contributor

Hi Kyarosh,

Thank you for using our SDK and submitting a pull request.

In 2.4.7 release we use Base64.NO_PADDING which omits the padding '=' characters. Does this fix your problem? Can you check with 2.4.7 version?

@kyarosh
Copy link
Author

kyarosh commented Sep 18, 2017

Hi @kvasukib,

The problem still persists in 2.4.7. Passing "Base64.NO_PADDING | Base64.NOWRAP" should incorporate both changes if needed, though I'm curious to know whether there are any details with regards to the 2.4.7 update and whether the padding characters really do need to be stripped.

@lexmakali lexmakali added CognitoAuth bug Something isn't working labels Sep 18, 2017
@drxeno02
Copy link

Hello, I would like to push my commit that resolves issue #315 . The fix was that encodeBase64() required its flags to be updated to include Base64.NO_PADDING.

@mreddyg
Copy link
Contributor

mreddyg commented Dec 22, 2017

@drxeno02 Thank you. We will review the changes and merge them in a future release.

@lexmakali
Copy link
Contributor

Hi, @kyarosh and @drxeno02, Sorry for the inconvenience caused. We will be doing the merge in the next few days. We will communicate here once we test and merge in the changes.

@lexmakali
Copy link
Contributor

Hi @kyarosh and @drxeno02, We have fixed it and merged the PR #367 in the v2.6.13 release. Closing this PR.

@lexmakali lexmakali closed this Jan 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants