Ganesh Gautam created CODEC-345:
-----------------------------------
Summary: URLCodec.encodeUrl(BitSet, byte[]) ignores the
caller-supplied BitSet for % and +
Key: CODEC-345
URL: https://issues.apache.org/jira/browse/CODEC-345
Project: Commons Codec
Issue Type: Bug
Affects Versions: 1.22.1
Reporter: Ganesh Gautam
{{URLCodec.encodeUrl(BitSet urlsafe, byte[] bytes)}} is public API and is
widely used as a general-purpose percent-encoder, with the caller passing the
character set appropriate to the URI component being encoded. The javadoc
describes the parameter as {_}"bitset of characters deemed URL safe"{_}.
CODEC-339 changed the method to percent-escape {{%}} and {{+}} unconditionally,
even when the caller has explicitly marked them safe:
{code:java}
- if (urlsafe.get(b)) {
+ if (urlsafe.get(b) && b != ESCAPE_CHAR && b != PLUS_CHAR) {
{code}
For callers that use the {{www-form-urlencoded}} default set this is a no-op.
For callers that supply their own set, which is the only reason the overload
exists, it now silently changes the output:
* a legal literal {{+}} in a URI path or query becomes {{{}%2B{}}}, changing
the meaning of the URI;
* an already-percent-encoded input is {*}double-encoded{*}, because {{%}}
becomes {{{}%25{}}}. Encoders of this shape are routinely applied to whole URLs
that may already contain valid escapes, so {{Caf%C3%A9}} becomes
{{{}Caf%25C3%25A9{}}}.
The same reasoning applies to {{{}PercentCodec{}}}, where the {{plusForSpace}}
constructor now adds {{+}} to {{{}alwaysEncodeChars{}}}, so a literal {{+}} in
the input is no longer round-trippable.
We appreciate that the intent was to make {{encodeUrl}} or {{decodeUrl}}
round-trip safely. Our point is narrower: that guarantee is only meaningful for
the codec's _own_ {{WWW_FORM_URL}} alphabet, and enforcing it inside the BitSet
overload removes the caller's ability to encode a URI component correctly.
h3. Reproduction
Self-contained and no dependency beyond commons-codec itself. Full source:
{code:java}
import java.util.BitSet;
import org.apache.commons.codec.net.URLCodec;
public class Repro {
public static void main(String[] args) throws Exception {
// RFC 2396 abs_path, i.e. what HtmlUnit's UrlUtils and HttpClient
3.x's URI build.
BitSet allowed = new BitSet(256);
for (int c = 'a'; c <= 'z'; c++) allowed.set(c);
for (int c = 'A'; c <= 'Z'; c++) allowed.set(c);
for (int c = '0'; c <= '9'; c++) allowed.set(c);
for (char c : "-_.!~*'()".toCharArray()) allowed.set(c); // mark
allowed.set('%'); // escaped
for (char c : ":@&=+$,;/".toCharArray()) allowed.set(c); // pchar
for (String s : new String[] {"/pages/1/Test+Page",
"/display/TST/Caf%C3%A9"}) {
System.out.println(s + " -> "
+ new String(URLCodec.encodeUrl(allowed, s.getBytes("UTF-8")),
"US-ASCII"));
}
}
}
{code}
----
{code:java}
1.22.0 (expected — the BitSet is honoured):
```
/pages/1/Test+Page -> /pages/1/Test+Page
/display/TST/Caf%C3%A9 -> /display/TST/Caf%C3%A9
```
1.22.1 (actual):
```
/pages/1/Test+Page -> /pages/1/Test%2BPage
/display/TST/Caf%C3%A9 -> /display/TST/Caf%25C3%25A9
{code}
----
h3. Downstream impact
Two libraries were found that call this overload with their own BitSet:
* *HtmlUnit* — {{org.htmlunit.util.UrlUtils}} /
{{com.gargoylesoftware.htmlunit.util.UrlUtils}} (present in 2.15 through
3.11.0). {{UrlUtils.encode}} passes {{PATH_ALLOWED_CHARS}} /
{{QUERY_ALLOWED_CHARS}} / {{{}ANCHOR_ALLOWED_CHARS{}}}, all of which include
{{%}} and {{+}} by construction, and {{WebClient}} runs every request URL —
including every redirect target through {{{}UrlUtils.encodeUrl{}}}.
* *Commons HttpClient 3.x* — {{org.apache.commons.httpclient.URI}} and
{{{}org.apache.commons.httpclient.util.URIUtil{}}}, same pattern.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
