Skip to content

Commit

Permalink
fix: make redirect URL checking more strict
Browse files Browse the repository at this point in the history 
The OAuth 2.0 Client's Redirect URL and the Redirect URL used in the OAuth 2.0 flow do not check if the query string is equal:

1. Registering a client with allowed redirect URL `https://example.com/callback`
2. Performing OAuth2 flow and requesting redirect URL `https://example.com/callback?bar=foo`
3. Instead of an error, the browser is redirected to `https://example.com/callback?bar=foo` with a potentially successful OAuth2 response.

Additionally, matching Redirect URLs used `strings.ToLower` normalization:

1. Registering a client with allowed redirect URL `https://example.com/callback`
2. Performing OAuth2 flow and requesting redirect URL `https://example.com/CALLBACK`
3. Instead of an error, the browser is redirected to `https://example.com/CALLBACK ` with a potentially successful OAuth2 response.

This patch addresses all of these issues and adds regression tests to keep the implementation secure in future releases.
  • Loading branch information
@aeneasr
aeneasr committed Oct 2, 2020
1 parent eb87048 commit cdee51e
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 27 deletions.
24 changes: 17 additions & 7 deletions authorize_helper.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,24 +118,34 @@ func isMatchingRedirectURI(uri string, haystack []string) bool {
}

for _, b := range haystack {
if strings.ToLower(b) == strings.ToLower(uri) || isLoopbackURI(requested, b) {
if b == uri || isMatchingAsLoopback(requested, b) {
return true
}
}
return false
}

