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

HTTP(s) basic auth failed if password contained ampersand passed via … #104

Merged
merged 1 commit into from Sep 5, 2019

Conversation

pendor
Copy link
Contributor

@pendor pendor commented Oct 2, 2017

…basic.password URL parameter

A double-decode bug caused URLDecode to be applied twice to parameters passed in
via URL including basic.username and basic.password. The parameters were automatically
decoded by the call to URI.getQuery() then again as each parameter was parsed and added
to the returned Map in MulticastConnectionFactory.URIs.parseQuery(). parseQuery() splits the
query string on the ampersand character then explictly URLDecode's each value. Since
URI.getQuery() had already decoded the basic.password parameter, the splitting process
in parseQuery truncated the password at the first ampersand character.

Instead, URI.getRawQuery() should be called to get the still URLEncoded query string. The
splitting and subsequent decoding in parseQuery() then correctly extracts the full password
from the query string.

PR contains failing unit test & fix.

…basic.password URL parameter

A double-decode bug caused URLDecode to be applied twice to parameters passed in
via URL including basic.username and basic.password.  The parameters were automatically
decoded by the call to URI.getQuery() then again as each parameter was parsed and added
to the returned Map in MulticastConnectionFactory.URIs.parseQuery().  parseQuery() splits the
query string on the ampersand character then explictly URLDecode's each value.  Since
URI.getQuery() had already decoded the basic.password parameter, the splitting process
in parseQuery truncated the password at the first ampersand character.

Instead, URI.getRawQuery() should be called to get the still URLEncoded query string.  The
splitting and subsequent decoding in parseQuery() then correctly extracts the full password
from the query string.
@rzo1
Copy link
Contributor

rzo1 commented May 27, 2019

I can follow your explanation. I took a quick look into MulticastConnectionFactory and confirm, that the decoding is happening twice. I think, that we do not have a related JIRA ticket for this issue.

I think, that we can enhance the test-case with try-with-resources and without the e.printStacktrace() statements inside the test-case.

WDYT @jeanouii ?

@rzo1
Copy link
Contributor

rzo1 commented Jun 14, 2019

Shall we create a JIRA-Ticket for this? @jeanouii

@rzo1
Copy link
Contributor

rzo1 commented Sep 5, 2019

I created https://issues.apache.org/jira/browse/TOMEE-2656 for this.

Did not get an answer for the comment:

I think, that we can enhance the test-case with try-with-resources and without the e.printStacktrace() statements inside the test-case.

WDYT @cesarhernandezgt ?

@asf-ci
Copy link

asf-ci commented Sep 5, 2019

Can one of the admins verify this patch?

2 similar comments
@asf-ci
Copy link

asf-ci commented Sep 5, 2019

Can one of the admins verify this patch?

@asf-ci
Copy link

asf-ci commented Sep 5, 2019

Can one of the admins verify this patch?

@jgallimore
Copy link
Contributor

I think this looks good to merge. I'll give it a quick test and get it merged.

@asfgit asfgit merged commit 1905fcc into apache:master Sep 5, 2019
@jgallimore
Copy link
Contributor

Merged and backported to 7.1.x and 7.0.x. Thanks for the PR!

@rzo1
Copy link
Contributor

rzo1 commented Sep 5, 2019

I closed the related JIRA and set the fix version(s)

@jgallimore
Copy link
Contributor

@rzo1 Many thanks for the review, the JIRA admin and for pushing this through, its really appreciated.

jgallimore added a commit to jgallimore/tomee that referenced this pull request Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants