AccessAwareRouter does not respect HTTP method

This reads totally reasonable. Do you think its possible to write a test for that?

Fair, but some test coverage would be nice nevertheless.

I am currently experiencing this issue on a project I am working on where we have a route similar to this:

mymodule_api.fetch_something:
  path: '/mymodule/something'
  defaults:
    _controller: '\Drupal\mymodule_api\Controller\MyModuleApiController::fetchSomething'
    _title: 'title'
  requirements:
    _access: 'TRUE'
    _method: 'POST'

When a logged in user tries to access it the Language negotiation fails because of the route matching creates an incomplete request object (it basically thinks a POST request can be matched like a GET request).

It do not seem farfetched to have a route where only POST is a valid method and not allowing for proper route matching due to the request method seems to be an wrong way to "solve" the issue.

I would aim for a solution that takes the current request into account when doing the route matching instead of mocking a new request.

I tried with the following implementation which takes the request method into consideration, haven't verified if it passes any of the internal tests, but seems to solve the problem at hand.

diff --git a/core/modules/user/src/Plugin/LanguageNegotiation/LanguageNegotiationUserAdmin.php b/core/modules/user/src/Plugin/LanguageNegotiation/LanguageNegotiationUserAdmin.php
index c00509f..a3a5c10 100644
--- a/core/modules/user/src/Plugin/LanguageNegotiation/LanguageNegotiationUserAdmin.php
+++ b/core/modules/user/src/Plugin/LanguageNegotiation/LanguageNegotiationUserAdmin.php
@@ -11,6 +11,7 @@
 use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
+use Symfony\Component\Routing\Exception\MethodNotAllowedException;
 use Symfony\Component\Routing\Exception\ResourceNotFoundException;
 use Symfony\Component\Routing\Matcher\UrlMatcherInterface;

