CAMEL-24499: camel-spring-boot - route detail view bypasses the route.start.exception serialization filter - #1898
Conversation
…ws honour the configured exposure Only emit error.stacktrace in the health details at the 'full' exposure level; at the default level report the exception type alongside the existing message and keep the full trace in the server log. Remove the properties field and getter that RouteDetailsEndpointInfo declared on top of the annotated RouteEndpointInfo property, which bypassed the route.start.exception serialization filter added in CAMEL-20993. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
davsclaus
left a comment
There was a problem hiding this comment.
Reviewed against this project's build/test/contribution rules. I checked out the branch locally, built the module, ran the new tests, and — importantly — reverted the two fix files back to main while keeping the new tests to confirm they actually catch the described regressions before trusting the PR's own claims about them.
Verification performed
- Build:
mvn -DskipTests installincore/camel-spring-bootsucceeds. - New tests:
CamelHealthHelperExposureTest(3 cases) andCamelRoutesEndpointStartExceptionTest(2 cases) all pass on the PR branch. - Regression check: reverted
CamelHealthHelper.javaandCamelRoutesEndpoint.javatomainwhile keeping the new tests — both defect tests failed. The failure output is a striking confirmation of the bugs: the health check test leaked a full stack trace at the default exposure level, and the route detail test serialized the entireroute.start.exceptionobject graph (nested causes, every stack frame with classloader name, module name, JDK version, etc.), exactly as described in the PR body. Restored the fix and confirmed green again. - Full suite:
mvn testincore/camel-spring-boot— 141 tests, 0 failures, matching the PR description exactly. - History check:
git show ebccbfe5408(CAMEL-20993) confirms the@JsonIgnoreProperties(value = {"route.start.exception"})annotation and the shadowingpropertiesfield inRouteDetailsEndpointInfowere introduced in the same commit — the filter escaping the subclass looks like an accidental oversight, not an intentional design choice being reverted. - "No public API change" claim: verified the public base class
RouteEndpointInfostill declares both the annotated field andgetProperties(), so removing the subclass's duplicate override is source/binary compatible as claimed.
Findings
No blocking issues. This is a clean, well-scoped, well-verified information-disclosure fix.
- Both root causes are independently verified as real defects, not reverts of intentional behavior.
- Test coverage is genuinely meaningful — empirically confirmed the tests fail without the fix.
- Single commit, correctly formatted per project guidelines (
CAMEL-24499: ...). - No public API changes, no new dependencies, no Lombok/Records — compliant with project standards.
- Tight scope: two related root causes in the same actuator exposure area, no drift into unrelated code.
No nitpicks or open questions on this one.
This review does not replace specialized tools like CodeRabbit, Sourcery, or SonarCloud/static analysis.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Review verdict: Request changesReviewed exact head Findings[P1] Do not silently redefine the established default health-exposure contract[ CAMEL-18832 introduced this helper to align Spring Boot with MicroProfile health. MicroProfile’s default-exposure regression still explicitly requires This PR removes that longstanding JSON field only from Spring Boot and characterizes its placement as accidental without acknowledging the compatibility change or cross- References:
Either retain the established default behavior or explicitly approve and document the compatibility change, including the resulting MicroProfile divergence. [P2] Preserve the message for every failing health check[ Default exposure can return multiple DOWN results. The indicator applies them to one An exact-head probe with two failures produced:
Store References:
[P2] Establish the asynchronous route-failure precondition in the test[ The supervising controller starts asynchronously after a configured 100 ms delay. The tests immediately serialize the route and only assert that If serialization occurs before the failed start installs that property, both tests pass against the unfixed implementation without exercising the leak. Use Awaitility with an explicit
[P3] Remove or implement the server-log guarantee[ The comment states that the full trace remains in the server log, but neither the helper nor its caller logs or attaches the throwable using Spring Boot only logs exceptions attached to its builder or thrown by the indicator. Some Camel health checks merely store exceptions in their result, so the trace is [Spring Boot exception-logging path](https://github.com/spring-projects/spring-boot/blob/v4.1.0/module/spring-boot-health/src/main/java/org/springframework/boot/health/ Remove or qualify the claim unless intentional, non-repetitive logging is implemented. Verification
This review does not replace static analysis or specialized review tooling. Generated by Codex on behalf of Luigi De Masi. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying. |
Drop the health-check exposure change. CAMEL-18832 placed the error block outside the exposure-level branch deliberately, to align the Spring Boot output with camel-microprofile-health, whose testExposureLevelDefault asserts error.stacktrace is present at the default level. Restricting it only here would diverge the two runtimes; the unauthenticated-exposure concern belongs with CAMEL-24498, which is what raises exposure-level to full. Establish the asynchronous start-failure precondition in the route test with Awaitility, so the assertions cannot pass vacuously before the supervising controller records route.start.exception, and assert the benign route properties are still serialized. The error.message overwrite across multiple DOWN checks is tracked separately as CAMEL-24512. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
|
Thanks @luigidemasi — this was a good catch on P1, and I checked each point rather than taking them as read. Head is now [P1] Default health-exposure contract — agreed, change dropped. You're right, and my framing of it as accidental placement was wrong. I verified it: the original CAMEL-18832 implementation ( I checked git history for the route-detail half (CAMEL-20993) but not for the health half, which is exactly how I ended up reverting an intentional decision. The health change is removed from this PR, along with its test; the PR is now the route-detail fix only. The underlying exposure concern is real but belongs with CAMEL-24498, where the observability starter raises [P2] Verified: [P2] Asynchronous route-failure precondition — fixed. Fair point. The tests did fail against the unfixed implementation when I ran them, but that was timing-dependent rather than guaranteed. A [P3] Server-log guarantee — gone with the health change. The comment making that claim was part of the reverted hunk, so it no longer exists. You were right that nothing guaranteed it. Full |
luigidemasi
left a comment
There was a problem hiding this comment.
Thanks for addressing the feedback. I rechecked exact head 85b446273ae0aba20f749e5fbf08f6a83f58d23e.
The PR-specific findings are resolved:
- The incompatible health-exposure change and unsupported logging claim were removed.
- The Awaitility precondition makes the route regression test non-vacuous.
- The updated test passes on the PR head and fails as expected on the base implementation.
- The pre-existing multi-DOWN
error.messageissue is appropriately tracked separately in CAMEL-24512.
I have no remaining blocking findings. Thanks for the thorough follow-up.
Generated by Codex on behalf of Luigi De Masi. This response was generated by an AI agent and may contain inaccuracies.
…-exception filter (#1908) RouteEndpointInfo annotates its properties map with @JsonIgnoreProperties({"route.start.exception"}), but RouteDetailsEndpointInfo re-declared the same field without the annotation plus a getProperties() override, so the actuator detail operation serialized the property the base view filters. That property holds the Throwable itself, so Jackson emitted the whole object graph - nested causes, every stack frame with class, file and line, classloader names and the JDK version. Both the annotation and the shadowing field were introduced together under CAMEL-20993, so the filter was intended and the subclass field simply escaped it. The shadow and its getter are removed and the annotated base-class property is used instead. getProperties() stays available on the subclass by inheritance, so this is source and binary compatible. The regression test uses Awaitility to wait until the supervising route controller has actually recorded route.start.exception before building either view, so the assertions cannot pass vacuously. Closes #1898
…-exception filter (#1909) RouteEndpointInfo annotates its properties map with @JsonIgnoreProperties({"route.start.exception"}), but RouteDetailsEndpointInfo re-declared the same field without the annotation plus a getProperties() override, so the actuator detail operation serialized the property the base view filters. That property holds the Throwable itself, so Jackson emitted the whole object graph - nested causes, every stack frame with class, file and line, classloader names and the JDK version. Both the annotation and the shadowing field were introduced together under CAMEL-20993, so the filter was intended and the subclass field simply escaped it. The shadow and its getter are removed and the annotated base-class property is used instead. getProperties() stays available on the subclass by inheritance, so this is source and binary compatible. The regression test uses Awaitility to wait until the supervising route controller has actually recorded route.start.exception before building either view, so the assertions cannot pass vacuously. Closes #1898
RouteEndpointInfoannotates its properties map soroute.start.exceptionis not serialized:RouteDetailsEndpointInfoextends it but re-declared the same field without the annotation, plus agetProperties()override, so the detail operation serialised the property the base view filters.Both the annotation and the shadowing field arrived in the same commit under CAMEL-20993 — the filter was
clearly intended, and the subclass field simply escaped it. The shadow and its getter are removed; the
annotated base-class property is used instead.
route.start.exceptionholds the actualThrowable(InternalRouteStartupManagerputs it there), so Jacksonwas serialising the whole object graph — nested causes, every stack frame with class/file/line, classloader
names and the JDK version.
No public API change:
RouteDetailsEndpointInfo.getProperties()was an override of a method the publicbase class
RouteEndpointInfoalso declares, so it stays available on the subclass by inheritance — sourceand binary compatible.
Tests
CamelRoutesEndpointStartExceptionTest— 2 cases over the existingcontrolled-barfixture (a route thatfails to start): neither the info nor the detail view serialises
route.start.exception, and both stillserialise the benign route properties.
The supervising controller starts routes asynchronously, so a
@BeforeEachuses Awaitility to wait until theraw route properties actually hold a
Throwableunderroute.start.exceptionbefore either view is built —otherwise the assertions could pass with nothing to leak.
Verified meaningful: the detail case fails against the unpatched code and passes with it; the info case passes
either way, which also confirms
@JsonIgnorePropertiesgenuinely filters map entries, so removing the shadowis sufficient. Full
core/camel-spring-bootsuite: 138 tests, 0 failures.Changed after review
This PR originally also restricted the health-check
error.stacktracedetail to thefullexposure level.That has been dropped, after @luigidemasi pointed out it was not accidental placement:
CamelHealthHelperexplicitly to "align output to be similar to microprofile-health",and its original implementation already put the error block outside the exposure-level branch — the
fullcheck governs metadata filtering of the details map, not the error block.
camel-microprofile-health'stestExposureLevelDefaultassertserror.stacktraceis present at thedefault level.
default"only include detailed information for health check that was DOWN".Restricting it only in Spring Boot would silently diverge the two runtimes. The unauthenticated-exposure
concern is real but belongs with CAMEL-24498, where the observability starter raises
camel.health.exposure-leveltofullandshow-detailstoalwayson an all-interfaces listener — atSpring Boot's own default (
show-details=never) these details are not exposed at all.The
error.messageoverwrite across multiple DOWN checks that @luigidemasi also found is a pre-existingdefect independent of this PR, now tracked as
CAMEL-24512.
Filed by Claude Code on behalf of Andrea Cosentino.