Fix exception message for MachineNameController replace_token query parameter [#2829791]

Problem/Motivation

In the security release for Drupal Core - Moderately Critical - Multiple Vulnerabilities - SA-CORE-2016-005 https://www.drupal.org/SA-CORE-2016-005
commit http://cgit.drupalcode.org/drupal/commit/?id=c5da97f
http://cgit.drupalcode.org/drupal/diff/core/modules/system/src/MachineNa...

in \Drupal\system\MachineNameController
the exception is attempted to be thrown like

    if (isset($replace_pattern) && isset($replace)) {
      if (!isset($replace_token)) {
        throw new AccessDeniedException("Missing 'replace_token' query parameter.");
      }
      elseif (!$this->tokenGenerator->validate($replace_token, $replace_pattern)) {
        throw new AccessDeniedException("Invalid 'replace_token' query parameter.");
      }

The actual exception message then ends up reading like:
'The file Missing 'replace_token' query parameter. could not be accessed'

Background

(for security team members with access, see https://security.drupal.org/node/160109#comment-132057 please dont request access if you dont have it, this is just to preserve background history)

Steps to reproduce

1. install

2. drush cset system.logging error_level verbose -y
(to turn on error reporting with backtrace)

3. And then go to a page like:
http://localhost:8888/drupal/machine_name/transliterate?text=00000000000...((0)(0)?)*+&replace=_&lowercase=true

Proposed resolution

Use (correct) Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException instead of accidental Symfony\Component\HttpFoundation\File\Exception\AccessDeniedException

Remaining tasks

(done) Investigate
(done) Give command to reproduce that will show what the message actually is.

User interface changes

API changes

Data model changes

Comment	File	Size	Author
#5	2829791.5.patch	3.37 KB	YesCT
#5	8.3.x: PHP 5.5 & MySQL 5.5 21,103 pass
#5	2829791.5.testsonly.patch	1.96 KB	YesCT
#5	8.3.x: PHP 5.5 & MySQL 5.5 21,091 pass, 1 fail
#5	2829791.5.notestchange.patch	1.41 KB	YesCT
#5	8.3.x: PHP 5.5 & MySQL 5.5 21,091 pass, 1 fail
#2	2829791.2.notapatch.patch	1006 bytes	YesCT
#2	8.3.x: PHP 5.5 & MySQL 5.5 21,091 pass, 1 fail

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

22 November 2016 at 19:40

YesCT created an issue. See original summary.

Comment #2

YesCT CreditAttribution: YesCT commented 22 November 2016 at 20:31

File	Size
2829791.2.notapatch.patch	1006 bytes
8.3.x: PHP 5.5 & MySQL 5.5 21,091 pass, 1 fail

to see the exception message

apply this patch (which adds an X to the message) and then run phpunit and it will show the fail.

$ cd core
$ ../vendor/bin/phpunit modules/system/tests/src/Unit/Transliteration/MachineNameControllerTest.php --verbose
PHPUnit 4.8.27 by Sebastian Bergmann and contributors.

Runtime:	PHP 5.6.7
Configuration:	/Users/ctheys/foo/drupal2/core/phpunit.xml.dist

..........F.

Time: 366 ms, Memory: 7.50Mb

There was 1 failure:

1) Drupal\Tests\system\Unit\Transliteration\MachineNameControllerTest::testMachineNameControllerWithInvalidReplacePattern
Failed asserting that exception message 'The file Invalid 'replace_token' query parameter. could not be accessed' contains 'Invalid X 'replace_token' query parameter.'.

FAILURES!
Tests: 12, Assertions: 14, Failures: 1.

note the exception is:
'The file Invalid 'replace_token' query parameter. could not be accessed'
which does't totally make sense.

Comment #3

YesCT CreditAttribution: YesCT commented 22 November 2016 at 22:24

Issue summary:

View changes

Without a patch, can do:

1. install

2. drush cset system.logging error_level verbose -y
(to turn on error reporting with backtrace)

3. And then go to a page like:
http://localhost:8888/drupal/machine_name/transliterate?text=00000000000...((0)(0)?)*+&replace=_&lowercase=true

then see:

The website encountered an unexpected error. Please try again later.

Symfony\Component\HttpFoundation\File\Exception\AccessDeniedException: The file Missing 'replace_token' query parameter. could not be accessed in Drupal\system\MachineNameController->transliterate() (line 81 of core/modules/system/src/MachineNameController.php).
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 574)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
call_user_func_array(Object, Array) (Line: 139)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 62)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 50)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 656)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

