[I] A response header named set-header becomes a request header directive [stormcrawler]

["rzo1 (via GitHub)" <gi...@apache.org>]

rzo1 opened a new issue, #2090:
URL: https://github.com/apache/stormcrawler/issues/2090

   
   ## 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.
   


-- 
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: dev-unsubscribe@stormcrawler.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] A response header named set-header becomes a request header directive [stormcrawler]

["rzo1 (via GitHub)" <gi...@apache.org>]

rzo1 closed issue #2090: A response header named set-header becomes a request header directive
URL: https://github.com/apache/stormcrawler/issues/2090


-- 
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: dev-unsubscribe@stormcrawler.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org