Copilot commented on code in PR #3313:
URL: https://github.com/apache/tika/pull/3313#discussion_r4230183536
##########
tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-pdf-module/src/main/java/org/apache/tika/parser/pdf/PDFParser.java:
##########
@@ -272,6 +272,16 @@ public void parse(TikaInputStream tis, ContentHandler
handler, Metadata metadata
}
private PDFParserConfig getConfig(ParseContext parseContext) throws
TikaException, IOException {
+ if (parseContext.hasJsonConfig("pdf-parser")
+ && !parseContext.getJsonConfig("pdf-parser").trusted()) {
+ // resolved for its guards only: per-request JSON may not name an
image graphics
+ // engine factory, operator JSON (config file, presets) still may
+ ParseContextConfig.getConfig(
+ parseContext,
+ "pdf-parser",
+ PDFParserConfig.RuntimeConfig.class,
+ new PDFParserConfig.RuntimeConfig());
Review Comment:
`parseContext.getJsonConfig("pdf-parser")` is fetched multiple times (here
and later). Consider storing it in a local variable once (after
`hasJsonConfig`), which reduces duplication and avoids any chance of
inconsistent reads if the underlying implementation changes.
##########
tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-pdf-module/src/main/java/org/apache/tika/parser/pdf/PDFParserConfig.java:
##########
@@ -740,6 +741,39 @@ public boolean isThrowOnEncryptedPayload() {
return throwOnEncryptedPayload;
}
+ /**
+ * Runtime-only PDFParserConfig that refuses the image graphics engine
factory, which
+ * {@link PDFParserConfig#setImageGraphicsEngineFactoryClass(String)}
loads and
+ * instantiates by name.
+ * <p>
+ * This class is deserialized by ConfigDeserializer (in
tika-serialization), which populates
+ * fields via setters, so a per-request {@code pdf-parser} block naming a
factory throws
+ * here. The factory is operator configuration.
+ */
+ public static class RuntimeConfig extends PDFParserConfig {
+
+ private static final long serialVersionUID = 6492570218190936987L;
+
+ @Override
+ public void setImageGraphicsEngineFactoryClass(String className) {
+ if (!StringUtils.isBlank(className)) {
+ throw new IllegalArgumentException("Cannot modify
imageGraphicsEngineFactoryClass"
+ + " at runtime. The image graphics engine factory must
be configured at"
+ + " parser initialization time.");
+ }
+ }
Review Comment:
The guard currently allows blank/whitespace values through (`isBlank`),
which still constitutes an attempt to set the field and can behave unexpectedly
depending on how the underlying config merge/deserializer treats empty strings.
To fully prevent per-request mutation (including “clearing” attempts), consider
rejecting any non-null value (or otherwise ensuring presence of the key always
fails), not just non-blank.
##########
tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-pdf-module/src/main/java/org/apache/tika/parser/pdf/PDFParserConfig.java:
##########
@@ -740,6 +741,39 @@ public boolean isThrowOnEncryptedPayload() {
return throwOnEncryptedPayload;
}
+ /**
+ * Runtime-only PDFParserConfig that refuses the image graphics engine
factory, which
+ * {@link PDFParserConfig#setImageGraphicsEngineFactoryClass(String)}
loads and
+ * instantiates by name.
+ * <p>
+ * This class is deserialized by ConfigDeserializer (in
tika-serialization), which populates
+ * fields via setters, so a per-request {@code pdf-parser} block naming a
factory throws
+ * here. The factory is operator configuration.
+ */
+ public static class RuntimeConfig extends PDFParserConfig {
+
+ private static final long serialVersionUID = 6492570218190936987L;
+
+ @Override
+ public void setImageGraphicsEngineFactoryClass(String className) {
+ if (!StringUtils.isBlank(className)) {
+ throw new IllegalArgumentException("Cannot modify
imageGraphicsEngineFactoryClass"
+ + " at runtime. The image graphics engine factory must
be configured at"
+ + " parser initialization time.");
Review Comment:
The exception message doesn’t clearly indicate that this comes from
*untrusted/per-request* JSON config validation (vs. an internal runtime
mutation). Consider including wording like “per-request (untrusted)
configuration” and/or the config block name (`pdf-parser`) to make operational
diagnosis easier when this is surfaced via tika-server.
--
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]
