davsclaus commented on code in PR #26268: URL: https://github.com/apache/camel/pull/26268#discussion_r3986588654
########## components/camel-sql/src/test/java/org/apache/camel/component/sql/stored/SqlStoredAllowTemplateFromHeaderTest.java: ########## @@ -0,0 +1,80 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.camel.component.sql.stored; + +import java.util.HashMap; +import java.util.Map; + +import org.apache.camel.CamelExecutionException; +import org.apache.camel.builder.RouteBuilder; +import org.apache.camel.test.junit6.CamelTestSupport; +import org.junit.jupiter.api.Test; +import org.springframework.jdbc.datasource.embedded.EmbeddedDatabase; +import org.springframework.jdbc.datasource.embedded.EmbeddedDatabaseBuilder; +import org.springframework.jdbc.datasource.embedded.EmbeddedDatabaseType; + +import static org.junit.jupiter.api.Assertions.assertThrows; + +public class SqlStoredAllowTemplateFromHeaderTest extends CamelTestSupport { + + private static final String PROC = "SUBNUMBERS(INTEGER :#num1,INTEGER :#num2,OUT INTEGER resultofsum)"; + + private EmbeddedDatabase db; + + @Override + public void doPreSetup() throws Exception { + db = new EmbeddedDatabaseBuilder() + .setName(getClass().getSimpleName()) + .setType(EmbeddedDatabaseType.HSQL) + .addScript("sql/storedProcedureTest.sql").build(); + } + + @Override + public void doPostTearDown() throws Exception { + if (db != null) { + db.shutdown(); + } + } + + @Test + public void headerTemplateIgnoredByDefault() { + // allowTemplateFromHeader defaults to false, so the CamelSqlStoredTemplate header must not override the + // endpoint-configured template; the (placeholder) endpoint template is used instead and fails to parse, + // which is what confirms the header was ignored rather than executed. + Map<String, Object> params = new HashMap<>(); + params.put("num1", 3); + params.put("num2", 1); + Map<String, Object> headers = new HashMap<>(); + headers.put(SqlStoredConstants.SQL_STORED_TEMPLATE, PROC); + headers.put(SqlStoredConstants.SQL_STORED_PARAMETERS, params); + + assertThrows(CamelExecutionException.class, Review Comment: **This test does not actually test the gate — it passes either way.** I set `allowTemplateFromHeader = true` in `SqlStoredEndpoint` (i.e. simulated the gate being broken/reverted), forced a full recompile, and this test still passed. Root cause: `direct:gated` leaves `useMessageBodyForTemplate=false`, so `SqlStoredProducer` reads the stored-procedure parameters from the **message body** (`"unused"`), not from the `CamelSqlStoredParameters` header set on line 63. So the call throws `CamelExecutionException` regardless of which template was selected — the failure never depends on the template-selection branch this PR changed. For contrast, the camel-sql counterpart is genuinely effective: the same mutation makes `testQueryFromHeaderIsIgnoredByDefault` fail at `SqlRouteTest.java:75`. It is only this one that needs rework. Suggested shape: point `direct:gated` at a **valid but different** stored procedure, pass the parameters where the producer actually reads them (the message body), and assert the endpoint procedure's result — mirroring how `SqlRouteTest` asserts `PROJECT == "Linux"`. Asserting on an exception here is inherently brittle; asserting on the result of the *endpoint* template is what proves the header was ignored. And please add the positive counterpart with `allowTemplateFromHeader=true`. ########## components/camel-sql/src/main/java/org/apache/camel/component/sql/DefaultSqlEndpoint.java: ########## @@ -93,6 +93,11 @@ public abstract class DefaultSqlEndpoint extends DefaultPollingEndpoint implemen private boolean breakBatchOnConsumeFail; @UriParam(defaultValue = "true", description = "Whether to allow using named parameters in the queries.") private boolean allowNamedParameters = true; + @UriParam(defaultValue = "false", label = "security", Review Comment: Two annotation issues here: **a) Missing `security = "insecure:dev"`.** `CLAUDE.md` (Annotations) and `design/security.adoc` require insecure opt-in flags to carry the `security` attribute so the security-policy framework can act on them. The exact precedent is one entry up in this same upgrade-guide file — `SmooksEndpoint.allowExternalEntities` uses `security = "insecure:dev", insecureValue = "true"`. Without it, `camel.main.profile=prod` cannot warn or fail when someone flips this gate back on, which is a large part of the value of having the gate at all. **b) `label` should be `producer,security`.** This field lives on `DefaultSqlEndpoint`, which backs both the consumer and the producer, but only `SqlProducer` reads it. As a result the regenerated DSL now offers `allowQueryFromHeader(...)` on `SqlEndpointConsumerBuilder` (`SqlEndpointBuilderFactory.java:966`), where it is a no-op. Compare `useMessageBodyForSql`, which is `label = "producer"`. ```suggestion @UriParam(defaultValue = "false", label = "producer,security", security = "insecure:dev", insecureValue = "true", ``` (Needs a catalog/DSL regen afterwards.) ########## components/camel-sql/src/main/java/org/apache/camel/component/sql/stored/SqlStoredEndpoint.java: ########## @@ -62,6 +62,11 @@ public class SqlStoredEndpoint extends DefaultEndpoint implements EndpointServic private boolean batch; @UriParam(description = "Whether to use the message body as the stored procedure template and then headers for parameters. If this option is enabled then the template in the uri is not used.") private boolean useMessageBodyForTemplate; + @UriParam(defaultValue = "false", label = "security", Review Comment: Same as on `DefaultSqlEndpoint`: this insecure opt-in flag should carry `security = "insecure:dev", insecureValue = "true"` per `CLAUDE.md` (Annotations) and `design/security.adoc`, matching `SmooksEndpoint.allowExternalEntities`. Otherwise `camel.main.profile=prod` cannot report when the gate has been re-opened. `label = "security"` is correct here — sql-stored is producer-only, so no `producer` qualifier is needed. ```suggestion @UriParam(defaultValue = "false", label = "security", security = "insecure:dev", insecureValue = "true", ``` (Needs a catalog/DSL regen afterwards.) ########## docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_23.adoc: ########## @@ -1864,3 +1864,21 @@ Documents carrying an internal DTD subset still parse. To restore the previous behaviour and allow external entity resolution, set the new `allowExternalEntities` option to `true` on the data format or on the endpoint (`smooks:config.xml?allowExternalEntities=true`). + +=== camel-sql, camel-sql-stored - the query/template override headers are gated + +A message header could override the endpoint-configured SQL by default, letting an incoming message choose the +executed statement: + +* `camel-sql`: the `CamelSqlQuery` header replaced the endpoint query. +* `camel-sql-stored`: the `CamelSqlStoredTemplate` header replaced the endpoint template, and its value was + resolved through `SqlHelper.resolveQuery`, which resolves `file:` / `http:` / `classpath:` resources. Review Comment: Small wording mismatch: this line says `file:` / `http:` / `classpath:`, but the `@UriParam` description on `SqlStoredEndpoint` (and the generated catalog JSON) says only `file:/http:`. Worth aligning the two so the docs and the option tooltip agree. ########## components/camel-sql/src/main/java/org/apache/camel/component/sql/stored/SqlStoredProducer.java: ########## @@ -106,15 +106,27 @@ public void execute(StatementWrapper ps) throws SQLException, DataAccessExceptio private StatementWrapper createStatement(Exchange exchange) throws SQLException { String sql; + boolean fromHeader = false; if (getEndpoint().isUseMessageBodyForTemplate()) { sql = exchange.getIn().getBody(String.class); } else { - String templateHeader = exchange.getIn().getHeader(SqlStoredConstants.SQL_STORED_TEMPLATE, String.class); - sql = templateHeader != null ? templateHeader : resolvedTemplate; + String templateHeader = getEndpoint().isAllowTemplateFromHeader() + ? exchange.getIn().getHeader(SqlStoredConstants.SQL_STORED_TEMPLATE, String.class) : null; + if (templateHeader != null) { + sql = templateHeader; + fromHeader = true; + } else { + sql = resolvedTemplate; + } } try { - sql = SqlHelper.resolveQuery(getEndpoint().getCamelContext(), sql, null); + // A header-supplied template is untrusted input, so it must not be resolved as a file:/http: resource + // (SqlHelper.resolveQuery -> ResourceHelper does that) - resolve placeholders only. The endpoint-configured + // template is already resolved in doInit/doStart. + sql = fromHeader + ? SqlHelper.resolvePlaceholders(sql, null) Review Comment: Worth documenting: this also changes behaviour for users who **opt in** with `allowTemplateFromHeader=true`, not just for the default-off case. `SqlHelper.resolveQuery` only calls `resolvePlaceholders` inside `if (ResourceHelper.hasScheme(query))`, so a plain (schemeless) header template previously passed through completely untouched. Now every header template goes through `resolvePlaceholders`, which strips `--` comment lines and blank lines and re-joins the remainder with `\n`. For single-line stored-procedure call syntax that is harmless, so I do not think it needs changing — but it is an undocumented delta for opted-in users and could reasonably get a line in the upgrade-guide entry. ########## components/camel-sql/src/main/java/org/apache/camel/component/sql/stored/SqlStoredProducer.java: ########## @@ -106,15 +106,27 @@ public void execute(StatementWrapper ps) throws SQLException, DataAccessExceptio private StatementWrapper createStatement(Exchange exchange) throws SQLException { String sql; + boolean fromHeader = false; if (getEndpoint().isUseMessageBodyForTemplate()) { sql = exchange.getIn().getBody(String.class); Review Comment: This branch leaves `fromHeader = false`, so with `useMessageBodyForTemplate=true` the **message body** still flows through `SqlHelper.resolveQuery` -> `ResourceHelper` below and can still resolve `file:` / `http:` / `classpath:` schemes — the same resource-resolution vector the PR closes for the header. It is a route-author opt-in, so this is plausibly deliberate scope. Just flagging that the PR's own rationale ("a header-supplied template is untrusted input") applies to body-supplied templates too, and it would be good to say explicitly in the description why the body path is treated differently, or open a follow-up. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
