Fixed wrong header concatenation

[felipehertzer]

The new version of PRS7 2.2.1 header validation is stricter this commit solves the header validation problem.

Current:

Vary: Origin\n
X-Origin\n
Referer\n

Should be:

Vary: Origin, X-Origin, Referer

https://github.com/guzzle/psr7

More information about concatenation of headers:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Vary

See #2230

[vishwarajanand]

Do you think adding a test or extending an existing test in Http/BatchTest.php will make sense?

[felipehertzer]

Hey @vishwarajanand currently the testBatchRequest function already performs this test, if you run the test with guzzlehttp/psr7 (2.2.1) the test does not pass without this fix.

[vishwarajanand]

LGTM

[pimjansen]

This will work but will break set-cookie. The keys should be passed as array instead and should not be concat

[pimjansen]

Havent tried it but now we are concatting all headers and also those who should not.

Isnt $headers[$header][] = $value; the way to go here? Havent tried it yet though

[felipehertzer]

Hi @pimjansen I can do something like this, it will work, but maybe it's overkill, do you know how to simulate a return that might contain unwanted headers?

Array method:

$headers[$header] = array_merge(is_array($headers[$header]) ? $headers[$header] : [$headers[$header]], [$value]);

Output:

array(1) {
  [0]=>
  array(2) {
    ["vary"]=>
    array(3) {
      [0]=>
      string(6) "Origin"
      [1]=>
      string(8) "X-Origin"
      [2]=>
      string(7) "Referer"
    }
    ["content-type"]=>
    string(31) "application/json; charset=UTF-8"
  }
}

Concatenation:

$headers[$header] .= ", " . $value;

Output:

array(1) {
  [0]=>
  array(2) {
    ["vary"]=>
    string(25) "Origin, X-Origin, Referer"
    ["content-type"]=>
    string(31) "application/json; charset=UTF-8"
  }
}

[pimjansen]

If in correct the header one is the correct one though.

For example when i return 2 content type headers due whatever reason it will concat where it is invalid if im correct?

[TimWolla]

Just checking in from my phone, thus didn't look at all the details. Passing a single element array should behave identically to passing a string. The PSR-7 getHeader method will always return an array anyway and getHeaderLine the comma concatenated array.

I recommend to always pass an array to the Response class, it is never wrong.

[felipehertzer]

Yes, I understand what you mean, but this code only parses the responses of batches sent to google api, and in tests google only returns 3 vary and 1 content-type.

[TimWolla]

I don't have any deeper interest in the Google API client, I don't use it myself, so ultimately I don't particularly care about whatever is done here.

That said: I don't think it's a particularly strong argument to implement a fix that is known to break in some cases, just because these cases are currently impossible to trigger. The array version preserves the semantics of the message correctly in 100% of the cases, whereas the comma-concatenated version does not. The array version is not really any harder to implement either, it should just be a []= instead of .=.

[shubhamchugh]

working fine with these changes

[bshaffer]

This change makes me very nervous, as it changes the existing functionality on a library that has been around for a very long time and shouldn't really be changing in ways that could potentially break existing applications.

At the very least we need a test for the new behavior and a better justification for why we need this. Does something break? Is there a PHP warning or error anywhere? Or is this just what some PSR wants? What's the downside to not making this change?

[felipehertzer]

Hey @bshaffer the tests already handle the changes, if you run the tests with the latest versions of prs the batch tests will fail. This fix is to fix this problem. The main disadvantage without this fix is that the batch will not work anymore, because now prs only accepts sending headers concatenated with comma (,) and not with newline (\n)

[ejunker]

@bshaffer this started not working with guzzlehttp/psr7 2.2.1 I believe when they added validation of headers as part of a security fix. According to the RFC the way the Google client is sending headers is not correct. See guzzle/psr7#494 (comment)

[bshaffer]

Thank you for your work @felipehertzer (and your explanation @ejunker). This looks great now.

[pimjansen]

@bshaffer any idea on when you can release this? Since it breaks a lot if implementations that use the PSR package

[bshaffer]

@pimjansen it's been released in v2.12.2