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

Fix UserInfo with plus sign (#2713) #2727

Merged
merged 3 commits into from Jul 19, 2019

Conversation

@valydia
Copy link

commented Jul 16, 2019

No description provided.

Valy Dia
@rossabaker
Copy link
Member

left a comment

Thanks, @valydia! Good find on the + being what triggers this. However, + is a valid character there, and does not mean space in this context. I think the problem is not with the encoding, but the decoding. The decode in the parser delegates to URLDecoder.decode, which is designed for the similar-but-not-the-same application/x-www-form-urlencoded. That's not something we should use in a URI outside the query string. I think replacing the decode call with UrlCoding.urlDecode might fix the bug, and let us continue to treat + as the other subdelims.


"skip encoding subdelims except '+' in password" in {
renderString(UserInfo("hi", Some("!$&'()*+,;="))) must_== "hi:!$&'()*%2B,;="
}

This comment has been minimized.

Copy link
@rossabaker

rossabaker Jul 16, 2019

Member

These tests will break, but maybe it's good to explicitly test that all the subdelims aren't encoded.

val userInfo = UserInfo("username+", Some("password+"))
HttpCodec[UserInfo].parse(renderString(userInfo)) must_== Right(userInfo)
}
}

This comment has been minimized.

Copy link
@rossabaker

rossabaker Jul 16, 2019

Member

Definitely keep this test.

@valydia

This comment has been minimized.

Copy link
Author

commented Jul 16, 2019

@rossabaker Thanks for the guidance, I wasn't entirely sure which way to fix it...

@rossabaker
Copy link
Member

left a comment

👍 if green, but I'm not sure: see below.

@@ -144,6 +144,6 @@ private[http4s] trait Rfc3986Parser

def SubDelims = rule { "!" | "$" | "&" | "'" | "(" | ")" | "*" | "+" | "," | ";" | "=" }

protected def decode(s: String) = URLDecoder.decode(s, charset.name)
protected def decode(s: String) = UrlCodingUtils.urlDecode(s, charset)

This comment has been minimized.

Copy link
@rossabaker

rossabaker Jul 16, 2019

Member

I'm afraid this might break tests in the query string, which do encode +. I was thinking of replacing the decode calls in UserInfo.Parser. But if it works, 👍, because this is correct in more cases.

This comment has been minimized.

Copy link
@valydia

valydia Jul 16, 2019

Author

Ok I see, I think I should correct this, even if the ci passes, because I am not sure that explicit test cases for '+' exist ...

This comment has been minimized.

Copy link
@rossabaker

rossabaker Jul 17, 2019

Member

I confirmed that the query is not using that function. It's path and regname.

Valy Dia
@rossabaker
Copy link
Member

left a comment

Looks like my query string fear was unfounded. 👍

@rossabaker rossabaker added the bug label Jul 17, 2019

@rossabaker rossabaker merged commit 5909d2b into http4s:master Jul 19, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.