davsclaus commented on code in PR #26737:
URL: https://github.com/apache/camel/pull/26737#discussion_r4071659731
##########
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:
🟡 `normalize()` is lexical, so this containment check does not resolve
symlinks: a symlink inside `outputBaseDirectory` pointing outside it passes.
That matches `resolveWithinInputBaseDirectory` exactly, so consistency — the
stated goal of the PR — is preserved, and switching to `toRealPath()` would
change semantics for paths that don't exist yet, which is the normal case for
an output directory. So I'm not suggesting a code change; just a sentence in
the `outputBaseDirectory` javadoc and/or the option description, so operators
don't read the option as a stronger boundary than it is.
##########
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:
🟠This is the branch the upgrade guide leads with — "the header is now
normalized lexically (redundant separators and `.`/`..` segments are resolved)
before it is passed to Docling" — and it is the case most existing routes will
hit, since `outputBaseDirectory` is unset by default. But no test asserts what
actually reaches `--output`.
The two "accepted" tests only assert
`hasMessageNotContaining("outputBaseDirectory")`, which passes for any failure
that happens not to mention the option — including a failure caused by
normalization going wrong. A test that captures the built command and checks,
say, `out/./sub/../x` → `out/x` would pin the documented behaviour down.
##########
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:
🟡 `baseDir().resolve("..").resolve("outside")` is already an absolute path
(`@TempDir` hands out absolute paths), so this exercises the same branch as
`absoluteOutputPathOutsideOutputBaseDirectoryIsRejected` above rather than the
relative-traversal one it's named for.
A genuinely relative value — `"../outside"` — would cover the
`baseDir.resolve(...)` + `normalize()` path that nothing else reaches.
--
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]
