Copilot commented on code in PR #3351:
URL: https://github.com/apache/tika/pull/3351#discussion_r4290742287
##########
tika-serialization/src/main/java/org/apache/tika/serialization/serdes/ParseContextDeserializer.java:
##########
@@ -207,6 +209,10 @@ private static void scanForBlocked(JsonNode node) throws
IOException {
for (JsonNode child : node) {
scanForBlocked(child);
}
+ } else if (node.isTextual() &&
ComponentNameResolver.isWireBlockedName(node.asText())) {
+ throw new IOException("Component '" + node.asText() + "' may not
be supplied via a "
+ + "request parseContext. It must be configured server-side
in the Tika config, "
+ + "not at request time.");
Review Comment:
This rejects *any* textual value equal to a wire-blocked name anywhere in a
scanned subtree, even when that string is just a data value (not a component
reference). That can cause false positives in otherwise-safe request configs.
Consider moving the wire-blocked-name check to the point where a string is
actually interpreted as a component identifier (e.g., within the
resolver/binding logic for array configs / ComponentNameResolver), or restrict
the scan to contexts that represent component specs (such as elements of known
component-list arrays), rather than all textual nodes.
##########
tika-serialization/src/test/java/org/apache/tika/serialization/WireRestrictedParseContextTest.java:
##########
@@ -157,6 +157,18 @@ public void
keysOnlyScanLetsInertBareStringReferencesThrough() {
assertDoesNotThrow(() -> restrictedMapper().readValue(json,
ParseContext.class));
}
+ @Test
+ public void restrictedRejectsBareStringWireBlockedName() {
+ // the bare-string form still binds a working defaults instance of an
allowed type
+ for (String name : new String[]{"openai-embedding-filter",
"jina-embedding-filter"}) {
+ String json = "{\"metadata-filters\":[\"" + name + "\"]}";
+ Exception e = assertThrows(Exception.class,
+ () -> restrictedMapper().readValue(json,
ParseContext.class), name);
Review Comment:
Using `assertThrows(Exception.class, ...)` is broad enough that unrelated
failures (including assertion errors from the test itself, or unexpected
runtime exceptions) can satisfy the assertion and mask the real behavior.
Prefer asserting the narrowest expected exception type (e.g., an
`IOException`/Jackson mapping exception type used elsewhere in this suite) so
the test specifically verifies the intended rejection path.
##########
tika-serialization/src/main/java/org/apache/tika/serialization/serdes/ParseContextDeserializer.java:
##########
@@ -207,6 +209,10 @@ private static void scanForBlocked(JsonNode node) throws
IOException {
for (JsonNode child : node) {
scanForBlocked(child);
}
+ } else if (node.isTextual() &&
ComponentNameResolver.isWireBlockedName(node.asText())) {
+ throw new IOException("Component '" + node.asText() + "' may not
be supplied via a "
+ + "request parseContext. It must be configured server-side
in the Tika config, "
+ + "not at request time.");
Review Comment:
The exception message is constructed inline and calls `node.asText()`
multiple times. To keep messages consistent across call sites and reduce
duplication, consider storing the text in a local variable and (optionally)
centralizing the message construction (e.g., a small helper used by both
key-based and string-based rejections).
--
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]
