From d263b91f932c7477ca739e59db7a5da75ae1fe7c Mon Sep 17 00:00:00 2001 From: abhinav-phi Date: Sat, 29 Aug 2026 21:52:02 +0530 Subject: [PATCH 1/3] Escape CR and LF in WARC metadata records and resource Content-Type MetadataRecordFormat.format() wrote one line per metadata value into the application/warc-fields payload without checking for CR or LF. A value containing CR LF (e.g. feed.description set by FeedParserBolt, or values of parse.* filters such as the XPath, LDJson and Tika filters) therefore became additional field lines that look exactly like fields written by the crawler - for example a fabricated hopsFromSeed or via. Framing stayed valid because Content-Length is computed from the finished payload, so WARC readers had no way to detect the injected fields. - replace CR and LF by spaces in metadata values written into the warc-fields payload, and drop metadata keys that are not valid WARC field names (printable ASCII without colon, RFC 5322 section 2.2) - sanitise the server-supplied Content-Type used for resource records in WARCRecordFormat.format(), which was appended verbatim into the WARC header block - log MetadataRecordFormat messages under MetadataRecordFormat instead of WARCRequestRecordFormat Fixes #2105 --- .../warc/MetadataRecordFormat.java | 14 ++- .../stormcrawler/warc/WARCRecordFormat.java | 37 +++++- .../warc/MetadataRecordFormatCRLFTest.java | 119 ++++++++++++++++++ .../warc/WARCRecordFormatTest.java | 41 ++++++ 4 files changed, 208 insertions(+), 3 deletions(-) create mode 100644 external/warc/src/test/java/org/apache/stormcrawler/warc/MetadataRecordFormatCRLFTest.java diff --git a/external/warc/src/main/java/org/apache/stormcrawler/warc/MetadataRecordFormat.java b/external/warc/src/main/java/org/apache/stormcrawler/warc/MetadataRecordFormat.java index ad0549111..afac5958e 100644 --- a/external/warc/src/main/java/org/apache/stormcrawler/warc/MetadataRecordFormat.java +++ b/external/warc/src/main/java/org/apache/stormcrawler/warc/MetadataRecordFormat.java @@ -46,7 +46,7 @@ public class MetadataRecordFormat extends WARCRecordFormat { - private static final Logger LOG = LoggerFactory.getLogger(WARCRequestRecordFormat.class); + private static final Logger LOG = LoggerFactory.getLogger(MetadataRecordFormat.class); private List metadataKeys; @@ -73,6 +73,14 @@ public byte[] format(Tuple tuple) { // get the metadata key / values to save in the WARCs for (String key : metadataKeys) { + // the key becomes the name of a WARC field: drop it entirely if it is not a + // valid field name, otherwise the field line would be malformed + if (!isValidWarcFieldName(key)) { + LOG.warn( + "Skipping invalid WARC field name configured in warc.metadata.keys: {}", + key); + continue; + } final String[] values = metadata.getValues(key); if (values == null || values.length == 0) { continue; @@ -81,7 +89,9 @@ public byte[] format(Tuple tuple) { if (StringUtils.isBlank(value)) { continue; } - payload.append(key).append(": ").append(value).append(CRLF); + // metadata values often originate from the parsed content: replace CR and + // LF so that the value cannot forge additional field lines + payload.append(key).append(": ").append(sanitizeWarcFieldValue(value)).append(CRLF); } } diff --git a/external/warc/src/main/java/org/apache/stormcrawler/warc/WARCRecordFormat.java b/external/warc/src/main/java/org/apache/stormcrawler/warc/WARCRecordFormat.java index cee46c8fb..0a387fed6 100644 --- a/external/warc/src/main/java/org/apache/stormcrawler/warc/WARCRecordFormat.java +++ b/external/warc/src/main/java/org/apache/stormcrawler/warc/WARCRecordFormat.java @@ -89,6 +89,39 @@ public class WARCRecordFormat implements RecordFormat { protected static final Pattern HTTP_STATUS_CODE_PATTERN = Pattern.compile("^[0-9]{3}$"); protected static final String HTTP_VERSION_FALLBACK = "HTTP/1.1"; + /* + * Named fields (WARC header fields and the fields of an application/warc-fields payload) are + * terminated by CRLF: their names are limited to printable ASCII characters excluding the + * colon, cf. RFC 5322 section 2.2, and their values must not contain CR or LF, otherwise the + * remainder of the value would be read as additional field lines. + */ + private static final Pattern WARC_FIELD_NAME_PATTERN = Pattern.compile("[!-9;-~]+"); + + /** + * Check whether a string is a valid WARC field name, i.e. consists of printable ASCII + * characters without a colon, cf. RFC 5322 section 2.2. + * + * @param name field name candidate + * @return true if the name can safely be written as the name of a WARC field + */ + static boolean isValidWarcFieldName(String name) { + return name != null && WARC_FIELD_NAME_PATTERN.matcher(name).matches(); + } + + /** + * Replace CR and LF characters in a field value by spaces so that the value cannot forge + * additional field lines in a WARC header block or in an application/warc-fields payload. + * + * @param value field value to sanitise + * @return the value without CR and LF characters + */ + static String sanitizeWarcFieldValue(String value) { + if (value == null || (value.indexOf('\r') < 0 && value.indexOf('\n') < 0)) { + return value; + } + return value.replace('\r', ' ').replace('\n', ' '); + } + protected static final Pattern PROBLEMATIC_HEADERS = Pattern.compile("(?i)(?:Content-(?:Encoding|Length)|Transfer-Encoding)"); protected static final String X_HIDE_HEADER = "X-Crawler-"; @@ -445,7 +478,9 @@ public byte[] format(Tuple tuple) { if (StringUtils.isBlank(ct)) { ct = "application/octet-stream"; } - buffer.append("Content-Type: ").append(ct).append(CRLF); + // the content type is under the control of the remote server: replace CR and LF + // so that it cannot forge additional header lines in the WARC header block + buffer.append("Content-Type: ").append(sanitizeWarcFieldValue(ct)).append(CRLF); } String truncated = diff --git a/external/warc/src/test/java/org/apache/stormcrawler/warc/MetadataRecordFormatCRLFTest.java b/external/warc/src/test/java/org/apache/stormcrawler/warc/MetadataRecordFormatCRLFTest.java new file mode 100644 index 000000000..92430dc11 --- /dev/null +++ b/external/warc/src/test/java/org/apache/stormcrawler/warc/MetadataRecordFormatCRLFTest.java @@ -0,0 +1,119 @@ +/* + * 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.stormcrawler.warc; + +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 static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.List; +import org.apache.storm.tuple.Tuple; +import org.apache.stormcrawler.Metadata; +import org.junit.jupiter.api.Test; +import org.netpreserve.jwarc.MessageHeaders; +import org.netpreserve.jwarc.WarcMetadata; +import org.netpreserve.jwarc.WarcReader; +import org.netpreserve.jwarc.WarcRecord; + +/** + * Metadata values, e.g. feed descriptions or values extracted from the parsed content, may contain + * CR and LF characters. Written verbatim into the application/warc-fields payload of a WARC + * metadata record, a CR LF sequence would end the field line and the remainder of the value would + * look exactly like an additional field line generated by the crawler. + */ +class MetadataRecordFormatCRLFTest { + + private byte[] record(String key, String value) { + Metadata metadata = new Metadata(); + metadata.addValue(key, value); + Tuple tuple = mock(Tuple.class); + when(tuple.getStringByField("url")).thenReturn("https://www.example.org/"); + when(tuple.getValueByField("metadata")).thenReturn(metadata); + MetadataRecordFormat format = new MetadataRecordFormat(List.of(key)); + return format.format(tuple); + } + + private static List parseFields(byte[] warcBytes) { + List parsed = new ArrayList<>(); + try (WarcReader reader = new WarcReader(new ByteArrayInputStream(warcBytes))) { + for (WarcRecord rec : reader) { + assertTrue(rec instanceof WarcMetadata, "Can't parse as WARC metadata record"); + parsed.add(((WarcMetadata) rec).fields()); + } + } catch (IOException e) { + throw new RuntimeException(e); + } + return parsed; + } + + @Test + void valueWithCRLFDoesNotCreateExtraFields() { + byte[] warcBytes = record("feed.description", "some text\r\nhopsFromSeed: 1"); + String warcString = new String(warcBytes, StandardCharsets.UTF_8); + assertFalse( + warcString.contains("\r\nhopsFromSeed: 1\r\n"), + "a metadata value must not introduce a new warc-fields line"); + // CR and LF are replaced by spaces, the value remains on its own field line + assertTrue( + warcString.contains("feed.description: some text hopsFromSeed: 1\r\n"), + "the sanitised metadata value is expected on a single field line"); + + List records = parseFields(warcBytes); + assertEquals(1, records.size(), "expected a single WARC metadata record"); + MessageHeaders fields = records.get(0); + assertEquals( + List.of("some text hopsFromSeed: 1"), + fields.all("feed.description"), + "the sanitised metadata value must be kept on a single field line"); + assertFalse( + fields.contains("hopsFromSeed", "1"), + "parsers must not see a field the crawler did not write"); + } + + @Test + void valueWithBareLineBreaksDoesNotCreateExtraFields() { + for (String value : List.of("some text\nhopsFromSeed: 1", "some text\rhopsFromSeed: 1")) { + byte[] warcBytes = record("feed.description", value); + String warcString = new String(warcBytes, StandardCharsets.UTF_8); + assertFalse( + warcString.contains("\r\nhopsFromSeed: 1\r\n"), + "a metadata value must not introduce a new warc-fields line"); + for (MessageHeaders fields : parseFields(warcBytes)) { + assertFalse( + fields.contains("hopsFromSeed", "1"), + "parsers must not see a field the crawler did not write"); + } + } + } + + @Test + void invalidFieldNamesAreDropped() { + // a colon or a space makes the key unusable as WARC field name + for (String key : List.of("hopsFromSeed: 1", "hops FromSeed")) { + byte[] warcBytes = record(key, "any value"); + assertEquals( + 0, warcBytes.length, "a record with an invalid field name must not be written"); + } + } +} diff --git a/external/warc/src/test/java/org/apache/stormcrawler/warc/WARCRecordFormatTest.java b/external/warc/src/test/java/org/apache/stormcrawler/warc/WARCRecordFormatTest.java index 045579a89..6a6e2544b 100644 --- a/external/warc/src/test/java/org/apache/stormcrawler/warc/WARCRecordFormatTest.java +++ b/external/warc/src/test/java/org/apache/stormcrawler/warc/WARCRecordFormatTest.java @@ -331,6 +331,47 @@ void testWarcResourceRecord() { "WARC record: no or incorrect block, digest"); } + @Test + void testWarcResourceRecordContentTypeCRLFInjection() { + // test that a server-controlled Content-Type cannot forge additional WARC header + // lines in a resource record + String txt = "abcdef"; + byte[] content = txt.getBytes(StandardCharsets.UTF_8); + Metadata metadata = new Metadata(); + metadata.addValue( + protocolMDprefix + HttpHeaders.CONTENT_TYPE, + "text/html\r\nWARC-Truncated: length\r\n"); + Tuple tuple = mock(Tuple.class); + when(tuple.getBinaryByField("content")).thenReturn(content); + when(tuple.getStringByField("url")).thenReturn("https://www.example.org/"); + when(tuple.getValueByField("metadata")).thenReturn(metadata); + WARCRecordFormat format = new WARCRecordFormat(protocolMDprefix); + byte[] warcBytes = format.format(tuple); + String warcString = new String(warcBytes, StandardCharsets.UTF_8); + assertFalse( + warcString.contains("\r\nWARC-Truncated: length"), + "WARC record: Content-Type must not forge additional WARC header lines"); + // CR and LF are replaced by spaces, the content type remains on its header line + assertTrue( + warcString.contains("Content-Type: text/html WARC-Truncated: length"), + "WARC record: sanitised Content-Type expected on a single header line"); + + // try to read it with Jwarc + try (WarcReader reader = new WarcReader(new ByteArrayInputStream(warcBytes))) { + for (WarcRecord record : reader) { + assertFalse( + record.headers().contains("WARC-Truncated", "length"), + "WARC record: WARC header block must not contain a forged header"); + assertEquals( + 1, + record.headers().all("Content-Type").size(), + "WARC record: expected a single Content-Type header"); + } + } catch (IOException e) { + throw new RuntimeException(e); + } + } + @Test void testWarcMetadataRecord() { Metadata metadata = new Metadata(); From 311f3aba69de4498bed7d128b03fd454ee277a27 Mon Sep 17 00:00:00 2001 From: abhinav-phi Date: Tue, 1 Sep 2026 13:19:40 +0530 Subject: [PATCH 2/3] Validate warc.metadata.keys once on instantiation The configured metadata keys are fixed topology configuration: check them for valid WARC field names in the constructor and drop invalid keys with a single warning, instead of repeating the check and warning for every record written. Suggested in review. --- .../warc/MetadataRecordFormat.java | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/external/warc/src/main/java/org/apache/stormcrawler/warc/MetadataRecordFormat.java b/external/warc/src/main/java/org/apache/stormcrawler/warc/MetadataRecordFormat.java index afac5958e..75b79924e 100644 --- a/external/warc/src/main/java/org/apache/stormcrawler/warc/MetadataRecordFormat.java +++ b/external/warc/src/main/java/org/apache/stormcrawler/warc/MetadataRecordFormat.java @@ -17,6 +17,7 @@ import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; +import java.util.ArrayList; import java.util.List; import org.apache.commons.lang3.StringUtils; import org.apache.storm.tuple.Tuple; @@ -48,12 +49,24 @@ public class MetadataRecordFormat extends WARCRecordFormat { private static final Logger LOG = LoggerFactory.getLogger(MetadataRecordFormat.class); - private List metadataKeys; + private final List metadataKeys; public MetadataRecordFormat(List metadataKeys) { super(""); - this.metadataKeys = metadataKeys; - LOG.info("MetadataRecordFormat instantiated with {}", String.join(",", metadataKeys)); + // the keys are fixed configuration: validate them once here instead of + // for every record + final List validKeys = new ArrayList<>(metadataKeys.size()); + for (String key : metadataKeys) { + if (isValidWarcFieldName(key)) { + validKeys.add(key); + } else { + LOG.warn( + "Skipping invalid WARC field name configured in warc.metadata.keys: {}", + key); + } + } + this.metadataKeys = List.copyOf(validKeys); + LOG.info("MetadataRecordFormat instantiated with {}", String.join(",", this.metadataKeys)); } @Override @@ -73,14 +86,6 @@ public byte[] format(Tuple tuple) { // get the metadata key / values to save in the WARCs for (String key : metadataKeys) { - // the key becomes the name of a WARC field: drop it entirely if it is not a - // valid field name, otherwise the field line would be malformed - if (!isValidWarcFieldName(key)) { - LOG.warn( - "Skipping invalid WARC field name configured in warc.metadata.keys: {}", - key); - continue; - } final String[] values = metadata.getValues(key); if (values == null || values.length == 0) { continue; From f57d892ea73f740669cf98564662069685d0d8c7 Mon Sep 17 00:00:00 2001 From: abhinav-phi Date: Tue, 1 Sep 2026 20:04:31 +0530 Subject: [PATCH 3/3] Make null check in isValidWarcFieldName explicit The conjunction already returned false for a null name through short-circuit evaluation, but the intent was easy to miss. Return false explicitly and cover the field name and value sanitisation helpers with unit tests. Suggested in review. --- .../stormcrawler/warc/WARCRecordFormat.java | 5 +++- .../warc/WARCRecordFormatTest.java | 25 +++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/external/warc/src/main/java/org/apache/stormcrawler/warc/WARCRecordFormat.java b/external/warc/src/main/java/org/apache/stormcrawler/warc/WARCRecordFormat.java index 0a387fed6..ad8f2d46a 100644 --- a/external/warc/src/main/java/org/apache/stormcrawler/warc/WARCRecordFormat.java +++ b/external/warc/src/main/java/org/apache/stormcrawler/warc/WARCRecordFormat.java @@ -105,7 +105,10 @@ public class WARCRecordFormat implements RecordFormat { * @return true if the name can safely be written as the name of a WARC field */ static boolean isValidWarcFieldName(String name) { - return name != null && WARC_FIELD_NAME_PATTERN.matcher(name).matches(); + if (name == null) { + return false; + } + return WARC_FIELD_NAME_PATTERN.matcher(name).matches(); } /** diff --git a/external/warc/src/test/java/org/apache/stormcrawler/warc/WARCRecordFormatTest.java b/external/warc/src/test/java/org/apache/stormcrawler/warc/WARCRecordFormatTest.java index 6a6e2544b..d834d8ffd 100644 --- a/external/warc/src/test/java/org/apache/stormcrawler/warc/WARCRecordFormatTest.java +++ b/external/warc/src/test/java/org/apache/stormcrawler/warc/WARCRecordFormatTest.java @@ -19,6 +19,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -123,6 +124,30 @@ void testCanonicalizeIpAddress() { } } + @Test + void testWarcFieldNameValidation() { + assertFalse(WARCRecordFormat.isValidWarcFieldName(null), "null is not a field name"); + assertFalse(WARCRecordFormat.isValidWarcFieldName(""), "empty string is not a field name"); + assertFalse( + WARCRecordFormat.isValidWarcFieldName("hops FromSeed"), + "a space is not allowed in a field name"); + assertFalse( + WARCRecordFormat.isValidWarcFieldName("hopsFromSeed: 1"), + "a colon is not allowed in a field name"); + assertTrue(WARCRecordFormat.isValidWarcFieldName("via")); + assertTrue(WARCRecordFormat.isValidWarcFieldName("feed.description")); + assertTrue(WARCRecordFormat.isValidWarcFieldName("WARC-Truncated")); + } + + @Test + void testSanitizeWarcFieldValue() { + assertNull(WARCRecordFormat.sanitizeWarcFieldValue(null)); + assertEquals("unchanged", WARCRecordFormat.sanitizeWarcFieldValue("unchanged")); + assertEquals("a b", WARCRecordFormat.sanitizeWarcFieldValue("a\r\nb")); + assertEquals("a b", WARCRecordFormat.sanitizeWarcFieldValue("a\rb")); + assertEquals("a b", WARCRecordFormat.sanitizeWarcFieldValue("a\nb")); + } + @Test void testWarcRecord() { // test validity of WARC record