When you visit a route that is protected with basic authentication and you do not provide credentials, then the resulting 401 page shows a stacktrace rather than a human friendly error message.

Steps to replicate:

  1. Create a route that is protected with basic authentication. For example put this in user.routing.yml: 
    user.basic_auth:
  path: '/user/basic-auth'
  defaults:
    _form: '\Drupal\user\Form\UserLoginForm'
  options:
    _auth: [ 'basic_auth' ]
  2. Enable the basic_auth module.
  3. Visit the path user/basic-auth without an active session (easiest is to open an anonymous browser window to test)
  4. You will be asked to authenticate. Close the dialog without authenticating.

Result: the following backtrace is shown:

Symfony\Component\HttpKernel\Exception\UnauthorizedHttpException: No authentication credentials provided. in Drupal\basic_auth\Authentication\Provider\BasicAuth->challengeException() (line 138 of core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php).
Drupal\Core\Routing\AccessAwareRouter->matchRequest(Object)
Symfony\Component\HttpKernel\EventListener\RouterListener->onKernelRequest(Object, 'kernel.request', Object)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.request', Object)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\PageCache->fetch(Object, 1, 1)
Drupal\Core\StackMiddleware\PageCache->lookup(Object, 1, 1)
Drupal\Core\StackMiddleware\PageCache->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1)
Stack\StackedHttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)

Expected result: A human readable message such as:

Unauthorized - Please log in to access this page.

CommentFileSizeAuthor
#20 interdiff.txt1.49 KBDom.
#20 comprehensive-401-error--fix--2472371-19.patch5.45 KBDom.
#18 interdiff.txt828 bytespfrenssen
#18 2472371-18.patch4.71 KBpfrenssen
#12 comprehensive-401-error--fix--2472371-12.patch4.24 KBDom.
#11 screenshot.png8.72 KBDom.
#8 comprehensive-401-error--fix--2472371-8.patch4.13 KBDom.
#6 comprehensive-error--test-fails-2472371-6.patch1.6 KBDom.
#4 comprehensive-error--test-fails-2472371-4.patch1.6 KBDom.
#4 interdiff.txt992 bytesDom.
#1 comprehensive-error--test-fails-2472371-1.patch1.6 KBDom.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dom.’s picture

Dom. CreditAttribution: Dom. commented
Status: Active » Needs review
FileSize
comprehensive-error--test-fails-2472371-1.patch1.6 KB

Add test fail patch.

Status: Needs review » Needs work

