CAMEL-24295: anchor the security-scan option match on a token boundary, and mark the Simple nested option - #27117
Merged
Merged
Conversation
Contributor
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
Contributor
✅ Generated files are up to dateAn earlier CI run reported uncommitted generated changes; the latest run no longer does. |
gnodet-bot
approved these changes
Sep 30, 2026
gnodet-bot
left a comment
Contributor
There was a problem hiding this comment.
Solid fix. The unanchored indexOf was a genuine false-positive risk for short keys like tls, ssl, nested — the boundary check is the right approach and the test confirms revert-to-red.
Generated catalog/model JSONs are all consistent with the annotation change. SecurityUtils entries are correctly placed (alphabetical order, right category/owners).
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Contributor
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 575 of 697 tested, 26 compile-only — current: 575 all testedMaveniverse Scalpel detected 575 affected modules (current approach: 575). Skip-tests mode would test 575 modules (6 direct + 571 downstream), skip tests for 26 (generated code, meta-modules) Modules Scalpel would test (575)
Modules with tests skipped (26)
Build reactor — dependencies compiled but only changed modules were tested (6 modules, 1m 24s total)Total reactor time: 1m 24s
Top 20 slowest modules:
|
davsclaus
reviewed
Sep 30, 2026
davsclaus
left a comment
Contributor
There was a problem hiding this comment.
The scanner fix is correct: start-tls no longer matches after normalisation, and the generated catalog, model and schema files are consistent. Some concerns about the nested part:
- Runtime side effect. Adding
nestedtoSecurityUtils.SECURITY_OPTIONSalso affectscamel-mainenforceSecurityPolicies, not only the jbang scanner.getSecurityOptionmatches by option name alone when a key has no component, dataformat or language owner. So anycamel.*key ending in.nested=true(e.g.camel.beans.foo.nested=true) now counts as aninsecure:devviolation.- With
camel.main.profile=prod, startup then fails. nestedis a generic word, so please restrict it to its owner (the simple language) or at least add an upgrade-guide entry.
- There is no test that the scanner reports
nested: true, which is the one new rule. The only new test covers the tls boundary. - Category. Nested evaluation of an untrusted result is expression injection rather than a dev-only feature.
insecure:devmay be the closest existing category, but it is worth deciding on purpose. - Because the scanner matches by name only, it still flags unrelated YAML or JSON that has a
nested: truefield (e.g. a constant JSON body{"nested":true}). That is tolerable, but worth noting.
Claude Code on behalf of davsclaus
…y, and mark the Simple nested option SecurityScanTools.extractOptionValue matched an option key with an unanchored indexOf: it required a boundary after the key (the =/: separator) but not before it, so a longer identifier ending in a security option name matched - startTls=false was reported as insecure tls=false, and isNested / unnested would match a nested rule. The extractor now also requires the character before the key to not be a letter or digit, so the key must start at a token boundary. A delimited tls=false is still detected. The Simple nested attribute (re-evaluates a nested Simple expression in the result, off by default) is an opt-in option with security relevance but had no security metadata. It now carries security=insecure:dev and a security label, so it is generated into SecurityUtils' map and the scanner reports a route that enables it. camel.main.profile=prod only tests camel.* properties, so this gives the jbang scanner coverage rather than the prod profile. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
…, and test the nested scanner rule Review follow-up. Adding the simple/file language "nested" option to SecurityUtils made any configuration key that merely ends in ".nested" an insecure:dev violation, because a key without a component, data format or language owner matches a security option by name alone. camel-main feeds every auto-configured property into that check, so camel.beans.foo.nested=true would fail startup under camel.main.profile=prod. A configuration key without an owner no longer matches an option that only languages declare. Such an option is set on an expression in a route, which the security scanner checks by its bare option name, or under camel.language.<name>. Component and data format options keep matching owner-less keys by name, since camel.beans.* can configure a component or data format bean. "nested" is the only option declared by languages alone, so nothing else changes. SecurityScanToolsTest now covers the nested rule: nested: true on a simple expression is reported as insecure:dev, while nested: false, isNested and unnested: true are not. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
oscerd
force-pushed
the
fix/CAMEL-24295
branch
from
October 2, 2026 08:20
aaf7c9e to
720dc51
Compare
Contributor
Author
|
Thanks, all four points are addressed in 1. Runtime side effect. Restricted to the owner, as you suggested. Component and data format options keep matching such keys by name on purpose, because With that restriction there is no upgrade-guide entry: 2. Scanner test. Added 3. Category. Kept 4. Name-only scanner matches. Agreed: a constant body such as Claude Code on behalf of oscerd |
gnodet-bot
approved these changes
Oct 2, 2026
gnodet-bot
left a comment
Contributor
There was a problem hiding this comment.
Re-review after commit 720dc51 addressing davsclaus's 4 concerns — all addressed:
- Runtime side effect —
isDeclaredByLanguagesOnly+text.indexOf('.') >= 0guard ingetSecurityOptioncorrectly preventscamel.beans.foo.nested,camel.main.nested,camel.kamelet.*.nestedfrom matching, while the barenested(scanner) andcamel.language.simple.nestedstill match. ThetrustAllCertificatespath forcamel.beans.*is unaffected since it has component owners too. Tests confirm. - Scanner test —
detectsNestedSimpleExpressioncovers positive (nested: true→insecure:dev) and negative (nested: false,isNested,unnested: true→ not flagged). - Category —
insecure:devkept; consistent withallowTemplateFromHeader,allowPredicateFromMessage, etc. - Name-only scanner matches — acknowledged; boundary check now rejects
isNested/unnestedprefixed matches.
The extractOptionValue boundary check is clean — Character.isLetterOrDigit before the key position correctly rejects longer identifiers while allowing token-boundary starts (?, &, ,, {, ", line start).
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
approved these changes
Oct 2, 2026
gnodet-bot
left a comment
Contributor
There was a problem hiding this comment.
Re-reviewed after 720dc51 — all four points from the previous review are addressed:
-
Runtime side effect (camel.beans.foo.nested) —
getSecurityOptionnow filters out configuration keys that match a language-only option by bare name.isDeclaredByLanguagesOnlycorrectly distinguishes language options from component/dataformat options that should keep matchingcamel.beans.*keys. Thetext.indexOf('.') >= 0guard ensures the bare namenested(as the scanner checks expression attributes) still matches. -
Scanner test —
detectsNestedSimpleExpressioncovers both the positive case (nested: true→insecure:dev) and negative cases (nested: false,isNested,unnested: true). -
Category —
insecure:devis consistent with the other injection-shaped opt-ins (allowTemplateFromHeader,allowPredicateFromMessage,allowQueryFromHeader). The categories are a closed set with corresponding policy options, so a new one would need its own policy option. -
Name-only matches — Acknowledged as a known limitation of the line-based scanner; the boundary check eliminates the worst false positives (
isNested,unnested).
The SecurityUtilsTest additions (testLanguageOptionMatchesOnlyItsOwnLanguage, testDetectViolationsIgnoresBeanPropertyNamedLikeALanguageOption) are thorough — they cover camel.language.simple.nested (match), camel.language.xpath.nested (reject, wrong owner), camel.beans.foo.nested (reject, language-only), camel.main.nested (reject), bare nested (match), and confirm camel.beans.myClient.trustAllCertificates still matches (component option, not language-only).
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
approved these changes
Oct 2, 2026
davsclaus
left a comment
Contributor
There was a problem hiding this comment.
Thanks @oscerd, all four points are addressed: a language-only option now matches only its own language (so camel.beans.foo.nested and friends are no longer flagged), the scanner has a test for nested, and the insecure:dev category is a reasonable decision.
Optional follow-up, not from this PR: extractOptionValue stops at a " straight away, so quoted values (XML attributes like nested="true", YAML nested: "true") are missed for every option, and Java DSL .nested(true) isn't detected either.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CAMEL-24295: anchor the security-scan option match on a token boundary, and mark the Simple nested option
Background
Two related changes, following the investigation on the issue.
camel-jbang security scanner (the load-bearing part).
SecurityScanTools.extractOptionValuefound an option key with an unanchored
indexOf: it checked a token boundary after the key(the
=/:separator) but not before it. So a longer identifier that merely ends in a securityoption name matched — for example
startTls=falsewas reported as an insecuretls=false, andisNested=true/ an unrelated...nestedfield would match anestedrule. The scanner readsarbitrary route source text, which the catalog cannot vet, so this is a real false-positive risk.
The extractor now also requires the character before the key to not be a letter or digit, so the
key must start at a token boundary (
?,&,,,{,", line start, …). A boundary-delimitedtls=falseis still detected.Simple
nestedoption. Thenestedattribute ofSimpleExpressionre-evaluates a nested Simpleexpression contained in the result (off by default). It is an opt-in option with security relevance
but carried no security metadata. It now has
security = "insecure:dev"and asecuritylabel, so itis generated into
SecurityUtils' option map and the security scanner reports a route that enablesit. (As noted on the issue,
camel.main.profile=prodonly testscamel.*properties, so aroute-inline attribute is out of that detector's reach; this change gives the jbang scanner coverage,
which is why fixing the scanner's matching first matters — otherwise a short/generic key would be
prone to exactly the false positives fixed above.)
Owner-scoped language options (review follow-up).
SecurityUtils.getSecurityOptionmatches aconfiguration key that has no component, data format or language owner by the option name alone, and
camel-main feeds every auto-configured property into it. Adding
nestedtherefore turned any key thatmerely ends in
.nested(for examplecamel.beans.foo.nested=true) into aninsecure:devviolation,which fails startup under
camel.main.profile=prod. An owner-less configuration key no longer matchesan option that only languages declare: such an option is set on an expression in a route (checked by
the scanner under its bare name) or under
camel.language.<name>.. Component and data format optionskeep matching owner-less keys by name on purpose, since
camel.beans.*can configure a component ordata format bean.
nestedis the only option declared by languages alone, so no other option changes,and since
nestedis an expression attribute rather than a language property, camel-main has no realkey that is now flagged — hence no upgrade-guide entry.
Tests
SecurityScanToolsTest.securityOptionMatchingIsAnchoredOnTokenBoundaries:startTls=falseis notflagged, while a real
tls=falsestill is. Revert-to-red verified (removing the boundary checkre-introduces the
startTlsfalse positive).SecurityScanToolsTest.detectsNestedSimpleExpression:nested: trueon a simple expression isreported as
insecure:dev;nested: false,isNestedandunnested: trueare not.SecurityUtilsTest.testLanguageOptionMatchesOnlyItsOwnLanguageandtestDetectViolationsIgnoresBeanPropertyNamedLikeALanguageOption:camel.language.simple|file.nestedand the bare
nestedmatch,camel.beans.foo.nested/camel.main.nested/camel.kamelet.*.nesteddo not, and
camel.beans.myClient.trustAllCertificatesstill does. Revert-to-red verified (both failon the previous
SecurityUtils).Full reactor
mvn clean install -DskipTestsgreen with no regenerated-file drift;camel-util(294),
SecurityScanToolsTest(31) and the camel-main security tests (42) pass.Catalog, model metadata and
SecurityUtilsregenerated.Claude Code on behalf of @oscerd
🤖 Generated with Claude Code