Allow redirects to whitelisted hosts with ports

[kamaln7]

This updates IsValidRedirect to allow redirects to whitelisted hosts if a port is set on the redirect URL. Fixes #279.

Description

Use URL.Hostname() instead of URL.Host to strip the port when validating the host against the list of whitelisted domain.

How Has This Been Tested?

Added tests to cover the new use cases. I wasn't sure if I needed to add tests for invalid hosts with ports but let me know and I can add that in.

Checklist:

• My change requires a change to the documentation or CHANGELOG.
• I have updated the documentation/CHANGELOG accordingly.
• I have created a feature (non-master) branch for my PR.

[loshz]

Thank you and congrats on your first contribution! 🎉

[JoelSpeed]

@kamaln7 Thanks for submitting this PR, I've got an idea about potential improvements, I wonder if we could define some logic so that the whitelist domains could include the port? If no port is set we assume there should be no port set on the request for redirection (ie it's a standard port), if a port is set on the redirect then it must match a whitelisted domain/port combination, WDYT?

Added tests to cover the new use cases. I wasn't sure if I needed to add tests for invalid hosts with ports but let me know and I can add that in.

If you could add some invalid host tests that would be great!

Finally, please make sure to update the changelog and documentation before this gets merged 🙂

[kamaln7]

Thanks for the feedback! I'll be sure to update the documentation and changelog, but I want to confirm that the current behavior is what you were looking for.

Provided that the redirect URL matches a whitelisted domain:

• If the whitelisted domain doesn't contain a port, the redirect URL is considered valid regardless of its port
• If the whitelisted domain contains a port, the redirect URL will be considered valid only if it matches the same port

I've added some more tests which I believe cover all cases but an extra set of eyes is never a bad idea.

Now, there's something I'm not 100% sure about. Because the whitelisted domains aren't proper URLs, only hosts, I don't use url.Parse() but rather create a url.URL struct manually and set the Host field myself. Then I use URL.Hostname() and URL.Port() as with the redirect URLs.

The code for URL.Hostname() and URL.Port() doesn't use any fields other than Host, so it's fine and it works. But my concern is that by creating the struct manually and not using url.Parse(), the Host isn't validated. That should be fine if you trust operators to use proper domains in the config, but I feel like that might cause unpredictable behavior if an operator uses an invalid host. I do check if URL.Hostname() returns an empty string though I don't know if that's good enough.

I'm not sure how deep you want to go into this so I'd like to hear your thoughts.

[JoelSpeed]

If the whitelisted domain doesn't contain a port, the redirect URL is considered valid regardless of its port
If the whitelisted domain contains a port, the redirect URL will be considered valid only if it matches the same port

I think if there is no port set on the whitelist domain then we should assume that the request must be on the default port for the protocol (so 80 for http, 443 for https).

Though I would assume that the request wouldn't have a port set when you form it if it's to the default port so I think the check would be to check for an empty port, might be worth testing that assumption though 🤔

[kamaln7]

Yes, good point about the port being empty.

I’ll update the code. For wildcard ports I guess we could use “*” as the port assuming that wouldn’t cause issues with parsing. That way it would be backwards compatible too.

[JoelSpeed]

That sounds like a really good approach! Please make sure there are sufficient tests for all the cases when you make these changes

[kamaln7]

Let me know what you think. I had to copy splitHostPort and validOptionalPort from net/url to the package and modify validOptionalPort to allow * as a port. Neither net/url.URL.splitHostPort() nor net.SplitHostPort() work without workarounds.

I also took some time to refactor the tests.

[kamaln7]

Hey @JoelSpeed any updates? 😄

[JoelSpeed]

Found one minor thing, but other than that it looks great! Apologies for the delay btw 🙈

[kamaln7]

No worries, thanks for reviewing!

[mnaser]

This is super useful and it seems to have been forgotten till it hit merge conflict.

Perhaps @kamaln7 could be kind enough to rebase and we can help land this? :)

[loshz]

Once conflicts are fixed we can merge.

[kamaln7]

Fixed! 😃

[mnaser]

@syscll can this be merged now? :)

[loshz]

LGTM 👍

[loshz]

Can you give this a final review @JoelSpeed @steakunderscore

[JoelSpeed]

LGTM, thanks @kamaln7