[
https://issues.apache.org/jira/browse/CODEC-345?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Gary D. Gregory resolved CODEC-345.
-----------------------------------
Fix Version/s: 1.23.0
Resolution: Fixed
> 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
> Assignee: Gary D. Gregory
> Priority: Major
> Fix For: 1.23.0
>
>
> {{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)
