Skip to content

Commit

Permalink
feat: allow wildcard domains for redirect_to checks (#1528)
Browse files Browse the repository at this point in the history
Support wildcard domains in redirect_to checks.

Closes #943
  • Loading branch information
abador authored Nov 9, 2021
1 parent f08cecd commit 349cdcf
Show file tree
Hide file tree
Showing 8 changed files with 79 additions and 1 deletion.
2 changes: 2 additions & 0 deletions docs/docs/reference/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -1191,6 +1191,7 @@ selfservice:
# - - https://app.my-app.com/dashboard
# - /dashboard
# - https://www.my-app.com/
# - https://*.my-app.com/
#
# Set this value using environment variables on
# - Linux/macOS:
Expand All @@ -1202,6 +1203,7 @@ selfservice:
- https://app.my-app.com/dashboard
- /dashboard
- https://www.my-app.com/
- https://*.my-app.com/

## serve ##
#
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,27 @@ selfservice:
- https://www.myapp.com/
```
It's possible to add domain wildcards to our configuration.
As an example, we're adding all subdomains of `myapp.com` to the whitelist, thus
we can now specify a dynamic subdomain return url like so
`?return_to=https://foo.myapp.com` or `?return_to=https://bar.myapp.com`.

```yaml file="path/to/my/kratos.config.yml"
selfservice:
whitelisted_return_urls:
- https://*.myapp.com/
```

Please remember that adding wildcards for top level domains is blocked (to prevent
Open Redirect Attacks).If we try to add a wild card for a top level domain we'll get
an error: `Ignoring wildcard ...`. So adding something like this won't work:

```yaml file="path/to/my/kratos.config.yml"
selfservice:
whitelisted_return_urls:
- https://*.com/
```

### Post-Login Redirection

Post-login redirection considers the following configuration keys:
Expand Down
14 changes: 14 additions & 0 deletions driver/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import (
"testing"
"time"

"golang.org/x/net/publicsuffix"

"github.com/duo-labs/webauthn/protocol"

"github.com/duo-labs/webauthn/webauthn"
Expand Down Expand Up @@ -808,6 +810,18 @@ func (p *Config) SelfServiceBrowserWhitelistedReturnToDomains() (us []url.URL) {
p.l.WithError(err).Warnf("Ignoring URL \"%s\" from configuration key \"%s.%d\".", u, ViperKeyURLsWhitelistedReturnToDomains, k)
continue
}
if parsed.Host == "*" {
p.l.Warnf("Ignoring wildcard \"%s\" from configuration key \"%s.%d\".", u, ViperKeyURLsWhitelistedReturnToDomains, k)
continue
}
eTLD, icann := publicsuffix.PublicSuffix(parsed.Host)
if len(parsed.Host) > 0 &&
parsed.Host[:1] == "*" &&
icann &&
parsed.Host == fmt.Sprintf("*.%s", eTLD) {
p.l.Warnf("Ignoring wildcard \"%s\" from configuration key \"%s.%d\".", u, ViperKeyURLsWhitelistedReturnToDomains, k)
continue
}

us = append(us, *parsed)
}
Expand Down
1 change: 1 addition & 0 deletions driver/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ func TestViperProvider(t *testing.T) {
assert.Equal(t, []string{
"http://return-to-1-test.ory.sh/",
"http://return-to-2-test.ory.sh/",
"http://*.wildcards.ory.sh",
"/return-to-relative-test/",
}, ds)

Expand Down
5 changes: 5 additions & 0 deletions driver/config/stub/.kratos.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ selfservice:
whitelisted_return_urls:
- http://return-to-1-test.ory.sh/
- http://return-to-2-test.ory.sh/
- http://*.wildcards.ory.sh
- http://*.sh
- http://*.com
- http://*.com.pl
- http://*
- /return-to-relative-test/
methods:
totp:
Expand Down
2 changes: 2 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ require (
github.com/tidwall/sjson v1.2.2
github.com/urfave/negroni v1.0.0
golang.org/x/crypto v0.0.0-20210921155107-089bfa567519
golang.org/x/mod v0.5.1 // indirect
golang.org/x/net v0.0.0-20211020060615-d418f374d309
golang.org/x/oauth2 v0.0.0-20210514164344-f6687ab2804c
golang.org/x/sync v0.0.0-20210220032951-036812b2e83c
golang.org/x/tools v0.1.7
Expand Down
10 changes: 9 additions & 1 deletion x/http_secure_redirect.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,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 {
if allowed.Host != "" && allowed.Host[:1] == "*" {
return strings.HasSuffix(strings.ToLower(returnTo.Host), strings.ToLower(allowed.Host)[1:])
}
return strings.EqualFold(allowed.Host, returnTo.Host)
}

// SecureRedirectTo implements a HTTP redirector who mitigates open redirect vulnerabilities by
// working with whitelisting.
func SecureRedirectTo(r *http.Request, defaultReturnTo *url.URL, opts ...SecureRedirectOption) (returnTo *url.URL, err error) {
Expand Down Expand Up @@ -89,7 +97,7 @@ func SecureRedirectTo(r *http.Request, defaultReturnTo *url.URL, opts ...SecureR
var found bool
for _, allowed := range o.whitelist {
if strings.EqualFold(allowed.Scheme, returnTo.Scheme) &&
strings.EqualFold(allowed.Host, returnTo.Host) &&
SecureRedirectToIsWhiteListedHost(returnTo, allowed) &&
strings.HasPrefix(
stringsx.Coalesce(returnTo.Path, "/"),
stringsx.Coalesce(allowed.Path, "/")) {
Expand Down
25 changes: 25 additions & 0 deletions x/http_secure_redirect_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,31 @@ func TestSecureContentNegotiationRedirection(t *testing.T) {
})
}

func TestSecureRedirectToIsWhiteListedHost(t *testing.T) {
type testCase struct {
whitelistedURL string
redirectURL string
valid bool
}
tests := map[string]testCase{
"case=Domain is whitelisted": {whitelistedURL: "https://foo.bar", redirectURL: "https://foo.bar/redir", valid: true},
"case=Domain prefix is whitelisted": {whitelistedURL: "https://*.bar", redirectURL: "https://foo.bar/redir", valid: true},
"case=Subdomain prefix is whitelisted": {whitelistedURL: "https://*.foo.bar", redirectURL: "https://auth.foo.bar/redir", valid: true},
"case=Domain is not whitelisted": {whitelistedURL: "https://foo.baz", redirectURL: "https://foo.bar/redir", valid: false},
"case=Domain wildcard is not whitelisted": {whitelistedURL: "https://*.foo.baz", redirectURL: "https://foo.bar/redir", valid: false},
"case=Subdomain is not whitelisted": {whitelistedURL: "https://*.foo.baz", redirectURL: "https://auth.foo.bar/redir", valid: false},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
whitelistedURL, err := url.Parse(tc.whitelistedURL)
require.NoError(t, err)
redirectURL, err := url.Parse(tc.redirectURL)
require.NoError(t, err)
assert.Equal(t, x.SecureRedirectToIsWhiteListedHost(redirectURL, *whitelistedURL), tc.valid)
})
}
}

func TestSecureRedirectTo(t *testing.T) {

var newServer = func(t *testing.T, isTLS bool, isRelative bool, expectErr bool, opts func(ts *httptest.Server) []x.SecureRedirectOption) *httptest.Server {
Expand Down

0 comments on commit 349cdcf

Please sign in to comment.