Skip to content

Commit

Permalink
Check the content type when replacing tokens
Browse files Browse the repository at this point in the history
  • Loading branch information
leofeyer committed Sep 5, 2019
1 parent 51a69b1 commit 7982b80
Show file tree
Hide file tree
Showing 2 changed files with 77 additions and 18 deletions.
5 changes: 5 additions & 0 deletions core-bundle/src/EventListener/CsrfTokenCookieListener.php
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,11 @@ private function setCookies(Request $request, Response $response): void

private function replaceTokenOccurrences(Response $response): void
{
// Return if the response is not a HTML document
if (false === stripos((string) $response->headers->get('Content-Type'), 'text/html')) {
return;
}

$content = $response->getContent();

foreach ($this->tokenStorage->getUsedTokens() as $value) {
Expand Down
90 changes: 72 additions & 18 deletions core-bundle/tests/EventListener/CsrfTokenCookieListenerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use Contao\CoreBundle\Csrf\MemoryTokenStorage;
use Contao\CoreBundle\EventListener\CsrfTokenCookieListener;
use PHPUnit\Framework\TestCase;
use Symfony\Component\HttpFoundation\BinaryFileResponse;
use Symfony\Component\HttpFoundation\Cookie;
use Symfony\Component\HttpFoundation\ParameterBag;
use Symfony\Component\HttpFoundation\Request;
Expand Down Expand Up @@ -210,24 +211,6 @@ public function testRemovesTheTokenCookiesAndReplacesTokenOccurrencesIfNoOtherCo
->willReturn(true)
;

$response = new Response('<html><body><form><input name="REQUEST_TOKEN" value="tokenValue"></form></body></html>');

$responseEvent = $this->createMock(FilterResponseEvent::class);
$responseEvent
->method('isMasterRequest')
->willReturn(true)
;

$responseEvent
->method('getRequest')
->willReturn($request)
;

$responseEvent
->method('getResponse')
->willReturn($response)
;

$responseHeaders = $this->createMock(ResponseHeaderBag::class);
$responseHeaders
->expects($this->never())
Expand All @@ -245,8 +228,32 @@ public function testRemovesTheTokenCookiesAndReplacesTokenOccurrencesIfNoOtherCo
->with('csrf_foo')
;

$responseHeaders

This comment has been minimized.

Copy link
@Toflar

Toflar Sep 5, 2019

Member

Why are you mocking the response headers?

This comment has been minimized.

Copy link
@leofeyer

leofeyer Sep 5, 2019

Author Member

I am not – you are. 😂 I have just copied the code from the methods above.

This comment has been minimized.

Copy link
@Toflar

Toflar Sep 5, 2019

Member

Nah, I would never mock response headers. Just pass them to new Response() already?

This comment has been minimized.

Copy link
@leofeyer

leofeyer Sep 5, 2019

Author Member

You are right, it was not you. But the code makes sense, as there are assertions on the object, doesn't it?

This comment has been minimized.

Copy link
@Toflar

Toflar Sep 5, 2019

Member

Imho in that test you just need a response with a Content-Type: text/html header so I don't see why mocking them makes sense. But I'm fine with it either way. :)

This comment has been minimized.

Copy link
@leofeyer

leofeyer Sep 6, 2019

Author Member

I have reworked the test in 326bbb6.

->expects($this->once())
->method('get')
->with('Content-Type')
->willReturn('text/html')
;

$response = new Response('<html><body><form><input name="REQUEST_TOKEN" value="tokenValue"></form></body></html>');
$response->headers = $responseHeaders;

$responseEvent = $this->createMock(FilterResponseEvent::class);
$responseEvent
->method('isMasterRequest')
->willReturn(true)
;

$responseEvent
->method('getRequest')
->willReturn($request)
;

$responseEvent
->method('getResponse')
->willReturn($response)
;

$tokenStorage = new MemoryTokenStorage();
$tokenStorage->initialize(['tokenName' => 'tokenValue']);
$tokenStorage->getToken('tokenName');
Expand Down Expand Up @@ -282,4 +289,51 @@ public function testDoesNotAddTheTokenCookiesToTheResponseUponSubrequests(): voi
$listener = new CsrfTokenCookieListener($tokenStorage);
$listener->onKernelResponse($responseEvent);
}

public function testDoesNotReplaceTheTokenOccurrencesIfNotAHtmlDocument(): void
{
$request = $this->createMock(Request::class);
$request->cookies = new ParameterBag();

$responseHeaders = $this->createMock(ResponseHeaderBag::class);
$responseHeaders
->method('getCookies')
->willReturn([])
;

$responseHeaders
->expects($this->once())
->method('get')
->with('Content-Type')
->willReturn('application/octet-stream')
;

$response = $this->createMock(BinaryFileResponse::class);
$response->headers = $responseHeaders;

$responseEvent = $this->createMock(FilterResponseEvent::class);
$responseEvent
->method('isMasterRequest')
->willReturn(true)
;

$responseEvent
->method('getRequest')
->willReturn($request)
;

$responseEvent
->method('getResponse')
->willReturn($response)
;

$tokenStorage = $this->createMock(MemoryTokenStorage::class);
$tokenStorage
->expects($this->never())
->method('getUsedTokens')
;

$listener = new CsrfTokenCookieListener($tokenStorage);
$listener->onKernelResponse($responseEvent);
}
}

0 comments on commit 7982b80

Please sign in to comment.