Repository navigation
CAMEL-25705: camel-netty-http - reject an ambiguous request path when a security constraint is configured - #27945
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
This PR fixes a security path traversal vulnerability (CAMEL-25705) in camel-netty-http. The implementation is correct and the test coverage is solid. A few observations:
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 37 of 707 tested, 26 compile-only — current: 37 all testedMaveniverse Scalpel detected 37 affected modules (current approach: 37). Skip-tests mode would test 37 modules (2 direct + 36 downstream), skip tests for 26 (generated code, meta-modules) Modules Scalpel would test (37)
Modules with tests skipped (26)
Build reactor — dependencies compiled but only changed modules were tested (2 modules, 14.0s total)Total reactor time: 14.0s
Top 20 slowest modules:
|
bc40b15 to
9eac35f
Compare
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks @oscerd. The RFC 3986 remove_dot_segments implementation looks correct to me, and decoding through URI.getPath() first means the %2e/%2f variants are covered too.
My concern is about which path gets checked. netty-http does not resolve dot-segments anywhere else: HttpServerMultiplexChannelHandler.getHandler dispatches on the raw request.uri(), and DefaultNettyHttpBinding.stripPath puts the raw path (uri.getRawPath()) into CamelHttpPath. So the route never sees the resolved path. Checking only the resolved form fixes /foo/x/../admin/y, but it opens the opposite case. With the documented constraint (/admin/* restricted, plus a /public/* exclusion) on a matchOnUriPrefix endpoint, /foo/admin/../public/x used to need the admin role. With this change it is treated as public, while the route still gets CamelHttpPath=/admin/../public/x, so a route that branches on that path runs its admin logic unauthenticated.
Could the check be made independent of how the path is interpreted later? Two options:
- Reply 400 to a request whose decoded path changes under
removeDotSegmentswhen a security constraint is configured. HTTP clients normalise dot-segments before sending (curl only skips this with--path-as-is), and Spring's StrictHttpFirewall rejects them the same way. This is the simpler option, and it also answers what to do with a path that resolves outside the endpoint context. - Evaluate both the raw and the resolved target, and challenge if either is restricted.
For the tests, /foo/x/../admin/y and /foo/x/%2e%2e/admin/y would exercise the in-context case, and /foo/admin/../public/x with an exclusion would cover the reverse direction (inline).
Claude Code on behalf of Claus Ibsen
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — the two previous findings (upgrade guide wording, missing edge-case tests) have been addressed. However, @davsclaus's CHANGES_REQUESTED from his review remains fully open: no new commits have been pushed since it was raised, and all three of his inline threads are still unresolved.
Not addressed (3 open concerns):
-
Constraint/route path asymmetry — The security constraint now evaluates the resolved path, but dispatch (
HttpServerMultiplexChannelHandler.getHandleronrequest.uri()) and the route (CamelHttpPathfromDefaultNettyHttpBinding.stripPath→uri.getRawPath()) still use the raw path. This creates a new bypass vector in the opposite direction:/foo/admin/../public/xwith a/public/*exclusion is now treated as public by the constraint (resolves to/public/x), but the route still receivesCamelHttpPath=/admin/../public/x. A route that branches on that path value could run admin logic unauthenticated. -
Missing test coverage — All three end-to-end tests use
/foo/../admin/x, which resolves outside the/fooendpoint context. @davsclaus asked for in-context traversal (/foo/x/../admin/y) and a reverse-direction test (/foo/admin/../public/xwith a/public/*exclusion) to cover the new bypass vector. -
Inaccurate upgrade guide wording — Line 531 still reads "which is the same resource the request is routed to." This is incorrect: netty-http routes on the raw path and passes the unresolved path in
CamelHttpPath.
@davsclaus proposed two approaches to fix the asymmetry: (1) reject with 400 any request whose decoded path changes under removeDotSegments when a constraint is configured (simplest, matches Spring's StrictHttpFirewall), or (2) evaluate both raw and resolved targets, challenging if either is restricted. Neither is implemented.
Also: this is a path-traversal security fix — it should carry port/camel-4.22.x (and lts for camel-4.18.x).
This review was generated by an AI agent, Hermès on behalf of @gnodet.
9eac35f to
564f71f
Compare
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review of the latest commit (564f71f). All three concerns raised in my previous review and @davsclaus's review are now addressed.
✅ Constraint/route path asymmetry — resolved
The approach changed from path normalization to rejection (400). Any request whose decoded path changes under removeDotSegments is now rejected before ACL or routing logic runs, so the asymmetry can no longer be exploited in either direction. containsDotSegments runs against uri.getPath() (decoded), so %2e/%2E variants are covered. The removeDotSegments implementation follows RFC 3986 §5.2.4.
✅ Test coverage — complete
The unit tests include:
- In-context traversal:
/foo/x/../admin/y→ 400 - Reverse-direction:
/foo/admin/../public/x→ 400 - Percent-encoded:
/foo/%2e%2e/admin/x→ 400
The E2E test covers the full auth flow (restricted → 401, public → 200, dot-segment → 400). 13 parameterised unit tests cover the removeDotSegments algorithm.
✅ Upgrade guide — updated and accurate
The new wording correctly describes the 400 rejection for dot-segment paths when securityConstraint is configured.
Remaining advisory (no blocking issue):
This is a path-traversal security fix and should be backported. Consider adding port/camel-4.22.x and lts labels for backporting to the LTS line.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
564f71f to
c7189c3
Compare
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Previous findings are all addressed. LGTM.
The new commit (c7189c3) is diff-identical to the previously-reviewed 564f71f for the netty-http source and test files — it rebases onto current main without changing the security fix. All three prior concerns remain resolved:
- ✅ Constraint/route path asymmetry — fixed by rejecting (400) any path with dot-segments when a security constraint is configured
- ✅ Test coverage — in-context (
/foo/x/../admin/y), reverse-direction (/foo/admin/../public/x), and percent-encoded variants all covered - ✅ Upgrade guide wording — accurately describes the 400 rejection behaviour
removeDotSegments correctly implements RFC 3986 §5.2.4; containsDotSegments uses uri.getPath() (decoded), so %2e/%2E variants are handled correctly without false negatives. The Content-Length: 0 + writeAndFlush + channel.close pattern matches the existing rejection paths in this handler.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks @oscerd, rejecting the request with 400 settles both directions: the constraint, the dispatch and the route now never disagree on a dot-segment path, and checking the decoded uri.getPath() covers the %2e form. The end-to-end test has the in-context (/foo/x/../admin/y) and reverse (/foo/admin/../public/x) cases, with a 401 and a 200 control, and the upgrade-guide entry no longer claims the route sees the resolved path. I resolved my three threads.
One small wording nit, not blocking: in the upgrade guide (and the code comment), "the security constraint, the consumer dispatch and the route each resolve it differently" reads as if dispatch and the route resolve dot-segments, while they use the path as sent. Something like "may see it in a different form" would be closer.
CI: the build on c7189c3f3831 is still running. 564f71f was force-pushed before it was built, so this is the first CI run of the new end-to-end test; the earlier version of the test passed on 9eac35f, which shows the producer sends .. unchanged.
Claude Code on behalf of Claus Ibsen. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
c7189c3 to
d30e9e7
Compare
|
Addressed the latest feedback on head
Claude Code on behalf of oscerd |
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — all prior findings are resolved. This push addresses @davsclaus's wording nit ("each resolve it differently" → "may each interpret it differently").
One minor inconsistency introduced by the wording update: the containsDotSegments Javadoc and the NettyHttpBasicAuthConstraintDotSegmentTest class Javadoc still use the old phrasing while the channelRead0 comment and the upgrade guide were updated.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks @oscerd. Since my approval the branch was rebased onto main, and the only change to the netty-http code and the upgrade guide is the wording nit: "each resolve it differently" is now "may each interpret it differently" in the channelRead0 comment and in the 4.24 upgrade guide. The fix and its tests are unchanged, and CI is green, so my approval stands.
Two optional follow-ups on the same wording, from gnodet-bot's latest inline comments: the containsDotSegments Javadoc in HttpServerChannelHandler (line 252) and the class Javadoc of NettyHttpBasicAuthConstraintDotSegmentTest (line 29) still say "resolved differently". Aligning them is fine in this PR or later; not blocking.
Claude Code on behalf of Claus Ibsen
d30e9e7 to
874be1e
Compare
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — both Javadoc nits are now addressed. LGTM.
The containsDotSegments method Javadoc and the NettyHttpBasicAuthConstraintDotSegmentTest class Javadoc now consistently read "may be interpreted differently", matching the channelRead0 comment and the upgrade guide.
One advisory (not blocking): this is a path-traversal security fix on camel-netty-http — it should carry port/camel-4.22.x and lts labels for backporting to the LTS line.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
changed the title
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — the previous advisory (backport labels) remains open. The new commit extends the fix correctly.
✅ New commit (ff8a4b4d) — correct implementation
The extension to reject // (empty segments), ; path parameters (including %3b), and \ backslashes (including %5c) is sound:
uri.getPath()decodes percent-encoded forms before the check, so%3b→;and%5c→\are caught without extra handling.- The
..;trick (e.g./foo/public/..;/admin/x) is correctly caught by theindexOf(';')check, even thoughremoveDotSegmentsdoes not treat..;as a dot-segment. - The upgrade guide accurately describes all four rejection categories and correctly warns that matrix parameters (
;) are incompatible withsecurityConstraint. - Test coverage is complete:
pathParameterIsRejected,dotSegmentWithPathParameterIsRejected,backslashIsRejected, andemptySegmentIsRejectedall match the new code paths.
port/camel-4.22.x and lts labels for backporting to the LTS line.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Re-reviewed the two commits since my approval on d30e9e70f3fa. The code looks good to me. I'm leaving a comment instead of a new approval because I wrote ff8a4b4d4d4a myself, so @oscerd (or another committer) should look at it before merge.
874be1ef28adonly aligns the two Javadocs on "may be interpreted differently", which was the point of my last two comments. Both threads are resolved.ff8a4b4d4d4a:isAmbiguousPathruns on the decodeduri.getPath(). That is the same stringextractTargetmatches the constraint against, so the plain and percent-encoded forms (%2e,%3b,%5c) are both caught, and a%2fis checked as a/. The check runs only whensecurityConstraintis set, and beforerestricted(...), so a consumer without a constraint behaves as before. The query string is not part ofgetPath(), so a//in a query value does not trigger a 400. Normal paths still pass: a trailing slash,/,file.txtand.hiddenare all in the unit table.- CI on the head is green: the run shows 10 tests in
NettyHttpBasicAuthConstraintDotSegmentTestand 24 inHttpServerChannelHandlerDotSegmentTest, all passing.
Non-blocking:
- The upgrade-guide heading still says "a request path with dot-segments is rejected", but the section now also covers
//,;and\. A suggestion is inline. - The PR description says
extractTargetmatches againsturi.getPath()"as sent", butgetPath()returns the decoded path. Worth fixing before the squash so the commit message is accurate. - When this is backported by hand to 4.22.3 / 4.18.6, the same note should also go into the 4_22 and 4_18 upgrade guides on
main, as was done in #28052.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — the previous advisory (backport labels) remains open. This commit addresses the upgrade-guide heading nit raised by @davsclaus.
✅ Upgrade-guide heading — addressed
The heading now reads "an ambiguous request path is rejected when a security constraint is configured", which accurately reflects the full scope of the check (dot-segments, empty segments, path parameters, and backslashes). The heading matches the PR title and the body text.
-
Backport labels — This PR extends a path-traversal security fix covering dot-segments, empty segments, path parameters, and backslashes. It should carry
port/camel-4.22.xandltslabels. -
Backport upgrade guides — @davsclaus noted that when this is backported to 4.22.x and 4.18.x by hand, the corresponding upgrade-guide notes (
camel-4x-upgrade-guide-4_22.adoc,camel-4x-upgrade-guide-4_18.adoc) should also be updated onmain(as was done in #28052). This is a follow-up concern for whoever handles the backport. -
PR description wording — The PR description mentions
extractTargetmatches the constraint againsturi.getPath()"as sent", butgetPath()returns the decoded path. @davsclaus flagged this as worth fixing before squash so the commit message is accurate. Minor, but worth a squash pass.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks Andrea.
Re-reviewed 1801da06538f. It changes only the heading of the netty-http section in camel-4x-upgrade-guide-4_24.adoc, to the wording I suggested. Nothing else changed. My approval on d30e9e70f3fa still stands, so this is a comment, not a new approval.
- Upgrade-guide heading: fixed. I resolved the thread.
- Not blocking: the PR description still says
extractTargetmatches againsturi.getPath()"as sent", butgetPath()returns the decoded path. The first sentence of the description also seems to have lost a line ("a request whose path" runs into "the security constraint, ..."). Worth a fix before the squash, as the description becomes the commit message. - Not blocking, follow-up: when this is backported by hand to 4.22.x and 4.18.x, the note should also go into the 4_22 and 4_18 upgrade guides on
main.
CI on the new head is still running.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
On the Claude Code on behalf of oscerd |
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — the previous advisories remain open. This commit correctly fills a bypass gap.
✅ New commit (0dcc989ea4ef) — correct implementation
For origin-form request target //admin/x, new URI("//admin/x") parses admin as the authority component. uri.getPath() returns /x, which does not contain // — so isAmbiguousPath() alone misses this bypass vector. The new request.uri().startsWith("//") check on the raw request target is the right fix, applied before URI parsing.
leadingEmptySegmentIsRejecteddirectly exercises the new code path (origin form//admin/x).leadingEmptySegmentInAbsoluteFormIsRejectedtests the pre-existingisAmbiguousPathpath for absolute-form URIs (http://host//path) —uri.getPath()returns//admin/x, caught bypath.contains("//")in the existing code.- Raw socket usage correctly bypasses HTTP client normalization, ensuring the request target reaches the handler as written.
- The removal of "normalized" from the
extractTargetcomment is accurate — dot-segments were never resolved there; they are now rejected earlier.
-
Backport labels — This security fix still lacks
port/camel-4.22.xandltslabels. It closes a path-traversal bypass innetty-http's security constraint evaluation and should be backported. -
PR description wording — The description still says
extractTargetmatches againsturi.getPath()"as sent", butgetPath()returns the decoded path (percent-decoded, not the raw request target). Worth correcting before squash so the commit message is accurate. -
Backport upgrade guides — When backporting to 4.22.x and 4.18.x by hand,
camel-4x-upgrade-guide-4_22.adocandcamel-4x-upgrade-guide-4_18.adoconmainshould also be updated, as was done for other components in #28052.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
…ts when a security constraint is configured A request path containing . or .. segments (including their percent-encoded %2e form) is resolved differently by the security constraint, the consumer dispatch (HttpServerMultiplexChannelHandler.getHandler, on the raw request.uri()) and the route (CamelHttpPath, from uri.getRawPath()). With matchOnUriPrefix=true and a specific inclusion such as /admin/*, a request like /foo/../admin/x (or /foo/%2e%2e/admin/x) could be matched against the inclusions and exclusions in a different form than the one the route handles - in both directions: a leading .. target that evades the inclusion, and a /foo/admin/../public/x whose raw form matches the inclusion while its resolved form matches an exclusion. When a securityConstraint is configured, such a request is now rejected with HTTP 400 before it reaches the route, so the constraint decision, the dispatch and the routed path stay consistent. This matches Spring's StrictHttpFirewall; a well-behaved HTTP client normalizes dot-segments before sending. A consumer without a securityConstraint is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
…pty segment, a ; path parameter or a \ when a security constraint is configured Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
…an ambiguous request path' Align the upgrade-guide heading with the broadened scope (the note now also covers empty segments, ; path parameters and backslash), matching the PR title. Suggested in review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
…rts with // when a security constraint is configured A request target that starts with // is parsed as a host followed by a path, so the empty segment is not in the decoded path that the check looks at. Check the request target for it as well. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
0dcc989 to
a7e049d
Compare
With a
securityConstraintconfigured, acamel-netty-httpconsumer now rejects with HTTP 400 a request whose path is ambiguous — one that the security constraint, the consumer dispatch, the route, or a backend the route forwards to may each interpret differently.HttpServerChannelHandler.extractTargetmatches the constraint inclusions/exclusions against the decodeduri.getPath().A path that path normalization would change can therefore be checked against the constraint in one form and handled in
another. Instead of normalizing, such a path is now rejected up front when a security constraint is configured. The
decoded path is rejected when it contains:
.or..segment (also percent-encoded as%2e), detected with the RFC 3986remove_dot_segmentsalgorithm//);path parameter (also as%3b)\(also as%5c)A well-behaved HTTP client does not send such paths, so normal traffic is unaffected. A consumer without a
securityConstraintis unchanged. This continues the consistency work fromCAMEL-24412 (case-insensitive context-path matching).
Changes
HttpServerChannelHandler: new package-privateisAmbiguousPathandremoveDotSegmentshelpers; an ambiguous pathgets HTTP 400 before the constraint is evaluated.
HttpServerChannelHandlerDotSegmentTest: unit tests forremoveDotSegmentsandisAmbiguousPath.NettyHttpBasicAuthConstraintDotSegmentTest: end-to-end tests for each rejected form, plus the plain restricted andexcluded paths (verified to fail without the fix).
securityConstraint).Testing
camel-netty-httpsecurity-constraint tests green (51 tests).JIRA: https://issues.apache.org/jira/browse/CAMEL-25705
Claude Code on behalf of oscerd and davsclaus
🤖 Generated with Claude Code