Problem/Motivation

Contrib modules might want to throw an AccessDeniedException to deny access to certain pages depending on certain conditions. This exception frequently takes place in situations where no route has been established (e.g. in a kernel event subscriber etc.). Right now, this leads to an error message "Call to a member function hasRequirement() on null" due to the following code in core/lib/Drupal/Core/EventSubscriber/CsrfExceptionSubscriber.php, on403 method:

 if (!$route->hasRequirement('_csrf_token') || empty($route->getOption('_csrf_confirm_form_route'))) {
      return;
    }

The solution is as simple as this little change:

 if (is_null($route) || !$route->hasRequirement('_csrf_token') || empty($route->getOption('_csrf_confirm_form_route'))) {
      return;
    }

See also: https://www.drupal.org/project/drupal/issues/3511584
Over at this issue, the "ban" module is affected, but the ban module is about to be moved out of core and this feature request is IMHO absolut valid independent of this module. That's why I'm filing a new issue here to make it independent of the ban module and to specifiy that it's not strictly speaking a bug in core, because there are probably no test cases in core after the ban module is removed.

Steps to reproduce

I'm attaching my test module here again. To reproduce install the module "crash403" attached and call an URL on your Drupal site that ends with "/crash". I'll see if I can create a normal test case.

Proposed resolution

See above. Just add "is_null($route) ||"

Remaining tasks

User interface changes

None

Introduced terminology

None

API changes

None

Data model changes

None

Release notes snippet

Don't know if necessary. If at all perhaps the fact that you can now throw AccessDeniedException everywhere. But that's IMHO the expectation anyway.

Issue fork drupal-3566351

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

rgpublic created an issue. See original summary.

quietone’s picture

Version: 11.3.x-dev » 11.x-dev

In Drupal core changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies. Thanks.

sourav_paul’s picture

checking...

sourav_paul’s picture

StatusFileSize
new81.41 KB

@rgpublic I tested the proposed change in CsrfExceptionSubscriber and it results in an infinite redirect / exception loop, eventually causing a fatal “allowed memory size exhausted” error.

Flow observed:

  • A CSRF 403 triggers on403()
  • The subscriber redirects to _csrf_confirm_form_route
  • That request triggers another 403 (same route or another CSRF-protected route)
  • on403() runs again and redirects repeatedly

This is not a memory leak but an unbounded 403 → redirect → 403 loop during exception handling.

The implementation needs a guard (e.g. prevent redirecting to the same route, avoid redirecting to routes with _csrf_token, or mark the request as already handled). Without this, redirecting from an exception subscriber is unsafe.

Got this issue after applying changes:
img

rgpublic’s picture

StatusFileSize
new168 bytes

Thank you @sourav_paul for looking into this issue. Really appreciate it. You are right of course. In certain circumstances this can lead to an endless loop. I'd see that in the responsibility of the triggering module though. I've added a:

if (!$event->isMainRequest()) return;

To prevent that the subrequest that fetches the 403 retriggers the original condition. I've added the corrected module.

Now, how do we go from here? I guess we'd need a test, right? I've no experience with that but I might try my luck if you are saying that's what we need next.

sourav_paul’s picture

Hi @rgpublic your attached zip file is empty, can you provide the valid module?

rgpublic’s picture

StatusFileSize
new1.37 KB

@sourav_paul: D'oh. My bad. I've attached the valid module now. Sorry for the completely unnecessary roundtrip. My fault :-(

sourav_paul’s picture

Thanks @rgpublic, I've tested the fix to get AccessDeniedHttpException on a non-existing route, triggered my logic onKernelRequest event, it fixes the WSOD issue, hence moving it to NR.

Attaching test ss with it:

Before:
img

After:
img

Thanks...

sourav_paul’s picture

Status: Active » Needs review
smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Will need test coverage

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

sourav_paul’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work
sourav_paul’s picture

Status: Needs work » Needs review
smustgrave’s picture

Will leave for others then no one picks up after a few months I’ll relook. Thanks

smustgrave’s picture

Status: Needs review » Needs work
sourav_paul’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests +Needs Review Queue Initiative

Feedback appears to be addressed on this one.

alexpott’s picture

Title: It should be possible to throw AccessDeniedException without a route » Throwing AccessDeniedException without a route causes PHP errors
Category: Feature request » Bug report

  • alexpott committed 841cd847 on 11.3.x
    fix: #3566351 Throwing AccessDeniedException without a route causes PHP...

  • alexpott committed d40e456d on 11.x
    fix: #3566351 Throwing AccessDeniedException without a route causes PHP...

  • alexpott committed 051f4387 on main
    fix: #3566351 Throwing AccessDeniedException without a route causes PHP...
alexpott’s picture

Version: main » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 051f4387948 to main and d40e456dd0f to 11.x and 841cd847977 to 11.3.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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

scott_euser’s picture

Adding related issue that this resolved, thanks!