PathValidator validates based on a RequestContext leaked from the current request, resulting in false negatives during CLI requests and POST submissions

+++ b/core/lib/Drupal/Core/Path/PathValidator.php
@@ -151,24 +151,30 @@ protected function getPathAttributes($path, Request $request, $access_check) {
+    $router->setContext($request_context);

That sounds like a usecase for finally somehow?

+++ b/core/lib/Drupal/Core/Path/PathValidator.php
@@ -151,24 +151,30 @@ protected function getPathAttributes($path, Request $request, $access_check) {
+      $context = clone \Drupal::service('router.request_context');

Can we inject the request context as constructor parameter?

+++ b/core/lib/Drupal/Core/Path/PathValidator.php
@@ -152,23 +153,30 @@ protected function getPathAttributes($path, Request $request, $access_check) {
+    $request_context = $router->getContext() ? $router->getContext() : new RequestContext();

What about using $old_request_context and $request_context or so, to distinct the two cases a bit better.

WFM

Testbot hiccup.

And again

I ran into this same bug at #2293697-157: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available. You can see a full analysis there. I arrived at the exact same solution! So +1 for this patch.

Hence this is a blocker for a major bug. Updating issue metadata accordingly.

@Wim Leers
This is not fair. Its Drupal which abuses things here.

In #2293697: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available I wrote that I wasn't sure which one was wrong: Drupal or Symfony. Sometimes the line is difficult to draw.

So the WTF is then that Drupal exposes RequestContext as a service, even though it's meant to be a (mutable…) value object that one can create from a given request (and the current request can be retrieved using a service). Drupal exposing this as a service means you need to keep two services in sync: the request stack service (which is really meant to be a service) and the request context "service" (which is actually a value object, and was never meant to be a service).

+++ b/core/lib/Drupal/Core/Path/PathValidator.php
@@ -152,23 +153,30 @@ protected function getPathAttributes($path, Request $request, $access_check) {
 
...
+    $router->setContext($initial_request_context);

I don't understand why we're not doing something like this:

$existing_request_context = $router->getContext();
//... do everything
if ($existing_request_context) {
  $router->setContext($existing_request_context);
}

Otherwise path validation has the unexpected side effect of initialising the context on the router. Is that right?

Fair, this would cause less state changes on average.

Looking at this some more I'm not sure how $router->getContext() will ever not get a context given the service definition...

  router.no_access_checks:
    class: \Drupal\Core\Routing\Router
    arguments: ['@router.route_provider', '@path.current', '@url_generator']
    tags:
      - { name: service_collector, tag: non_lazy_route_enhancer, call: addRouteEnhancer }
      - { name: service_collector, tag: non_lazy_route_filter, call: addRouteFilter }
    calls:
      - [setContext, ['@router.request_context']]

And maybe if for whatever reason the content is not set then re-initializing with an empty request context is better than leaving a request context that has been set from the request passed into the PathValidator.

Great catch! Both @dawehner and I missed that. I actually made a similar mistake in the similar patch I rolled at #2293697-157: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available.

Fixed that. And fixed nits ("Contains" comment, unused "use", non-sentence comment and lack of FQCN).

Absolutely nice catch and the new patch looks great as well.

Hmmm... but the thing is in #30 I point out that I'm not sure that #16 is wrong because the context changing done in #31 might actually put us in a worse position :(.

Damn this one is hard to work out the impacts because of static state.

I think the fact that both #16 and #31 both passed with no test changes shows there is an untested code path. And I agree with @webflo maybe #16 it a better approach.

I've ran into an issue with xmlsitemap.module not using URL aliases when the sitemap.xml files were regenerated during cron jobs executed via the Drush CLI.

And I've managed to pin it down to the \Drupal\Core\Path\PathValidator::getUrlIfValidWithoutAccessCheck() and \Drupal\Core\Path\PathValidator::getPathAttributes() throwing MethodNotAllowedException (the method was actually empty)

Applying this patch against 8.2.6 seems to fix the issue. So +1 for this patch.

(P.S.: I realized that the issue was present when using Drush v8.1.7 but not with v8.1.9, they have probably changed something and probably Drush is now providing a custom context.)

I am wondering whether we should clone the route object before doing any state manipulation. This limits the risk of the additional site effect, while it doesn't remove it completely.

So, back to what #16 did.

The test here is a kernel test. All kernel tests have a request created automatically in \Drupal\KernelTests\KernelTestBase::bootKernel() (it pushes a request on the stack and ensures the router request context service is in sync — see \Drupal\Core\DrupalKernel::prepareLegacyRequest()). We can only properly test what is described in #30+#33+#34 if we create a unit test. Because that would not suffer from this auto-created default request.

So, here's expanded test coverage that shows what happens if you add another test case in which no request has happened yet.

#39 fails with

1) Drupal\KernelTests\Core\Path\PathValidatorTest::testGetUrlIfValidWithoutAccessCheck
SplObjectStorage::offsetExists() expects parameter 1 to be object, null given

/Users/wim.leers/Work/d8/core/lib/Drupal/Core/Render/Renderer.php:594
/Users/wim.leers/Work/d8/core/lib/Drupal/Core/Render/Renderer.php:562
/Users/wim.leers/Work/d8/core/lib/Drupal/Core/Render/MetadataBubblingUrlGenerator.php:84
/Users/wim.leers/Work/d8/core/lib/Drupal/Core/Render/MetadataBubblingUrlGenerator.php:107
/Users/wim.leers/Work/d8/core/lib/Drupal/Core/Url.php:753
/Users/wim.leers/Work/d8/core/tests/Drupal/KernelTests/Core/Path/PathValidatorTest.php:61

FAILURES!
Tests: 1, Assertions: 19, Errors: 1.

because $entity->toUrl()->toString() tries to do automatic bubbling, for which it needs the renderer, which keeps a render context per request… and no request exists yet in this case.

(Yes, it's really turtles cockroaches all the way down.)

The problem described in the IS and explicitly tested in the current patch (#16 and later) is that some CLI scripts set no request method. Notably: Drush. But #38 shows that this was fixed in Drush. The question is then: do we expect all CLI scripts to do this? If the answer is yes, then the original reason for this patch to be applied no longer exists…

And #40 shows once again why pretty much all Drupal code assumes there is a request. Without a request, most things don't work.

So… is it still worthwhile to fix this?

Well, there's still the problem I encountered in #2293697: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available: it's failing to validate the path for a route that only allows the GET method to be used if the validation happens during a non-GET request. So, in this case, the PathValidator is running in a form's submit callback. Forms are submitted via POST requests. So PathValidator is using the global request to check if the path being validated exists. But because it uses a POST request to do so, a MethodNotAllowedException is thrown.

IOW: this issue was created first and foremost for the NULL case in the test coverage. But it's also very much necessary for the POST case.

See #2293697-165: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available, that shows that this patch in fact does fix the fails encountered there. Again, see #2293697-157: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available for the full analysis.

This might seem like a silly question. Why is the path validator checking the method?

#43: it's not. Path validator does this:

$router->match($path);

i.e. it executes full-blown route matching. And it's route matching that checks the method (among many other things), due to \Drupal\Core\Routing\MethodFilter.

If you're wondering whether it then probably doesn't make sense to even use route matching, I agree. What we need, is for this to exist whether any of the routes match the specified path. But apparently there's not really a way to do that.

@Wim Leers well couldn't we return TRUE on MethodNotAllowedException? Since at this point we have validated the path and the method validation is irrelevant?

Also, just in case this isn't complicated enough, if you want to see something reallly fun: #2837833-5: 'Required contexts without a value: node' when editing menu ;)

As I said in #42, #2293697: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available is now passing with this patch applied.

#45: We could, but then where do we draw the line? Why not also return TRUE for AccessDeniedHttpException, because it's possible that another user is allowed to access this URL?

#46: Eh… ok. Excuse me while I go and weep. Looks like #2293697 will land months, not days or weeks :(

@Wim Leers but access is something the PathValidator is specifically designed to test for - it is not designed to test for method applicability hence the bug.

Menu links created from CLI are stored url-based instead route-base in the menu tree storage because of this.

This seems like it's a data integrity bug then, and therefore, potentially Critical. Raising it to such for triage.

Also, setting version to 8.3, because whether Critical or merely Major, I think this will warrant fixing in a patch release of 8.3, even if that's after 8.3.0 is released. It's a non-disruptive fix to a data validation bug.

Why is the path validator checking the method?

To me, answering that question belongs in a separate issue. Seems like whether we want method validation or not depends on the caller. If we're validating a contact form redirect or a menu link, then what we're wanting to know is whether the path is valid for a GET request. However, if we're validating the source path on the path alias form, then potentially we don't want method validation. But regardless of how we want to handle a MethodNotAllowedException, so long as we're calling $router->match($path), we need to give the router a $request_context appropriate for what's being validated, not what just so happens to be in the request that's invoking the validation. So, I think this issue's scope should just be what's in #40, potentially with more test coverage or other refinements to how we manage the request context, and punt any changes to how we handle a MethodNotAllowedException to a followup.

@effulgentsia so how to we not have any side-effects then? The problem is that we're affecting the global static request context that is set on the router. That just feels wrong - it's not changed - and in fact should never change.

To me, answering that question belongs in a separate issue. Seems like whether we want method validation or not depends on the caller. If we're validating a contact form redirect or a menu link, then what we're wanting to know is whether the path is valid for a GET request. However, if we're validating the source path on the path alias form, then potentially we don't want method validation. But regardless of how we want to handle a MethodNotAllowedException, so long as we're calling $router->match($path), we need to give the router a $request_context appropriate for what's being validated, not what just so happens to be in the request that's invoking the validation. So, I think this issue's scope should just be what's in #40, potentially with more test coverage or other refinements to how we manage the request context, and punt any changes to how we handle a MethodNotAllowedException to a followup.

I agree with that. This issue has a clear scope so far, expanding that, just makes it harder to reason about.

I added #2852107: PathValidator::getUrlIfValid() does not support non-HTML/non-GET routes as a follow up to discuss those other bugs.

Thanks @dawehner. Let's do #40 then and hope no one swaps out the router and if they do then they'll need to cope with things like this.

Given on my experience with the routing system I doubt that many people will override the router, and expect a different behaviour.
In an ideal world we would determine the information from the request directly on the longrun.

Committed and pushed 39d1e85 to 8.4.x and 5e41fa5 to 8.3.x. Thanks!

Thanks for opening the followup @dawehner!

Hm, I read #49 but still seems a stretch to me to call this critical. Setting back to major. Tagging for the release notes, though.

Next up: #2852107: PathValidator::getUrlIfValid() does not support non-HTML/non-GET routes.