apache / apache/stormcrawler

A response header named set-header becomes a request header directive

Open Beginner friendly
#2,090 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Java
Stars
995
Forks
292
Avg merge
2d 49m
Merged PRs (30d)
62

Description

## What happens
`FetcherBolt` merges every response header into the tuple metadata under the protocol prefix, lowercased. The okhttp protocol reads the `set-header` directive from that same prefixed namespace. A server that returns a header literally named `set-header` therefore produces the metadata key `protocol.set-header`, which is exactly the key `addHeadersToRequest` uses to add arbitrary headers to an outgoing request. The merge already strips `robots.crawl.delay` for the same reason, so the collision class is known, but `set-header` is not stripped. The other request shaping keys (`http.post.json`, `http.method.head`, `http.content.limit`, `http.proxy*`) are read without the prefix and are not reachable this way.

## Where
`core/src/main/java/org/apache/stormcrawler/bolt/FetcherBolt.java:802`, with the existing strip four lines below at `:806`:

```java
mergedMetadata.putAll(response.getMetadata(), protocolMetadataPrefix);

// Only the locally parsed robots.txt value may populate this control signal.
// A colliding protocol prefix/header must not pace an unrelated queue.
mergedMetadata.remove(Constants.ROBOTS_CRAWL_DELAY_KEY);
```

`core/src/main/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocol.java:299`:

```java
final String[] headerStrings = md.getValues(SET_HEADER_BY_REQUEST, protocolMetadataPrefix);
```

Config keys involved: `protocol.md.prefix`, `metadata.persist`, `metadata.transfer`.

## Why it matters
The value only becomes live on a later fetch if the operator persists or transfers it, and `metadata.transfer` is empty in every shipped configuration. It is not an exotic setup, though: internals.adoc tells operators to persist `protocol.` keys such as `protocol.etag` and `protocol.set-cookie`, and both lists accept wildcards, so a `protocol.*` entry carries the forged key along with the intended ones. The result is that a crawled site chooses headers sent on subsequent requests, including requests to other hosts when the key is transferred to outlinks. Request shaping should come from configuration, not from a response body or header.

## Reproduction

Save as `core/src/test/java/org/apache/stormcrawler/protocol/okhttp/ProtocolSetHeaderFromResponseTest.java`:

```java
/*
* 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.protocol.okhttp;

import okhttp3.Request;
import org.apache.storm.Config;
import org.apache.stormcrawler.Metadata;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

/**
* Documents the current handling of a response header named "set-header". The header merge in
* FetcherBolt writes response headers under the protocol metadata prefix, which is the same place
* addHeadersToRequest reads the set-header directive from. The value should not become a request
* header on a later fetch.
*/
class ProtocolSetHeaderFromResponseTest {

private static Config protocolConfig() {
Config conf = new Config();
conf.put("http.agent.name", "test");
conf.put("http.agent.version", "1.0");
conf.put("http.agent.description", "test");
conf.put("http.agent.url", "http://test.example.com");
conf.put("http.agent.email", "test@example.com");
conf.put("protocol.md.prefix", "protocol.");
return conf;
}

@Test
void responseHeaderNamedSetHeaderBecomesARequestHeader() {
HttpProtocol protocol = new HttpProtocol();
protocol.configure(protocolConfig());

// what a server returned, lowercased and merged the way FetcherBolt does it
Metadata fromResponse = new Metadata();
fromResponse.addValue("set-header", "X-Forged=1");

Metadata carried = new Metadata();
carried.putAll(fromResponse, "protocol.");
Assertions.assertEquals("X-Forged=1", carried.getFirstValue("protocol.set-header"));

Request.Builder rb = new Request.Builder().url("https://example.com/");
protocol.addHeadersToRequest(rb, carried);
Request request = rb.build();

// current behaviour: the server-supplied value is sent on the next request.
// It should be ignored: only operator configuration may set request headers.
Assertions.assertEquals("1", request.header("X-Forged"));
}
}
```

Run it:

```
mvn -pl core test -Dtest=ProtocolSetHeaderFromResponseTest
```

It asserts the present behaviour and passes on main, with a comment saying what should happen instead; the fix can land either in the bolt or in the protocol, so a failing test would prejudge where.

```
[INFO] Running org.apache.stormcrawler.protocol.okhttp.ProtocolSetHeaderFromResponseTest
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.243 s -- in org.apache.stormcrawler.protocol.okhttp.ProtocolSetHeaderFromResponseTest
```

The test builds metadata the way the bolt does (`putAll(responseMetadata, "protocol.")`), calls `addHeadersToRequest`, and finds `X-Forged: 1` on the request.

## Suggested fix
Strip `set-header` from the merged metadata in `FetcherBolt.fetch` right after the `putAll` with the protocol prefix, next to the existing `robots.crawl.delay` removal. Beyond that one key, treat this as a class: reserve a namespace for directives that shape a request and have `MetadataTransfer` refuse to transfer or persist keys in it unless the operator names them one by one, so that a wildcard cannot pick them up. Operators who rely on a `protocol.*` wildcard today will see fewer keys carried, which belongs in the release notes.

Contributor guide

Open the contributing guide

Research direction

Start with the metadata merge at core/src/main/java/org/apache/stormcrawler/bolt/FetcherBolt.java:802 and compare it with addHeadersToRequest in core/src/main/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocol.java:299. Run core/src/test/java/org/apache/stormcrawler/protocol/okhttp/ProtocolSetHeaderFromResponseTest.java with the Maven command given; done means a response header named set-header is not emitted as a request header while intended request directives still work.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
backend, security
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.