Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exception while parsing header of batch request #2230

Closed
brainformatik opened this issue Mar 21, 2022 · 8 comments
Closed

Exception while parsing header of batch request #2230

brainformatik opened this issue Mar 21, 2022 · 8 comments
Labels
🚨 This issue needs some love. triage me I really want to be triaged.

Comments

@brainformatik
Copy link

brainformatik commented Mar 21, 2022

Lately we have an issue with a batch request to GMail API that worked before. We get a list of all labels a user has in gmail with the first request, then we create a batch to get details for all labels.

Environment details

  • OS: Debian Linux Server
  • PHP version: 7.3.33
  • Package name and version: google/apiclient 2.12.1

Steps to reproduce

  1. Get list of all user labels using users_labels->listUsersLabels('me')
  2. Create batch with single calls to users_labels->get() after calling setUseBatch before.
  3. Execute batch with execute() method
  4. Throws an exception during parsing of headers

Further details

In my opinion, there is an issue with the resulting header of the batch requests, and the parsing inside the PHP client.

Here is a snipped of a single batch result

--batch_KrShUDjgnVv3chxBsrI2iopwhvYu4fjU
Content-Type: application/http
Content-ID: response-1730032590

HTTP/1.1 200 OK
Content-Type: application/json; charset=UTF-8
Vary: Origin
Vary: X-Origin
Vary: Referer

{
  "id": "IMPORTANT",
  "name": "IMPORTANT",
  "messageListVisibility": "hide",
  "labelListVisibility": "labelShow",
  "type": "system",
  "messagesTotal": 204,
  "messagesUnread": 0,
  "threadsTotal": 187,
  "threadsUnread": 0
}

--batch_KrShUDjgnVv3chxBsrI2iopwhvYu4fjU

There are multiple Vary header in the response compared to official docs where these headers are missing:
https://developers.google.com/gmail/api/guides/batch#batch-example-response

Inside src/Http/Batch.php in parseRawHeaders method these headers are combined using \n.

When debugging further, we get to src/MessageTrait.php and the trimAndValidateHeaderValues method.
There is a line with $this->assertValue($trimmed); where inside there is a check for allowed characters and \n seems not to be allowed.

If we change the concat from \n to , it would run.

@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Mar 22, 2022
@robtesch
Copy link

I've done a bit of further digging on this (since it's broken our app entirely).

I think this is actually related to a recent change in guzzlehttp/psr7 (https://github.com/guzzle/psr7/blob/2.2.1/CHANGELOG.md).

Temporarily forcing guzzle/psr7 to v2.1.0 in composer.json seems to do the job of fixing this (because it removes the newly added header validation). Unfortunately this is not a safe thing to do, thanks to this: GHSA-q7rv-6hp3-vh96

Making the change suggested by @brainformatik would solve the issue properly.

@TimWolla
Copy link

If we change the concat from \n to , it would run.

Headers should not be concatenated, but passed as an array. See my response in this issue for guzzlehttp/psr7:

guzzle/psr7#494 (comment)

@pimjansen
Copy link

pimjansen commented Mar 23, 2022

Isnt the bug not in the SDK but the API itself? If i look at the definition it seems that the Vary header needs to be comma seperated?
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Vary

For now we just receive multiple Vary headers. I have to admit that this is not my overall expertise area on how the spec works on this. Maybe @TimWolla knows something about this?

Fixing this by pushing them in an array is not possible since the use the header value as key. So it will cause sidefx all over.

@TimWolla
Copy link

Isnt the bug not in the SDK but the API itself? If i look at the definition it seems that the Vary header needs to be comma seperated?

The API is fine, this is defined in RFC 7230#3.2.2:

A sender MUST NOT generate multiple header fields with the same field
name in a message unless either the entire field value for that
header field is defined as a comma-separated list [i.e., #(values)]
or the header field is a well-known exception (as noted below).

and further

A recipient MAY combine multiple header fields with the same field
name into one "field-name: field-value" pair, without changing the
semantics of the message, by appending each subsequent field value to
the combined field value in order, separated by a comma.

The Vary header is defined to be a comma-separated list of values and thus may be split across multiple header fields with the same header name.

If an array doesn't work, then concatenating them with a comma is fine according to the spec, but set-cookie is the one header where this will break semantics. A newline (like it is currently implemented) is never correct and rightfully rejected by Guzzle.

@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Mar 26, 2022
@juanparati
Copy link

I am also facing issues with the Google API client.

The following error is received by Guzzle when I perform batch requests.

 InvalidArgumentException

  "Origin
X-Origin
Referer" is not valid header value

  at vendor/guzzlehttp/psr7/src/MessageTrait.php:263
    259▕         // Clients must not send a request with line folding and a server sending folded headers is
    260▕         // likely very rare. Line folding is a fairly obscure feature of HTTP/1.1 and thus not accepting
    261▕         // folding is not likely to break any legitimate use case.
    262▕         if (! preg_match('/^[\x20\x09\x21-\x7E\x80-\xFF]*$/', $value)) {
  ➜ 263▕             throw new \InvalidArgumentException(sprintf('"%s" is not valid header value', $value));
    264▕         }
    265▕     }
    266▕ }

It seems that problem is that Guzzle doesn't allow headers with break lines.

@tehmaestro
Copy link
Contributor

tehmaestro commented Apr 3, 2022

Hi, I just stumbled upon this issue too. Is there any way we can work around this?
For now I did a composer require guzzlehttp/psr7:2.1.0

@pimjansen
Copy link

Hi, I just stumbled upon this issue too. Is there any way we can work around this?

For now I did a composer require guzzlehttp/psr7:2.1.0

Wait till a maintainer resolves the open pr i guess

@brainformatik
Copy link
Author

fixed in v2.12.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚨 This issue needs some love. triage me I really want to be triaged.
Projects
None yet
Development

No branches or pull requests

7 participants