CAMEL-23543: Centralize and harden Camel* header filtering in DefaultHeaderFilterStrategy#23652
CAMEL-23543: Centralize and harden Camel* header filtering in DefaultHeaderFilterStrategy#23652ammachado wants to merge 7 commits into
Conversation
davsclaus
left a comment
There was a problem hiding this comment.
Well-executed security hardening — the change aligns with design/headers.adoc which states that DefaultHeaderFilterStrategy should strip Camel* and org.apache.camel.* by default. Making that the actual default rather than requiring each component to opt in is the right move.
All 6 deleted strategy classes were verified as pure no-ops, callers are properly updated, the .clone() on field initializers prevents shared mutable state, and the JMS opt-out is correctly implemented with good test coverage.
One minor suggestion on the upgrade guide below.
This review covers project conventions and correctness. It does not replace specialized review tools such as CodeRabbit, Sourcery, or SonarCloud.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
Note that |
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
Build reactor — dependencies compiled but only changed modules were tested (27 modules)
|
davsclaus
left a comment
There was a problem hiding this comment.
Convention & Correctness Review
Well-structured security hardening PR that centralizes header filtering from 20+ component-specific strategies into DefaultHeaderFilterStrategy. The approach is sound, the .clone() on field initializers prevents shared mutable state, the JMS opt-out is correctly implemented with good test coverage, and all existing subclasses are touched — no unintentional inheritance gaps.
Findings
1. PR description claims CAMEL_FILTER_STARTS_WITH extended — but it wasn't
The description states CAMEL_FILTER_STARTS_WITH was extended to ["Camel", "camel", "org.apache.camel"]. The code does not change this constant — it remains ["Camel", "camel"]. The new core tests correctly assert that org.apache.camel.* headers pass through (which aligns with the committer's comment that this prefix is a Camel 1.x artifact). The PR description should be corrected to avoid misleading future readers.
2. Dead/inconsistent org.apache.camel code in CAMEL_FILTER_PATTERN and tryPattern
The deprecated CAMEL_FILTER_PATTERN regex and the tryPattern optimization were updated to also match org.apache.camel, but CAMEL_FILTER_STARTS_WITH (the active mechanism) does not include it, and no code in the codebase references CAMEL_FILTER_PATTERN anymore. This creates an inconsistency — the deprecated pattern matches org.apache.camel, the startsWith doesn't, and the tryPattern optimization handles it but never executes. Consider reverting these changes for consistency.
3. HttpProtocolHeaderFilterStrategy asymmetric opt-out
Only setInFilterStartsWith((String[]) null) is added. The outbound direction now inherits the default Camel filter via the field initializer. While this strategy is primarily used for inbound filtering in HttpProducer.copyHeaders(), the asymmetry could surprise someone using it for outbound. Consider also nulling outbound for symmetry, or adding a comment explaining the inbound-only intent.
4. Minor: regex bug fix in deprecated CAMEL_FILTER_PATTERN not called out
The original pattern had A-z (which accidentally includes ASCII chars [\]^_\``) — the PR fixes this to A-Z`. Genuine bug fix but not mentioned in the description or upgrade guide.
No Issues Found
- All 6 deleted subclasses verified as pure no-ops — correct to remove
ClassicJmsHeaderFilterStrategyopt-out properly implemented and well-testedsetLowerCase(true)removal safe — default alreadytrueon main- Upgrade guide is comprehensive with opt-out guidance
evalFilterMatchrefactoring is logically equivalent
This review covers project conventions and correctness. It does not replace specialized review tools such as CodeRabbit, Sourcery, or SonarCloud.
This review was generated by an AI agent (Claude Code on behalf of Claus Ibsen) and may contain inaccuracies. Please verify all suggestions before applying.
…efaultHeaderFilterStrategy
- Default `inFilterStartsWith` and `outFilterStartsWith` to `CAMEL_FILTER_STARTS_WITH` in
`DefaultHeaderFilterStrategy`, so all consumers and producers block `Camel*`, `camel*`, and
`org.apache.camel.*` headers by default without per-component boilerplate
- Extend `CAMEL_FILTER_STARTS_WITH` to include `org.apache.camel.` alongside `Camel` and `camel`
- Remove now-redundant `setInFilterStartsWith` / `setOutFilterStartsWith(CAMEL_FILTER_STARTS_WITH)`
calls from 20+ component-specific strategies
- `ClassicJmsHeaderFilterStrategy` explicitly opts out (null) to preserve legacy pass-through behavior
- `KafkaHeaderFilterStrategy` builds its array from the constant + `kafka.` via `Arrays.copyOf`
- `Sns2` / `Sqs2` / `KnativeHttp` replace redundant regex patterns with a `getOutFilter().add("breadcrumbId")`
set entry (SNS/SQS) or no extra filtering (Knative), relying on the default startsWith
- Add tests verifying both directions filter by default; update 4.21 upgrade guide
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
…trategy subclasses - DefaultHeaderFilterStrategy: use .clone() on field initializers so each instance gets its own array copy; mutating the public CAMEL_FILTER_STARTS_WITH constant no longer corrupts all instances - Change CAMEL_FILTER_STARTS_WITH from "org.apache.camel." to "org.apache.camel" (no trailing dot) so the bare prefix "org.apache.camel" is also blocked - Update deprecated CAMEL_FILTER_PATTERN regex and its fast-path in tryPattern to cover the org.apache.camel namespace consistently - Remove redundant setLowerCase(true) calls from HTTP strategies (lowerCase=true is already the field-initializer default) - Fix HttpBridgeMultipartRouteTest inner strategy: remove setLowerCase(true) and the now-redundant setOutFilterStartsWith(CAMEL_FILTER_STARTS_WITH) call - Add ClassicJmsHeaderFilterStrategyTest to directly assert Camel headers pass through in both directions in legacy mode (opt-out via null startsWith) - Expand upgrade guide to cover subclass authors, not just direct instantiators; add pointer to ClassicJmsHeaderFilterStrategy as a worked opt-out example - Remove six no-op header filter strategy subclasses whose only body was setLowerCase(true), which is now the default: CoAPHeaderFilterStrategy, CometdHeaderFilterStrategy, IggyHeaderFilterStrategy, NatsHeaderFilterStrategy, VertxWebsocketHeaderFilterStrategy, XmppHeaderFilterStrategy. Update all callers to instantiate DefaultHeaderFilterStrategy directly. Delete associated test files. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
- Remove org.apache.camel from CAMEL_FILTER_STARTS_WITH: this prefix was a Camel 1.x artifact; Camel 2+ uses Camel* syntax only - Add upgrade guide note listing the 6 removed header filter strategy classes and directing users to use DefaultHeaderFilterStrategy instead Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Adriano Machado <60320+ammachado@users.noreply.github.com> Signed-off-by: Adriano Machado <60320+ammachado@users.noreply.github.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
…pt-out DefaultHeaderFilterStrategy already defaults lowerCase to true; remove the redundant setLowerCase(true) calls from all subclasses. Also add upgrade guide note on setLowerCase(false) for users who need case-sensitive header matching. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
…eaders in copyHeaders HttpProtocolHeaderFilterStrategy inherited the new default inFilterStartsWith (["Camel", "camel"]) from DefaultHeaderFilterStrategy, causing MessageHelper.copyHeaders in HttpProducer to silently drop Camel-prefixed headers (e.g. CamelFileName) from the request exchange when building the response message. Reset inFilterStartsWith to null in initialize() since this strategy only filters HTTP protocol headers, not Camel internals. Also: switch HttpProducer to import from camel-http-base directly, deprecate the empty camel-http-common wrapper class, and document both in the 4.21 upgrade guide. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
- Revert CAMEL_FILTER_PATTERN and tryPattern to their original state; the org.apache.camel prefix is a Camel 1.x artifact and adding it to the deprecated constant created inconsistency with CAMEL_FILTER_STARTS_WITH. - Add setOutFilterStartsWith(null) to HttpProtocolHeaderFilterStrategy for symmetry with the inbound opt-out already present. - Clarify upgrade guide: deleted strategy classes should be replaced with `new DefaultHeaderFilterStrategy()` (not just the class name). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Root cause analysis for Claude Code on behalf of Adriano Machado Both tests were broken by commit Both failing tests use These issues are addressed by PR #23811. |
Description
Centralises Camel-internal header filtering in
DefaultHeaderFilterStrategyso every component gets secure defaults without having to callsetInFilterStartsWith/setOutFilterStartsWithexplicitly.DefaultHeaderFilterStrategynow initializesinFilterStartsWithandoutFilterStartsWithtoCAMEL_FILTER_STARTS_WITHby default (previouslynull).ClassicJmsHeaderFilterStrategyexplicitly opts out viasetInFilterStartsWith((String[]) null)to preserve legacy Camel-header-passthrough behaviour.camel-4x-upgrade-guide-4_21.adoc).setLowerCase(true)calls: removed fromcamel-http-baseandcamel-http-commonHTTP strategies;lowerCase=trueis already the field-initialiser default.ClassicJmsHeaderFilterStrategyTestdirectly asserting that Camel headers pass through in both directions.CoAPHeaderFilterStrategy,CometdHeaderFilterStrategy,IggyHeaderFilterStrategy,NatsHeaderFilterStrategy,VertxWebsocketHeaderFilterStrategy, andXmppHeaderFilterStrategy. All six classes and their test files have been deleted; callers instantiateDefaultHeaderFilterStrategydirectly.Target
mainbranch)Tracking
Apache Camel coding standards and style
mvn clean install -DskipTestslocally from root folder and I have committed all auto-generated changes.Claude Code on behalf of Adriano Machado