Contrib.social

Handle invalid destination parameters better

Open on Drupal.org →
Created on 9 May 2025, about 2 months ago

Problem/Motivation

We use this on an intranet where all URL's except a few are redirected to the login page. Some of these are typical bots that look for security issues.

One that we've been hit with recently is a request like this:

https://ourdomain.tld/:88/favicon.ico

This redirects to:

/saml/login?destination=/:88/favicon.ico

Which logs an exception like this:

InvalidArgumentException encountered during initiating SAML login: The internal path component ':88/favicon.ico' is external. You are not allowed to specify an external URL together with internal:/

And redirects away, but becauase there's a destination query string, it goes straight back to that page, which redirects again back to saml/login, resulting in a redirect loop and lots of warnings in our logs.

Steps to reproduce

Proposed resolution

Not sure. Ignore invalid destinations? Could also make sure to remove the destination when redirecting, would still come back then in our case, but without the destination.

📌 Task
Status

Active

Version

4.0

Component

Code

Created by

🇨🇭Switzerland berdir Switzerland

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @berdir
  • Comment about 1 month ago →
    🇳🇱Netherlands roderik Amsterdam,NL / Budapest,HU

    (1AM typo in commit message. Commit is here.)
    .
    Thanks for the report. Yes, this is a bug... which I never consciously registered.

    There's supposedly-always an exception handler active, which takes care of returning a (redirect) response to an error page. But the 'destination' parameter should always be removed in that case. In other words: the 'destination' wasn't removed early enough.

    This makes DOMAIN/saml/login?destination=/:88/favicon.ico at DOMAIN/:88/favicon.ico, instead of the error page... which is a bug.

    Bug fixed by removing the 'destination' properly/earlier. "The error page" by default is the homepage (with the standard message saying there was an error which has been logged). If that isn't good for your specific case, you can set an "Error redirect URL" in config.

    (I briefly considered (additionally) checking/removing invalid 'destination' values, but that seems to be the RequestSanitizer::checkDestination()'s job, so I won't duplicate that.)

  • Comment about 1 month ago →
    🇳🇱Netherlands roderik Amsterdam,NL / Budapest,HU
  • Comment 25 days ago →
    System Message

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

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024