Copilot commented on code in PR #3364: URL: https://github.com/apache/tika/pull/3364#discussion_r4312158078
########## docs/modules/ROOT/pages/using-tika/server/index.adoc: ########## @@ -277,7 +277,9 @@ curl -X POST http://localhost:9998/async -H "Content-Type: application/json" \ A tuple's fields are `id`, `fetcher`, `fetchKey`, `emitter`, `emitKey`, and optionally `fetchRangeStart`, `fetchRangeEnd`, `metadata`, `parse-context` and `onParseException`. Any other -field is a `400` — there is no silent tolerance for a typo. There is no field for content: +field is a `400` — there is no silent tolerance for a typo. `parse-context` is per-request +configuration, gated like a `config` part: without `allowPerRequestConfig` a tuple that carries one +is a `403`; a `preset` names operator config and needs no flag. There is no field for content: Review Comment: The text references a tuple `preset`, but the preceding field list doesn’t include it. If `preset` is a supported tuple field, add it to the documented field list to avoid ambiguity; if it is not a field, the mention here should be clarified/removed. ########## tika-server/tika-server-core/src/main/java/org/apache/tika/server/core/resource/PipesResource.java: ########## @@ -56,16 +56,20 @@ public class PipesResource { private final PipesParser pipesParser; private final PipesConfig pipesConfig; + private final boolean allowPerRequestConfig; Review Comment: This changes the public constructor signature and can break downstream code that instantiates `PipesResource` directly. Consider keeping the old constructor as an overload (ideally `@Deprecated`) that delegates to the new one with a compatibility default (e.g., `allowPerRequestConfig=true` to preserve prior behavior), while the server wiring continues to pass the real config flag. ########## tika-server/tika-server-core/src/test/java/org/apache/tika/server/core/PipesPerRequestConfigDisabledTest.java: ########## @@ -0,0 +1,195 @@ +/* + * 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.tika.server.core; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.Reader; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; + +import jakarta.ws.rs.core.Response; +import org.apache.commons.io.FileUtils; +import org.apache.cxf.jaxrs.JAXRSServerFactoryBean; +import org.apache.cxf.jaxrs.client.WebClient; +import org.apache.cxf.jaxrs.lifecycle.ResourceProvider; +import org.apache.cxf.jaxrs.lifecycle.SingletonResourceProvider; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInstance; + +import org.apache.tika.config.loader.TikaJsonConfig; +import org.apache.tika.metadata.Metadata; +import org.apache.tika.metadata.TikaCoreProperties; +import org.apache.tika.pipes.core.EmitStrategy; +import org.apache.tika.pipes.core.EmitStrategyConfig; +import org.apache.tika.pipes.core.PipesConfig; +import org.apache.tika.pipes.core.PipesParser; +import org.apache.tika.serialization.JsonMetadataList; +import org.apache.tika.server.core.resource.AsyncResource; +import org.apache.tika.server.core.resource.PipesResource; +import org.apache.tika.server.core.writer.JSONObjWriter; + +/** + * /pipes and /async honor allowPerRequestConfig the way the multipart config part does: + * a tuple's parseContext is per-request configuration, so with the flag off (the default, + * which CXFTestBase mirrors) a tuple that carries one is refused with 403 before anything + * in it is resolved, and a tuple without one still parses. + */ +@TestInstance(TestInstance.Lifecycle.PER_CLASS) +public class PipesPerRequestConfigDisabledTest extends CXFTestBase { + private static final String PIPES_PATH = "/pipes"; + private static final String ASYNC_PATH = "/async"; + private static final String HELLO_WORLD = "hello_world.xml"; + // the kind of entry the wire allowlist leaves unscanned: it relaxes the server's limits + private static final String PARSE_CONTEXT = "{\"timeout-limits\":{\"progressTimeoutMillis\":100}}"; + + private Path tmpDir; + private Path tmpOutputFile; + private Path tikaConfigPath; + private PipesParser pipesParser; + private AsyncResource asyncResource; + + // isAllowPerRequestConfig() is intentionally NOT overridden; it defaults to false + // so a parseContext must be rejected. + + @Override + @BeforeAll + public void setUp() throws Exception { + tmpDir = Files.createTempDirectory("tika-pipes-config-disabled-test-"); + Path inputDir = tmpDir.resolve("input"); + Path tmpOutputDir = tmpDir.resolve("output"); + tmpOutputFile = tmpOutputDir.resolve(HELLO_WORLD + ".json"); + Files.createDirectories(inputDir); + Files.createDirectories(tmpOutputDir); + Files.copy(PipesPerRequestConfigDisabledTest.class.getResourceAsStream("/test-documents/mock/" + HELLO_WORLD), + inputDir.resolve(HELLO_WORLD)); + tikaConfigPath = Files.createTempFile(tmpDir, "tika-pipes-config-", ".json"); + CXFTestBase.createPluginsConfig(tikaConfigPath, inputDir, tmpOutputDir, null, 10000L); + super.setUp(); + } + + @Override + @AfterAll + public void tearDown() throws Exception { + if (pipesParser != null) { + pipesParser.close(); + pipesParser = null; + } + if (asyncResource != null) { + asyncResource.shutdownNow(); + asyncResource = null; + } + super.tearDown(); + if (tmpDir != null) { + FileUtils.deleteDirectory(tmpDir.toFile()); + } + } + + @BeforeEach + public void setUpEachTest() throws Exception { + Files.deleteIfExists(tmpOutputFile); + } + + @Override + protected void setUpResources(JAXRSServerFactoryBean sf) { + List<ResourceProvider> providers = new ArrayList<>(); + try { + TikaJsonConfig tikaJsonConfig = TikaJsonConfig.load(tikaConfigPath); + PipesConfig pipesConfig = PipesConfig.load(tikaJsonConfig); + pipesConfig.setEmitStrategy(new EmitStrategyConfig(EmitStrategy.EMIT_ALL)); + pipesParser = PipesParser.load(tikaJsonConfig, pipesConfig); + providers.add(new SingletonResourceProvider( + new PipesResource(pipesParser, pipesConfig, isAllowPerRequestConfig()))); + asyncResource = new AsyncResource(tikaConfigPath, isAllowPerRequestConfig()); + providers.add(new SingletonResourceProvider(asyncResource)); + } catch (Exception e) { + throw new RuntimeException(e); + } + sf.setResourceProviders(providers); + } + + @Override + protected void setUpProviders(JAXRSServerFactoryBean sf) { + List<Object> providers = new ArrayList<>(); + providers.add(new TikaServerParseExceptionMapper()); + providers.add(new BadRequestExceptionMapper()); + providers.add(new JSONObjWriter()); + sf.setProviders(providers); + } + + @Override + protected InputStream getTikaConfigInputStream() throws IOException { + return new ByteArrayInputStream(Files.readAllBytes(tikaConfigPath)); + } + + private static String tuple(String parseContextJson) { + return "{\"id\":\"" + HELLO_WORLD + "\"," + + "\"fetcher\":\"" + FETCHER_ID + "\"," + + "\"fetchKey\":\"" + HELLO_WORLD + "\"," + + "\"emitter\":\"" + EMITTER_JSON_ID + "\"," + + "\"emitKey\":\"\"" + + (parseContextJson == null ? "" : ",\"parse-context\":" + parseContextJson) + "}"; + } + + private Response post(String path, String body) { + return WebClient + .create(endPoint + path) + .accept("application/json") + .post(body); + } + + private static void assertPerRequestConfigDisabled(Response response) throws IOException { + assertEquals(403, response.getStatus()); + String msg = getStringFromInputStream((InputStream) response.getEntity()); + assertTrue(msg.contains("Per-request configuration is disabled"), + "expected the config-disabled message, got: " + msg); + } + + @Test + public void testPipesRejectsParseContextWhenDisabled() throws Exception { + assertPerRequestConfigDisabled(post(PIPES_PATH, tuple(PARSE_CONTEXT))); + assertFalse(Files.isRegularFile(tmpOutputFile), "nothing should be emitted for a rejected request"); + } + + @Test + public void testAsyncRejectsParseContextWhenDisabled() throws Exception { + assertPerRequestConfigDisabled(post(ASYNC_PATH, "{\"tuples\":[" + tuple(PARSE_CONTEXT) + "]}")); + assertFalse(Files.isRegularFile(tmpOutputFile), "nothing should be queued for a rejected request"); + } + + @Test + public void testPipesWithoutParseContextStillParses() throws Exception { + Response response = post(PIPES_PATH, tuple(null)); + assertEquals(200, response.getStatus()); Review Comment: The test code does not close `Response` (and reads the entity `InputStream` without closing it). This can leak connections/resources and cause flaky tests under load. Use try-with-resources for `Response` in callers and wrap the entity stream read in a try-with-resources inside `assertPerRequestConfigDisabled` (or buffer the entity via `readEntity(String.class)` if supported in this test setup). ########## tika-server/tika-server-core/src/main/java/org/apache/tika/server/core/resource/PipesResource.java: ########## @@ -56,16 +56,20 @@ public class PipesResource { private final PipesParser pipesParser; private final PipesConfig pipesConfig; + private final boolean allowPerRequestConfig; /** * @param pipesParser shared parser, also used by /tika, /rmeta, and /unpack. * Lifecycle (construction, shutdown) is owned by whoever * built it, not by this class. * @param pipesConfig the parser's config; read here for the {@code Retry-After} value. + * @param allowPerRequestConfig whether a tuple may carry a parseContext; see + * {@link PipesParsingHelper#rejectPerRequestConfig} */ - public PipesResource(PipesParser pipesParser, PipesConfig pipesConfig) { + public PipesResource(PipesParser pipesParser, PipesConfig pipesConfig, boolean allowPerRequestConfig) { this.pipesParser = pipesParser; this.pipesConfig = pipesConfig; + this.allowPerRequestConfig = allowPerRequestConfig; } Review Comment: This changes the public constructor signature and can break downstream code that instantiates `PipesResource` directly. Consider keeping the old constructor as an overload (ideally `@Deprecated`) that delegates to the new one with a compatibility default (e.g., `allowPerRequestConfig=true` to preserve prior behavior), while the server wiring continues to pass the real config flag. -- 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]
