diff --git a/server/oauth2.go b/server/oauth2.go index 79e54c32..18146d61 100644 --- a/server/oauth2.go +++ b/server/oauth2.go @@ -588,12 +588,16 @@ 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 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 8db9ea59..c926a3e1 100644 --- a/server/oauth2_test.go +++ b/server/oauth2_test.go @@ -340,7 +340,9 @@ func TestValidRedirectURI(t *testing.T) { RedirectURIs: []string{"http://foo.com/bar"}, }, redirectURI: "http://foo.com/bar/baz", + wantValid: false, }, + // These special desktop + device + localhost URIs are allowed by default. { client: storage.Client{ Public: true, @@ -348,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, @@ -369,6 +378,113 @@ 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, + }, + // 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: false, + }, + { + client: storage.Client{ + Public: true, + RedirectURIs: []string{"http://foo.com/bar"}, + }, + redirectURI: "/device/callback", + wantValid: false, + }, + { + client: storage.Client{ + Public: true, + RedirectURIs: []string{"http://foo.com/bar"}, + }, + redirectURI: "http://localhost:8080/", + wantValid: false, + }, + { + client: storage.Client{ + Public: true, + RedirectURIs: []string{"http://foo.com/bar"}, + }, + redirectURI: "http://localhost:991/bar", + wantValid: false, + }, + { + client: storage.Client{ + Public: true, + 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. + { + client: storage.Client{ + Public: true, + }, + redirectURI: "http://foo.com/bar", + wantValid: false, + }, { client: storage.Client{ Public: true,