CAMEL-24161: camel-salesforce - fix medium-severity bugs from code review#24857
Conversation
…view Fixes 11 bugs found during a deep code review of camel-salesforce: - SalesforceLoginConfig: mask refreshToken in toString() to prevent credential leak to debug logs (other secrets were already masked) - SalesforceSecurityHandler: check body.rewind() return value to detect unreplayable multipart content on 401 retry - AbstractClientBase: detect Jetty failure chunks in replay-buffering loop to prevent silently truncated requests - DefaultRawClient: use put() instead of add() for auth headers to prevent duplicate X-SFDC-Session/Authorization on retry - SalesforceSecurityHandler: coordinate re-logins via session.login() on worker pool so concurrent 401s share one login request instead of each firing its own - SubscriptionHelper: use getOrDefault() to prevent NPE when channel is concurrently unsubscribed, and remove from channelsToSubscribe on unsubscribe to prevent stale entries - PubSubApiClient: invoke consumer exception handler on per-event decode failures instead of silently swallowing them - PubSubApiClient: stop infinite retry on corrupted replay ID when fallbackToLatestReplayId=false, and make replay state per-observer to prevent preset overwrite across consumers - JsonRestProcessor: clean up RESPONSE_CLASS_DEFERRED and RESPONSE_CLASS_PREFIX exchange properties in processResponse() to match processStreamResultResponse() cleanup - CompositeApiProcessor: preserve exception when Salesforce returns both error body and exception (raw, batch, and composite response) - AbstractRestProcessor: fix deleteSObjectWithId to restore DTO fields before completing exchange (matching all sibling operations), and null-check external-id field value to prevent NPE Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Claude Code on behalf of gnodet
Excellent work — thorough, well-organized fix for 11 distinct medium-severity bugs in camel-salesforce. Each fix is narrow, clearly motivated, and well-documented in the PR description.
Changes verified:
Auth / session / HTTP client:
-
SalesforceLoginConfig.toString()— refreshToken masking ✅
refreshTokenwas the only secret printed in cleartext whileclientSecret,password, andkeystorewere already masked since CAMEL-15425. Trivially correct. -
AbstractClientBase— failure chunk detection ✅
Follows the JettyContent.ChunkAPI contract:isFailure()must be checked beforeisLast(). Without this, a mid-read I/O error silently truncated the request body. Properly releases the chunk before throwing. -
DefaultRawClient.setAccessToken()—put()vsadd()✅
Confirmedadd()in the currentmainbranch — would produce duplicateX-SFDC-Session/Authorizationheaders on auth retry.put()correctly replaces. -
SalesforceSecurityHandler— coordinated re-login via worker pool ✅
Moves from per-401 inline login tosession.login()on the worker pool for single-flight coordination. Also checksbody.rewind()return value — appropriate warning-only approach since bodyless requests (GET/DELETE) would still succeed.
Streaming / Pub/Sub:
-
PubSubApiClient— per-observer replay state ✅
MovinginitialReplayPreset,initialReplayId,fallbackToLatestReplayIdfrom class-level toFetchResponseObserverinstance fields prevents cross-consumer preset overwrite when multiple consumers share onePubSubApiClient. Clean solution. -
PubSubApiClient— stop infinite retry on corrupt replay ID ✅
WhenfallbackToLatestReplayId=false, thereturnafter settingreplayId = nullcorrectly stops the retry loop. The exception handler is already invoked to report the error. -
PubSubApiClient— per-event decode failure handling ✅
Previously only logged errors — events were silently lost (replay ID advanced past them). Now routes toconsumer.getExceptionHandler(). -
SubscriptionHelper—getOrDefault()andchannelsToSubscribecleanup ✅
Prevents NPE from concurrent unsubscribe race, and removes stale entries fromchannelsToSubscribeto prevent silent channel loss on reconnect.
REST / Bulk / Composite:
-
JsonRestProcessor—RESPONSE_CLASS_DEFERRED/RESPONSE_CLASS_PREFIXproperty cleanup ✅
RESPONSE_CLASSwas already cleaned up but these two were leaked into subsequent producers on the same exchange. -
CompositeApiProcessor— exception preservation ✅
Critical fix — three response methods all had the same bug: exception was silently swallowed. All three now setexchange.setException(exception)when non-null. -
AbstractRestProcessor—deleteSobjectWithIdfixes ✅- Ordering: confirmed all sibling methods (
getSobjectWithId,upsertSobject) callrestoreFieldsbeforeprocessResponse— this was the only one with reversed order. - Null-check:
oldValue.toString()NPE risk confirmed inmainbranch. Correct fallback togetParameter().
- Ordering: confirmed all sibling methods (
Test update:
PubSubApiTest correctly reflects the behavioral change — expects 1 subscribe call and 1 exception handler call (was 3 of each). Thread.sleep(1000) removal is a bonus.
No issues found.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 11 tested, 29 compile-only — current: 11 all testedMaveniverse Scalpel detected 40 affected modules (current approach: 11).
|
2 participants
Summary
Fixes 11 medium-severity bugs found during the July 2026 deep code review of camel-salesforce (CAMEL-24161). Companion to #24848 which fixed the high-severity findings.
Auth / session / HTTP client:
refreshTokeninSalesforceLoginConfig.toString()— was printed in clear text while all other secrets were masked (since CAMEL-15425)body.rewind()return value inSalesforceSecurityHandler— multipart uploads silently sent empty body on 401 retryAbstractClientBasereplay-buffering loop — mid-read I/O error produced silently truncated requestsput()instead ofadd()inDefaultRawClient.setAccessToken()— auth retry produced duplicateX-SFDC-Session/Authorizationheaderssession.login()on worker pool inSalesforceSecurityHandler— N concurrent 401s now share one login instead of each firing its ownStreaming / Pub/Sub:
getOrDefault()inSubscriptionHelperconnect listener and clean upchannelsToSubscribeon unsubscribe — prevents NPE and silent channel loss on concurrent unsubscribeconsumer.getExceptionHandler()on per-event decode failures inPubSubApiClient— events were silently lost (log only, replay ID advanced past them)fallbackToLatestReplayId=falseinPubSubApiClient, and make replay preset state per-observer instead of shared — prevents infinite loop and cross-consumer preset overwriteREST / Bulk / Composite:
RESPONSE_CLASS_DEFERRED/RESPONSE_CLASS_PREFIXexchange properties inJsonRestProcessor.processResponse()— leaked into subsequent producers on the same exchangeCompositeApiProcessor— HTTP failures were silently reported as success in raw/batch/composite modesdeleteSObjectWithIdinAbstractRestProcessorto restore DTO fields before completing the exchange (matching all sibling operations), and null-check external-id field value to prevent NPETest plan
PubSubApiTest.testInvokesExceptionHandlerWhenReplayIdIsCorruptedAndFallbackToLatestReplayIdIsDisabledto verify fixed behavior (stops after one attempt instead of infinite retry)Claude Code on behalf of davsclaus
🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com