CAMEL-24192: camel-lra - separate compensation/completion URI allowlists and prune on route removal#24904
Merged
Conversation
…sts and prune on route removal Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
Contributor
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
approved these changes
Jul 19, 2026
gnodet
left a comment
gnodet
left a comment
Contributor
There was a problem hiding this comment.
Review: CAMEL-24192 — Separate LRA compensation/completion URI allowlists
Overall: Clean security improvement — splits the flat sagaURIs allowlist into type-specific sets so compensation callbacks can only invoke compensation endpoints and vice versa, and adds automatic cleanup when routes are removed.
What this PR does well
-
Security hardening — The original single
Set<String>allowed cross-type callback invocation (a completion callback could dispatch to a compensation URI). Splitting intocompensationURIsandcompletionURIswith type-specific validation inverifyRequest(Exchange, boolean)closes this gap. -
Correct cleanup design —
unregisterSteps()properly handles shared URIs: it collects URIs still referenced by other routes before removing, preventing premature pruning. TheLifecycleStrategyhook fires ononRoutesRemove()to trigger cleanup automatically. -
Backward-compatible
getRegisteredURIs()— Returns the union of both sets, so existing callers are unaffected. -
API fix — Changed
Optional.map()toOptional.ifPresent()for side-effect operations inregisterStep()(the old.map(this.sagaURIs::add)misusedmapfor side effects). -
Good test coverage — 6 tests covering separation, union, pruning, shared-URI retention, and unknown-route no-op.
-
Import cleanup — Wildcard
java.util.*replaced with specific imports inLRASagaRoutes.
Non-blocking observations
-
Thread safety in
unregisterSteps()— There's a narrow TOCTOU window betweenstepsByRouteId.remove(routeId)and iteratingstepsByRouteId.values(): a concurrentregisterStep()could add a step referencing the same URI between those two operations. In practice this isn't a problem since route registration and removal are sequential during lifecycle transitions, but worth noting. -
Lifecycle strategy cleanup — Consider removing the lifecycle strategy in
doStop()(viacamelContext.removeLifecycleStrategy(this.lifecycleStrategy)) alongside clearing the maps, and resettingthis.lifecycleStrategy = nullso it gets re-registered on restart. Currently the null-guard prevents double-registration, but explicit removal would be cleaner for restart scenarios. -
Assertion style — Tests use JUnit
assertTrue/assertFalse/assertEquals. The project preference is AssertJ (assertThat(...).contains(...),.doesNotContain(...),.isEmpty()), which would also give better failure messages for set membership checks.
Claude Code on behalf of gnodet — AI-generated review
Contributor
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 550 tested, 29 compile-only — current: 550 all testedMaveniverse Scalpel detected 579 affected modules (current approach: 550).
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
Contributor
Author
|
Claude Code on behalf of davsclaus Thanks for the review! Addressed observation #2 — lifecycle strategy is now explicitly removed in On the other observations:
|
gnodet
reviewed
Jul 19, 2026
gnodet
left a comment
gnodet
left a comment
Contributor
There was a problem hiding this comment.
Re-review after 2nd commit (d84f42e): Lifecycle strategy cleanup in doStop() looks good 👍
The new commit properly deregisters the LifecycleStrategy from the CamelContext during doStop():
if (this.lifecycleStrategy != null) {
camelContext.getLifecycleStrategies().remove(this.lifecycleStrategy);
this.lifecycleStrategy = null;
}This prevents the strategy from lingering if the service is stopped while the CamelContext continues running. Clean follow-up. My earlier approval stands.
Claude Code on behalf of gnodet — AI-generated review
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.
Summary
Claude Code on behalf of davsclaus
Splits the single shared
sagaURIsallowlist inLRASagaServiceinto separate compensation and completion URI sets, and adds automatic cleanup when routes are removed.Problem: The LRA Saga service maintained one flat
Set<String>for both compensation and completion callback URIs. This meant (1) a completion callback could invoke a compensation endpoint and vice versa, and (2) URIs from dynamically removed routes remained authorized for the entire context lifetime (memory leak + stale authorization).Changes:
CamelSagaStep— addedrouteIdfield so steps can be tracked by the route that registered themSagaReifier— setsrouteIdon the step before callingregisterStep()LRASagaService— replacedsagaURIswith separatecompensationURIsandcompletionURIssets; addedstepsByRouteIdtracking map; registered aLifecycleStrategySupportthat prunes URIs when routes are removed; shared URIs are only pruned when all referencing routes are removedLRASagaRoutes— splitverifyRequest()intoverifyCompensationRequest()andverifyCompletionRequest()so each callback type validates only against its own allowlistLRASagaServiceAllowlistTest— covers URI separation, pruning, shared URI retention, and no-op for unknown routesNo interface changes —
CamelSagaService.registerStep(CamelSagaStep)signature is preserved.InMemorySagaServiceis unaffected.Test plan
LRASagaServiceAllowlistTest— 6 tests covering separation, pruning, shared URIs, union, and no-opmvn verifyincomponents/camel-lra)camel-core-reifiercompiles with the newstep.setRouteId()call🤖 Generated with Claude Code