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

ServerRequest::fromGlobals can produce incorrect results #363

Closed
Firehed opened this issue Dec 12, 2020 · 9 comments
Closed

ServerRequest::fromGlobals can produce incorrect results #363

Firehed opened this issue Dec 12, 2020 · 9 comments
Milestone

Comments

@Firehed
Copy link

Firehed commented Dec 12, 2020

PHP version: 7.4.12 (not relevant)

Description
ServerRequest::fromGlobals() is producing an object with Content-length and Content-type set to '' on GET requests when neither header is present. This differs from the behavior of the abandoned Zend Diactoros equivalent, which strips those values from the superglobals when empty. I haven't cross-checked other implementations.

How to reproduce
var_dump(\GuzzleHttp\Psr7\ServerRequest::fromGlobals()->getHeader('content-type')) on any GET request.

This appears to be specific to some server configurations, including common/default nginx configs: fastcgi_params can set the $_SERVER['CONTENT_TYPE'] and $_SERVER['CONTENT_LEGNTH'] to empty string values.

Possible Solution
Something functionally equivalent to:

- $headers = getallheaders();
+ $headers = array_filter(getallheaders(), fn ($v) => $v !== '');

Given the PHP versions this supports, it would need a bit of adjusting.

It may be more appropriate to handle elsewhere, as I think empty header values violate the actual HTTP specs.

Additional context
While this can probably be worked around by changing server configs, this is not expected out-of-the-box behavior, and differs from other similar PSR-7 implementations.

This can similarly be worked around in user-code in the same way, but walking and stripping headers is less performant than doing it at the source in the library, and is probably marginally more error-prone when considering how repeated headers work.

@GrahamCampbell
Copy link
Member

This is a breaking change we could make on 2.x, if we think this behaviour is better.

@bbrala
Copy link

bbrala commented Jun 18, 2021

The http 1.1 spec does say the only valid value for this header is an integer greater than 0. I think it would make sense not to keep the empty headers.

@GrahamCampbell
Copy link
Member

I think it would make sense not to keep the empty headers.

Seems fair. I'd be happy to make this change on 2.x 2.0.0 is coming out in the next few weeks. :)

@GrahamCampbell
Copy link
Member

If you'd like to send a PR @bbrala to do that, that's be great. :)

@bbrala
Copy link

bbrala commented Jun 18, 2021

Sure, i would only filter the content headers though so we don't get possible weird side effects.

@bbrala
Copy link

bbrala commented Jun 18, 2021

There you go :)

@bbrala
Copy link

bbrala commented Jun 18, 2021

Ok, stripping empty headers is a bad idea. Some parts of http allow empty headers, http2 also. This issue is a good source of multiple other sources: jetty/jetty.project#1116

@GrahamCampbell
Copy link
Member

Conclusion here is that this is not incorrect.

@GrahamCampbell
Copy link
Member

Zend is actually implementing this wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants