Trailing slash also added to external redirects

[brasileric]

Module version(s) affected

5.1.0

Description

If add_trailing_slash is set true:

SilverStripe\Control\Controller:
  add_trailing_slash: true

a trailing slash is also added to external redirects like:

$this->redirect('https//www.example.com/about-us');

is redirected to https//www.example.com/about-us/ and:

$this->redirect('https//www.example.com/about-us?rank=1');

is redirected to https//www.example.com/about-us/?rank=1 (so with a slash after about-us)

In my opinion a trailing slash must be added to internal redirects only.

How to reproduce

$this->redirect('https//www.example.com/about-us');

Possible Solution

No response

Additional Context

No response

Validations

• Check that there isn't already an issue that reports the same bug
• Double check that your reproduction steps work in a fresh installation of silverstripe/installer (with any code examples you've provided)

PRs

• FIX Do not suffix trailing slash to external links #11321

[maxime-rainville]

I agree that Controller::redirect() should probably respect whatever choice the developer hard coding the link made.

But I'm not sure this would have any practical impact, so I'm disinclined to prioritise this issue. Is there a scenario where this could lead to a problem for the end user?

[brasileric]

I agree that Controller::redirect() should probably respect whatever choice the developer hard coding the link made.

But I'm not sure this would have any practical impact, so I'm disinclined to prioritise this issue. Is there a scenario where this could lead to a problem for the end user?

Hi Maxime. Thanks for your response. In most cases this will not be a problem I agree. But I have a redirect that goes to an affiliate link which is not working when added a trailing slash. I realize this is an issue on the side of the affiliate platform as well.

I agree with the low priority.

[brasileric]

I'm experiencing a problem with some api authentication as well. Like:

https://login.daisycon.com/oauth/authorize?response_type=code&client_id=

Is changing to:

https://login.daisycon.com/oauth/authorize/?response_type=code&client_id=

And refused by the api.

[lekoala]

just realised the same, spent 30 minutes troubleshooting why my openid provider wasn't working as expected because silverstripe decided to add a slash at the end of the redirect url...

[lekoala]

@maxime-rainville i don't agree it's low priority, this can break things in unexpected ways!

i think the issue is due to Director::absoluteUrl() usage instead of what was in place before

lekoala@21c9c13

this fixes the issue but maybe it should be considered that Director::absoluteUrl shouldn't try to rewrite an url that is working fine

in the meantime, i'm having a static utility doing this

    public static function redirect(string $url, int $code = 302): HTTPResponse
    {
        $response = new HTTPResponse();
        return $response->redirect($url, $code);
    }

it's really odd to have to do that :-)

[johannesx75]

@maxime-rainville please reconsider the low impact rating.

This breaks things with external services that are hard to trace and debug. For us it was "Sign in with Apple".

[lekoala]

So actually, this is really annoying . It hit me again : i have a whitelisted url without a trailing slash in a third party service, and obviously, it doesn't work with the trailing slash... so... using absoluteURL has again broken my website :(

I've improved my snippet a bit to generate absolute urls it looks like this now

    /**
     * A simpler approach to absolute urls
     * Does not mutate the url (ie trailing slashes) unless specified
     * @param string|null $url
     * @param ?bool $trailing With (true) or without (false) trailing slash. Leave null for no changes
     * @return string
     */
    public static function absURL(?string $url, ?bool $trailing = null): string
    {
        $url = $url ?? '';
        $url = ltrim($url, '/');
        $base = rtrim(Director::absoluteBaseURL(), '/');
        // Append base url if needed
        if (!str_contains($url, $base)) {
            // Add base if it's not an external redirect
            if (!preg_match('/^http(s)?:/', $url)) {
                $url = $base . '/' . $url;
            }
        }
        // Add or remove trailing slash
        if ($trailing !== null) {
            $split = explode('?', $url);
            $url = rtrim($split[0], '/');
            if ($trailing) {
                $url .= '/';
            }
            if (!empty($split[1])) {
                $url .= '?' . $split[1];
            }
        }
        return $url;
    }

[chtombleson]

This is causing an issue with Windcave/Omnipay module. I have had to implement a workaround until this is fixed.

[kinglozzer]

IMO the best fix here would be to add something like this to the start of Controller::normaliseTrailingSlash():

if (!Director::is_site_url($url)) {
    return $url;
}

I can’t see any reason why this behaviour ever should apply to external URLs. Anyone disagree?

[lekoala]

@kinglozzer fully agree with you, adding trailing / to external url's don't make any sense

[draghu9]

This is causing an issue with Windcave/Omnipay module. I have had to implement a workaround until this is fixed.

What is the workaround you applied @chtombleson. I have added a redirect function within my controller class to remove the trailing slash. Unfortunately, that did not work for me.

[GuySartorelli]

@emteknetnz Please link all relevant PRs in the issue description

[emteknetnz]

Done

[GuySartorelli]

PR merged. This will be automatically tagged by GitHub actions