This is an automated email from the ASF dual-hosted git repository. tballison pushed a commit to branch TIKA-4769 in repository https://gitbox.apache.org/repos/asf/tika.git
commit 9214bd55dbe5615ebc95cca6c8ea3c2397c650d1 Author: tallison <[email protected]> AuthorDate: Thu Jul 2 07:04:39 2026 -0400 TIKA-4769 - improve metadata robustness --- .../java/org/apache/tika/metadata/Metadata.java | 66 ++++++++++++++++- .../parser/multiple/AbstractMultipleParser.java | 10 +-- .../java/org/apache/tika/utils/ParserUtils.java | 7 +- .../metadata/MetadataInternalKeyGuardTest.java | 85 ++++++++++++++++++++++ .../serialization/serdes/MetadataDeserializer.java | 4 +- .../tika/serialization/JsonMetadataTest.java | 22 ++++++ 6 files changed, 179 insertions(+), 15 deletions(-) diff --git a/tika-core/src/main/java/org/apache/tika/metadata/Metadata.java b/tika-core/src/main/java/org/apache/tika/metadata/Metadata.java index b0ede3526c..d006792c69 100644 --- a/tika-core/src/main/java/org/apache/tika/metadata/Metadata.java +++ b/tika-core/src/main/java/org/apache/tika/metadata/Metadata.java @@ -34,6 +34,9 @@ import java.util.Objects; import java.util.Properties; import java.util.TimeZone; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import org.apache.tika.metadata.Property.PropertyType; import org.apache.tika.metadata.writefilter.MetadataWriteLimiter; import org.apache.tika.metadata.writefilter.MetadataWriteLimiterFactory; @@ -47,6 +50,7 @@ public class Metadata implements CreativeCommons, Geographic, HttpHeaders, Message, ClimateForcast, TIFF, TikaMimeKeys, Serializable { + private static final Logger LOG = LoggerFactory.getLogger(Metadata.class); private static final MetadataWriteLimiter ACCEPT_ALL = new MetadataWriteLimiter() { @Override @@ -304,9 +308,55 @@ public class Metadata * @param value the metadata value. */ public void add(final String name, final String value) { + if (blockReservedKeyWrite(name)) { + return; + } + addUnchecked(name, value); + } + + /** Trusted add, bypassing the reserved-key guard; used by the Property overloads. TIKA-4769. */ + private void addUnchecked(final String name, final String value) { writeLimiter.add(name, value, metadata); } + /** + * TIKA-4769: drop a String write to a reserved {@code X-TIKA:} key so untrusted file content + * can't overwrite control-plane metadata. Reserved keys are writable only via their Property. + */ + private static boolean blockReservedKeyWrite(String name) { + if (name != null && name.startsWith(TikaCoreProperties.TIKA_META_PREFIX)) { + LOG.debug("Dropping String write to reserved metadata key '{}'; use its Property " + + "(TIKA-4769).", name); + return true; + } + return false; + } + + /** + * Trusted write for reconstruction paths (clone/merge/deserialize): a reserved {@code X-TIKA:} + * key goes through its Property so the guard doesn't drop it; others by name. TIKA-4769. + * + * @param append add rather than set + */ + public void reconstruct(String name, String value, boolean append) { + if (name != null && name.startsWith(TikaCoreProperties.TIKA_META_PREFIX)) { + Property property = Property.get(name); + if (property != null) { + if (append) { + add(property, value); + } else { + set(property, value); + } + return; + } + } + if (append) { + add(name, value); + } else { + set(name, value); + } + } + /** * Add a metadata name/value mapping. Add the specified value to the list of * values associated to the specified metadata name. @@ -320,7 +370,7 @@ public class Metadata set(name, newValues); } else { for (String val : newValues) { - add(name, val); + addUnchecked(name, val); } } } @@ -351,7 +401,7 @@ public class Metadata set(property, value); } else { if (property.isMultiValuePermitted()) { - add(property.getName(), value); + addUnchecked(property.getName(), value); } else { throw new PropertyTypeException( property.getName() + " : " + property.getPropertyType()); @@ -384,6 +434,14 @@ public class Metadata * @param value the metadata value, or <code>null</code> */ public void set(String name, String value) { + if (blockReservedKeyWrite(name)) { + return; + } + setUnchecked(name, value); + } + + /** Trusted set, bypassing the reserved-key guard. TIKA-4769. */ + private void setUnchecked(String name, String value) { writeLimiter.set(name, value, metadata); } @@ -393,7 +451,7 @@ public class Metadata if (values != null) { metadata.remove(name); for (String v : values) { - add(name, v); + addUnchecked(name, v); } } else { metadata.remove(name); @@ -419,7 +477,7 @@ public class Metadata } } } else { - set(property.getName(), value); + setUnchecked(property.getName(), value); } } diff --git a/tika-core/src/main/java/org/apache/tika/parser/multiple/AbstractMultipleParser.java b/tika-core/src/main/java/org/apache/tika/parser/multiple/AbstractMultipleParser.java index 17eff0515e..4e10fdb398 100644 --- a/tika-core/src/main/java/org/apache/tika/parser/multiple/AbstractMultipleParser.java +++ b/tika-core/src/main/java/org/apache/tika/parser/multiple/AbstractMultipleParser.java @@ -125,7 +125,7 @@ public abstract class AbstractMultipleParser implements Parser { if (newVals == null || newVals.length == 0) { // Metadata only in previous run, keep old values for (String val : oldVals) { - newMetadata.add(n, val); + newMetadata.reconstruct(n, val, true); } } else if (Arrays.deepEquals(oldVals, newVals)) { // Metadata is the same, nothing to do @@ -136,7 +136,7 @@ public abstract class AbstractMultipleParser implements Parser { // Use the earlier value(s) in place of this/these one/s newMetadata.remove(n); for (String val : oldVals) { - newMetadata.add(n, val); + newMetadata.reconstruct(n, val, true); } continue; case LAST_WINS: @@ -147,11 +147,11 @@ public abstract class AbstractMultipleParser implements Parser { List<String> vals = new ArrayList<>(Arrays.asList(oldVals)); newMetadata.remove(n); for (String oldVal : oldVals) { - newMetadata.add(n, oldVal); + newMetadata.reconstruct(n, oldVal, true); } for (String newVal : newVals) { if (!vals.contains(newVal)) { - newMetadata.add(n, newVal); + newMetadata.reconstruct(n, newVal, true); vals.add(newVal); } } @@ -318,7 +318,7 @@ public abstract class AbstractMultipleParser implements Parser { for (String n : metadata.names()) { originalMetadata.remove(n); for (String val : metadata.getValues(n)) { - originalMetadata.add(n, val); + originalMetadata.reconstruct(n, val, true); } } } diff --git a/tika-core/src/main/java/org/apache/tika/utils/ParserUtils.java b/tika-core/src/main/java/org/apache/tika/utils/ParserUtils.java index 552e418859..ecd5287894 100644 --- a/tika-core/src/main/java/org/apache/tika/utils/ParserUtils.java +++ b/tika-core/src/main/java/org/apache/tika/utils/ParserUtils.java @@ -43,11 +43,10 @@ public class ParserUtils { for (String n : m.names()) { if (!m.isMultiValued(n)) { - clone.set(n, m.get(n)); + clone.reconstruct(n, m.get(n), false); } else { - String[] vals = m.getValues(n); - for (String val : vals) { - clone.add(n, val); + for (String val : m.getValues(n)) { + clone.reconstruct(n, val, true); } } } diff --git a/tika-core/src/test/java/org/apache/tika/metadata/MetadataInternalKeyGuardTest.java b/tika-core/src/test/java/org/apache/tika/metadata/MetadataInternalKeyGuardTest.java new file mode 100644 index 0000000000..94e97e112d --- /dev/null +++ b/tika-core/src/test/java/org/apache/tika/metadata/MetadataInternalKeyGuardTest.java @@ -0,0 +1,85 @@ +/* + * 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.metadata; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +import org.junit.jupiter.api.Test; + +/** TIKA-4769: reserved {@code X-TIKA:} keys can't be overwritten by String writes, only via Property. */ +public class MetadataInternalKeyGuardTest { + + @Test + public void testStringWriteToInternalKeyIsDropped() { + Metadata metadata = new Metadata(); + // hostile scrape + metadata.set(TikaCoreProperties.TIKA_CONTENT.getName(), "injected"); + assertNull(metadata.get(TikaCoreProperties.TIKA_CONTENT), + "String write to an internal key must be dropped"); + assertNull(metadata.get(TikaCoreProperties.TIKA_CONTENT.getName())); + } + + @Test + public void testStringAddToInternalMultiValueKeyIsDropped() { + Metadata metadata = new Metadata(); + metadata.add(TikaCoreProperties.TIKA_PARSED_BY.getName(), "org.evil.FakeParser"); + assertArrayEquals(new String[0], metadata.getValues(TikaCoreProperties.TIKA_PARSED_BY), + "String add to an internal key must be dropped"); + } + + @Test + public void testStringWriteCannotOverwriteTrustedInternalValue() { + Metadata metadata = new Metadata(); + metadata.set(TikaCoreProperties.TIKA_CONTENT, "trusted"); + // String-path attempt must not clobber + metadata.set(TikaCoreProperties.TIKA_CONTENT.getName(), "injected"); + assertEquals("trusted", metadata.get(TikaCoreProperties.TIKA_CONTENT)); + } + + @Test + public void testPropertyWriteToInternalKeyStillWorks() { + Metadata metadata = new Metadata(); + metadata.set(TikaCoreProperties.TIKA_CONTENT, "legit"); + assertEquals("legit", metadata.get(TikaCoreProperties.TIKA_CONTENT)); + + metadata.add(TikaCoreProperties.TIKA_PARSED_BY, "p1"); + metadata.add(TikaCoreProperties.TIKA_PARSED_BY, "p2"); + assertArrayEquals(new String[] {"p1", "p2"}, + metadata.getValues(TikaCoreProperties.TIKA_PARSED_BY)); + } + + @Test + public void testNonReservedStringKeysAreUnaffected() { + Metadata metadata = new Metadata(); + // unregistered key passes through + metadata.set("my:customKey", "value"); + assertEquals("value", metadata.get("my:customKey")); + + // a registered non-reserved property (dc:description) written by name still works + metadata.set(TikaCoreProperties.DESCRIPTION.getName(), "hello"); + assertEquals("hello", metadata.get(TikaCoreProperties.DESCRIPTION)); + } + + @Test + public void testNullNameDoesNotThrow() { + Metadata metadata = new Metadata(); + // no NPE on a null name + metadata.set((String) null, "x"); + } +} diff --git a/tika-serialization/src/main/java/org/apache/tika/serialization/serdes/MetadataDeserializer.java b/tika-serialization/src/main/java/org/apache/tika/serialization/serdes/MetadataDeserializer.java index f02e12f30e..248783f97f 100644 --- a/tika-serialization/src/main/java/org/apache/tika/serialization/serdes/MetadataDeserializer.java +++ b/tika-serialization/src/main/java/org/apache/tika/serialization/serdes/MetadataDeserializer.java @@ -50,14 +50,14 @@ public class MetadataDeserializer extends JsonDeserializer<Metadata> { } if (token.isScalarValue()) { - metadata.set(field, jsonParser.getText()); + metadata.reconstruct(field, jsonParser.getText(), false); } else if (jsonParser.isExpectedStartArrayToken()) { token = jsonParser.nextToken(); while (token != null) { if (token == JsonToken.END_ARRAY) { return true; } else if (token.isScalarValue()) { - metadata.add(field, jsonParser.getText()); + metadata.reconstruct(field, jsonParser.getText(), true); } else { break; } diff --git a/tika-serialization/src/test/java/org/apache/tika/serialization/JsonMetadataTest.java b/tika-serialization/src/test/java/org/apache/tika/serialization/JsonMetadataTest.java index e62289ba5d..e42c1623b7 100644 --- a/tika-serialization/src/test/java/org/apache/tika/serialization/JsonMetadataTest.java +++ b/tika-serialization/src/test/java/org/apache/tika/serialization/JsonMetadataTest.java @@ -16,6 +16,7 @@ */ package org.apache.tika.serialization; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -130,4 +131,25 @@ public class JsonMetadataTest { Metadata deserialized = JsonMetadata.fromJson(new StringReader(writer.toString())); assertEquals(m, deserialized); } + + @Test + public void testInternalKeysRoundTrip() throws Exception { + //TIKA-4769: reserved keys must survive serialize->deserialize (reconstructed via Property). + Metadata m = new Metadata(); + m.set(TikaCoreProperties.TIKA_CONTENT, "the content"); + m.add(TikaCoreProperties.TIKA_PARSED_BY, "org.apache.tika.parser.DefaultParser"); + m.add(TikaCoreProperties.TIKA_PARSED_BY, "org.apache.tika.parser.pdf.PDFParser"); + m.add("k1", "v1"); + + StringWriter writer = new StringWriter(); + JsonMetadata.toJson(m, writer); + Metadata deserialized = JsonMetadata.fromJson(new StringReader(writer.toString())); + + assertEquals("the content", deserialized.get(TikaCoreProperties.TIKA_CONTENT)); + assertArrayEquals( + new String[] {"org.apache.tika.parser.DefaultParser", + "org.apache.tika.parser.pdf.PDFParser"}, + deserialized.getValues(TikaCoreProperties.TIKA_PARSED_BY)); + assertEquals(m, deserialized); + } }