@@ -131,7 +132,12 @@ protected function isAdminPath(Request $request) {
           // prefixes and other path components that inbound processing would
           // clear out, so we can attempt to load the route clearly.
           $path = $this->pathProcessorManager->processInbound(urldecode(rtrim($request->getPathInfo(), '/')), $request);
-          $attributes = $this->router->match($path);
+          if (method_exists($this->router, 'matchRequest')) {
+            $attributes = $this->router->matchRequest(Request::create($path, $request->getMethod()));
+          }
+          else {
+            $attributes = $this->router->match($path);
+          }
         }
         catch (ResourceNotFoundException $e) {
           return FALSE;

Second though, here is a patch fil with a minor update checking if we are dealing with an instance of RequestMatcherInterface as the router.

I cannot reproduce this issue using the steps in the issue summary.

Including with page_manager-alpha14

Is this still a problem?

@dpi, this is reproducible without page_manager module - so yes this is still an issue.

I checked in drupal 8.3 and couldn't reproduce this issue.
Steps i followed:
1. Enabled Rest, language, page_manager
2. Created languages
3. changed settings in /admin/config/regional/language/detection and checked Account administration pages
4. and in user edit page i have set language in "Administration pages language".
5. went to node/view pages and i found no error.

#16 the steps above might not reproduce the issue, but I could not at a quick glance at the 8.3.x codebase see changes that would prevent this from happening in the previous mentioned case I ran into.

Will take a look at this over the weekend and see where we are in regards to this functioning as needed.

This was reported again at #2878785: MethodNotAllowedException using POST or PATCH with REST on 8.3.2, which was marked as a duplicate.

This is very similar to #2822190: PathValidator validates based on a RequestContext leaked from the current request, resulting in false negatives during CLI requests and POST submissions, which fixed \Drupal\Core\Path\PathValidator::getPathAttributes(). That method is very similar to \Drupal\user\Plugin\LanguageNegotiation\LanguageNegotiationUserAdmin::isAdminPath().

Actually, I think this belongs in routing system, since that's where the root cause lies.

I'm still encountering issues when trying to create files over REST using the file_entity module as shown here:
https://www.drupal.org/node/2682341#comment-11965256

The issue in this case seems to be that some of the original headers are not getting passed along.
It seems that we might need to pass in an exact clone of the $request object, that only has the URI modified.

I'm not seeing a great way of doing that though.
As a work around for my current needs, I'm just adding an additional check so that isAdminPath() only gets checked on GET requests.

I'll add a patch in case that is helpful for anyone else.

It seems like this issue more succinctly summarizes the problem at hand: #2857132: "Account administration pages" language negotiation ignores _format blocking REST resources. It shouldn't be caught rather than it should respect the _format parameter.

Retitling based on #22 and the work done in PathValidator. When \Drupal\Core\Routing\AccessAwareRouter::match recreates the request to match, it does not pass the proper HTTP method from the original request.

This causes a POST request to be matched as a GET request, causing LanguageNegotiationUserAdmin to fail at MethodNotAllowed exception for valid routes.

I did some digging in https://www.drupal.org/project/drupal/issues/2857132#comment-12865440, where I thought it would be an appropriate issue. But it's more than just respecting _format. The problem is that AccessAwareRouter creates an invalid request to match against known routes for the path.

Incoming patch which updates the AccessAwareRouter to use its route context to pass the proper HTTP method when creating a request object to match on.

  /**
   * {@inheritdoc}
   *
   * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
   *   Thrown when access checking failed.
   */
  public function match($pathinfo) {
    $request_context = $this->getContext() ? $this->getContext() : new RequestContext();
    return $this->matchRequest(Request::create($pathinfo, $request_context->getMethod()));
  }

Here is the patch which updates \Drupal\Core\Routing\AccessAwareRouter::match to provide a correct request object.

+++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
@@ -140,7 +140,8 @@ public function generate($name, $parameters = [], $referenceType = self::ABSOLUT
+    $request_context = $this->getContext() ? $this->getContext() : new RequestContext();
+    return $this->matchRequest(Request::create($pathinfo, $request_context->getMethod()));

We're past the initial issue, however this is still insufficient. We need to ensure the _format parameter is passed so that MediaTypeMismatch is not thrown.

This fixes POST/PATCH/DELETE requests for our rest.module plugins.

This needs tests. This scenario seems to only happen when routes share similar paths and the missing HTTP method / _format cause the routing system to become confused.

The bug is exposed by \Drupal\user\Plugin\LanguageNegotiation\LanguageNegotiationUserAdmin. But that's not what needs to be tested.

It may be feasible to test in \Drupal\Tests\Core\Routing\AccessAwareRouterTest with \Drupal\Tests\Core\Routing\RoutingFixtures.

We did some more testing with this. As soon as the admin language is configured things break, again.

The content type header filter causes the route to become ineligible: \Drupal\Core\Routing\ContentTypeHeaderMatcher::filter.

+++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
@@ -140,7 +140,8 @@ public function generate($name, $parameters = [], $referenceType = self::ABSOLUT
+    $request_context = $this->getContext() ?: new RequestContext();
+    return $this->matchRequest(Request::create($pathinfo, $request_context->getMethod(), $request_context->getParameters()));

The request context parameters is an empty array, and the _format is not passed as the query string. Also, the content type is not set.

In our case JSON payloads were being rejected on a media mismatch due to this logic:

        switch (strtoupper($method)) {
            case 'POST':
            case 'PUT':
            case 'DELETE':
                if (!isset($server['CONTENT_TYPE'])) {
                    $server['CONTENT_TYPE'] = 'application/x-www-form-urlencoded';
                }

I'm wondering if the problem is \Drupal\Core\Routing\AccessAwareRouter::match or the fact \Drupal\user\Plugin\LanguageNegotiation\LanguageNegotiationUserAdmin::isAdminPath invokes this method over matchRequest. Without a preferred administration language configured the matchRequest is invoked.

The other problem is that core cannot decide if $this->route should be type hinted against UrlMatcherInterface or RequestMatcherInterface. It choses either or based on the required methods. AccessAwareRouterInterface implements both and is also the default interface for Drupal's default router service.

I'm giving up on elegance here. I'm falling back to what was mentioned in #20: it works around the MethodNotAllowedException thrown by \Drupal\Core\Routing\AccessAwareRouter::match. If we fix the method, we end up with a MediaMismatch exception. This fixes the problem in the negotiator and punts fixing the access router (which feels like a rabbit hole) and allows REST plugins to work as expected.

This is essentially a reroll of #2 after a lot of failed attempts to fix the router.

The patch in #34 seems to resolve the issue. RTBC +1

I'm giving up on elegance here. I'm falling back to what was mentioned in #20

😭

I think this absolutely needs to be reviewed/RTBC'd by the routing system maintainer.

😭

The problem is that, inherently, match by path info alone cannot work for POST, PUT, PATCH. No matter how much request context we pass the _format is lost and so is the CONTENT_TYPE header. That means any matching of a path on those methods will instantly fail. Fixing the MethodNotAllowedException exception uncovered this -- trading off for constantly having a mismatched media exception.

The match method is fine for GET, HEAD, and DELETE when there is no request body and content type matching requirement.

Can we get tests to codify the steps to reproduce from the IS?
Step 3 involves contrib, so we'd need a test module to simulate that portion of the flow.

I had to combine the fix from the most recent patch with the workaround in #23 to get our API calls to succeed.

@tim.plunkett - sorry, what does "IS" mean in #38?

Just for the record,
Due to this issue, ajax flag links in Flag module doesn't work (together with enabled "Administration pages language" of course).

They're expecting request method to be "POST" but it never happens in AccessAwareRouter::match() method.

Just found a way to trigger this issue on Drupal 8.8.5 with the Media module:

1. Have the Media module and CKEditor enabled
2. Go to the text format configuration
3. Enable "Embed media"
4. Make sure a non-admin user has the permission to use the text format and to embed Media items
5. Log in as a non-admin user
6. Edit a node where there is a rich text field
7. Use the "Embed media" button to embed a media item from the library. It works.
8. Click on the "Edit Media" button on top of the media item in the rich text area, as if you want to add a caption or change the view mode
9. It silently fails with an HTTP 500 error, the AJAX POST request failed because of MethodNotAllowedException

It turns out opening the Media editor dialog box calls the route '/editor/dialog/media/{editor}' which is restricted to the POST method. When it goes through the AccessAwareRouter, the HTTP verb context is lost and a MethodNotAllowed exception is triggered. Using the patch at #34 fixed the issue.

Note: tested on a multilingual site, I don't know if it has an impact.

#3048288: MethodNotAllowedException thrown on POST requests if user has set admin language in their profile uses the same patch and is also core only.

A test to prove the issue. Should fail.

The fix is basically the same as in #2.

Maybe unit test here should be enough?

Haven't tried the patches yet, But confirming that the steps suggested in #42 are valid and resulting in PHP-500 error on Drupal-8.9.2 as well.

I can confirm that the findings in #42 can be reproduced and that the patch in #34 works with 8.9.3. The dialog box finally appears and the 500 error is gone. Thank you!

Patch in #34 works for me. Thanks!

Had a problem with the contrib module Flag described in this ticket: https://www.drupal.org/project/flag/issues/3110134

Using Drupal 9.0.3, Multilanguage enabled, default language set to german and admin language set to english.

Rerolled.

#46 worked great and fixed the issue for Commerce Cart Flyout's update quantity patch: #3144651: Add quantity to form. Thank you!

This is #52 ported to 9.2.x, and also applies to 9.1.x.

We are experiencing this problem when editing media on a multilingual site. This is identical to the report @mdupont described in #42.

I have this problem on a Drupal 9.2 site. I have confirmed that the latest patch in #54 works to solve the immediate problem.

That said, I don't think the patch takes the right approach. When opening a media dialog, the route is /editor/dialog/media/{editor} and should be considered an administrative path, since it was called from an administrative path (/node/add/page). By catching the MethodNotAllowedException exception and returning FALSE, you could end up with the media dialog displaying in the front-end language rather than the admin language.

I think the right solution here would be to preserve the current page request method (POST) when validating the route, rather than suppressing the exception and returning a potentially incorrect value.

#54 fixes the issue for me. Thanks!!
@quicksketch: I can confirm the issue regarding media dialog showing up in front-end rather than back-end language.

On the other hand, very similar problems have been solved by catching MethodNotAllowedException and returning FALSE as well:

#2619700: \Drupal\Core\Path\PathValidator::getPathAttributes() does not catch MethodNotAllowedException

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

...

Shouldn't we take the same direction in here and than check in a separate issue for all places where catching MethodNotAllowedException and returning FALSE is causing trouble?

For the moment, media dialog showing up in the wrong language still is better than it not loading at all.

#52 Worked.

I experienced this issue with Acquia Site Studio/Cohesion when importing UI Kit YML file in a multilingual site studio powered site

Thanks
Rakesh

I found this issue via #2857132: "Account administration pages" language negotiation ignores _format blocking REST resources.

If we are going with the solution to catch different exceptions, we might as well add \Symfony\Component\HttpKernel\Exception\NotAcceptableHttpException, which would solve issue 2857132 as well.

Attached a patch based on #54, with the addition. (But without tests for now, since they didn't apply cleanly from #54 on 9.3.x)

Hi, patch #54 works on D 9.3. Thank you.

Back to needs work since Patch #54 needs a reroll for Drupal 9.4.

Rerolled the patch in #54 and updated the deprecated code ($this->drupalPostForm('/user-language-test/form', [], 'Send');) in core/modules/user/tests/src/Functional/UserAdminLanguageTest.php file, thanks!

If you are using Site Studio with Content/Interface translation, you absolutely need the patch in #59 (Drupal 9.3.3) or the package export interface will be broken.

Can we also re-add the exception added in #59?

I re-added the exception that was added in #59.

With PATCH request to REST endpoint on my project with core 9.4.2 I got error with Symfony\Component\Routing\Exception\MethodNotAllowedException: in Symfony\Component\Routing\Matcher\UrlMatcher->match() for route with PATCH method. I applied the patch from #67 and the problem is gone. Nice to have this fix in core.

If we include #67 then it would be great to add some test coverage of that case?

Another option...

+++ b/core/modules/user/src/Plugin/LanguageNegotiation/LanguageNegotiationUserAdmin.php
@@ -137,10 +139,7 @@ protected function isAdminPath(Request $request) {
-        catch (ResourceNotFoundException $e) {
-          return FALSE;
-        }
-        catch (AccessDeniedHttpException $e) {
+        catch (ResourceNotFoundException | AccessDeniedHttpException | MethodNotAllowedException | NotAcceptableHttpException $exception) {

How about catching \Symfony\Component\Routing\Exception\ExceptionInterface and \Symfony\Component\HttpKernel\Exception\HttpException exceptions... like if routing fails for whatever reason here I think we should return FALSE. I don't think any of the Http or Routing exceptions here should result in an exception reaching the user.

Patch #67 worked for me. Agree some test coverage for it would be good.

Tests exists in the patch #46, but weren't added to re-rolled patches.

One minor change here is that we can remove $exception variable for D10 patch as we don't need to support PHP 7.

Re-rolled #67 patch to work with Drupal 9.4.12

This also effects Webprofiler #64 where the webprofiler toolbar reports data to the backend via POST /admin/reports/profiler/frontend/{profile}/navigation. The route only allows HTTP POST but as the admin path check of the "admin language from user preferences" language negotiator can't pass the HTTP method to the AccessAwareRouter it fails with the MethodNotAllowedException, which happens at least once per main request.

Patch #74 seems to fix this. I can't 100% confirm yet as I have another issue with webprofiler to solve first.

This seems to be the root cause for #3351241: ckeditor5 upload image MethodNotAllowedException.

Let's first get a test-only should-fail patch on 11.x based on #74 up.

And here's the full patch, based on #74, taking #70 on board.

I tested the patch 2706241-79.patch and identified a syntax error in the catch statement. I made a minor syntax adjustment to resolve the issue.

@sanket.addweb

There is no syntax problem, so long as you're on PHP8.

Make sure you upload an interdiff, you would have noticed you omitted a significant portion of the previous patch.

The current reviewable patch remains #79.

Updated the issue summary to include the solution from #79.

Majority of the patch is the added tests which #78 show are failing as expected.

Think this is good.

Known random test failure (#3380433: [random test failure] CronRunTest::testAutomatedCron), ordered retest.

Committed/pushed to 11.x and cherry-picked to 10.1.x, thanks!