[
https://issues.apache.org/jira/browse/CAMEL-24524?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Work on CAMEL-24524 started by Claus Ibsen.
-------------------------------------------
> camel-core: URISupport.normalizeUri fast path skips value encoding when
> parameters are already sorted, causing duplicate endpoints
> ----------------------------------------------------------------------------------------------------------------------------------
>
> Key: CAMEL-24524
> URL: https://issues.apache.org/jira/browse/CAMEL-24524
> Project: Camel
> Issue Type: Bug
> Reporter: Claus Ibsen
> Assignee: Claus Ibsen
> Priority: Major
>
> h3. Summary
> {{URISupport.normalizeUri()}} is used to compute the registry/cache key for
> Camel endpoints, and is documented to reorder query parameters alphabetically
> so that two endpoint URIs differing only in parameter order are treated as
> identical. This holds for most URIs, but breaks whenever a parameter value
> contains a character that requires URL-encoding (e.g. a colon in a
> {{host:port}} value). In that case, the *original* parameter order silently
> affects the normalized output, so two logically identical URIs can normalize
> to two different strings - causing duplicate endpoints (and duplicate
> producers/consumers/connections) to be created silently, with no error or
> warning.
> Originally reported/analyzed here:
> https://tushar-c23.github.io/posts/camel-uri-normalization/
> h3. Reproduction
> {code:java}
> String uri1 = "kafka:mytopic?brokers=localhost:19092&groupId=mygroup"; //
> already alphabetical (brokers < groupId)
> String uri2 = "kafka:mytopic?groupId=mygroup&brokers=localhost:19092"; //
> needs reordering
> System.out.println(URISupport.normalizeUri(uri1));
> System.out.println(URISupport.normalizeUri(uri2));
> {code}
> Actual output (verified on {{main}}, camel-util 4.22.1-SNAPSHOT):
> {noformat}
> kafka://mytopic?brokers=localhost:19092&groupId=mygroup
> kafka://mytopic?brokers=localhost%3A19092&groupId=mygroup
> {noformat}
> The two normalized strings differ (raw {{:}} vs {{%3A}}), even though the
> input URIs are semantically identical. Any code that keys a map/registry by
> the normalized URI (e.g. the endpoint cache) will treat these as two
> different endpoints.
> h3. Root cause
> {{URISupport.normalizeUri()}} has two code paths (see
> {{core/camel-util/src/main/java/org/apache/camel/util/URISupport.java}}):
> * *Fast path* - {{doFastNormalizeUri()}} -> {{buildReorderingParameters()}}
> (~line 799). This path only rebuilds the query string via
> {{createQueryString(array, parameters, true)}} - the *only* place encoding
> (via {{URLEncoder.encode}}) is applied - when it detects that the parameter
> keys are *not* already in alphabetical order:
> {code:java}
> boolean sort = false;
> String prev = null;
> for (String key : entries) {
> if (prev != null) {
> int comp = key.compareTo(prev);
> if (comp < 0) {
> sort = true;
> break;
> }
> }
> prev = key;
> }
> if (sort) {
> // only place where createQueryString(...) / encoding happens
> query = URISupport.createQueryString(array, parameters, true);
> }
> {code}
> If the keys are already sorted, or the query has a single parameter (no
> {{&}}), the original raw query string is returned completely untouched - i.e.
> *not encoded at all*.
> * *Complex/legacy path* - {{doComplexNormalizeUri()}} (used when the URI
> contains characters outside the fast parser's "safe" set). This path *always*
> calls {{createQueryString(...)}} regardless of whether reordering was needed,
> so it always produces a consistently encoded, canonical result. This path is
> *not* affected by this bug.
> Because a colon is considered "safe" by
> {{UnsafeUriCharactersEncoder.isSafeFastParser()}} (so URIs containing it
> still take the fast path), but {{URLEncoder.encode()}} (used inside
> {{createQueryString}}) *does* encode colons, the fast path's output for the
> same logical URI depends on whether the original parameter order happened to
> require a rebuild.
> This was introduced as a performance optimization in CAMEL-14648 (2020) - the
> intent was purely to avoid rebuilding the query string when reordering wasn't
> necessary; encoding consistency was not part of the design consideration at
> the time. It is not a deliberate behavioral decision.
> Checked for duplicates: CAMEL-24187 (already fixed, released in
> 4.22.0/4.18.4) covers a related but distinct bug - non-idempotent
> double-normalization of {{%}} characters. It does not cover this
> parameter-order/encoding inconsistency.
> h3. Impact
> * Silent duplicate {{Endpoint}} instances (and therefore duplicate
> producers/consumers, connections, threads) for logically identical URIs, with
> no exception or log warning.
> * Reported real-world impact: a Kafka producer leak where varying parameter
> order (due to a colon in the broker address) caused multiple
> {{KafkaProducer}} instances to be created for what should have been a single
> shared endpoint.
> * Affects any component/DSL usage where endpoint URIs are hand-assembled with
> parameters in non-canonical order and at least one value requires encoding
> (colons, spaces, etc.). Endpoints built via the fluent Java DSL (which stores
> parameters in a {{TreeMap}}, always alphabetical) are not affected in
> practice, but this is incidental, not guaranteed by contract.
> h3. Suggested fix direction
> Align the fast path's behavior with the complex path's guarantee that the
> result is always canonically encoded, without regressing the performance win
> from CAMEL-14648 for the common case:
> * When scanning parameter keys to decide whether a re-sort is needed, also
> detect whether any value requires encoding (e.g. by comparing each value to
> its encoded form, or checking against the same "unsafe" character set used
> elsewhere). If either condition is true, rebuild via
> {{createQueryString(...)}}.
> * Alternatively (simpler, slightly more conservative on performance): always
> rebuild the query via {{createQueryString(...)}} in
> {{buildReorderingParameters()}}, only keeping the sort-detection to choose
> whether the keys array itself needs sorting first. This removes the "skip
> rebuild" fast-path micro-optimization but keeps the larger win of using the
> lightweight {{CamelURIParser}} instead of {{java.net.URI}}.
> * Also fix the single-parameter short-circuit (query with no {{&}}) which
> currently returns the raw, unencoded value verbatim - for consistency with
> the complex path, which always encodes single-parameter queries too.
> A regression test should assert that {{URISupport.normalizeUri()}} produces
> the *same* output for two URIs that differ only in parameter order, for
> values containing characters requiring encoding (colon, space, etc.),
> including the single-parameter case.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
