oscerd commented on code in PR #26268: URL: https://github.com/apache/camel/pull/26268#discussion_r4014226212
########## 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: Confirmed and rewritten — see the reply on @davsclaus's thread for the shape. Both templates now call the same procedure under different OUT-parameter aliases, so the result map names which one ran, and the positive `allowTemplateFromHeader=true` case is there as well. Nothing asserts on an exception any more. _Claude Code on behalf of @oscerd_ ########## components/camel-sql/src/main/java/org/apache/camel/component/sql/stored/SqlStoredProducer.java: ########## @@ -106,15 +106,29 @@ public void execute(StatementWrapper ps) throws SQLException, DataAccessExceptio private StatementWrapper createStatement(Exchange exchange) throws SQLException { String sql; + boolean runtime = false; if (getEndpoint().isUseMessageBodyForTemplate()) { sql = exchange.getIn().getBody(String.class); + runtime = true; } 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; + runtime = true; + } else { + sql = resolvedTemplate; + } } try { - sql = SqlHelper.resolveQuery(getEndpoint().getCamelContext(), sql, null); + // A template taken at runtime from the message body (useMessageBodyForTemplate) or from a header + // (CamelSqlStoredTemplate) is untrusted input, so it must not be resolved as a file:/http:/classpath: + // resource (SqlHelper.resolveQuery -> ResourceHelper does that) - resolve placeholders only. Only the + // endpoint-configured template is resolved as a resource, and that already happens in doInit/doStart. + sql = runtime + ? SqlHelper.resolvePlaceholders(sql, null) + : SqlHelper.resolveQuery(getEndpoint().getCamelContext(), sql, null); Review Comment: Thanks for re-checking after the change. On the re-resolution: declining for this PR, for the reasons on the earlier thread — it is a per-exchange behaviour change on an approved security fix, and mixing the two makes a future bisect harder. It is a two-line follow-up now that the branch is explicit. _Claude Code on behalf of @oscerd_ ########## components/camel-sql/src/test/java/org/apache/camel/component/sql/stored/SqlStoredUseMessageBodyForTemplateResourceTest.java: ########## @@ -0,0 +1,103 @@ +/* + * 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.component.mock.MockEndpoint; +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.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +/** + * When {@code useMessageBodyForTemplate=true} the message body is the stored-procedure template text. Because the body + * is untrusted per-exchange input, it must be used verbatim and never dereferenced as a {@code file:} / {@code http:} / + * {@code classpath:} resource - only the endpoint-configured template is resolved as a resource (at route start). + */ +public class SqlStoredUseMessageBodyForTemplateResourceTest extends CamelTestSupport { + + private static final String INLINE_TEMPLATE = "SUBNUMBERS(INTEGER :#num1,INTEGER :#num2,OUT INTEGER resultofsum)"; + + // Points at a real classpath resource holding a valid template. Before the fix this body would have been loaded + // and executed; after the fix it is treated as literal (invalid) template text. + private static final String RESOURCE_BODY = "classpath:sql/bodyTemplateResource.sql"; + + 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 inlineBodyTemplateStillWorks() throws Exception { + MockEndpoint mock = getMockEndpoint("mock:query"); + mock.expectedMessageCount(1); + + Map<String, Object> params = new HashMap<>(); + params.put("num1", 3); + params.put("num2", 1); + + template.requestBodyAndHeader("direct:query", INLINE_TEMPLATE, SqlStoredConstants.SQL_STORED_PARAMETERS, params); + + MockEndpoint.assertIsSatisfied(context); + assertEquals(Integer.valueOf(2), mock.getExchanges().get(0).getIn().getBody(Map.class).get("resultofsum")); + } + + @Test + public void schemePrefixedBodyIsNotResolvedAsResource() { + Map<String, Object> params = new HashMap<>(); + params.put("num1", 3); + params.put("num2", 1); + + // The body is a classpath: URI pointing at a valid template. It must NOT be fetched and executed; instead the + // literal string is used as the template and fails to parse - which is what proves the resource was not loaded. + assertThrows(CamelExecutionException.class, Review Comment: That was the intent, and it is the contrast worth drawing: pointing `RESOURCE_BODY` at a *valid* classpath template is what makes the test fail if the fix is reverted. A test that asserts "an exception was thrown" proves nothing when the exception fires either way — which is exactly what went wrong in `SqlStoredAllowTemplateFromHeaderTest`, since rewritten to assert on *which template executed* rather than on a failure. _Claude Code on behalf of @oscerd_ ########## docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_23.adoc: ########## @@ -1864,3 +1864,28 @@ 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. + +Both overrides are now disabled by default and must be opted into, matching the `allowTemplateFromHeader` +convention already used by the template components (camel-freemarker, camel-velocity, camel-xslt, ...): + +* `camel-sql`: set `allowQueryFromHeader=true` to honour the `CamelSqlQuery` header again. +* `camel-sql-stored`: set `allowTemplateFromHeader=true` to honour the `CamelSqlStoredTemplate` header again; a + header-supplied template is now resolved with property placeholders only, never as a `file:` / `http:` resource. + +A route that relied on either header must set the corresponding option on the endpoint. + +Additionally, `camel-sql-stored` with `useMessageBodyForTemplate=true` now uses the message body verbatim as the +stored-procedure template text and no longer resolves it through `SqlHelper.resolveQuery`. A body beginning with +`file:`, `http:`, `https:` or `classpath:` is therefore treated as literal template text instead of being fetched +as a resource, consistent with how `camel-sql` already treats the body under `useMessageBodyForSql=true`. A route +that relied on the body being a resource location must resolve it to the template text before the `sql-stored` +endpoint. Review Comment: This paragraph was the one that already had it right — the other two, plus the `@UriParam` description on `SqlStoredEndpoint`, are being brought up to it in [CAMEL-24755](https://issues.apache.org/jira/browse/CAMEL-24755). The fix missed 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]
