This is an automated email from the ASF dual-hosted git repository.
jnioche pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/stormcrawler.git
The following commit(s) were added to refs/heads/main by this push:
new 0663b8c7 Escape CR and LF in WARC metadata records and resource
Content-Type (#2109)
0663b8c7 is described below
commit 0663b8c7f49140daaeba35572e4488b3766d9e8c
Author: Abhinav <[email protected]>
AuthorDate: Tue Sep 1 21:38:28 2026 +0530
Escape CR and LF in WARC metadata records and resource Content-Type (#2109)
* 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
* 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.
* 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/MetadataRecordFormat.java | 25 ++++-
.../apache/stormcrawler/warc/WARCRecordFormat.java | 40 ++++++-
.../warc/MetadataRecordFormatCRLFTest.java | 119 +++++++++++++++++++++
.../stormcrawler/warc/WARCRecordFormatTest.java | 66 ++++++++++++
4 files changed, 244 insertions(+), 6 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 ad054911..75b79924 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 @@ package org.apache.stormcrawler.warc;
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;
@@ -46,14 +47,26 @@ fetchTimeMs: 565
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<String> metadataKeys;
+ private final List<String> metadataKeys;
public MetadataRecordFormat(List<String> 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<String> 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
@@ -81,7 +94,9 @@ public class MetadataRecordFormat extends WARCRecordFormat {
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 cee46c8f..ad8f2d46 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,42 @@ 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) {
+ if (name == null) {
+ return false;
+ }
+ return 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 +481,9 @@ public class WARCRecordFormat implements RecordFormat {
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 00000000..92430dc1
--- /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<MessageHeaders> parseFields(byte[] warcBytes) {
+ List<MessageHeaders> 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<MessageHeaders> 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 045579a8..d834d8ff 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 @@ 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.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 @@ class WARCRecordFormatTest {
}
}
+ @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
@@ -331,6 +356,47 @@ class WARCRecordFormatTest {
"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();
