Skip to content

Conversation

jasonjoo2010
Copy link

Affected method:

DefaultMultipartHttpServletRequest::getParameterValues
DefaultMultipartHttpServletRequest::getParameterMap

getParameterValues is used to return multi values of a same key in request, including query string, post contents.
And DefaultMultipartHttpServletRequest would be used instead of default servlet wrapper when using "multipart/form-data" post method to realize including vars in multipart body. And there is a problem in its getter in dealing values of same keys. It will not merge them to values in query string.

It can be reproduced by following steps:

curl -F "key1=test" "http://test.com/hello?key1=other"

You will get a parameter map like:

key1:
	test
curl -d "key1=test" "http://test.com/hello?key1=other"

And you will get:

key1:
	test
	other

Let's sit down to discuss whether fix it?

@pivotal-issuemaster
Copy link

@jasonjoo2010 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@jasonjoo2010 Thank you for signing the Contributor License Agreement!

@jasonjoo2010 jasonjoo2010 changed the title getParameterValues would work incorrectly SPR-16590: getParameterValues would work incorrectly Mar 14, 2018
Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR is incomplete without any tests. Just mentioning it in case you want to make it complete.

@@ -87,14 +89,28 @@ public String getParameter(String name) {
}
return super.getParameter(name);
}

public String[] mergeTwoParameterValues(String[] val1, String[] val2) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need not be a public method.

return val1;
}
//merge them together before return
String[] merged = Arrays.copyOf(val2, val1.length + val2.length);
Copy link
Contributor

@rstoyanchev rstoyanchev Mar 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's counter-intuitive for a method that merges to arrays, two put the second one first.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So i fix them and add getParameterValues test in CommonsMultipartResolverTests.

But i used forced pushing to avoid unnecessary commits and found i cannot reopen this.

Should i open another PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Force push is fine. Just keep the branch updated, no need to re-create the PR.

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