CAMEL-24158: Fix medium and high-severity bugs across Azure components - #24884
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
left a comment
There was a problem hiding this comment.
Review — CAMEL-24158: Fix medium and high-severity bugs across Azure components
Thorough batch of 9 genuine bug fixes across 5 Azure components. Each fix addresses a real correctness or security issue. Nice work on the scope coverage.
Fix-by-fix analysis
azure-eventhubs: Header filter strategy ✅ (Security)
Extending HeaderFilterStrategyComponent is the right pattern — this prevents Camel internal headers (Camel*, org.apache.camel.*) from leaking into AMQP application properties, which aligns with the project's security model (CVE-2025-27636 family). The null-guard in EventHubsProducerOperations.createEventDataFromObject() cleanly preserves backward compat when no strategy is set (ITs pass null).
The consumer-side fix (only setting PARTITION_KEY when non-null) is correct — prevents the producer path from choking on a null partition key header.
Suggestion: This changes the default behavior for existing users — Camel internal headers that were previously forwarded to Event Hubs will now be filtered. Consider adding a brief upgrade guide entry noting this, e.g. "azure-eventhubs producer now filters Camel-internal headers via DefaultHeaderFilterStrategy; set
headerFilterStrategy=#noneto restore the old pass-through behavior."
azure-servicebus: pendingExchanges leak fix ✅
The try-catch after pendingExchanges.incrementAndGet() is the correct pattern. Decrementing the counter and abandoning the message (PEEK_LOCK) ensures no orphaned messages or counter drift. The catch (Exception abandonEx) swallow on the abandon path is fine — the primary exception is the one that matters.
azure-storage-blob: Lease ID fix ✅
Moving the null-check from acquireLeaseIfConfigured() to getCommonRequestOptions() ensures the BlobRequestConditions reference is never null at any downstream call site. Previously, acquireLeaseIfConfigured() would create a new object, but it was a local — the lease ID would be set on a discarded reference when the method returned. Now the shared reference lives in the caller.
azure-storage-blob: Non-mark InputStream fix ✅
Good layered approach:
BlobUtils.getInputStreamLength()addsFileInputStreamfast path and returns-1(not0) for unknown lengthBlobStreamAndLengthandBlobBlockwrap non-mark streams inBufferedInputStreambefore callinggetInputStreamLength()- This ensures the
-1path is effectively unreachable in normal use (wrapped streams support mark)
azure-storage-blob: moveAfterRead copy fix ✅
Switching from copyFromUrl() to beginCopy() is correct — copyFromUrl() requires the source URL to be publicly accessible or have a SAS token appended, which breaks for private blobs using shared key auth. The waitForCompletion(timeout) with a configurable timeout is a good pattern.
azure-storage-blob: Page-blob range off-by-one ✅
end - start → end - start + 1 — Azure page blob ranges are inclusive on both ends. Clear fix.
azure-storage-datalake: openInputStream fix ✅
Replacing openQueryInputStream("SELECT * from BlobStorage") with the standard openInputStream().getInputStream() removes the dependency on the Blob Query API (which has known issues, hence the SkipLastByteInputStream workaround). The deprecation of SkipLastByteInputStream is properly annotated with @Deprecated.
azure-storage-queue: popReceipt fix ✅
The old save/restore mechanism was actively harmful — it restored pre-processing header values in the completion handler, which would clobber any legitimate header updates made during route processing (e.g., after updateMessage changes the popReceipt). Removing it is correct.
Checklist
- All fixes address genuine bugs
- Generated files regenerated (catalog JSON, configurer, component DSL)
- IT tests updated for constructor changes
-
SkipLastByteInputStreamproperly deprecated with@Deprecated - No new dependencies added
- Upgrade guide entries: Several behavioral changes should be documented:
- EventHubs now filters Camel internal headers (most impactful)
- Data Lake
openInputStream()no longer uses Query API - Page-blob range calculation corrected (existing users with workarounds may need adjustment)
SkipLastByteInputStreamdeprecated
CI
Builds still pending.
Reviewed with Claude Code (claude-code/1.0) on behalf of gnodet. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
|
Claude Code on behalf of davsclaus @gnodet Thanks for the thorough review! Good catch on the upgrade guide entries — added them in 7a85bcb for the three most impactful behavioral changes:
|
gnodet
left a comment
There was a problem hiding this comment.
Claude Code on behalf of gnodet
Follow-up review — commit 3 (7a85bcb): Upgrade guide entries ✅
The new commit adds well-crafted upgrade guide entries for the three behavioral changes in this PR:
- EventHubs header filtering — documents the new
DefaultHeaderFilterStrategybehavior and provides a clear escape hatch (headerFilterStrategy=#none) for users who need the old pass-through behavior - Data Lake openInputStream — documents the switch from Blob Query API to standard
openInputStream()and theSkipLastByteInputStreamdeprecation - Page-blob range calculation — documents the off-by-one correction with guidance to remove any existing workarounds
All entries follow the project's upgrade guide conventions and are placed correctly in camel-4x-upgrade-guide-4_22.adoc. These address my earlier observation about documenting behavioral changes — nice work.
My approval from the previous review still stands.
🤖 This review was generated by an AI agent (Claude Code) on behalf of gnodet. AI-generated reviews are advisory — human judgment should guide final decisions.
gnodet
left a comment
There was a problem hiding this comment.
Claude Code on behalf of gnodet
Follow-up review — commit 4 (f0d2fba): Backport upgrade guide entries ✅
Adds the EventHubs header filtering upgrade guide entry to camel-4x-upgrade-guide-4_14.adoc and camel-4x-upgrade-guide-4_18.adoc, matching the backport PRs #24886 and #24887. The text is identical to the 4.22 entry from commit 3, and both are placed in the correct upgrade section.
Approval still stands.
🤖 This review was generated by an AI agent (Claude Code) on behalf of gnodet. AI-generated reviews are advisory — human judgment should guide final decisions.
gnodet
left a comment
There was a problem hiding this comment.
Claude Code on behalf of gnodet
Follow-up review — commit 5 (794a0af): Remove incorrect escape hatch ✅
Good catch — removes the headerFilterStrategy=#none advice from all three upgrade guides. There's no bean named none registered by default, so suggesting that syntax would have been misleading. The entries now correctly state that ordinary application headers are unaffected, without suggesting an escape hatch that doesn't exist out of the box.
Approval still stands.
🤖 This review was generated by an AI agent (Claude Code) on behalf of gnodet. AI-generated reviews are advisory — human judgment should guide final decisions.
…ent Camel headers leaking to AMQP properties Backport of header filter strategy fix from PR #24884 to 4.18.x. EventHubsComponent now extends HeaderFilterStrategyComponent so that the default DefaultHeaderFilterStrategy filters out Camel-internal headers before they are put on EventData application properties. Also guards against setting a null PARTITION_KEY header on the consumer side. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
…ent Camel headers leaking to AMQP properties Backport of header filter strategy fix from PR #24884 to 4.14.x. EventHubsComponent now extends HeaderFilterStrategyComponent so that the default DefaultHeaderFilterStrategy filters out Camel-internal headers before they are put on EventData application properties. Also guards against setting a null PARTITION_KEY header on the consumer side. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 562 tested, 26 compile-only — current: 559 all testedMaveniverse Scalpel detected 588 affected modules (current approach: 559).
|
Blob: fix lease ID lost on discarded BlobRequestConditions, fix non-mark InputStream uploaded as 0-byte blob (add StreamCache/FileInputStream support), fix moveAfterRead copy using beginCopy instead of bare URL copyFromUrl, fix page-blob range off-by-one. ServiceBus: fix processMessage exception leaking pendingExchanges counter and unsettled messages. EventHubs: fix consumer setting both PARTITION_ID and PARTITION_KEY causing producer IllegalArgumentException, add HeaderFilterStrategy to prevent all Camel headers leaking to AMQP application properties. Queue: fix popReceipt clobbering by removing stale header save/restore in completion handler. DataLake: fix openInputStream using Blob Query API instead of standard read, deprecate unused SkipLastByteInputStream. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
…ategy Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
…9 upgrade guides Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
… guides Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
794a0af to
30e97e1
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> CAMEL-24158: camel-azure-eventhubs - Add HeaderFilterStrategy to prevent Camel headers leaking to AMQP properties Backport of header filter strategy fix from PR #24884 to 4.18.x. EventHubsComponent now extends HeaderFilterStrategyComponent so that the default DefaultHeaderFilterStrategy filters out Camel-internal headers before they are put on EventData application properties. Also guards against setting a null PARTITION_KEY header on the consumer side. Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> CAMEL-24158: camel-azure-eventhubs - Add HeaderFilterStrategy to prevent Camel headers leaking to AMQP properties Backport of header filter strategy fix from PR #24884 to 4.14.x. EventHubsComponent now extends HeaderFilterStrategyComponent so that the default DefaultHeaderFilterStrategy filters out Camel-internal headers before they are put on EventData application properties. Also guards against setting a null PARTITION_KEY header on the consumer side. Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Claude Code on behalf of davsclaus
Fixes 9 medium and high-severity bugs found in a deep code review of 5 Azure components (blob, servicebus, eventhubs, queue, datalake). See CAMEL-24158.
azure-storage-blob (4 fixes)
getCommonRequestOptions()now ensuresBlobRequestConditionsis never null, soacquireLeaseIfConfigured()mutates the shared reference visible at all 7+ call sitesStreamCacheandFileInputStreamfast paths inBlobUtils.getInputStreamLength()(aligned with AWS S3 pattern), wrap non-mark streams inBufferedInputStreaminBlobStreamAndLengthandBlobBlock, return -1 instead of 0 for unknown lengthcopyFromUrl()(requires publicly accessible URL or SAS) tobeginCopy()(supports same-account shared key auth) with configurable timeoutend - start→end - start + 1azure-servicebus (1 fix)
pendingExchanges.incrementAndGet(), ensuring counter is always decremented and messages are abandoned (PEEK_LOCK) on errorazure-eventhubs (2 fixes)
PARTITION_KEYheader wheneventData.getPartitionKey()is non-nullEventHubsComponentnow extendsHeaderFilterStrategyComponent, producer filters headers throughDefaultHeaderFilterStrategybefore putting onEventDataazure-storage-queue (1 fix)
deleteMessageafter mid-routeupdateMessagecallsazure-storage-datalake (1 fix)
openQueryInputStream("SELECT * from BlobStorage")+SkipLastByteInputStreamworkaround with standardclient.openInputStream().getInputStream(). DeprecatedSkipLastByteInputStreamincamel-utilTest plan
camel-azure-storage-blobunit tests passcamel-azure-servicebusunit tests passcamel-azure-eventhubsunit tests pass (updated IT constructor calls for new parameter)camel-azure-storage-queueunit tests passcamel-azure-storage-datalakeunit tests pass🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com