func isLoopbackURI(requested *url.URL, registeredURI string) bool {
func isMatchingAsLoopback(requested *url.URL, registeredURI string) bool {
registered, err := url.Parse(registeredURI)
if err != nil {
return false
}

if registered.Scheme != "http" || !isLoopbackAddress(registered.Host) {
return false
}

if requested.Scheme == "http" && isLoopbackAddress(requested.Host) && registered.Path == requested.Path {
// Native apps that are able to open a port on the loopback network
// interface without needing special permissions (typically, those on
// desktop operating systems) can use the loopback interface to receive
// the OAuth redirect.
//
// Loopback redirect URIs use the "http" scheme and are constructed with
// the loopback IP literal and whatever port the client is listening on.
//
// Source: https://tools.ietf.org/html/rfc8252#section-7.3
if requested.Scheme == "http" &&
isLoopbackAddress(requested.Host) &&
registered.Hostname() == requested.Hostname() &&
// The port is skipped here - see codedoc above!
registered.Path == requested.Path &&
registered.RawQuery == requested.RawQuery {
return true
}

Expand Down
62 changes: 43 additions & 19 deletions authorize_helper_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,18 +137,6 @@ func TestDoesClientWhiteListRedirect(t *testing.T) {
isError: false,
expected: "https://bar.com/cb",
},
{
client: &DefaultClient{RedirectURIs: []string{"https://bar.Com/cb"}},
url: "https://bar.com/cb",
isError: false,
expected: "https://bar.com/cb",
},
{
client: &DefaultClient{RedirectURIs: []string{"https://bar.com/cb"}},
url: "https://bar.Com/cb",
isError: false,
expected: "https://bar.Com/cb",
},
{
client: &DefaultClient{RedirectURIs: []string{"https://bar.com/cb"}},
url: "https://bar.com/cb123",
Expand All @@ -157,8 +145,8 @@ func TestDoesClientWhiteListRedirect(t *testing.T) {
{
client: &DefaultClient{RedirectURIs: []string{"http://[::1]"}},
url: "http://[::1]:1024",
isError: false,
expected: "http://[::1]:1024",
isError: false,
},
{
client: &DefaultClient{RedirectURIs: []string{"http://[::1]"}},
Expand All @@ -168,8 +156,8 @@ func TestDoesClientWhiteListRedirect(t *testing.T) {
{
client: &DefaultClient{RedirectURIs: []string{"http://[::1]/cb"}},
url: "http://[::1]:1024/cb",
isError: false,
expected: "http://[::1]:1024/cb",
isError: false,
},
{
client: &DefaultClient{RedirectURIs: []string{"http://[::1]"}},
Expand All @@ -179,14 +167,14 @@ func TestDoesClientWhiteListRedirect(t *testing.T) {
{
client: &DefaultClient{RedirectURIs: []string{"http://127.0.0.1"}},
url: "http://127.0.0.1:1024",
isError: false,
expected: "http://127.0.0.1:1024",
isError: false,
},
{
client: &DefaultClient{RedirectURIs: []string{"http://127.0.0.1/cb"}},
url: "http://127.0.0.1:64000/cb",
isError: false,
expected: "http://127.0.0.1:64000/cb",
isError: false,
},
{
client: &DefaultClient{RedirectURIs: []string{"http://127.0.0.1"}},
Expand All @@ -196,14 +184,14 @@ func TestDoesClientWhiteListRedirect(t *testing.T) {
{
client: &DefaultClient{RedirectURIs: []string{"http://127.0.0.1"}},
url: "http://127.0.0.1",
isError: false,
expected: "http://127.0.0.1",
isError: false,
},
{
client: &DefaultClient{RedirectURIs: []string{"http://127.0.0.1/Cb"}},
url: "http://127.0.0.1:8080/Cb",
isError: false,
expected: "http://127.0.0.1:8080/Cb",
isError: false,
},
{
client: &DefaultClient{RedirectURIs: []string{"http://127.0.0.1"}},
Expand All @@ -215,9 +203,45 @@ func TestDoesClientWhiteListRedirect(t *testing.T) {
url: ":/invalid.uri)bar",
isError: true,
},
{
client: &DefaultClient{RedirectURIs: []string{"http://127.0.0.1:8080/cb"}},
url: "http://127.0.0.1:8080/Cb",
isError: true,
},
{
client: &DefaultClient{RedirectURIs: []string{"http://127.0.0.1:8080/cb"}},
url: "http://127.0.0.1:8080/cb?foo=bar",
isError: true,
},
{
client: &DefaultClient{RedirectURIs: []string{"http://127.0.0.1:8080/cb?foo=bar"}},
url: "http://127.0.0.1:8080/cb?foo=bar",
expected: "http://127.0.0.1:8080/cb?foo=bar",
isError: false,
},
{
client: &DefaultClient{RedirectURIs: []string{"http://127.0.0.1:8080/cb?foo=bar"}},
url: "http://127.0.0.1:8080/cb?baz=bar&foo=bar",
isError: true,
},
{
client: &DefaultClient{RedirectURIs: []string{"http://127.0.0.1:8080/cb?foo=bar&baz=bar"}},
url: "http://127.0.0.1:8080/cb?baz=bar&foo=bar",
isError: true,
},
{
client: &DefaultClient{RedirectURIs: []string{"https://www.ory.sh/cb"}},
url: "http://127.0.0.1:8080/cb",
isError: true,
},
{
client: &DefaultClient{RedirectURIs: []string{"http://127.0.0.1:8080/cb"}},
url: "https://www.ory.sh/cb",
isError: true,
},
} {
redir, err := MatchRedirectURIWithClientRedirectURIs(c.url, c.client)
assert.Equal(t, c.isError, err != nil, "%d: %s", k, err)
assert.Equal(t, c.isError, err != nil, "%d: %+v", k, c)
if err == nil {
require.NotNil(t, redir, "%d", k)
assert.Equal(t, c.expected, redir.String(), "%d", k)
Expand Down
2 changes: 1 addition & 1 deletion helper_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func TestEscapeJSONString(t *testing.T) {
for _, str := range []string{"", "foobar", `foo"bar`, `foo\bar`, "foo\n\tbar"} {
escaped := EscapeJSONString(str)
var unmarshaled string
err := json.Unmarshal([]byte(`"` + escaped + `"`), &unmarshaled)
err := json.Unmarshal([]byte(`"`+escaped+`"`), &unmarshaled)
require.NoError(t, err, str)
assert.Equal(t, str, unmarshaled, str)
}
Expand Down

0 comments on commit cdee51e

Please sign in to comment.