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.

Sign up for GitHub

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

Jump to bottom

Fix request->is(xml) returning true for HTML requests #16585

Merged
merged 5 commits into from Jun 28, 2022
Merged

Fix request->is(xml) returning true for HTML requests #16585

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
97d3982
Fix request->is(xml) returning true for HTML requests
markstory Jun 25, 2022
688b27a
Make content type detection more robust.
markstory Jun 25, 2022
fa5d521
Fix phpcs
markstory Jun 27, 2022
3f1c054
Apply suggestions from code review
markstory Jun 28, 2022
e7aaba3
Use ?? instead of ?:
markstory Jun 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/Http/ServerRequest.php
View file
Open in desktop
Expand Up @@ -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.
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

@markstory markstory Jun 25, 2022 •
edited

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.

Copy link
Member

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.

$options[] = 'text/html';
$accepted = $content->preferredType($this, $options);

return $accepted !== null;
return $accepted !== null && $accepted != 'text/html';
}

/**
Expand Down
15 changes: 15 additions & 0 deletions tests/TestCase/Http/ContentTypeNegotiationTest.php
View file
Open in desktop
Expand Up @@ -26,6 +26,21 @@ public function testPreferredTypeNoAccept()
$this->assertNull($content->preferredType($request));
}

public function testPreferredTypeFirefoxHtml()
{
$content = new ContentTypeNegotiation();
$request = new ServerRequest([
'url' => '/dashboard',
'environment' => [
'HTTP_ACCEPT' => 'text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,*/*;q=0.8',
],
]);
$this->assertEquals('text/html', $content->preferredType($request));
$this->assertEquals('text/html', $content->preferredType($request, ['text/html', 'application/json']));
markstory marked this conversation as resolved.
Show resolved Hide resolved
$this->assertEquals('application/xml', $content->preferredType($request, ['application/xml']));
$this->assertNull($content->preferredType($request, ['application/json']));
}

public function testPreferredTypeFirstMatch()
{
$content = new ContentTypeNegotiation();
Expand Down
6 changes: 6 additions & 0 deletions tests/TestCase/Http/ServerRequestTest.php
View file
Open in desktop
Expand Up @@ -92,6 +92,12 @@ public function testAcceptHeaderDetector(): void
$request = new ServerRequest();
$request = $request->withEnv('HTTP_ACCEPT', 'text/plain, */*');
$this->assertFalse($request->is('json'));

$request = new ServerRequest();
$request = $request->withEnv('HTTP_ACCEPT', 'text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,*/*;q=0.8');
$this->assertFalse($request->is('json'));
$this->assertFalse($request->is('xml'));
$this->assertFalse($request->is('xml'));
}

public function testConstructor(): void
Expand Down