From b894d9c88813ac876076b338c1e51126ea728ee9 Mon Sep 17 00:00:00 2001 From: Martin Heide Date: Mon, 5 Oct 2020 18:19:33 +0000 Subject: [PATCH 1/4] Allow public clients (e.g. using implicit flow or PKCE) to have redirect URIs configured Signed-off-by: Martin Heide --- server/oauth2.go | 13 ++++++++----- server/oauth2_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/server/oauth2.go b/server/oauth2.go index 79e54c32..9ae825ab 100644 --- a/server/oauth2.go +++ b/server/oauth2.go @@ -588,12 +588,15 @@ func (s *Server) validateCrossClientTrust(clientID, peerID string) (trusted bool } func validateRedirectURI(client storage.Client, redirectURI string) bool { - if !client.Public { - for _, uri := range client.RedirectURIs { - if redirectURI == uri { - return true - } + // Allow named RedirectURIs for both public and non-public clients. + // This is required make PKCE-enabled web apps work, when configured as public clients. + for _, uri := range client.RedirectURIs { + if redirectURI == uri { + return true } + } + // For non-public clients, only named RedirectURIs are allowed. + if !client.Public { return false } diff --git a/server/oauth2_test.go b/server/oauth2_test.go index 8db9ea59..f75015f0 100644 --- a/server/oauth2_test.go +++ b/server/oauth2_test.go @@ -340,6 +340,7 @@ func TestValidRedirectURI(t *testing.T) { RedirectURIs: []string{"http://foo.com/bar"}, }, redirectURI: "http://foo.com/bar/baz", + wantValid: false, }, { client: storage.Client{ @@ -369,6 +370,30 @@ func TestValidRedirectURI(t *testing.T) { redirectURI: "http://localhost", wantValid: true, }, + // Both Public + RedirectURIs configured: Could e.g. be a PKCE-enabled web app. + { + client: storage.Client{ + Public: true, + RedirectURIs: []string{"http://foo.com/bar"}, + }, + redirectURI: "http://foo.com/bar", + wantValid: true, + }, + { + client: storage.Client{ + Public: true, + RedirectURIs: []string{"http://foo.com/bar"}, + }, + redirectURI: "http://foo.com/bar/baz", + wantValid: false, + }, + { + client: storage.Client{ + Public: true, + }, + redirectURI: "http://foo.com/bar", + wantValid: false, + }, { client: storage.Client{ Public: true, From 1ea481bb73ef4ed565b8a62c3801fd723a1cb316 Mon Sep 17 00:00:00 2001 From: Martin Heide Date: Mon, 2 Nov 2020 12:52:52 +0000 Subject: [PATCH 2/4] Fix gofmt in oauth2_test.go Signed-off-by: Martin Heide --- server/oauth2_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/oauth2_test.go b/server/oauth2_test.go index f75015f0..1d9fa083 100644 --- a/server/oauth2_test.go +++ b/server/oauth2_test.go @@ -373,7 +373,7 @@ func TestValidRedirectURI(t *testing.T) { // Both Public + RedirectURIs configured: Could e.g. be a PKCE-enabled web app. { client: storage.Client{ - Public: true, + Public: true, RedirectURIs: []string{"http://foo.com/bar"}, }, redirectURI: "http://foo.com/bar", @@ -381,7 +381,7 @@ func TestValidRedirectURI(t *testing.T) { }, { client: storage.Client{ - Public: true, + Public: true, RedirectURIs: []string{"http://foo.com/bar"}, }, redirectURI: "http://foo.com/bar/baz", From c15e2887bc262a7dae2df741c7ba06f866272272 Mon Sep 17 00:00:00 2001 From: Martin Heide Date: Mon, 2 Nov 2020 13:41:56 +0000 Subject: [PATCH 3/4] Add oob, device and localhost redirect URI tests Signed-off-by: Martin Heide --- server/oauth2_test.go | 50 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/server/oauth2_test.go b/server/oauth2_test.go index 1d9fa083..997373fc 100644 --- a/server/oauth2_test.go +++ b/server/oauth2_test.go @@ -342,6 +342,7 @@ func TestValidRedirectURI(t *testing.T) { redirectURI: "http://foo.com/bar/baz", wantValid: false, }, + // These special desktop + device + localhost URIs are allowed by default. { client: storage.Client{ Public: true, @@ -349,6 +350,13 @@ func TestValidRedirectURI(t *testing.T) { redirectURI: "urn:ietf:wg:oauth:2.0:oob", wantValid: true, }, + { + client: storage.Client{ + Public: true, + }, + redirectURI: "/device/callback", + wantValid: true, + }, { client: storage.Client{ Public: true, @@ -387,6 +395,48 @@ func TestValidRedirectURI(t *testing.T) { redirectURI: "http://foo.com/bar/baz", wantValid: false, }, + // These special desktop + device + localhost URIs are allowed even when RedirectURIs is non-empty. + { + client: storage.Client{ + Public: true, + RedirectURIs: []string{"http://foo.com/bar"}, + }, + redirectURI: "urn:ietf:wg:oauth:2.0:oob", + wantValid: true, + }, + { + client: storage.Client{ + Public: true, + RedirectURIs: []string{"http://foo.com/bar"}, + }, + redirectURI: "/device/callback", + wantValid: true, + }, + { + client: storage.Client{ + Public: true, + RedirectURIs: []string{"http://foo.com/bar"}, + }, + redirectURI: "http://localhost:8080/", + wantValid: true, + }, + { + client: storage.Client{ + Public: true, + RedirectURIs: []string{"http://foo.com/bar"}, + }, + redirectURI: "http://localhost:991/bar", + wantValid: true, + }, + { + client: storage.Client{ + Public: true, + RedirectURIs: []string{"http://foo.com/bar"}, + }, + redirectURI: "http://localhost", + wantValid: true, + }, + // Non-localhost URIs are not allowed implicitly. { client: storage.Client{ Public: true, From 162073b33e46e502696b47ee97f871b944540114 Mon Sep 17 00:00:00 2001 From: Martin Heide Date: Mon, 2 Nov 2020 14:05:47 +0000 Subject: [PATCH 4/4] No longer allow desktop/mobile redirect URIs implicitly if RedirectURIs is set Signed-off-by: Martin Heide --- server/oauth2.go | 5 +++-- server/oauth2_test.go | 51 ++++++++++++++++++++++++++++++++++++++----- 2 files changed, 49 insertions(+), 7 deletions(-) diff --git a/server/oauth2.go b/server/oauth2.go index 9ae825ab..18146d61 100644 --- a/server/oauth2.go +++ b/server/oauth2.go @@ -595,8 +595,9 @@ func validateRedirectURI(client storage.Client, redirectURI string) bool { return true } } - // For non-public clients, only named RedirectURIs are allowed. - if !client.Public { + // For non-public clients or when RedirectURIs is set, we allow only explicitly named RedirectURIs. + // Otherwise, we check below for special URIs used for desktop or mobile apps. + if !client.Public || len(client.RedirectURIs) > 0 { return false } diff --git a/server/oauth2_test.go b/server/oauth2_test.go index 997373fc..c926a3e1 100644 --- a/server/oauth2_test.go +++ b/server/oauth2_test.go @@ -395,14 +395,14 @@ func TestValidRedirectURI(t *testing.T) { redirectURI: "http://foo.com/bar/baz", wantValid: false, }, - // These special desktop + device + localhost URIs are allowed even when RedirectURIs is non-empty. + // These special desktop + device + localhost URIs are not allowed implicitly when RedirectURIs is non-empty. { client: storage.Client{ Public: true, RedirectURIs: []string{"http://foo.com/bar"}, }, redirectURI: "urn:ietf:wg:oauth:2.0:oob", - wantValid: true, + wantValid: false, }, { client: storage.Client{ @@ -410,7 +410,7 @@ func TestValidRedirectURI(t *testing.T) { RedirectURIs: []string{"http://foo.com/bar"}, }, redirectURI: "/device/callback", - wantValid: true, + wantValid: false, }, { client: storage.Client{ @@ -418,7 +418,7 @@ func TestValidRedirectURI(t *testing.T) { RedirectURIs: []string{"http://foo.com/bar"}, }, redirectURI: "http://localhost:8080/", - wantValid: true, + wantValid: false, }, { client: storage.Client{ @@ -426,7 +426,7 @@ func TestValidRedirectURI(t *testing.T) { RedirectURIs: []string{"http://foo.com/bar"}, }, redirectURI: "http://localhost:991/bar", - wantValid: true, + wantValid: false, }, { client: storage.Client{ @@ -434,6 +434,47 @@ func TestValidRedirectURI(t *testing.T) { RedirectURIs: []string{"http://foo.com/bar"}, }, redirectURI: "http://localhost", + wantValid: false, + }, + // These special desktop + device + localhost URIs can still be specified explicitly. + { + client: storage.Client{ + Public: true, + RedirectURIs: []string{"http://foo.com/bar", "urn:ietf:wg:oauth:2.0:oob"}, + }, + redirectURI: "urn:ietf:wg:oauth:2.0:oob", + wantValid: true, + }, + { + client: storage.Client{ + Public: true, + RedirectURIs: []string{"http://foo.com/bar", "/device/callback"}, + }, + redirectURI: "/device/callback", + wantValid: true, + }, + { + client: storage.Client{ + Public: true, + RedirectURIs: []string{"http://foo.com/bar", "http://localhost:8080/"}, + }, + redirectURI: "http://localhost:8080/", + wantValid: true, + }, + { + client: storage.Client{ + Public: true, + RedirectURIs: []string{"http://foo.com/bar", "http://localhost:991/bar"}, + }, + redirectURI: "http://localhost:991/bar", + wantValid: true, + }, + { + client: storage.Client{ + Public: true, + RedirectURIs: []string{"http://foo.com/bar", "http://localhost"}, + }, + redirectURI: "http://localhost", wantValid: true, }, // Non-localhost URIs are not allowed implicitly.