Stop using getMainRequest() to build $form['#action']

It's especially funny because both tests do something related to forms on access denied pages, weird.

#2263569: Bypass form caching by default for forms using #ajax. is in.

I very much agree that getMasterRequest() is not correct there. Let's see what breaks.

The problem is that 403/404 is rendered in a subrequest and the URI includes the path of the main request in the destination query parameter (why?). If that ends up in the form action, then that will win over the redirect set by RedirectBlockForm::submitForm().

Special case exception subscriber subrequest. Maybe this needs to be configurable in some way? The destination is useful for login forms on 403 pages. I feel it is bad for most other things, like, e.g. a search field on the 404 one.

The redirection behavior after form submissions from exception pages should be left up to the implementing form. The login form already special cases that case in HEAD, presumably in order to fix problems when a 403 is rendered as a result of a POST request. In that case the destination parameter is missing from the URL (its added to POST parameters by the exception handler).

In this patch:

1. Do not set destination in the html exception handler but instead _exception_location in order to prevent an automatic redirect.
2. Fix the login form to respect that query/request parameter.

It would be great to get rid of that hack indeed! Thank you znerol

1. +++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php
   @@ -128,10 +128,10 @@ protected function makeSubrequest(GetResponseForExceptionEvent $event, $url, $st
   +        $sub_request = Request::create($url, 'POST', ['_exception_location' => $this->redirectDestination->get(), '_exception_statuscode' => $status_code] + $request->request->all(), $request->cookies->all(), [], $request->server->all());
   

   Can we add a comment somewhere explaining why setting destination itself doesn't work?

2. +++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php
   @@ -128,10 +128,10 @@ protected function makeSubrequest(GetResponseForExceptionEvent $event, $url, $st
   +        $sub_request = Request::create($url, 'GET', $request->query->all() + ['_exception_location' => $this->redirectDestination->get(), '_exception_statuscode' => $status_code], $request->cookies->all(), [], $request->server->all());
   
   +++ b/core/modules/user/src/Form/UserLoginForm.php
   @@ -135,16 +135,17 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    if ($original_location = $this->getRequest()->get('_exception_location')) {
   

   It would be nice if we could introduce a constant for _exception_location

Addresses #10.

Also fix semantics in ExternalFormUrlTest.

uhm.

Reroll.

Patch in #20 failed for me on 8.0.2.

Reject files are attached (renamed to *_.rej_.txt).

patching file core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php
patching file core/lib/Drupal/Core/Form/FormBuilder.php
Hunk #1 succeeded at 829 (offset 107 lines).
patching file core/modules/system/src/Tests/Form/ExternalFormUrlTest.php
patching file core/modules/system/src/Tests/Routing/DestinationTest.php
patching file core/modules/system/tests/modules/system_test/src/Controller/SystemTestController.php
Hunk #1 FAILED at 9.
Hunk #2 succeeded at 150 (offset 1 line).
1 out of 2 hunks FAILED -- saving rejects to file core/modules/system/tests/modules/system_test/src/Controller/SystemTestController.php.rej
patching file core/modules/system/tests/modules/system_test/system_test.routing.yml
patching file core/modules/user/src/Form/UserLoginForm.php
Hunk #1 FAILED at 8.
Hunk #2 succeeded at 135 (offset -1 lines).
1 out of 2 hunks FAILED -- saving rejects to file core/modules/user/src/Form/UserLoginForm.php.rej
patching file core/tests/Drupal/Tests/Core/EventSubscriber/CustomPageExceptionHtmlSubscriberTest.php

Rerolled the patch from #20.

Patch is looking good overall.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php
   @@ -45,6 +45,16 @@ class DefaultExceptionHtmlSubscriber extends HttpExceptionSubscriberBase {
   +   * URL query attribute to specify the status code resulting from an exception.
   ...
   +   * URL query attribute to specify the location where an exception occured.
   

   s/URL query/Request/

2. +++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php
   @@ -127,11 +137,22 @@ protected function makeSubrequest(GetResponseForExceptionEvent $event, $url, $st
   +      // to redirect to the original location after a successfull login. Note
   

   s/successfull/successful/

3. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -829,9 +829,7 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
      protected function buildFormAction() {
   -    // @todo Use <current> instead of the master request in
   -    //   https://www.drupal.org/node/2505339.
   -    $request = $this->requestStack->getMasterRequest();
   +    $request = $this->requestStack->getCurrentRequest();
   

   This definitely makes more sense.

4. +++ b/core/modules/system/src/Tests/Form/ExternalFormUrlTest.php
   @@ -74,10 +74,11 @@ protected function setUp() {
   +    // Create a new request which has a request uri with multiple leading
   

   s/uri/URI/

Trying to reroll this, would be useful for better cache performance of the login block (#2867703: Change #action in UserLoginBlock to a #lazy_builder to avoid the route cache context , #2847972: Missing context on UserLoginBlock causes render-cache poisoning of the "destination").

As far as I see, #2595695: 4xx handling using subrequests: no longer able to vary by URL basically already solved a lot of what we are trying todo here, except it kept the default destination argument, and just added the _exception_statuscode.

Trying to go back to a minimal patch, then we can see from there if we still need to do something more.

Also combining it with the patch from #2867703: Change #action in UserLoginBlock to a #lazy_builder to avoid the route cache context to see if that works too.

So this is passing (yay), but it doesn't fix the user login form test.

Checked that test and the problem is that we explicitly need to force the destination even on non 403 pages, so this won't help with that. That means we definitely need a custom lazy builder there, fair enough.

But it also means that this seems to be good to go like this?

It also didn't fix it because the user login form patch was completely bogus and didn't do what it should have done. But it wouldn't have done it even if it would have been correct.

Since #2867703: Change #action in UserLoginBlock to a #lazy_builder to avoid the route cache context is in, the non-combined patch from #29 should be all that's left to do here. Let's try it.

Nice, this should be good to go based on #31.

I bet it needs test coverage

I'm not sure we need any additional test coverage, #29/#31 implies that we don't.

If this broke something, we'd have a zillion tests failing. 😄

I agree with this not needing test coverage.

I think this is better fix cos
- it documents what actually expected
- this method mostly empty but using external service dependency

Actually this method require only request object to but actually supposed to hide implementation details:

- build URI from request removing "ajax implementation nits" - which has follow-up to be removed #2504709: Prevent _wrapper_format and ajax_form parameters from bleeding through to generated URLs
- checking for // (guessing the request stack can contain weird request object) - maybe it should be cleaned in follow-up

As method protected it needs unit test coverage and probably could be removed (BC?)

https://www.drupal.org/core/d8-bc-policy#protected-methods allows to change argument

core8$ git grep renderPlaceholderFormAction

core/lib/Drupal/Core/Form/FormBuilder.php:648:  public function renderPlaceholderFormAction() {
core/lib/Drupal/Core/Form/FormBuilder.php:696:        '#lazy_builder' => ['form_builder:renderPlaceholderFormAction', []],
core/modules/big_pipe/src/Render/Placeholder/BigPipeStrategy.php:152:      // @see \Drupal\Core\Form\FormBuilder::renderPlaceholderFormAction()
core/modules/big_pipe/tests/modules/big_pipe_test/src/BigPipePlaceholderTestCases.php:103:        '#lazy_builder' => ['form_builder:renderPlaceholderFormAction', []],
core/modules/user/src/Plugin/Block/UserLoginBlock.php:108:      '#lazy_builder' => ['\Drupal\user\Plugin\Block\UserLoginBlock::renderPlaceholderFormAction', []],
core/modules/user/src/Plugin/Block/UserLoginBlock.php:151:   * @see \Drupal\Core\Form\FormBuilder::renderPlaceholderFormAction()
core/modules/user/src/Plugin/Block/UserLoginBlock.php:153:  public static function renderPlaceholderFormAction() {

Here's a test

This approach looks fine as well, thanks for the test :)

I don't see the benefit of the changes to Formbuilder in #41 over #36. Changing the method signature is not necessary to do the fix and is unnecessarily disruptive. If there are compelling reasons down the line to make it the way it is in #41 then we should do this then. +1 for the new test though.

I also think that this change has caused some disruption to core forms in the past we should have a CR and only commit to the development branch and have some idea if major contrib is affected.

@alexpott why chamging protected method you find disruptive?

@andypost because if someone has extended the FormBuilder class this change might break them. And whilst this is not public API we should still seek to minimise breaking change unless it is truly necessary. I don't think it is in this issue. Maybe a future issue it will be but we should justify that on that issue and not second guess ourselves here.

if someone has extended the FormBuilder class this change might break them.

In which case someone would extend FormBuilder class? Can we have an example, please?

Let's not make perfect the enemy of good, it's really not worth it for such a small @todo cleanup :)

This patch contains the fix from #36 with the test from #42.

Okay. So now looking at the history of this issue - what's the chance of this breaking contrib in the same way the core has been broken over the years. Like what happens if contrib has something where it would need to do the corollary of #2867703: Change #action in UserLoginBlock to a #lazy_builder to avoid the route cache context . At the very least we need a change record that details the implications for contrib - it would also be good to know what might break - especially if popular contrib is affected.

+++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
@@ -878,6 +878,39 @@ public function providerTestFormTokenCacheability() {
 
+  /**
+   * @covers ::renderPlaceholderFormAction
+   * @covers ::buildFormAction
+   *
+   * @dataProvider providerTestRenderPlaceholderFormAction
+   */
+  public function testRenderPlaceholderFormAction($expected, $request_uri) {
+    $request = new Request([], [], [], [], [], ['REQUEST_URI' => $request_uri]);
+    $request->headers->set('HOST', 'example.com');
+    $this->requestStack->push($request);
+    $element = $this->formBuilder->renderPlaceholderFormAction();
+    $this->assertEquals($expected, $element['#markup']);
+  }
+
+  /**
+   * Data provider for testRenderPlaceholderFormAction.
+   *
+   * @return array[]
+   *   Items are arrays of two items:
+   *   - The expected result;
+   *   - The request URI to create a request with.
+   */
+  public function providerTestRenderPlaceholderFormAction() {
+    return [
+      ['/path', '/path'],
+      ['/path?t=1', '/path?t=1'],
+      // Cases of CSRF protection in buildFormAction() method.
+      ['http://example.com//path', '//path'],
+      ['http://example.com///path', '///path'],
+      ['http://example.com///path', '///path?t=1'],
+    ];
+  }
+

I'm a bit dubious about this test coverage. For me this doesn't really prove anything, beside the code being implemented as it is. Is there a way to test more something which is described in the issue summary?

Using getCurrentRequest() caused test failures in \Drupal\system\Tests\Form\RedirectTest, but continuing to use the master request with caused failures in \Drupal\system\Tests\System\AccessDeniedTest

This is due to conflicting use cases covered by RedirectTest and AccessDeniedTest. The latter asserts that people are sent to the original destination after a login while the former tests that it is possible for a form to set a redirect destination (even on exception pages).

The problem is that in HEAD the destination query parameter is used to pass the original location into the subrequest where the exception is rendered. However, the destination parameter always overrides any redirect set during form building. Usage of the master request in the FormBuilder does not really resolve that conflict but actually hides it.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -842,9 +842,7 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
+    $request = $this->requestStack->getCurrentRequest();
     $request_uri = $request->getRequestUri();

Ran into this today. We've got inbound/outbound path rewriting going on and Forms set the action to the wrong URL. I believe that using the <current> route would fix things for us.

I think the URL should be generated the same way it is generated in RedirectDestination::get(). i.e.

$query = $this->requestStack->getCurrentRequest()->query;
$request_uri = $this->urlGenerator->generateFromRoute('<current>', [], ['query' => UrlHelper::filterQueryParameters($query->all())]);

No interdiff as the tests didn't change and the approach used in the form builder is entirely different.

No interdiff as the tests didn't change and the approach used in the form builder is entirely different.

Btw #50 patch looks enough

Added reroll of patch #50 on Drupal core 9.5.x. as patch #50 was not applying. I think it can be reviewed if It passes the test as per comment 366.

still needs CR and MR for 11.x

Needs only CR now

Added CR draft which needs better wording

Small change requests

fixed

Code wise this looks good! All tests are passing 👍

Also updated the readability of the CR a little bit. Not really sure if the CR needs more info or not.

I think we can set this to RTBC now.

I still don't think @alexpott or @dawehner's comments from #51 and #52 have been addressed. The CR doesn't really explain why you might be affected by this nor what you should do if you are, and the test coverage is very specific to the fix; it doesn't really prove anything about the original problem.

This is blocking #3566881: Add a submitForm() method to HttpKernelUiHelperTrait, because in a kernel test, the main request is the request from PHPUnit whose path is always '/'.

Should this be escalated to a bug in light of this?