Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Allow wildcard domains for redirect_to checks #1528

Merged
merged 15 commits into from
Nov 9, 2021

Conversation

abador
Copy link
Contributor

@abador abador commented Jul 12, 2021

Related issue

resolves #943
@aeneasr

Proposed changes

Allow to whitelist a wildcard domain. Domains are filtered out using golang.org/x/net/publicsuffix

Checklist

Further comments

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you for your contribution! This looks pretty good and I have some ideas how to improve it further :)

continue
}
eTLD, icann := publicsuffix.PublicSuffix(parsed.Host)
if parsed.Host[:1] == "*" &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This panics if host is an empty string. Also, what about foo.*.bar?

Copy link
Contributor Author

@abador abador Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the cases I can think of is adding some return domain eg: page1.foo.bar or page2.foo.bar where those changes are valid. If we want to make more complicated situations work I would probably go with regex, but is this case really needed?

@@ -55,6 +55,14 @@ func SecureRedirectOverrideDefaultReturnTo(defaultReturnTo *url.URL) SecureRedir
}
}

// SecureRedirectToIsWhitelisted validates if the redirect_to param is allowed for a given wildcard
func SecureRedirectToIsWhiteListedHost(returnTo *url.URL, allowed url.URL) bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add tests for this?

// SecureRedirectToIsWhitelisted validates if the redirect_to param is allowed for a given wildcard
func SecureRedirectToIsWhiteListedHost(returnTo *url.URL, allowed url.URL) bool {
if allowed.Host != "" && allowed.Host[:1] == "*" {
return strings.HasSuffix(returnTo.Host, allowed.Host[1:])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the plan for case sensitivity here?

  • On documentation, the function docstring is ambiguous,
  • when wildcards are used, the match is case-sensitive,
  • when wildcards are not used, the match is case-insensitive.

To follow the principle of least surprise, you might want to do one of

  • Do strings.HasSuffix(strings.ToLower(returnTo.Host), strings.ToLower(allowed.Host[1:]) here to be case-insensitive.
  • .. or do return allowed.Host == returnTo.Host few lines later to be case-sensitive.
  • .. or mention this semi-case-insensitivity behavior in the function comment if it is intentional and preferred.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hostnames are case insensitive, so the correct approach would be to have the wildcard matches also case insensitive :)

@aeneasr
Copy link
Member

aeneasr commented Jul 30, 2021

While the PR is being worked on I will mark it as a draft. That declutters our review backlog :)

Once you're done with your changes and would like someone to review them, mark the PR as ready and request a review from one of the maintainers.

Thank you!

@aeneasr aeneasr marked this pull request as draft July 30, 2021 08:50
@aeneasr
Copy link
Member

aeneasr commented Oct 12, 2021

@abador are you still up for the changes or should I close this?

@abador
Copy link
Contributor Author

abador commented Oct 14, 2021

@abador are you still up for the changes or should I close this?
Sorry @aeneasr I'll get back to all upstream tickets on friday or next week

@abador abador changed the title feat:Allow wildcard domains for redirect_to checks feat: Allow wildcard domains for redirect_to checks Oct 21, 2021
@abador
Copy link
Contributor Author

abador commented Oct 22, 2021

@aeneasr it seems that e2e tests are failing, but it doesn't look it's connected to my changes. If we really need a solution that works with wildcards inside a subdomain please ping me

@abador abador marked this pull request as ready for review October 22, 2021 06:23
@abador abador requested a review from zepatrik as a code owner October 22, 2021 06:23
@aeneasr
Copy link
Member

aeneasr commented Oct 27, 2021

I tried pushing some changes required for merging the PR to your fork & branch, but it appears that I am not allowed to do so 😕

% git push ...
ERROR: Permission to push denied to aeneasr.
fatal: could not read from the remote repository.

Please make sure that you have the correct access rights
and the repository exists.

But the good news is, giving access is easy! ☺️ All you need to do is enable write access for maintainers. Thank you! 😄

If the repository belongs to an organization, please add me for the project as a collaborator!

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Now, one last thing :) We need to document it! Could you please add some docs here: https://www.ory.sh/kratos/docs/concepts/browser-redirect-flow-completion

Thank you! 🙏

@abador
Copy link
Contributor Author

abador commented Nov 8, 2021

Great! Now, one last thing :) We need to document it! Could you please add some docs here: https://www.ory.sh/kratos/docs/concepts/browser-redirect-flow-completion

Thank you! 🙏

I've added some more docs in my last commit.

@aeneasr
Copy link
Member

aeneasr commented Nov 9, 2021

Great stuff!

@aeneasr aeneasr merged commit 349cdcf into ory:master Nov 9, 2021
martinloesethjensen added a commit to martinloesethjensen/kratos that referenced this pull request Sep 26, 2023
Updated example for `allowed_return_urls` to include wildcard url.

[Merged PR from 2021](ory#1528)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support wildcard in whitelisted_return_urls
3 participants