Repository navigation
CAMEL-26335: camel-cometd - configure allowedTransports and disable callback-polling by default - #28555
Conversation
…allback-polling by default Configure the allowedTransports parameter on CometdEndpoint and CometdComponent to restrict accepted Bayeux transport mechanisms. By default, allowedTransports defaults to "websocket,long-polling", disabling callback-polling. Users requiring callback-polling can explicitly configure allowedTransports on the endpoint. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
🔄 Backport BotThis bugfix targets
Add a Backport PRs will be created automatically on merge (once a port label is added). Comment ℹ️ If you push additional commits after |
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
✅ Generated files are up to dateAn earlier CI run reported uncommitted generated changes; the latest run no longer does. |
|
🧪 CI tested the following changed modules:
🔁 1 test passed only after a retry on JDK 21 (1 retried attempt) Recovered flaky tests on JDK 21 (1)
🔬 Scalpel shadow comparison — Scalpel: 8 of 708 tested, 24 compile-only — current: 8 all testedMaveniverse Scalpel detected 8 affected modules (current approach: 8). Skip-tests mode would test 8 modules (4 direct + 7 downstream), skip tests for 24 (generated code, meta-modules) Modules Scalpel would test (8)
Modules with tests skipped (24)
All tested modules (35 modules, 6m 16s total)Total reactor time: 6m 16s
Top 20 slowest modules:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks Andrea, this is a clean, focused change. Making callback-polling something users turn on, with a new endpoint option and an upgrade-guide note, is the right shape. The default only gets stricter, and the tests check the real HTTP behaviour on both sides (default and opted-in). I like that testDefaultTransportsAllowsLongPolling also checks that the handshake reply no longer offers callback-polling. None of the points below block the merge. They are small hardening and wording points.
1. Spaces in the list (CometdComponent.java:239)
CometD 9.0.1 (BayeuxServerImpl.initializeServerTransports) splits allowedTransports on , without trimming, and it silently drops any name it does not know. So allowedTransports=long-polling, callback-polling, written the way many Camel list options are, gives only long-polling, and JSONP stays off with no warning. If every name has a leading space, the servlet fails to start with "does not contain at least one configured server transport name". Can we normalise the value before passing it on?
if (ObjectHelper.isNotEmpty(endpoint.getAllowedTransports())) {
String transports = Arrays.stream(endpoint.getAllowedTransports().split(","))
.map(String::trim)
.filter(s -> !s.isEmpty())
.collect(Collectors.joining(","));
holder.setInitParameter("allowedTransports", transports);
}A test with spaces, e.g. allowedTransports=long-polling, callback-polling on the custom endpoint, would cover it.
2. The websocket entry and the upgrade-guide wording (camel-4x-upgrade-guide-4_24.adoc:3597)
Out of the box, CometD only creates a WebSocket transport when its Jakarta WebSocket transport (org.cometd.server.websocket.jakarta.WebSocketTransport) is on the classpath. camel-cometd ships cometd-java-server-websocket-jetty, which BayeuxServerImpl does not add by default. So with the default value, long-polling is the only active transport, and websocket in the list is a no-op unless a user adds that transport. Keeping it in the default is fine. However, "Only the websocket and long-polling transports are enabled out of the box" reads as if WebSocket works out of the box. Maybe: "Only long-polling (and websocket, when the CometD WebSocket transport is on the classpath) is enabled by default."
3. Option description (CometdEndpoint.java:95-97, 302-304)
- It would help users to list the valid names (
websocket,long-polling,callback-polling), say that the list is comma separated, and say thatcallback-pollingis the JSONP transport. - Like
timeout,intervaland the CORS options, this value applies to the whole CometD server of a host and port. The endpoint that starts the server first sets it, and other endpoints on the same port cannot change it. A sentence about that avoids surprises when two routes share a port with different values. - The text is in both the
descriptionattribute and the setter javadoc. The other options of this endpoint use only the setter javadoc, so we can drop the attribute.
A short paragraph about transports/JSONP in cometd-component.adoc would also help users find the option (the catalog copy needs regenerating then).
4. Test assertion (CometdAllowedTransportsTest.java:47)
When no allowed transport accepts the request, CometD answers 400 Unknown Bayeux Transport. assertEquals(400, response.statusCode()) is stricter than the || form and gives a clearer failure message.
5. JIRA and PR description
- CAMEL-25993 (open, fix version 4.24.0) asks for the same change. Its second part,
nullinallowedOriginswithout credentials, already landed with CAMEL-25984 (isNamedOrigins). Can we link the two and close CAMEL-25993 when this is merged? - CAMEL-26335 has fix versions 4.18.6 and 4.22.3, and CAMEL-25993 proposed main only because the default changes. If this is ported, the same note belongs in the 4.18 and 4.22 upgrade guides on
main. - The PR description says the init parameter is
org.cometd.bayeux.server.BayeuxServer.allowedTransports, but the code (correctly) usesallowedTransports(BayeuxServerImpl.ALLOWED_TRANSPORTS_OPTION). A small fix to the description would help readers.
I checked the generated files (configurer, URI factory, catalog JSON, Endpoint DSL after the Regen commit). They match the new option.
Reviewed with Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
- CometdComponent: normalize allowedTransports by trimming spaces between transport names - CometdEndpoint: enhance option javadoc and describe server-level scope - Add Allowed Transports and JSONP documentation in cometd-component.adoc and sync catalog - CometdAllowedTransportsTest: assert exact 400 status code on rejection and test spaced configuration - Update upgrade guide wording regarding default enabled transports Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
|
Thanks @davsclaus! Addressed the feedback in commit b92bcf2:
Claude Code on behalf of oscerd |
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
LGTM. No issues found.
Shared-server semantics are consistent with all other init params — first endpoint wins, which the setter Javadoc documents accurately. Tests correctly use separate ports to avoid interference. Upgrade guide is accurate.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
added
port/camel-4.18.x
🔄 Backport BotConflicts (🤖 agent dispatched to resolve):
ℹ️ If you push additional commits after |
|
✅ Backport to |
|
✅ Backport to |
Jira: https://issues.apache.org/jira/browse/CAMEL-26335
CometD endpoints in
camel-cometdaccept transport connections through the CometDBayeuxServer. By default, CometD registers all transports available on its classpath, which historically includedcallback-polling(JSONP) in addition towebsocketandlong-polling.This change introduces an
allowedTransportsconfiguration option onCometdEndpoint(and forwarded to the servlet init parameters inCometdComponent), defaulting to"websocket,long-polling". As a result,callback-pollingis disabled by default.Changes
allowedTransportsURI option toCometdEndpoint(default:"websocket,long-polling"), normalized by trimming whitespace between comma-separated entries.CometdComponent.createServletForConnector(), passedallowedTransportsas an init parameter toallowedTransports(BayeuxServerImpl.ALLOWED_TRANSPORTS_OPTION), which CometD uses to initialize its active server transports.cometd-component.adocand synchronized catalog docs.callback-pollingif needed for legacy environments.Tests
Added
CometdAllowedTransportsTestcovering:testDefaultTransportsRejectsJsonp: verifies that handshake requests withcallback-polling(JSONP) transport are rejected by default with HTTP 400.testDefaultTransportsAllowsLongPolling: verifies that handshake requests with standardlong-pollingtransport continue to be accepted.testCustomTransportsAllowsJsonpWhenConfigured: verifies that configuringallowedTransports=websocket, long-polling, callback-pollingrestores support forcallback-pollingand handles spaces between transport names.Claude Code on behalf of oscerd