Problem/Motivation

_customerror_login_submit() runs on every login form. It is what sends a user who logs in from the 403 page back to the page they were denied. It has three bugs:

  • A normal login posts no destination, and the else branch then sets the query destination to NULL. /user/login?destination=/node/1 lands on the user page.
  • A posted destination goes to Url::fromUserInput(), which throws unless it starts with /, ? or #.
  • Core's destination already includes the base path, and Url::fromUserInput() adds it again. On a site in a subdirectory, logging in from the 403 page gives a 404.

Steps to reproduce

  1. Log in at /user/login?destination=/node/1. You land on your user page.
  2. Submit the login form with an extra field destination=node/1. The response is a 500.
  3. On a site in a subdirectory, turn on the 403 login form, open a page you can't access, and log in. The response is a 404.

Proposed resolution

  • Drop the else.
  • Only redirect to a posted destination that is a local path starting with /.
  • Build the redirect from the scheme and host plus the destination, as core's RedirectResponseSubscriber does.

Tests:

  • ?destination on the login page is honored (fails on HEAD).
  • A posted destination without a leading slash redirects instead of giving a 500 (fails on HEAD).
  • A posted destination that includes the base path is followed as is.

Issue fork customerror-3637907

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:

About issue forks

Comments

loze created an issue. See original summary.

loze’s picture

loze commented
Related issues: +#3401346: Redirect after login does not work, +#3401535: TypeError: Drupal\Core\Form\FormState::setRedirectUrl(): Argument #1 ($url) must be of type Drupal\Core\Url, null given, +#3440389: Allow disabling destination redirection after login
loze’s picture

loze commented
Issue summary: View changes

loze opened merge request !10

loze’s picture

loze commented
Title: Login form ignores ?destination, and an external destination causes a fatal error » Login form ignores ?destination, and a posted destination without a leading slash causes a fatal error
Issue summary: View changes
loze’s picture

loze commented
Issue summary: View changes
loze’s picture

loze commented
Issue summary: View changes
loze’s picture

loze commented
Title: Login form ignores ?destination, and a posted destination without a leading slash causes a fatal error » Login form loses ?destination, crashes on a destination without a leading slash, and 404s on sites in a subdirectory
Issue summary: View changes
loze’s picture

loze commented
Issue summary: View changes
loze’s picture

loze commented
Assigned: loze » Unassigned
Status: Active » Needs review
nitinkumar_7’s picture

nitinkumar_7 commented

The changes look good overall. I have suggestion regarding test coverage:

Could we add a test to verify that an external destination is rejected? Since the new implementation explicitly checks UrlHelper::isExternal(), it would be good to cover this behaviour.

nitinkumar_7’s picture

nitinkumar_7 commented

Could we also test the redirect on an actual subdirectory installation? The current base-path test is useful, but it doesn't appear to reproduce that exact environment.

This would give us better coverage for the reported issues.

loze’s picture

loze commented

Thanks for the review.

I added a kernel test, LoginSubmitHandlerTest, that posts https://example.com/ and //example.com/ to the submit handler and checks that no redirect is set. It calls the handler directly because core's request sanitizer removes external destinations before a real request gets that far. The test-only job shows both cases failing on the current code.

You're right about the subdirectory test. The pipeline installs the site at /web, and the same test-only job shows testPostedDestinationKeepsBasePath passing on the current code, so it wasn't testing the bug. I removed it. The doubled base path shows up when you log in from the login form on the 403 page. Testing that needs the enable_login schema that #3637882: 403 page starts a session for every anonymous visitor, which bypasses the page cache adds, so I'll add that test once it's in.