CAMEL-24294: enforce the typeFilters allow-list in the SnakeYAML TagInspector - #25285
Conversation
…nspector SnakeYAMLDataFormat's TrustedTagInspector.isGlobalTagAllowed() returned true unconditionally, so the SnakeYAML 2.x TagInspector layer was effectively disabled and the typeFilters allow-list was enforced only by the getClassForName constructor override. Make the inspector consult the same allowTypeFilter(...) check so both layers enforce the configured filters. When typeFilters/unmarshalType is configured, a disallowed global tag is now rejected earlier (during composing) as a ComposerException, instead of the previous ConstructorException caused by an IllegalArgumentException from getClassForName. The set of accepted types is unchanged; the security tests and an upgrade-guide note are updated accordingly. Routes without typeFilters are unaffected. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 10 tested, 28 compile-only — current: 9 all testedMaveniverse Scalpel detected 38 affected modules (current approach: 9).
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Well-scoped defence-in-depth fix that correctly aligns the SnakeYAML TagInspector with the existing typeFilters allow-list.
Security analysis:
- Correctness verified: SnakeYAML's Composer guards
isGlobalTagAllowed()behindtag.isCustomGlobal(), which is!secondary && !standardTags.contains(this). Standard YAML tags (!!str,!!int,!!map,!!seq, etc.) are NOT custom global, so they bypass theTagInspectorentirely. The change only affects custom class tags like!!com.example.MyClass. - When
typeFiltersis null (no filtering configured),allowTypeFilter()returnstrue— identical to the previous behavior.SafeConstructorstill blocks custom types at the constructor level. - When
allowAnyTypeis true,typeFiltersis set to"*"which short-circuits totrue— behavior unchanged. - Both
TrustedTagInspectorinstances (Yaml/Composer and BaseConstructor) are updated, providing consistent enforcement at both layers. - CI is green. The 19 camel-snakeyaml tests cover safe constructor rejection, type-filtered allow/deny, regex patterns, and allow-all-types mode.
- The upgrade guide entry correctly notes the exception type change.
This aligns with Camel's security model emphasis on defence-in-depth for deserialization.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of @gnodet
# Conflicts: # docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_22.adoc
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
This review was generated by Claude Code, an AI assistant, on behalf of @gnodet.
Re-review after new commit c059f49 (merge from main)
The new commit is a merge of origin/main into the branch, with the only conflict being in docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_22.adoc. No security-critical code was modified in this merge commit. The CAMEL-20199 changes (ThreadLocal/WeakReference removal, getYaml() renamed to createYaml()) that came in from main do not affect the TrustedTagInspector logic or the allowTypeFilter() check.
Security verification (unchanged from prior review):
- The fix correctly wires
TrustedTagInspector.isGlobalTagAllowed()to consultallowTypeFilter(tag.getClassName()), the same check enforced by thegetClassForName()constructor override. Both layers now enforce the configuredtypeFiltersallow-list consistently. PatternHelper.matchPattern(name, pattern)returnsfalsewhennameisnull(fail-closed), so edge cases inTag.getClassName()cannot bypass the filter.- Standard YAML tags (
!!str,!!int,!!map,!!seq, etc.) are not custom global tags, so SnakeYAML's Composer never callsisGlobalTagAllowed()for them -- unaffected. - When
typeFiltersis null (no filtering configured),allowTypeFilter()returnstrueandSafeConstructorblocks custom types at the constructor level -- behavior unchanged. - Both
TrustedTagInspectorusage sites (increateYaml()for the Composer and indefaultConstructor()for the BaseConstructor) use the same class and both benefit from the fix.
CI: Builds are pending after the merge commit push.
Minor style nit (non-blocking): The touched test assertions in SnakeYAMLTypeFilterHelper use JUnit assertTrue(x instanceof Y) — consider migrating to AssertJ assertThat(x).isInstanceOf(Y.class) and assertThat(x).hasMessageContaining(...) per project conventions.
3 participants
What
SnakeYAMLDataFormat.TrustedTagInspector.isGlobalTagAllowed()returnedtrueunconditionally, so the SnakeYAML 2.xTagInspectorlayer was effectively disabled — thetypeFiltersallow-list was enforced only by thegetClassForNameconstructor override. This makes the inspector consult the sameallowTypeFilter(...)check, so both layers enforce the configured filters (defence-in-depth + consistency).Behaviour
Only affects configurations that set
typeFilters/unmarshalType(non-default). The set of accepted types is unchanged. A disallowed global tag is now rejected earlier — during composing — surfacing as anorg.yaml.snakeyaml.composer.ComposerException("Global tag is not allowed: ...") instead of the previousConstructorExceptioncaused by anIllegalArgumentExceptionfromgetClassForName. Routes withouttypeFiltersare unaffected.Tests
Updated the rejection assertions in
SnakeYAMLTypeFilterHelperto expect theComposerException(all 19 camel-snakeyaml tests green). The allowed-type, SafeConstructor, and all-constructor cases are unchanged. Full-reactormvn clean install -DskipTestsis green.Docs
Added an upgrade-guide note documenting the changed exception on the disallowed-type path.
Backport
Applicable to
camel-4.18.x/camel-4.14.x(both carry the no-op inspector). Since this is defence-in-depth (the allow-list is already enforced bygetClassForNameon all branches) with a changed exception surface, the backport is left to reviewer discretion; PRs can follow the merge if wanted.Closes CAMEL-24294.
Claude Code on behalf of Andrea Cosentino (@oscerd)