oscerd commented on code in PR #26737:
URL: https://github.com/apache/camel/pull/26737#discussion_r4082117119
##########
components/camel-ai/camel-docling/src/main/java/org/apache/camel/component/docling/DoclingProducer.java:
##########
@@ -2277,17 +2278,41 @@ private void addLayoutArguments(List<String> command) {
}
}
- private void addOutputDirectoryArguments(List<String> command, Exchange
exchange, String outputDirectory) {
+ private void addOutputDirectoryArguments(List<String> command, Exchange
exchange, String outputDirectory)
+ throws IOException {
String outputPath =
exchange.getIn().getHeader(DoclingHeaders.OUTPUT_FILE_PATH, String.class);
+ command.add("--output");
if (outputPath != null) {
- command.add("--output");
- command.add(outputPath);
+ // the header is caller-provided, so it gets the same
normalization and optional base-directory
+ // containment as input paths do, instead of reaching the CLI
verbatim
+
command.add(resolveWithinOutputBaseDirectory(outputPath).toString());
} else {
- command.add("--output");
command.add(outputDirectory);
}
}
+ /**
+ * Normalizes the given output directory and, when {@code
outputBaseDirectory} is configured, verifies that it stays
+ * inside that directory. Mirrors {@link
#resolveWithinInputBaseDirectory(String)} so that the output directory
+ * carried by the {@link DoclingHeaders#OUTPUT_FILE_PATH} header receives
the same treatment as input paths.
+ */
+ private Path resolveWithinOutputBaseDirectory(String outputPath) throws
IOException {
+ String base = configuration.getOutputBaseDirectory();
+ if (base == null || base.isEmpty()) {
+ // no directory restriction: normalize lexically only, and leave
relative paths relative so that they keep
+ // resolving the way they did before - against the CLI working
directory, when one is set
+ return Paths.get(outputPath).normalize();
Review Comment:
Added `outputPathIsNormalizedInTheBuiltCommand`: it builds the command via
`buildDoclingCommand` and asserts `out/./sub/../x` reaches `--output` as
`out/x`, pinning the documented normalization for the default
(no-`outputBaseDirectory`) branch rather than relying on the failure message.
_Claude Code on behalf of oscerd_
##########
components/camel-ai/camel-docling/src/main/java/org/apache/camel/component/docling/DoclingProducer.java:
##########
@@ -2277,17 +2278,41 @@ private void addLayoutArguments(List<String> command) {
}
}
- private void addOutputDirectoryArguments(List<String> command, Exchange
exchange, String outputDirectory) {
+ private void addOutputDirectoryArguments(List<String> command, Exchange
exchange, String outputDirectory)
+ throws IOException {
String outputPath =
exchange.getIn().getHeader(DoclingHeaders.OUTPUT_FILE_PATH, String.class);
+ command.add("--output");
if (outputPath != null) {
- command.add("--output");
- command.add(outputPath);
+ // the header is caller-provided, so it gets the same
normalization and optional base-directory
+ // containment as input paths do, instead of reaching the CLI
verbatim
+
command.add(resolveWithinOutputBaseDirectory(outputPath).toString());
} else {
- command.add("--output");
command.add(outputDirectory);
}
}
+ /**
+ * Normalizes the given output directory and, when {@code
outputBaseDirectory} is configured, verifies that it stays
+ * inside that directory. Mirrors {@link
#resolveWithinInputBaseDirectory(String)} so that the output directory
+ * carried by the {@link DoclingHeaders#OUTPUT_FILE_PATH} header receives
the same treatment as input paths.
+ */
+ private Path resolveWithinOutputBaseDirectory(String outputPath) throws
IOException {
+ String base = configuration.getOutputBaseDirectory();
+ if (base == null || base.isEmpty()) {
+ // no directory restriction: normalize lexically only, and leave
relative paths relative so that they keep
+ // resolving the way they did before - against the CLI working
directory, when one is set
+ return Paths.get(outputPath).normalize();
+ }
+ Path baseDir = Paths.get(base).toAbsolutePath().normalize();
+ // resolve relative paths against the base directory itself, so that
the path checked here is exactly the path
+ // used downstream regardless of the process working directory; an
absolute header value that escapes is rejected
+ Path path = baseDir.resolve(Paths.get(outputPath)).normalize();
+ if (!path.startsWith(baseDir)) {
Review Comment:
Agreed, no code change — it deliberately mirrors
`resolveWithinInputBaseDirectory`, and `toRealPath()` would change semantics
for not-yet-existing output directories. Added the caveat that containment is
lexical and does not resolve symbolic links to the `outputBaseDirectory` option
description, the helper javadoc, and the component docs.
_Claude Code on behalf of oscerd_
##########
components/camel-ai/camel-docling/src/test/java/org/apache/camel/component/docling/DoclingOutputPathValidationTest.java:
##########
@@ -0,0 +1,151 @@
+/*
+ * 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.docling;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+
+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.junit.jupiter.api.io.TempDir;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+/**
+ * Tests that the {@code CamelDoclingOutputFilePath} header is normalized and,
when {@code outputBaseDirectory} is
+ * configured, confined to that directory before it reaches the docling CLI
{@code --output} flag, consistently with how
+ * input paths honour {@code inputBaseDirectory}.
+ */
+class DoclingOutputPathValidationTest extends CamelTestSupport {
+
+ private static final String CONTENT = "just some document text";
+
+ @TempDir
+ Path tempDir;
+
+ // ------------------------------------------------------- no
outputBaseDirectory
+
+ @Test
+ void outputPathWithoutBaseDirectoryIsAllowed() {
+ // no restriction is configured: the header is normalized only and
passes through, so the run fails later on the
+ // absent docling binary rather than on a containment check
+ assertThatThrownBy(() ->
template.requestBodyAndHeader("direct:default", CONTENT,
+ DoclingHeaders.OUTPUT_FILE_PATH,
tempDir.resolve("out").toString()))
+ .isInstanceOf(CamelExecutionException.class)
+ .cause()
+ .hasMessageNotContaining("outputBaseDirectory");
+ }
+
+ // ------------------------------------------------------
outputBaseDirectory jail
+
+ @Test
+ void outputPathInsideOutputBaseDirectoryIsAccepted() throws Exception {
+ String inside = baseDir().resolve("out").toString();
+
+ // the docling binary is absent so execution still fails, but it must
not fail on the jail check
+ assertThatThrownBy(() ->
template.requestBodyAndHeader("direct:jailed", CONTENT,
+ DoclingHeaders.OUTPUT_FILE_PATH, inside))
+ .isInstanceOf(CamelExecutionException.class)
+ .cause()
+ .hasMessageNotContaining("outputBaseDirectory");
+ }
+
+ @Test
+ void outputPathOutsideOutputBaseDirectoryIsRejected() throws Exception {
+ baseDir();
+ String outside = tempDir.resolve("outside").toString();
+
+ assertThatThrownBy(() ->
template.requestBodyAndHeader("direct:jailed", CONTENT,
+ DoclingHeaders.OUTPUT_FILE_PATH, outside))
+ .isInstanceOf(CamelExecutionException.class)
+ .cause()
+ .isInstanceOf(IOException.class)
+ .hasMessageContaining("outputBaseDirectory");
+ }
+
+ @Test
+ void absoluteOutputPathOutsideOutputBaseDirectoryIsRejected() throws
Exception {
+ baseDir();
+
+ // an absolute header value ignores the base directory when resolved,
so it must be rejected as escaping
+ assertThatThrownBy(() ->
template.requestBodyAndHeader("direct:jailed", CONTENT,
+ DoclingHeaders.OUTPUT_FILE_PATH, "/var/www/html/uploads"))
+ .isInstanceOf(CamelExecutionException.class)
+ .cause()
+ .isInstanceOf(IOException.class)
+ .hasMessageContaining("outputBaseDirectory");
+ }
+
+ @Test
+ void traversalOutOfOutputBaseDirectoryIsRejected() throws Exception {
Review Comment:
Right — `@TempDir` hands out absolute paths, so the old value hit the
absolute branch. Switched `traversalOutOfOutputBaseDirectoryIsRejected` to a
genuinely relative `"../outside"`, which now exercises the
`baseDir.resolve(...)` + `normalize()` path.
_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]
