oscerd commented on code in PR #26268: URL: https://github.com/apache/camel/pull/26268#discussion_r4014224229
########## 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: You were right, and the mutation test is the part that matters — thank you for running it rather than reading it. Rewritten along the lines you suggested, with one change: rather than a second procedure, both templates call the same `SUBNUMBERS` but alias the OUT parameter differently (`resultfromendpoint` vs `resultfromheader`), so the key *present in the result map* names which template text executed. That makes both directions mutation-resistant rather than just the negative one: - `headerTemplateIgnoredByDefault` asserts `resultfromendpoint == 2` **and** that `resultfromheader` is absent. - `headerTemplateHonouredWhenAllowed` (the positive counterpart you asked for) asserts the reverse on a `?allowTemplateFromHeader=true` endpoint. No assertion is on an exception any more, so neither test can pass for a reason unrelated to template selection. Apologies this sat unanswered since the 11th — the code went in on the 14th and the replies did not follow it. _Claude Code on behalf of @oscerd_ ########## 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: Both applied, exactly as suggested: ```java @UriParam(defaultValue = "false", label = "producer,security", security = "insecure:dev", insecureValue = "true", ``` The `SmooksEndpoint.allowExternalEntities` precedent is the right one to follow — a gate the prod profile cannot see is a gate that only helps the people who were not going to re-open it. Catalog and both DSL mirrors regenerated, so `allowQueryFromHeader` no longer appears on `SqlEndpointConsumerBuilder`. _Claude Code on behalf of @oscerd_ ########## 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: Applied, with `label = "security"` as you note — sql-stored is producer-only. Regenerated. _Claude Code on behalf of @oscerd_ ########## 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: Agreed it does not need changing, and it should be documented — it was the one item from your round still outstanding when this merged, so it is going in as [CAMEL-24755](https://issues.apache.org/jira/browse/CAMEL-24755) rather than being lost: > Opting back in is not quite the previous behaviour. `SqlHelper.resolveQuery` applied placeholder resolution only to a value that had a scheme, so a schemeless header template used to reach the database untouched; it now always goes through `SqlHelper.resolvePlaceholders`, which drops `--` comment lines and blank lines and re-joins the rest with newlines. That is immaterial for the single-line call syntax a stored-procedure template normally uses, but a route passing a multi-line commented template through the header will see the comments stripped. Framed as what an opted-in user would notice, since for everyone else the header is off and the delta is unreachable. _Claude Code on behalf of @oscerd_ ########## 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: You talked me out of the narrow scope — the body path is now closed too. `createStatement` carries a `runtime` flag set by *both* branches: the body under `useMessageBodyForTemplate=true` and the header under `allowTemplateFromHeader=true`. A runtime template gets `resolvePlaceholders` only; `SqlHelper.resolveQuery` (and so `ResourceHelper`) is reached only by the endpoint-configured template, which is resolved once at startup. You are right that the PR's own rationale applied to the body, and it would have been an odd place to stop: `useMessageBodyForTemplate` is an opt-in about *where the template text comes from*, not a statement that the body is trusted to name a file. It matches what `camel-sql` already does with `useMessageBodyForSql`. Documented in the upgrade guide and covered by `SqlStoredUseMessageBodyForTemplateResourceTest`. _Claude Code on behalf of @oscerd_ ########## 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: You were right, and it was worse than a mismatch between two places — the guide said three different things and the `@UriParam` a fourth: - "resolves `file:` / `http:` / `classpath:` resources" - "never as a `file:` / `http:` resource" - "`file:`, `http:`, `https:` or `classpath:`" (the complete one) - `SqlStoredEndpoint`: "never as a `file:/http:` resource" On a security gate that is not cosmetic: a reader checking whether the gate covers their case could conclude from the shorter lists that `classpath:` and `https:` header templates are still resolved, which they are not. This landed before I got the fix pushed, so it is going in as [CAMEL-24755](https://issues.apache.org/jira/browse/CAMEL-24755) — all four aligned on the full list, catalog regenerated. _Claude Code on behalf of @oscerd_ ########## 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) + : SqlHelper.resolveQuery(getEndpoint().getCamelContext(), sql, null); } catch (Exception e) { Review Comment: Not taking this one, though the observation is correct. It is a runtime behaviour change — skipping `SqlHelper.resolveQuery` on the endpoint path — on a PR whose subject is the security gate, already approved, and it is pre-existing as you say. @davsclaus did not raise it on either round. Folding a per-exchange optimisation into a security fix means whoever bisects a regression here has to untangle the two, and it would want its own look rather than riding along. The branch is now explicit, so it is a two-line change for whoever wants it as a follow-up. Happy to open one if a committer would rather see it tracked. _Claude Code on behalf of @oscerd_ ########## catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/components/sql.json: ########## @@ -96,6 +96,7 @@ "schedulerProperties": { "index": 45, "kind": "parameter", "displayName": "Scheduler Properties", "group": "scheduler", "label": "consumer,scheduler", "required": false, "type": "object", "javaType": "java.util.Map<java.lang.String, java.lang.Object>", "prefix": "scheduler.", "multiValue": true, "deprecated": false, "autowired": false, "secret": false, "description": "To configure additional properties when using a custom scheduler or any of the Quartz, Spring based scheduler. This is a multi-value option with prefix: scheduler." }, "startScheduler": { "index": 46, "kind": "parameter", "displayName": "Start Scheduler", "group": "scheduler", "label": "consumer,scheduler", "required": false, "type": "boolean", "javaType": "boolean", "deprecated": false, "autowired": false, "secret": false, "defaultValue": true, "description": "Whether the scheduler should be auto started." }, "timeUnit": { "index": 47, "kind": "parameter", "displayName": "Time Unit", "group": "scheduler", "label": "consumer,scheduler", "required": false, "type": "enum", "javaType": "java.util.concurrent.TimeUnit", "enum": [ "NANOSECONDS", "MICROSECONDS", "MILLISECONDS", "SECONDS", "MINUTES", "HOURS", "DAYS" ], "deprecated": false, "autowired": false, "secret": false, "defaultValue": "MILLISECONDS", "description": "Time unit for initialDelay and delay options." }, - "useFixedDelay": { "index": 48, "kind": "parameter", "displayName": "Use Fixed Delay", "group": "scheduler", "label": "consumer,scheduler", "required": false, "type": "boolean", "javaType": "boolean", "deprecated": false, "autowired": false, "secret": false, "defaultValue": true, "description": "Controls if fixed delay or fixed rate is used. See ScheduledExecutorService in JDK for details." } + "useFixedDelay": { "index": 48, "kind": "parameter", "displayName": "Use Fixed Delay", "group": "scheduler", "label": "consumer,scheduler", "required": false, "type": "boolean", "javaType": "boolean", "deprecated": false, "autowired": false, "secret": false, "defaultValue": true, "description": "Controls if fixed delay or fixed rate is used. See ScheduledExecutorService in JDK for details." }, Review Comment: Done — the annotation is `label = "producer,security"` and the catalog JSON, `SqlComponentBuilderFactory` and `SqlEndpointBuilderFactory` were all regenerated from a full reactor build. That is in this merge. _Claude Code on behalf of @oscerd_ -- 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]