The last submitted patch, 1: comprehensive-error--test-fails-2472371-1.patch, failed testing.

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented 
+++ b/core/modules/basic_auth/src/Tests/Authentication/BasicAuthTest.php
@@ -157,6 +157,30 @@ function testLocale() {
+    $this->drupalGet($url);
+    $this->assertResponse('401', 'The user is blocked when no credentials are passed.');
+    $this->assertText('Access denied', "A user friendly access denied message is displayed");

401 is not "access denied" but "unauthorized". Access denied is 403. The difference is between "We know who you are and you are not allowed here", and "This is a restricted area, please identify".

Dom.’s picture

Dom. CreditAttribution: Dom. commented
Status: Needs work » Needs review
FileSize
interdiff.txt992 bytes
comprehensive-error--test-fails-2472371-4.patch1.6 KB

Changed the failing-test patch accordingly to #3

Dom.’s picture

Dom. CreditAttribution: Dom. commented
Status: Needs review » Needs work

This test should fail: it is made for that. Here it does not (in #4) because the stacktrace contains the "Unauthorized" field :
Symfony\Component\HttpKernel\Exception\UnauthorizedHttpException: No authentication credentials provided. in Drupal\basic_auth\Authentication\Provider\BasicAuth->challengeException() (line 138 of core\modules\basic_auth\src\Authentication\Provider\BasicAuth.php).
So it take rework to actually figure out it the display is a stracktrace or a user friendly message.
Anyone have a clue on that ?

Dom.’s picture

Dom. CreditAttribution: Dom. commented
Status: Needs work » Needs review
FileSize
comprehensive-error--test-fails-2472371-6.patch1.6 KB

Try another way : check no "Exception" text is displayed.

Interdiff :
$this->assertText('Unauthorized', "A user friendly access unauthorized message is displayed");
changed to
$this->assertNoText('Exception', "A user friendly access unauthorized message is displayed");

Status: Needs review » Needs work

The last submitted patch, 6: comprehensive-error--test-fails-2472371-6.patch, failed testing.

Dom.’s picture

Dom. CreditAttribution: Dom. commented
FileSize
comprehensive-401-error--fix--2472371-8.patch4.13 KB

Adding patch fix to be reviewed.

Dom.’s picture

Dom. CreditAttribution: Dom. commented
Status: Needs work » Needs review
znerol’s picture

znerol CreditAttribution: znerol commented

@Dom.: The reason why the stacktrace is displayed is because we explicitly enable verbose logging in tests (see WebTestBase::setUp() line 918). There is a test in ErrorHandlerTest.php which makes sure that no backtrace is displayed if the error_level is not verbose. Also note that error_level defaults to hide (see core/modules/system/config/install/system.logging.yml).

Therefore I do not think that there is anything to do here in this issue.

Dom.’s picture

Dom. CreditAttribution: Dom. commented
Issue summary: View changes
FileSize
screenshot.png8.72 KB

Still with standard profile, and error page is sent instead of a nice 401 page. Plus the exception is in the logfile.

Dom.’s picture

Dom. CreditAttribution: Dom. commented
FileSize
comprehensive-401-error--fix--2472371-12.patch4.24 KB

I forgot a wording was actually available in the issue summary.
- Simply changed the 401 page wording regarding this new.
- Also added a test for this sentence thus in BasicAuthTest.php

Note: I still think it would be better to ask for credentials instead of login since it more in conjunction with the reason why 401 happens regarding authentication (basic, oauth or whatever).

Interdiff:
'#markup' => $this->t('Please provide valid credentials.'),
changed to :
'#markup' => $this->t('Please log in to access this page.'),

$this->assertNoText('Exception', "A user friendly access unauthorized message is displayed");
changed to :
$this->assertNoText('Exception', "No raw exception is displayed on the page.");
and added :
$this->assertText('Please log in to access this page.', "A user friendly access unauthorized message is displayed.");

znerol’s picture

znerol CreditAttribution: znerol commented

Oh, given the screenshot in #11 I agree that we should do something about it.

The last submitted patch, 8: comprehensive-401-error--fix--2472371-8.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 12: comprehensive-401-error--fix--2472371-12.patch, failed testing.

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented
Assigned: Unassigned » pfrenssen

Assigning for review. I am also looking into the test failure. On a 401 page we have to output a "WWW-Authenticate" HTTP header that informs the client about the authentication type and realm. With the patch applied this HTTP header is no longer output.

znerol’s picture

znerol CreditAttribution: znerol commented

possibly related #2432657: BasicAuth challenge never sent to browser

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented
Status: Needs work » Needs review
FileSize
2472371-18.patch4.71 KB
interdiff.txt828 bytes

This makes sure that any special HTTP headers that are output on an HTTP exception page is also output when a human friendly message is shown.

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented
Assigned: pfrenssen » Unassigned

Leaving this in the capable hands of Dom.

Dom.’s picture

Dom. CreditAttribution: Dom. commented
FileSize
comprehensive-401-error--fix--2472371-19.patch5.45 KB
interdiff.txt1.49 KB

@pfrenssen, thanks so much for help !
Adding new patch with interdiff for review. The purpose is just to reduce the scope of added headers to HttpExceptions only.

Dom.’s picture

Dom. CreditAttribution: Dom. commented

Any updates on the #20 ? Is this okay ?

znerol’s picture

znerol CreditAttribution: znerol commented
Issue tags: +WSCCI

I'm okay with this. Tagging it in order to give people behind #2323759: Modularize kernel exception handling a chance to review.

Crell’s picture

Crell CreditAttribution: Crell at Palantir.net commented
Component: user system » request processing system
Status: Needs review » Reviewed & tested by the community

This is consistent with how the exception system is supposed to work, and I like the addition of ensuring that HTTP headers get transferred. That's a useful bug fix on its own. Thanks all!

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +drupaldevdays

  • xjm committed a33414e on 8.0.x 
    Issue #2472371 by Dom., pfrenssen: Exception shown on 401 Unauthorized
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes.

Thanks for the clear, specific steps to reproduce the issue. I was able to confirm both the bug and fix using them. I also like that the fix is clean and consistent with our existing 4xx handling.

  1. +++ b/core/modules/basic_auth/src/Tests/Authentication/BasicAuthTest.php
@@ -156,6 +156,31 @@ function testLocale() {
+   * Tests if a comprehensive message is displayed rather then a raw exception
+   * when route is denied.

    Minor: This summary is supposed to be only one line of 80 characters or fewer. See https://www.drupal.org/node/1354#drupal. I fixed this on commit:

    diff --git a/core/modules/basic_auth/src/Tests/Authentication/BasicAuthTest.php b/core/modules/basic_auth/src/Tests/Authentication/BasicAuthTest.php
index 81484c1..6a3d348 100644
--- a/core/modules/basic_auth/src/Tests/Authentication/BasicAuthTest.php
+++ b/core/modules/basic_auth/src/Tests/Authentication/BasicAuthTest.php
@@ -154,8 +154,7 @@ function testLocale() {
   }
 
   /**
-   * Tests if a comprehensive message is displayed rather then a raw exception
-   * when route is denied.
+   * Tests if a comprehensive message is displayed when the route is denied.
    */
   function testUnauthorizedErrorMessage() {
     $account = $this->drupalCreateUser();

      

  2. +++ b/core/modules/basic_auth/src/Tests/Authentication/BasicAuthTest.php
@@ -156,6 +156,31 @@ function testLocale() {
+    $this->assertNoText('Exception', "No raw exception is displayed on the page.");

    This assertion will not test what is expected if errors aren't being reported to the screen (and, indeed, it failed to fail in https://qa.drupal.org/pifr/test/1027378). However, the other assertions provide the coverage needed. Under normal circumstances I'd suggest asserting the response was not a 500 instead of checking for the text but that obviously is irrelevant when we're asserting it's a 401. :)
      

  3. +++ b/core/modules/system/src/Controller/Http4xxController.php
@@ -15,10 +15,22 @@
+   * The default 401 content.

    Minor: This is technically supposed to start with a verb (see https://www.drupal.org/node/1354#functions), but since it's exactly the same as the other similar methods on the class, I'll take it.
     

I didn't consider any of the above worth delaying this patch on, so committed and pushed to 8.0.x. Thanks!

znerol’s picture

znerol CreditAttribution: znerol commented

Re #26.2: In fact the error_level is set to verbose in WebTestBase::setUp(), so the stack trace and the term Exception is displayed without the patch. Note that the wording of the assert-message was subsequently changed in #12, that's why the message in the test result of the test-only patch in #6 is different from the one committed.

Status: Fixed » Closed (fixed)

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