feat: Allow wildcard domains for redirect_to checks #1528

abador · 2021-07-12T10:05:40Z

Related issue

resolves #943
@aeneasr

Proposed changes

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

Checklist

I have read the contributing guidelines.
I am following the
contributing code guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

Further comments

aeneasr

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

aeneasr · 2021-07-12T12:01:21Z

driver/config/config.go

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


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

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?

aeneasr · 2021-07-12T12:02:50Z

x/http_secure_redirect.go

@@ -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 {


Could you please add tests for this?

ttimonen · 2021-07-14T22:56:26Z

x/http_secure_redirect.go

+// 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:])


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.

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

aeneasr · 2021-07-30T08:50:01Z

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 · 2021-10-12T05:51:38Z

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

abador · 2021-10-14T09:14:11Z

@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

…t domain

…rect-to-domains # Conflicts: # driver/config/config.go # go.mod

abador · 2021-10-22T06:23:14Z

@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

aeneasr · 2021-10-27T12:17:23Z

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!

aeneasr

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 · 2021-11-08T08:06:38Z

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 · 2021-11-09T08:17:41Z

Great stuff!

Updated example for `allowed_return_urls` to include wildcard url. [Merged PR from 2021](ory#1528)

…3533) See #1528

feat:Allow wildcard domains for redirect_to checks

67d79ef

abador mentioned this pull request Jul 12, 2021

Allow wildcard redirect to domains Wikia/kratos#4

Closed

6 tasks

aeneasr requested changes Jul 12, 2021

View reviewed changes

aeneasr modified the milestones: v0.8.0-alpha.1, v0.9.0-alpha.1 Jul 12, 2021

ttimonen reviewed Jul 14, 2021

View reviewed changes

aeneasr marked this pull request as draft July 30, 2021 08:50

aeneasr modified the milestones: v0.9.0-alpha.1, v0.10.0-alpha.1, Incubating Release Aug 17, 2021

Check if host is set and case-insensitive verification of the redirec…

b5351c3

…t domain

abador changed the title ~~feat:Allow wildcard domains for redirect_to checks~~ feat: Allow wildcard domains for redirect_to checks Oct 21, 2021

abador added 4 commits October 21, 2021 10:31

Add SecureRedirectToIsWhiteListedHost tests

67481b6

Merge remote-tracking branch 'origin/master' into allow-wildcard-redi…

3d25bcc

…rect-to-domains # Conflicts: # driver/config/config.go # go.mod

Add go mod for publicsuffix

e826cc5

fix tests

add81f0

abador marked this pull request as ready for review October 22, 2021 06:23

abador requested a review from zepatrik as a code owner October 22, 2021 06:23

abador added 2 commits October 25, 2021 12:35

Merge branch 'master' into allow-wildcard-redirect-to-domains

8021987

Merge branch 'master' into allow-wildcard-redirect-to-domains

0d6e69b

abador added 3 commits October 28, 2021 10:00

Merge branch 'master' into allow-wildcard-redirect-to-domains

31f60f5

Merge branch 'master' into allow-wildcard-redirect-to-domains

b0931b8

Merge branch 'master' into allow-wildcard-redirect-to-domains

17e9bcc

Merge branch 'master' into allow-wildcard-redirect-to-domains

40c3702

aeneasr requested changes Nov 1, 2021

View reviewed changes

abador added 3 commits November 2, 2021 08:05

Merge branch 'master' into allow-wildcard-redirect-to-domains

008f704

Merge branch 'master' into allow-wildcard-redirect-to-domains

3b8a606

Add info in documentation

5ff7ee5

aeneasr approved these changes Nov 9, 2021

View reviewed changes

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

Update config.schema.json

0f44938

Updated example for `allowed_return_urls` to include wildcard url. [Merged PR from 2021](ory#1528)

martinloesethjensen mentioned this pull request Sep 26, 2023

docs: update allowed_return_urls examples in config.schema.json to include wildcard #3533

Merged

7 tasks

aeneasr pushed a commit that referenced this pull request Oct 2, 2023

docs: add example for allowed_return_urls to include wildcard url (#…

39b0c3c

…3533) See #1528

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow wildcard domains for redirect_to checks #1528

feat: Allow wildcard domains for redirect_to checks #1528

abador commented Jul 12, 2021 •

edited by zepatrik

aeneasr left a comment

aeneasr Jul 12, 2021

abador Oct 21, 2021 •

edited

aeneasr Jul 12, 2021

ttimonen Jul 14, 2021

aeneasr Jul 15, 2021

aeneasr commented Jul 30, 2021

aeneasr commented Oct 12, 2021

abador commented Oct 14, 2021

abador commented Oct 22, 2021

aeneasr commented Oct 27, 2021

aeneasr left a comment

abador commented Nov 8, 2021

aeneasr commented Nov 9, 2021

feat: Allow wildcard domains for redirect_to checks #1528

feat: Allow wildcard domains for redirect_to checks #1528

Conversation

abador commented Jul 12, 2021 • edited by zepatrik

Related issue

Proposed changes

Checklist

Further comments

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr Jul 12, 2021

Choose a reason for hiding this comment

abador Oct 21, 2021 • edited

Choose a reason for hiding this comment

aeneasr Jul 12, 2021

Choose a reason for hiding this comment

ttimonen Jul 14, 2021

Choose a reason for hiding this comment

aeneasr Jul 15, 2021

Choose a reason for hiding this comment

aeneasr commented Jul 30, 2021

aeneasr commented Oct 12, 2021

abador commented Oct 14, 2021

abador commented Oct 22, 2021

aeneasr commented Oct 27, 2021

aeneasr left a comment

Choose a reason for hiding this comment

abador commented Nov 8, 2021

aeneasr commented Nov 9, 2021

abador commented Jul 12, 2021 •

edited by zepatrik

abador Oct 21, 2021 •

edited