You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by "vlsi (via GitHub)" <gi...@apache.org> on 2024/03/18 10:46:15 UTC
[PR] HTTPCLIENT-2325 Avoid adding "; charset=" for multipart/form-data requests [httpcomponents-client]
vlsi opened a new pull request, #556:
URL: https://github.com/apache/httpcomponents-client/pull/556
See https://issues.apache.org/jira/browse/HTTPCLIENT-2325
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2325 Avoid adding "; charset=" for multipart/form-data requests [httpcomponents-client]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #556:
URL: https://github.com/apache/httpcomponents-client/pull/556#discussion_r1528368485
##########
httpclient5/src/main/java/org/apache/hc/client5/http/entity/mime/MultipartEntityBuilder.java:
##########
@@ -253,12 +253,13 @@ MultipartFormEntity buildEntity() {
if (charsetCopy == null && contentType != null) {
charsetCopy = contentType.getCharset();
}
- final List<NameValuePair> paramsList = new ArrayList<>(2);
- paramsList.add(new BasicNameValuePair("boundary", boundaryCopy));
- if (charsetCopy != null) {
- paramsList.add(new BasicNameValuePair("charset", charsetCopy.name()));
- }
- final NameValuePair[] params = paramsList.toArray(EMPTY_NAME_VALUE_ARRAY);
+ // Previusly, "charset" parameter was added to the Content-Type header, however adding "charset=..."
+ // is not specified in RFC 7578, and it causes issues with (flawed?) HTTP servers.
+ // For instance:
+ // https://github.com/owasp-modsecurity/ModSecurity/commit/6e56950cdf258c9b39f12cf6eb014cb59797cfd3
+ // https://github.com/akka/akka-http/issues/338
+ // https://bz.apache.org/bugzilla/show_bug.cgi?id=61384
+ final NameValuePair[] params = new NameValuePair[]{new BasicNameValuePair("boundary", boundaryCopy)};
Review Comment:
This should not be here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2325 Avoid adding "; charset=" for multipart/form-data requests [httpcomponents-client]
Posted by "vlsi (via GitHub)" <gi...@apache.org>.
vlsi commented on code in PR #556:
URL: https://github.com/apache/httpcomponents-client/pull/556#discussion_r1528354232
##########
httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartEntityBuilder.java:
##########
@@ -88,7 +88,7 @@ public void testMultipartCustomContentType() throws Exception {
.setLaxMode()
.buildEntity();
Assertions.assertNotNull(entity);
- Assertions.assertEquals("application/xml; boundary=blah-blah; charset=UTF-8", entity.getContentType());
+ Assertions.assertEquals("application/xml; charset=UTF-8; boundary=blah-blah", entity.getContentType());
Review Comment:
Please clarify what do you mean here. I kept the test to verify the behaviour for the code that did use weird content types for multipart previously.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2325 Avoid adding "; charset=" for multipart/form-data requests [httpcomponents-client]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #556:
URL: https://github.com/apache/httpcomponents-client/pull/556#discussion_r1528332718
##########
httpclient5/src/main/java/org/apache/hc/client5/http/entity/mime/MultipartEntityBuilder.java:
##########
@@ -253,12 +253,13 @@ MultipartFormEntity buildEntity() {
if (charsetCopy == null && contentType != null) {
charsetCopy = contentType.getCharset();
}
- final List<NameValuePair> paramsList = new ArrayList<>(2);
- paramsList.add(new BasicNameValuePair("boundary", boundaryCopy));
- if (charsetCopy != null) {
- paramsList.add(new BasicNameValuePair("charset", charsetCopy.name()));
- }
- final NameValuePair[] params = paramsList.toArray(EMPTY_NAME_VALUE_ARRAY);
+ // Previusly, "charset" parameter was added to the Content-Type header, however adding "charset=..."
+ // is not specified in RFC 7578, and it causes issues with (flawed?) HTTP servers.
+ // For instance:
+ // https://github.com/owasp-modsecurity/ModSecurity/commit/6e56950cdf258c9b39f12cf6eb014cb59797cfd3
+ // https://github.com/akka/akka-http/issues/338
+ // https://bz.apache.org/bugzilla/show_bug.cgi?id=61384
+ final NameValuePair[] params = new NameValuePair[]{new BasicNameValuePair("boundary", boundaryCopy)};
Review Comment:
This belongs into the commit message
##########
httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartEntityBuilder.java:
##########
@@ -88,7 +88,7 @@ public void testMultipartCustomContentType() throws Exception {
.setLaxMode()
.buildEntity();
Assertions.assertNotNull(entity);
- Assertions.assertEquals("application/xml; boundary=blah-blah; charset=UTF-8", entity.getContentType());
+ Assertions.assertEquals("application/xml; charset=UTF-8; boundary=blah-blah", entity.getContentType());
Review Comment:
Regardless of your change, I question this test because a multipart request shall start with `multipart/` type...
##########
httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartEntityBuilder.java:
##########
@@ -130,7 +130,7 @@ public void testMultipartCustomContentTypeUsingAddParameter() {
eb.buildEntity();
final MultipartFormEntity entity = eb.buildEntity();
Assertions.assertNotNull(entity);
- Assertions.assertEquals("multipart/related; boundary=yada-yada; charset=US-ASCII; my=stuff",
+ Assertions.assertEquals("multipart/related; boundary=yada-yada; charset=ascii; my=stuff",
entity.getContentType());
Review Comment:
Same for these two...
##########
httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartEntityBuilder.java:
##########
@@ -99,7 +99,7 @@ public void testMultipartContentTypeParameter() throws Exception {
new BasicNameValuePair("charset", "ascii")))
.buildEntity();
Assertions.assertNotNull(entity);
- Assertions.assertEquals("multipart/form-data; boundary=yada-yada; charset=US-ASCII", entity.getContentType());
+ Assertions.assertEquals("multipart/form-data; boundary=yada-yada; charset=ascii", entity.getContentType());
Review Comment:
The charset shouldn't appear at all, no?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2325 Avoid adding "; charset=" for multipart/form-data requests [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #556:
URL: https://github.com/apache/httpcomponents-client/pull/556#discussion_r1528452045
##########
httpclient5/src/main/java/org/apache/hc/client5/http/entity/mime/MultipartEntityBuilder.java:
##########
@@ -253,12 +253,13 @@ MultipartFormEntity buildEntity() {
if (charsetCopy == null && contentType != null) {
charsetCopy = contentType.getCharset();
}
- final List<NameValuePair> paramsList = new ArrayList<>(2);
- paramsList.add(new BasicNameValuePair("boundary", boundaryCopy));
- if (charsetCopy != null) {
- paramsList.add(new BasicNameValuePair("charset", charsetCopy.name()));
- }
- final NameValuePair[] params = paramsList.toArray(EMPTY_NAME_VALUE_ARRAY);
+ // Previusly, "charset" parameter was added to the Content-Type header, however adding "charset=..."
+ // is not specified in RFC 7578, and it causes issues with (flawed?) HTTP servers.
+ // For instance:
+ // https://github.com/owasp-modsecurity/ModSecurity/commit/6e56950cdf258c9b39f12cf6eb014cb59797cfd3
+ // https://github.com/akka/akka-http/issues/338
+ // https://bz.apache.org/bugzilla/show_bug.cgi?id=61384
+ final NameValuePair[] params = new NameValuePair[]{new BasicNameValuePair("boundary", boundaryCopy)};
Review Comment:
>
> If you still think the code comment should be removed, I can easily do that. It would make the maintenance harder though.
@vlsi Please remove that comment and move it to the commit message if you like. Those who cannot work with the commit history should not be committing to the repository.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2325 Avoid adding "; charset=" for multipart/form-data requests [httpcomponents-client]
Posted by "vlsi (via GitHub)" <gi...@apache.org>.
vlsi commented on code in PR #556:
URL: https://github.com/apache/httpcomponents-client/pull/556#discussion_r1528350724
##########
httpclient5/src/main/java/org/apache/hc/client5/http/entity/mime/MultipartEntityBuilder.java:
##########
@@ -253,12 +253,13 @@ MultipartFormEntity buildEntity() {
if (charsetCopy == null && contentType != null) {
charsetCopy = contentType.getCharset();
}
- final List<NameValuePair> paramsList = new ArrayList<>(2);
- paramsList.add(new BasicNameValuePair("boundary", boundaryCopy));
- if (charsetCopy != null) {
- paramsList.add(new BasicNameValuePair("charset", charsetCopy.name()));
- }
- final NameValuePair[] params = paramsList.toArray(EMPTY_NAME_VALUE_ARRAY);
+ // Previusly, "charset" parameter was added to the Content-Type header, however adding "charset=..."
+ // is not specified in RFC 7578, and it causes issues with (flawed?) HTTP servers.
+ // For instance:
+ // https://github.com/owasp-modsecurity/ModSecurity/commit/6e56950cdf258c9b39f12cf6eb014cb59797cfd3
+ // https://github.com/akka/akka-http/issues/338
+ // https://bz.apache.org/bugzilla/show_bug.cgi?id=61384
+ final NameValuePair[] params = new NameValuePair[]{new BasicNameValuePair("boundary", boundaryCopy)};
Review Comment:
I've updated the commit message
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2325 Avoid adding "; charset=" for multipart/form-data requests [httpcomponents-client]
Posted by "vlsi (via GitHub)" <gi...@apache.org>.
vlsi commented on code in PR #556:
URL: https://github.com/apache/httpcomponents-client/pull/556#discussion_r1528356852
##########
httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartEntityBuilder.java:
##########
@@ -99,7 +99,7 @@ public void testMultipartContentTypeParameter() throws Exception {
new BasicNameValuePair("charset", "ascii")))
.buildEntity();
Assertions.assertNotNull(entity);
- Assertions.assertEquals("multipart/form-data; boundary=yada-yada; charset=US-ASCII", entity.getContentType());
+ Assertions.assertEquals("multipart/form-data; boundary=yada-yada; charset=ascii", entity.getContentType());
Review Comment:
The charset was explicitly configured via `ontentType.MULTIPART_FORM_DATA.withParameters(...new BasicNameValuePair("charset", "ascii")`.
If the user explicitly creates content type with charset, they might know what they are doing, so there's no point in removing the charset.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2325 Avoid adding "; charset=" for multipart/form-data requests [httpcomponents-client]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #556:
URL: https://github.com/apache/httpcomponents-client/pull/556#discussion_r1528412914
##########
httpclient5/src/main/java/org/apache/hc/client5/http/entity/mime/MultipartEntityBuilder.java:
##########
@@ -253,12 +253,13 @@ MultipartFormEntity buildEntity() {
if (charsetCopy == null && contentType != null) {
charsetCopy = contentType.getCharset();
}
- final List<NameValuePair> paramsList = new ArrayList<>(2);
- paramsList.add(new BasicNameValuePair("boundary", boundaryCopy));
- if (charsetCopy != null) {
- paramsList.add(new BasicNameValuePair("charset", charsetCopy.name()));
- }
- final NameValuePair[] params = paramsList.toArray(EMPTY_NAME_VALUE_ARRAY);
+ // Previusly, "charset" parameter was added to the Content-Type header, however adding "charset=..."
+ // is not specified in RFC 7578, and it causes issues with (flawed?) HTTP servers.
+ // For instance:
+ // https://github.com/owasp-modsecurity/ModSecurity/commit/6e56950cdf258c9b39f12cf6eb014cb59797cfd3
+ // https://github.com/akka/akka-http/issues/338
+ // https://bz.apache.org/bugzilla/show_bug.cgi?id=61384
+ final NameValuePair[] params = new NameValuePair[]{new BasicNameValuePair("boundary", boundaryCopy)};
Review Comment:
Agreed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2325 Avoid adding "; charset=" for multipart/form-data requests [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c merged PR #556:
URL: https://github.com/apache/httpcomponents-client/pull/556
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2325 Avoid adding "; charset=" for multipart/form-data requests [httpcomponents-client]
Posted by "vlsi (via GitHub)" <gi...@apache.org>.
vlsi commented on code in PR #556:
URL: https://github.com/apache/httpcomponents-client/pull/556#discussion_r1528370944
##########
httpclient5/src/main/java/org/apache/hc/client5/http/entity/mime/MultipartEntityBuilder.java:
##########
@@ -253,12 +253,13 @@ MultipartFormEntity buildEntity() {
if (charsetCopy == null && contentType != null) {
charsetCopy = contentType.getCharset();
}
- final List<NameValuePair> paramsList = new ArrayList<>(2);
- paramsList.add(new BasicNameValuePair("boundary", boundaryCopy));
- if (charsetCopy != null) {
- paramsList.add(new BasicNameValuePair("charset", charsetCopy.name()));
- }
- final NameValuePair[] params = paramsList.toArray(EMPTY_NAME_VALUE_ARRAY);
+ // Previusly, "charset" parameter was added to the Content-Type header, however adding "charset=..."
+ // is not specified in RFC 7578, and it causes issues with (flawed?) HTTP servers.
+ // For instance:
+ // https://github.com/owasp-modsecurity/ModSecurity/commit/6e56950cdf258c9b39f12cf6eb014cb59797cfd3
+ // https://github.com/akka/akka-http/issues/338
+ // https://bz.apache.org/bugzilla/show_bug.cgi?id=61384
+ final NameValuePair[] params = new NameValuePair[]{new BasicNameValuePair("boundary", boundaryCopy)};
Review Comment:
I can easily imagine somebody could try re-adding `charset` property here. It would be insufficient to have the comment **only** in the commit message as the person who edits the file will not see "somebody did remove charset from here previously".
---
Exactly the same story in JMeter:
1) `multipartEntityBuilder.setContent(...)` removed in 2017 in https://bz.apache.org/bugzilla/show_bug.cgi?id=61384#c15
2) `multipartEntityBuilder.setContent(...)` added in 2023 in https://github.com/apache/jmeter/pull/5987
---
If you still think the code comment should be removed, I can easily do that. It would make the maintenance harder though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2325 Avoid adding "; charset=" for multipart/form-data requests [httpcomponents-client]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #556:
URL: https://github.com/apache/httpcomponents-client/pull/556#discussion_r1528366507
##########
httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartEntityBuilder.java:
##########
@@ -88,7 +88,7 @@ public void testMultipartCustomContentType() throws Exception {
.setLaxMode()
.buildEntity();
Assertions.assertNotNull(entity);
- Assertions.assertEquals("application/xml; boundary=blah-blah; charset=UTF-8", entity.getContentType());
+ Assertions.assertEquals("application/xml; charset=UTF-8; boundary=blah-blah", entity.getContentType());
Review Comment:
> Please clarify what do you mean here. I kept the test to verify the behaviour for the code that did use weird content types for multipart previously.
This remains as-is. Just a side comment. No change required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org