CAMEL-24298: extend the allowedSchemes allow-list to the sibling dynamic-URI EIPs - #25361
Open
oscerd wants to merge 1 commit into
Open
CAMEL-24298: extend the allowedSchemes allow-list to the sibling dynamic-URI EIPs#25361oscerd wants to merge 1 commit into
oscerd wants to merge 1 commit into
Conversation
…mic-URI EIPs Following the toD MVP (apache#25315), add the optional allowedSchemes allow-list to the remaining dynamic-URI EIPs: recipientList, routingSlip, dynamicRouter, enrich and pollEnrich. A resolved dynamic recipient whose scheme is not in the configured list is rejected. Enforcement is a shared ProcessorHelper.checkAllowedSchemes(...) called at each processor's recipient-resolution point: RoutingSlip (and DynamicRouter, which extends it), PollEnricher and RecipientListProcessor after prepareRecipient; enrich delegates to its internal SendDynamicProcessor which already enforces it. Default unset = any scheme allowed (no behavioural change). Because the check throws at the resolution point, a disallowed scheme follows each EIP's existing ignoreInvalidEndpoint semantics (hard-fail by default). Each of the 5 model definitions gets the allowedSchemes attribute + getter/setter + fluent builder, wired through its reifier. DynamicUriEipAllowedSchemesTest covers rejection across all five EIPs. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
davsclaus
requested changes
Aug 5, 2026
davsclaus
left a comment
davsclaus
left a comment
Contributor
There was a problem hiding this comment.
Thanks for extending allowedSchemes to the sibling dynamic-URI EIPs, @oscerd — the centralised ProcessorHelper approach is clean and the generated files are all consistent.
Two copy-constructor omissions need fixing before this can merge (details in the inline comments). Everything else is a suggestion or design question.
Questions
Positive test case — the test covers rejection well, but there is no positive test verifying an allowed scheme passes through. A single test (e.g. recipientList(header("target")).allowedSchemes("mock") with target=mock:result, asserting the message arrives) would strengthen confidence.
ignoreInvalidEndpoints interaction — for toD, a disallowed scheme always hard-fails (check is before the ignoreInvalidEndpoint catch). For these 5 EIPs, the check sits inside the existing try-catch, so ignoreInvalidEndpoints=true would silently skip a disallowed scheme. Is that intentional, or should allowedSchemes always be a hard boundary?
This review covers project rules and conventions. It does not replace specialised tools (CodeRabbit, SonarCloud) for deep static analysis.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
| + " may resolve to (e.g. http,https). When set, a dynamic endpoint whose scheme is not in the" | ||
| + " list is rejected. This is a defence-in-depth restriction, useful for low-code / Kamelet" | ||
| + " deployments; by default (unset) any scheme is allowed.") | ||
| private String allowedSchemes; |
Contributor
There was a problem hiding this comment.
The allowedSchemes field is added here, but the copy constructor (line ~123 in the new file) does not copy it. This means copyDefinition() — used by route advice (AdviceWith), route template instantiation, etc. — will silently lose the allowedSchemes setting.
All three other Definition classes with copy constructors (DynamicRouterDefinition, EnrichDefinition, PollEnrichDefinition) are correctly updated — this one is missed.
Fix: Add this.allowedSchemes = source.allowedSchemes; after the shareUnitOfWork line in the copy constructor.
| + " may resolve to (e.g. http,https). When set, a dynamic endpoint whose scheme is not in the" | ||
| + " list is rejected. This is a defence-in-depth restriction, useful for low-code / Kamelet" | ||
| + " deployments; by default (unset) any scheme is allowed.") | ||
| private String allowedSchemes; |
Contributor
There was a problem hiding this comment.
Same issue — the copy constructor (line ~73 in the new file) does not copy this new allowedSchemes field.
DynamicRouterDefinition (which extends this class) shadows the field and has its own copy constructor that IS updated, so dynamicRouter is fine. But standalone routingSlip usage via route templating or advice would lose the setting.
Fix: Add this.allowedSchemes = source.allowedSchemes; after the cacheSize line in the copy constructor.
Contributor
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 551 tested, 26 compile-only — current: 548 all testedMaveniverse Scalpel detected 577 affected modules (current approach: 548).
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Follows the toD MVP (#25315, CAMEL-24298) by extending the optional
allowedSchemesallow-list to the remaining 5 dynamic-URI EIPs:recipientList,routingSlip,dynamicRouter,enrich,pollEnrich. A resolved dynamic recipient whose component scheme is not in the configured list is rejected — a defence-in-depth restriction for low-code / Kamelet deployments.Enforcement
A shared
ProcessorHelper.checkAllowedSchemes(...)(+parseAllowedSchemes(...)) is called at each processor's recipient-resolution point:prepareRecipient.Enricherdelegates to its internalSendDynamicProcessor, which already enforcesallowedSchemes(from CAMEL-24298: add an optional allowedSchemes allow-list to the toD dynamic-URI EIP #25315).Each of the 5 model definitions gets the
allowedSchemesattribute + getter/setter + fluent builder, wired through its reifier.Behaviour
Default unset = any scheme allowed → no behavioural change. Because the check throws a
ResolveEndpointFailedExceptionat the resolution point, a disallowed scheme follows each EIP's existingignoreInvalidEndpointsemantics — hard-fail by default, and (like any invalid endpoint) skipped-not-sent if the route opted intoignoreInvalidEndpoint. This is slightly less strict thantoD, which hard-fails regardless — happy to align them if you'd prefer.Tests
DynamicUriEipAllowedSchemesTest— a disallowed scheme (seda:against anallowedSchemes("mock")route) is rejected across all five EIPs. Full-reactormvn clean install -DskipTestsgreen (model JSON, XML/YAML schemas, DSL writers/parsers/deserializers, catalog regenerated). The option is documented on each EIP's auto-generated options table (via the@Metadatadescription).Scope
Completes CAMEL-24298:
toDlanded in #25315,wireTapinherits it, and this covers the 5 sibling EIPs. Main only — additive feature (default unrestricted), not a bug fix.Closes CAMEL-24298.
Claude Code on behalf of Andrea Cosentino (@oscerd)