Note the error is
Symfony\Component\HttpFoundation\File\Exception\AccessDeniedException: The file Missing 'replace_token' query parameter. could not be accessed in Drupal\system\MachineNameController->transliterate() (line 81 of core/modules/system/src/MachineNameController.php).

Talking with @alexpott in irc, they note that:

it should be Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException

Comment #4

YesCT CreditAttribution: YesCT commented 23 November 2016 at 14:06

I'm gonna take a look at fixing this.

Comment #5

YesCT CreditAttribution: YesCT commented 24 November 2016 at 15:56

Title:	Improve exception message for MachineNameController replace_token query parameter	» Fix exception message for MachineNameController replace_token query parameter
Issue summary:	View changes
Status:	Active	» Needs review

File	Size
2829791.5.notestchange.patch	1.41 KB
8.3.x: PHP 5.5 & MySQL 5.5 21,091 pass, 1 fail
2829791.5.testsonly.patch	1.96 KB
8.3.x: PHP 5.5 & MySQL 5.5 21,091 pass, 1 fail
2829791.5.patch	3.37 KB
8.3.x: PHP 5.5 & MySQL 5.5 21,103 pass

with change to exception,
before fixing test,

$ ../vendor/bin/phpunit modules/system/tests/src/Unit/Transliteration/MachineNameControllerTest.php
PHPUnit 4.8.27 by Sebastian Bergmann and contributors.

..........FF

Time: 837 ms, Memory: 7.75Mb

There were 2 failures:

fails were:

1) Drupal\Tests\system\Unit\Transliteration\MachineNameControllerTest::testMachineNameControllerWithInvalidReplacePattern
Failed asserting that exception of type "Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException" matches expected exception "Symfony\Component\HttpFoundation\File\Exception\AccessDeniedException". Message was: "Invalid 'replace_token' query parameter." at
#0 /Users/ctheys/foo/drupal2/core/modules/system/tests/src/Unit/Transliteration/MachineNameControllerTest.php(107): Drupal\system\MachineNameController->transliterate(Object(Symfony\Component\HttpFoundation\Request))
...

and

2) Drupal\Tests\system\Unit\Transliteration\MachineNameControllerTest::testMachineNameControllerWithMissingToken
Failed asserting that exception of type "Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException" matches expected exception "Symfony\Component\HttpFoundation\File\Exception\AccessDeniedException". Message was: "Missing 'replace_token' query parameter." at
#0 /Users/ctheys/foo/drupal2/core/modules/system/tests/src/Unit/Transliteration/MachineNameControllerTest.php(117): Drupal\system\MachineNameController->transliterate(Object(Symfony\Component\HttpFoundation\Request))
...

exception message making much more sense now. :)

CreditAttribution: larowlan as a volunteer and at PreviousNext commented 24 November 2016 at 22:20

Status:

Needs review

» Reviewed & tested by the community

Thanks

+Quick fix

Note those fails are from tests only patches and a sample non-patch. The patch is passing fine.

Comment #12

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Chapter Three commented 26 November 2016 at 21:08

Status:

Reviewed & tested by the community

» Fixed

This was just the wrong exception - we should have caught it when we reviewed the security patch - well @YesCT did catch. Nice one :)

Committed and pushed f08a8c8 to 8.3.x and 4d6d7b2 to 8.2.x. Thanks!

Comment #13

26 November 2016 at 21:08

alexpott committed f08a8c8 on 8.3.x

Issue #2829791 by YesCT: Fix exception message for MachineNameController...

Comment #14

26 November 2016 at 21:08

alexpott committed 4d6d7b2 on 8.2.x

Issue #2829791 by YesCT: Fix exception message for MachineNameController...

Comment #15

10 December 2016 at 21:14

Status:

Fixed

» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Fix exception message for MachineNameController replace_token query parameter

Problem/Motivation

Background

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Comments

Comment #1

Comment #2

Comment #3

Comment #4

Comment #5

Comment #6

Comment #7

Comment #8

Comment #9

Comment #10

Comment #11

Comment #12

Comment #13

Comment #14

Comment #15

Thank you to these Drupal contributors

News items

Our community

Documentation

Drupal code base

Governance of community