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
Fix request->is(xml) returning true for HTML requests #16585
Conversation
Add html/text as an option for all content-type based detectors so that we can differentiate between requests that want HTML but also take xml or json from being confused as a non-html type. Assuming browser based traffic is a safe default in my opinion. Fixes #16583
src/Http/ServerRequest.php
Outdated
@@ -553,9 +553,12 @@ protected function _is(string $type, array $args): bool | |||
protected function _acceptHeaderDetector(array $detect): bool | |||
{ | |||
$content = new ContentTypeNegotiation(); | |||
$accepted = $content->preferredType($this, $detect['accept']); | |||
$options = $detect['accept']; | |||
// We add text/html as the browsers often combine xml + html types together. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs a clearer comment about the effect we want to have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In writing a better explanation I realized that this change would break detecting HTML types. I've change the implementation around a bit and used a more explicit ignore/exclude list instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we're saying if you have a request with html and xml accept headers and have only an xml renderer, then this isn't a valid xml request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we're saying if you have a request with html and xml accept headers and have only an xml renderer, then this isn't a valid xml request.
Kind of. It depends on the header value. An Accept header of text/html,application/xml
will not count as XML. As both XML and HTML are of equal preference value.
However, a request of text/html;q=0.8, application/xml
will count as XML as HTML is a lower preference value. Given that many browsers include text/html,application/xml;q=0.9
in their headers we can't only check if XML is accepted because while the browser will accept XML, it prefers HTML more.
Adding in a special case for the XML type seemed like the safest change to me. The previous implementation of _acceptDetector
was:
protected function _acceptHeaderDetector(array $detect): bool
{
$acceptHeaders = explode(',', (string)$this->getEnv('HTTP_ACCEPT'));
foreach ($detect['accept'] as $header) {
if (in_array($header, $acceptHeaders, true)) {
return true;
}
}
return false;
}
This logic was doing a strict check on only the unqualified types and application/xml
!= application/xml;q=0.9
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that makes sense to me. We're marking exclusions at equal preference. Instead of writing a sorting order, we're special casing exclusions. Other requests with matching equality get what they get from the server.
Someone could have a detector that wants to find text/html and the previous change would break that.
Co-authored-by: ADmad <ADmad@users.noreply.github.com>
Add html/text as an option for all content-type based detectors so that we can differentiate between requests that want HTML but also take xml or json from being confused as a non-html type. Assuming browser based traffic is a safe default in my opinion.
Fixes #16583