You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by "arturobernalg (via GitHub)" <gi...@apache.org> on 2025/03/11 15:56:06 UTC
[PR] Fix percent-encoding in fragment identifiers per RFC 3986 [httpcomponents-core]
arturobernalg opened a new pull request, #517:
URL: https://github.com/apache/httpcomponents-core/pull/517
This pull request addresses issue [HTTPCORE-778](https://issues.apache.org/jira/browse/HTTPCORE-778?page=com.atlassian.jira.plugin.system.issuetabpanels%3Aall-tabpanel), which noted that our encoding for fragment identifiers in selectors was not fully compliant with RFC 3986. The RFC defines a fragment as:
`fragment = *( pchar / "/" / "?" )`
with:
`pchar = unreserved / pct-encoded / sub-delims / ":" / "@"`
--
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] Fix percent-encoding in fragment identifiers per RFC 3986 [httpcomponents-core]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #517:
URL: https://github.com/apache/httpcomponents-core/pull/517#discussion_r1996675961
##########
httpcore5/src/main/java/org/apache/hc/core5/net/PercentCodec.java:
##########
@@ -113,6 +113,37 @@ public class PercentCodec {
RFC5987_UNRESERVED.set('~');
}
+ static final BitSet PCHAR = new BitSet(256);
+ static final BitSet USERINFO = new BitSet(256);
+ static final BitSet REG_NAME = new BitSet(256);
+ static final BitSet PATH_SEGMENT = new BitSet(256);
+ static final BitSet QUERY = new BitSet(256);
+ static final BitSet FRAGMENT = new BitSet(256);
+
+ static {
+ PCHAR.or(UNRESERVED);
+ PCHAR.or(SUB_DELIMS);
+ PCHAR.set(':');
+ PCHAR.set('@');
+ USERINFO.or(UNRESERVED);
+ USERINFO.or(SUB_DELIMS);
+ USERINFO.set(':');
+ REG_NAME.or(UNRESERVED);
+ REG_NAME.or(SUB_DELIMS);
+ REG_NAME.clear('!');
+ PATH_SEGMENT.or(PCHAR);
+ QUERY.or(PCHAR);
+ QUERY.set('/');
+ QUERY.set('?');
+ FRAGMENT.or(PCHAR);
+ FRAGMENT.set('/');
+ FRAGMENT.set('?');
+ // Some sub-delims remain encoded (RFC 3986 allows them unencoded, but we choose to be strict).
+ PATH_SEGMENT.clear('(');
+ PATH_SEGMENT.clear(')');
+ PATH_SEGMENT.clear('&');
+ }
Review Comment:
@arturobernalg You have overlooked static `formatQuery` and `formatPath` methods. They should also take the encoding policy into account. Apply this patch and everything should work
```
===================================================================
diff --git a/httpcore5/src/main/java/org/apache/hc/core5/net/PercentCodec.java b/httpcore5/src/main/java/org/apache/hc/core5/net/PercentCodec.java
--- a/httpcore5/src/main/java/org/apache/hc/core5/net/PercentCodec.java (revision 53477ecc93d331f8348a6748a918d86b25666471)
+++ b/httpcore5/src/main/java/org/apache/hc/core5/net/PercentCodec.java (date 1742025384945)
@@ -137,10 +137,6 @@
FRAGMENT.or(PCHAR);
FRAGMENT.set('/');
FRAGMENT.set('?');
- // Some sub-delims remain encoded (RFC 3986 allows them unencoded, but we choose to be strict).
- PATH_SEGMENT.clear('(');
- PATH_SEGMENT.clear(')');
- PATH_SEGMENT.clear('&');
}
private static final int RADIX = 16;
Index: httpcore5/src/main/java/org/apache/hc/core5/net/URIBuilder.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/httpcore5/src/main/java/org/apache/hc/core5/net/URIBuilder.java b/httpcore5/src/main/java/org/apache/hc/core5/net/URIBuilder.java
--- a/httpcore5/src/main/java/org/apache/hc/core5/net/URIBuilder.java (revision 53477ecc93d331f8348a6748a918d86b25666471)
+++ b/httpcore5/src/main/java/org/apache/hc/core5/net/URIBuilder.java (date 1742026397263)
@@ -34,6 +34,7 @@
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.Arrays;
+import java.util.BitSet;
import java.util.Collections;
import java.util.LinkedList;
import java.util.List;
@@ -343,19 +344,25 @@
return list;
}
- static void formatPath(final StringBuilder buf, final Iterable<String> segments, final boolean rootless, final Charset charset) {
+ static void formatPath(final StringBuilder buf, final Iterable<String> segments, final boolean rootless,
+ final Charset charset, final BitSet safechars) {
int i = 0;
for (final String segment : segments) {
if (i > 0 || !rootless) {
buf.append(PATH_SEPARATOR);
}
- PercentCodec.encode(buf, segment, charset, PercentCodec.PATH_SEGMENT, false);
+ PercentCodec.encode(buf, segment, charset, safechars, false);
i++;
}
}
- static void formatQuery(final StringBuilder buf, final Iterable<? extends NameValuePair> params, final Charset charset,
- final boolean blankAsPlus) {
+ static void formatPath(final StringBuilder buf, final Iterable<String> segments, final boolean rootless,
+ final Charset charset) {
+ formatPath(buf, segments, rootless, charset, PercentCodec.UNRESERVED);
+ }
+
+ static void formatQuery(final StringBuilder buf, final Iterable<? extends NameValuePair> params,
+ final Charset charset, final BitSet safechars, final boolean blankAsPlus) {
int i = 0;
for (final NameValuePair parameter : params) {
if (i > 0) {
@@ -364,12 +371,17 @@
PercentCodec.encode(buf, parameter.getName(), charset, blankAsPlus);
if (parameter.getValue() != null) {
buf.append(PARAM_VALUE_SEPARATOR);
- PercentCodec.encode(buf, parameter.getValue(), charset, blankAsPlus);
+ PercentCodec.encode(buf, parameter.getValue(), charset, safechars, blankAsPlus);
}
i++;
}
}
+ static void formatQuery(final StringBuilder buf, final Iterable<? extends NameValuePair> params,
+ final Charset charset, final boolean blankAsPlus) {
+ formatQuery(buf, params, charset, PercentCodec.UNRESERVED, blankAsPlus);
+ }
+
/**
* Builds a {@link URI} instance.
*/
@@ -429,13 +441,15 @@
}
sb.append(this.encodedPath);
} else if (this.pathSegments != null) {
- formatPath(sb, this.pathSegments, !authoritySpecified && this.pathRootless, this.charset);
+ formatPath(sb, this.pathSegments, !authoritySpecified && this.pathRootless, this.charset,
+ encodingPolicy == EncodingPolicy.ALL_RESERVED ? PercentCodec.UNRESERVED : PercentCodec.PATH_SEGMENT);
}
if (this.encodedQuery != null) {
sb.append("?").append(this.encodedQuery);
} else if (this.queryParams != null && !this.queryParams.isEmpty()) {
sb.append("?");
- formatQuery(sb, this.queryParams, this.charset, false);
+ formatQuery(sb, this.queryParams, this.charset,
+ encodingPolicy == EncodingPolicy.ALL_RESERVED ? PercentCodec.UNRESERVED : PercentCodec.QUERY, false);
} else if (this.query != null) {
sb.append("?");
PercentCodec.encode(sb, this.query, this.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] Fix percent-encoding in fragment identifiers per RFC 3986 [httpcomponents-core]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #517:
URL: https://github.com/apache/httpcomponents-core/pull/517#issuecomment-2719114105
> @arturobernalg What about `userinfo`, `reg-name`, path `segment?`
@ok2c here's a consolidated fix ensuring consistent RFC 3986–compliant encoding for userinfo, reg-name, path segments, query, and fragment.
--
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] Fix percent-encoding in fragment identifiers per RFC 3986 [httpcomponents-core]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #517:
URL: https://github.com/apache/httpcomponents-core/pull/517#discussion_r1994218502
##########
httpcore5/src/main/java/org/apache/hc/core5/net/PercentCodec.java:
##########
@@ -84,6 +84,16 @@ public class PercentCodec {
URIC.or(UNRESERVED);
}
+ static final BitSet FRAGMENT_SAFE = new BitSet(256);
Review Comment:
@arturobernalg `FRAGMENT_SAFE` does not seem to be used anymore.
--
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] Fix percent-encoding in fragment identifiers per RFC 3986 [httpcomponents-core]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #517:
URL: https://github.com/apache/httpcomponents-core/pull/517#discussion_r1995792098
##########
httpcore5/src/main/java/org/apache/hc/core5/net/URIBuilder.java:
##########
@@ -391,14 +438,16 @@ private String buildString() {
formatQuery(sb, this.queryParams, this.charset, false);
} else if (this.query != null) {
sb.append("?");
- PercentCodec.encode(sb, this.query, this.charset, PercentCodec.URIC, false);
+ PercentCodec.encode(sb, this.query, this.charset,
+ encodingPolicy == EncodingPolicy.ALL_RESERVED ? PercentCodec.URIC : PercentCodec.QUERY, false);
}
}
if (this.encodedFragment != null) {
sb.append("#").append(this.encodedFragment);
} else if (this.fragment != null) {
sb.append("#");
- PercentCodec.encode(sb, this.fragment, this.charset, PercentCodec.URIC, false);
+ PercentCodec.encode(sb, this.fragment, this.charset,
+ encodingPolicy == EncodingPolicy.ALL_RESERVED ? PercentCodec.UNRESERVED : PercentCodec.FRAGMENT, false);
Review Comment:
done
--
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] Fix percent-encoding in fragment identifiers per RFC 3986 [httpcomponents-core]
Posted by "peterhalicky (via GitHub)" <gi...@apache.org>.
peterhalicky commented on PR #517:
URL: https://github.com/apache/httpcomponents-core/pull/517#issuecomment-2724567181
> The same could be done for the fragment component.
The fragment component often contains all kinds of content. Sometimes it is `application/x-www-form-urlencoded`, other times it looks like a path, i.e. `/` separated path segments. Representing the fragment component as `List<NameValuePair>` will only support the former, but not the latter.
A possible solution could be to let the client build the fragment by parts, specifying how much encoding should be done for each part. Something like `.appendFragment("&", URIC)` and `.appendFragment("valuewith&", RESERVED)`.
--
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] Fix percent-encoding in fragment identifiers per RFC 3986 [httpcomponents-core]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #517:
URL: https://github.com/apache/httpcomponents-core/pull/517#discussion_r1994218502
##########
httpcore5/src/main/java/org/apache/hc/core5/net/PercentCodec.java:
##########
@@ -84,6 +84,16 @@ public class PercentCodec {
URIC.or(UNRESERVED);
}
+ static final BitSet FRAGMENT_SAFE = new BitSet(256);
Review Comment:
@arturobernalg `FRAGMENT_SAFE` does not seem to be used anymore. As well as `URIC`. They should probably be removed.
--
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] Fix percent-encoding in fragment identifiers per RFC 3986 [httpcomponents-core]
Posted by "peterhalicky (via GitHub)" <gi...@apache.org>.
peterhalicky commented on PR #517:
URL: https://github.com/apache/httpcomponents-core/pull/517#issuecomment-2724491903
The initial motivation for HTTPCORE-778 was the need to put some application/x-www-form-urlencoded formatted values into the fragment part of a URI. The format is pretty much the same as what is used in the query component of the URI, i.e. contains one or more key=value assignments separated by '&'.
With the above changes, if I read the diff correctly, we'll be able to use '&' as an unencoded separator. However, the keys/values (data) should be fully encoded as they can possibly contain the '&' character, and there likely isn't a way to create such URI -- either it will encode all the separators, or it won't any.
I understand that this probably opens a can of worms as it likely would require functionality similar to what is available for query parameters, but unlike query parameters in case of fragment this is only one of many ways how fragments are used.
--
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] Fix percent-encoding in fragment identifiers per RFC 3986 [httpcomponents-core]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #517:
URL: https://github.com/apache/httpcomponents-core/pull/517#discussion_r1995800177
##########
httpcore5/src/main/java/org/apache/hc/core5/net/PercentCodec.java:
##########
@@ -113,6 +113,37 @@ public class PercentCodec {
RFC5987_UNRESERVED.set('~');
}
+ static final BitSet PCHAR = new BitSet(256);
+ static final BitSet USERINFO = new BitSet(256);
+ static final BitSet REG_NAME = new BitSet(256);
+ static final BitSet PATH_SEGMENT = new BitSet(256);
+ static final BitSet QUERY = new BitSet(256);
+ static final BitSet FRAGMENT = new BitSet(256);
+
+ static {
+ PCHAR.or(UNRESERVED);
+ PCHAR.or(SUB_DELIMS);
+ PCHAR.set(':');
+ PCHAR.set('@');
+ USERINFO.or(UNRESERVED);
+ USERINFO.or(SUB_DELIMS);
+ USERINFO.set(':');
+ REG_NAME.or(UNRESERVED);
+ REG_NAME.or(SUB_DELIMS);
+ REG_NAME.clear('!');
+ PATH_SEGMENT.or(PCHAR);
+ QUERY.or(PCHAR);
+ QUERY.set('/');
+ QUERY.set('?');
+ FRAGMENT.or(PCHAR);
+ FRAGMENT.set('/');
+ FRAGMENT.set('?');
+ // Some sub-delims remain encoded (RFC 3986 allows them unencoded, but we choose to be strict).
+ PATH_SEGMENT.clear('(');
+ PATH_SEGMENT.clear(')');
+ PATH_SEGMENT.clear('&');
+ }
Review Comment:
@arturobernalg I cannot find anything in the RFC stating that. What am I missing? What a super strict mode we have `ALL_RESERVED`
##########
httpcore5/src/main/java/org/apache/hc/core5/net/PercentCodec.java:
##########
@@ -113,6 +113,37 @@ public class PercentCodec {
RFC5987_UNRESERVED.set('~');
}
+ static final BitSet PCHAR = new BitSet(256);
+ static final BitSet USERINFO = new BitSet(256);
+ static final BitSet REG_NAME = new BitSet(256);
+ static final BitSet PATH_SEGMENT = new BitSet(256);
+ static final BitSet QUERY = new BitSet(256);
+ static final BitSet FRAGMENT = new BitSet(256);
+
+ static {
+ PCHAR.or(UNRESERVED);
+ PCHAR.or(SUB_DELIMS);
+ PCHAR.set(':');
+ PCHAR.set('@');
+ USERINFO.or(UNRESERVED);
+ USERINFO.or(SUB_DELIMS);
+ USERINFO.set(':');
+ REG_NAME.or(UNRESERVED);
+ REG_NAME.or(SUB_DELIMS);
+ REG_NAME.clear('!');
Review Comment:
@arturobernalg Why clearing '!'? The reg-name is defimed as `*( unreserved / pct-encoded / sub-delims )`. The '!' should be permitted or am I missing something?
--
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] RFC 3986 conformant URI encoding policy [httpcomponents-core]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #517:
URL: https://github.com/apache/httpcomponents-core/pull/517#discussion_r1996872461
##########
httpcore5/src/test/java/org/apache/hc/core5/net/TestURIBuilder.java:
##########
@@ -472,6 +472,7 @@ void testFragmentEncoding() throws Exception {
.setScheme("https")
.setHost("somehost.com")
.setFragment("some fragment with all sorts of $tuff in it!!!")
+ .setEncodingPolicy(URIBuilder.EncodingPolicy.RFC_3986)
Review Comment:
@arturobernalg I think the test should also pass with `ALL_RESERVED` default
##########
httpcore5/src/test/java/org/apache/hc/core5/net/TestURIBuilder.java:
##########
@@ -617,7 +618,7 @@ void testHttpHost() throws Exception {
@Test
void testSetHostWithReservedChars() throws Exception {
- final URIBuilder uribuilder = new URIBuilder();
+ final URIBuilder uribuilder = new URIBuilder().setEncodingPolicy(URIBuilder.EncodingPolicy.ALL_RESERVED);
Review Comment:
@arturobernalg I think this should no longer be necessary. `ALL_RESERVED` is default now.
##########
httpcore5/src/test/java/org/apache/hc/core5/net/TestURIBuilder.java:
##########
@@ -171,7 +171,7 @@ void testFormatQuery() {
@Test
void testHierarchicalUri() throws Exception {
final URI uri = new URI("http", "stuff", "localhost", 80, "/some stuff", "param=stuff", "fragment");
- final URIBuilder uribuilder = new URIBuilder(uri);
+ final URIBuilder uribuilder = new URIBuilder(uri).setEncodingPolicy(URIBuilder.EncodingPolicy.ALL_RESERVED);
Review Comment:
@arturobernalg I think this should no longer be necessary. `ALL_RESERVED` is default now.
--
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] RFC 3986 conformant URI encoding policy [httpcomponents-core]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg merged PR #517:
URL: https://github.com/apache/httpcomponents-core/pull/517
--
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] Fix percent-encoding in fragment identifiers per RFC 3986 [httpcomponents-core]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #517:
URL: https://github.com/apache/httpcomponents-core/pull/517#discussion_r1995524429
##########
httpcore5/src/main/java/org/apache/hc/core5/net/URIBuilder.java:
##########
@@ -87,9 +87,36 @@ public static URIBuilder loopbackAddress() {
private Charset charset;
private String fragment;
private String encodedFragment;
+ private EncodingPolicy encodingPolicy = EncodingPolicy.RFC_3986;
Review Comment:
@arturobernalg Should not it be `EncodingPolicy.ALL_RESERVED` by default?
--
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] Fix percent-encoding in fragment identifiers per RFC 3986 [httpcomponents-core]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #517:
URL: https://github.com/apache/httpcomponents-core/pull/517#issuecomment-2717929048
@arturobernalg What about `userinfo`, `reg-name`, path `segment?`
--
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] Fix percent-encoding in fragment identifiers per RFC 3986 [httpcomponents-core]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #517:
URL: https://github.com/apache/httpcomponents-core/pull/517#issuecomment-2717596664
@arturobernalg It makes no sense to change formatting of the fragment component and ignore all other components.
--
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] Fix percent-encoding in fragment identifiers per RFC 3986 [httpcomponents-core]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #517:
URL: https://github.com/apache/httpcomponents-core/pull/517#issuecomment-2724515417
@peterhalicky no matter how hard we try we just cannot make everyone happy, can we?
The only way of accommodating your request would be to add support for `List<NameValuePair> fragmentParams` and make them work the same way as the query parameters.
--
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] Fix percent-encoding in fragment identifiers per RFC 3986 [httpcomponents-core]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #517:
URL: https://github.com/apache/httpcomponents-core/pull/517#discussion_r1995772108
##########
httpcore5/src/main/java/org/apache/hc/core5/net/URIBuilder.java:
##########
@@ -391,14 +438,16 @@ private String buildString() {
formatQuery(sb, this.queryParams, this.charset, false);
} else if (this.query != null) {
sb.append("?");
- PercentCodec.encode(sb, this.query, this.charset, PercentCodec.URIC, false);
+ PercentCodec.encode(sb, this.query, this.charset,
+ encodingPolicy == EncodingPolicy.ALL_RESERVED ? PercentCodec.URIC : PercentCodec.QUERY, false);
}
}
if (this.encodedFragment != null) {
sb.append("#").append(this.encodedFragment);
} else if (this.fragment != null) {
sb.append("#");
- PercentCodec.encode(sb, this.fragment, this.charset, PercentCodec.URIC, false);
+ PercentCodec.encode(sb, this.fragment, this.charset,
+ encodingPolicy == EncodingPolicy.ALL_RESERVED ? PercentCodec.UNRESERVED : PercentCodec.FRAGMENT, false);
Review Comment:
@arturobernalg We should use `URIC` here for consistency with the latest changes in 5.3.x
--
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] Fix percent-encoding in fragment identifiers per RFC 3986 [httpcomponents-core]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #517:
URL: https://github.com/apache/httpcomponents-core/pull/517#discussion_r1995090045
##########
httpcore5/src/main/java/org/apache/hc/core5/net/PercentCodec.java:
##########
@@ -84,6 +84,16 @@ public class PercentCodec {
URIC.or(UNRESERVED);
}
+ static final BitSet FRAGMENT_SAFE = new BitSet(256);
Review Comment:
Removed
--
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] Fix percent-encoding in fragment identifiers per RFC 3986 [httpcomponents-core]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #517:
URL: https://github.com/apache/httpcomponents-core/pull/517#issuecomment-2724547404
> I just wanted to point out that this is still a restriction that will not allow users to create all possible URIs that would otherwise be legal as per the RFC
@peterhalicky The whole point of representing the query component as `List<NameValuePair>` is make it possible for name/value pairs to contain arbitrary characters. The same could be done for the fragment component.
--
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] RFC 3986 conformant URI encoding policy [httpcomponents-core]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #517:
URL: https://github.com/apache/httpcomponents-core/pull/517#discussion_r1997284724
##########
httpcore5/src/test/java/org/apache/hc/core5/net/TestURIBuilder.java:
##########
@@ -617,7 +618,7 @@ void testHttpHost() throws Exception {
@Test
void testSetHostWithReservedChars() throws Exception {
- final URIBuilder uribuilder = new URIBuilder();
+ final URIBuilder uribuilder = new URIBuilder().setEncodingPolicy(URIBuilder.EncodingPolicy.ALL_RESERVED);
Review Comment:
done
##########
httpcore5/src/test/java/org/apache/hc/core5/net/TestURIBuilder.java:
##########
@@ -171,7 +171,7 @@ void testFormatQuery() {
@Test
void testHierarchicalUri() throws Exception {
final URI uri = new URI("http", "stuff", "localhost", 80, "/some stuff", "param=stuff", "fragment");
- final URIBuilder uribuilder = new URIBuilder(uri);
+ final URIBuilder uribuilder = new URIBuilder(uri).setEncodingPolicy(URIBuilder.EncodingPolicy.ALL_RESERVED);
Review Comment:
done
##########
httpcore5/src/main/java/org/apache/hc/core5/net/URIBuilder.java:
##########
@@ -175,6 +203,22 @@ public URIBuilder setCharset(final Charset charset) {
return this;
}
+ /**
+ * Sets the encoding policy for this {@link URIBuilder}.
+ * The encoding policy determines how URI components (e.g., query, fragment) are
+ * percent-encoded when building the URI string. If not set, the default policy
+ * is {@link EncodingPolicy#RFC_3986}.
+ *
+ * @param encodingPolicy the encoding policy to apply, or {@code null} to reset
+ * to the default ({@link EncodingPolicy#RFC_3986})
Review Comment:
corrected
--
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] Fix percent-encoding in fragment identifiers per RFC 3986 [httpcomponents-core]
Posted by "peterhalicky (via GitHub)" <gi...@apache.org>.
peterhalicky commented on PR #517:
URL: https://github.com/apache/httpcomponents-core/pull/517#issuecomment-2724525778
> @peterhalicky no matter how hard we try we just cannot make everyone happy, can we?
>
> The only way of accommodating your request would be to add support for `List<NameValuePair> fragmentParams` and make them work the same way as the query parameters.
Yes, I agree, I'd be quite hesitant to implement this myself. For my use case I can get away with restricting the keys and values to not containing `&`. I just wanted to point out that this is still a restriction that will not allow users to create all possible URIs that would otherwise be legal as per the RFC (not sure if the library aims for that or not).
--
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] Fix percent-encoding in fragment identifiers per RFC 3986 [httpcomponents-core]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #517:
URL: https://github.com/apache/httpcomponents-core/pull/517#discussion_r1996253015
##########
httpcore5/src/main/java/org/apache/hc/core5/net/PercentCodec.java:
##########
@@ -113,6 +113,37 @@ public class PercentCodec {
RFC5987_UNRESERVED.set('~');
}
+ static final BitSet PCHAR = new BitSet(256);
+ static final BitSet USERINFO = new BitSet(256);
+ static final BitSet REG_NAME = new BitSet(256);
+ static final BitSet PATH_SEGMENT = new BitSet(256);
+ static final BitSet QUERY = new BitSet(256);
+ static final BitSet FRAGMENT = new BitSet(256);
+
+ static {
+ PCHAR.or(UNRESERVED);
+ PCHAR.or(SUB_DELIMS);
+ PCHAR.set(':');
+ PCHAR.set('@');
+ USERINFO.or(UNRESERVED);
+ USERINFO.or(SUB_DELIMS);
+ USERINFO.set(':');
+ REG_NAME.or(UNRESERVED);
+ REG_NAME.or(SUB_DELIMS);
+ REG_NAME.clear('!');
+ PATH_SEGMENT.or(PCHAR);
+ QUERY.or(PCHAR);
+ QUERY.set('/');
+ QUERY.set('?');
+ FRAGMENT.or(PCHAR);
+ FRAGMENT.set('/');
+ FRAGMENT.set('?');
+ // Some sub-delims remain encoded (RFC 3986 allows them unencoded, but we choose to be strict).
+ PATH_SEGMENT.clear('(');
+ PATH_SEGMENT.clear(')');
+ PATH_SEGMENT.clear('&');
+ }
Review Comment:
> @arturobernalg I cannot find anything in the RFC stating that. What am I missing? What a super strict mode we have `ALL_RESERVED`
@olegk The test expects `(`, `)`, `&` to remain percent-encoded in path segments (e.g., `blah-%28%20-blah-%20%26%20-blah-%20%29-blah/`), but without the `PATH_SEGMENT.clear('(')`, `clear(')')`, `clear('&')` in `PercentCodec.java`, it outputs `blah-(%20-blah-%20&%20-blah-%20)-blah/`. Reintroducing the `clear()` calls fixes it, but I’m wondering if this aligns with our intended behavior or if we should adjust `formatPath` to handle encoding differently based on `encodingPolicy`.
I noticed the older `formatPath` used `PercentCodec.encode(buf, segment, charset, false)` with `UNRESERVED`, which worked, but the new version with `encodingPolicy` support might need tweaking. Could you take a look and advise?
--
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] Fix percent-encoding in fragment identifiers per RFC 3986 [httpcomponents-core]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #517:
URL: https://github.com/apache/httpcomponents-core/pull/517#issuecomment-2723977979
> > @arturobernalg What about `userinfo`, `reg-name`, path `segment?`
>
> @ok2c here's a consolidated fix ensuring consistent RFC 3986–compliant encoding for userinfo, reg-name, path segments, query, and fragment.
@ok2c Done. Please do another pas
--
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] Fix percent-encoding in fragment identifiers per RFC 3986 [httpcomponents-core]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #517:
URL: https://github.com/apache/httpcomponents-core/pull/517#discussion_r1995335981
##########
httpcore5/src/main/java/org/apache/hc/core5/net/URIBuilder.java:
##########
@@ -391,14 +438,16 @@ private String buildString() {
formatQuery(sb, this.queryParams, this.charset, false);
} else if (this.query != null) {
sb.append("?");
- PercentCodec.encode(sb, this.query, this.charset, PercentCodec.URIC, false);
+ PercentCodec.encode(sb, this.query, this.charset,
+ encodingPolicy == EncodingPolicy.ALL_RESERVED ? PercentCodec.UNRESERVED : PercentCodec.FRAGMENT, false);
Review Comment:
@arturobernalg Careful there. Here we are encoding a complete query that may use sub-delims. Even in `ALL_RESERVED` mode it should still be `URIC`.
I suppose this line should be
```
encodingPolicy == EncodingPolicy.ALL_RESERVED ? PercentCodec.URIC : PercentCodec.QUERY, false)
```
##########
httpcore5/src/main/java/org/apache/hc/core5/net/URIBuilder.java:
##########
@@ -356,18 +399,22 @@ private String buildString() {
} else if (this.userInfo != null) {
final int idx = this.userInfo.indexOf(':');
if (idx != -1) {
- PercentCodec.encode(sb, this.userInfo.substring(0, idx), this.charset);
+ PercentCodec.encode(sb, this.userInfo.substring(0, idx), this.charset,
+ encodingPolicy == EncodingPolicy.ALL_RESERVED ? PercentCodec.UNRESERVED : PercentCodec.URIC, false);
sb.append(':');
- PercentCodec.encode(sb, this.userInfo.substring(idx + 1), this.charset);
+ PercentCodec.encode(sb, this.userInfo.substring(idx + 1), this.charset,
+ encodingPolicy == EncodingPolicy.ALL_RESERVED ? PercentCodec.UNRESERVED : PercentCodec.URIC, false);
} else {
- PercentCodec.encode(sb, this.userInfo, this.charset);
+ PercentCodec.encode(sb, this.userInfo, this.charset,
+ encodingPolicy == EncodingPolicy.ALL_RESERVED ? PercentCodec.UNRESERVED : PercentCodec.URIC, false);
}
sb.append("@");
}
if (InetAddressUtils.isIPv6(this.host)) {
sb.append("[").append(this.host).append("]");
} else {
- sb.append(PercentCodec.encode(this.host, this.charset));
+ PercentCodec.encode(sb, this.host, this.charset,
+ encodingPolicy == EncodingPolicy.ALL_RESERVED ? PercentCodec.UNRESERVED : PercentCodec.URIC, false);
Review Comment:
@arturobernalg Should not it be `REG_NAME`, not `URIC`?
##########
httpcore5/src/main/java/org/apache/hc/core5/net/URIBuilder.java:
##########
@@ -356,18 +399,22 @@ private String buildString() {
} else if (this.userInfo != null) {
final int idx = this.userInfo.indexOf(':');
if (idx != -1) {
- PercentCodec.encode(sb, this.userInfo.substring(0, idx), this.charset);
+ PercentCodec.encode(sb, this.userInfo.substring(0, idx), this.charset,
+ encodingPolicy == EncodingPolicy.ALL_RESERVED ? PercentCodec.UNRESERVED : PercentCodec.URIC, false);
Review Comment:
@arturobernalg Should not it be `USERINFO`, not `URIC`? Same below.
--
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] Fix percent-encoding in fragment identifiers per RFC 3986 [httpcomponents-core]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #517:
URL: https://github.com/apache/httpcomponents-core/pull/517#issuecomment-2724577476
@peterhalicky Those folks who need this stuff are welcome to build their own builders.
--
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] Fix percent-encoding in fragment identifiers per RFC 3986 [httpcomponents-core]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #517:
URL: https://github.com/apache/httpcomponents-core/pull/517#issuecomment-2717763161
> @arturobernalg It makes no sense to change formatting of the fragment component and ignore all other components.
@ok2c please take another look.
--
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] RFC 3986 conformant URI encoding policy [httpcomponents-core]
Posted by "PascalSchumacher (via GitHub)" <gi...@apache.org>.
PascalSchumacher commented on code in PR #517:
URL: https://github.com/apache/httpcomponents-core/pull/517#discussion_r1996974475
##########
httpcore5/src/main/java/org/apache/hc/core5/net/URIBuilder.java:
##########
@@ -175,6 +203,22 @@ public URIBuilder setCharset(final Charset charset) {
return this;
}
+ /**
+ * Sets the encoding policy for this {@link URIBuilder}.
+ * The encoding policy determines how URI components (e.g., query, fragment) are
+ * percent-encoded when building the URI string. If not set, the default policy
+ * is {@link EncodingPolicy#RFC_3986}.
+ *
+ * @param encodingPolicy the encoding policy to apply, or {@code null} to reset
+ * to the default ({@link EncodingPolicy#RFC_3986})
Review Comment:
I think this is no longer the case `ALL_RESERVED` seems to be the default now (see above).
--
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