From 162073b33e46e502696b47ee97f871b944540114 Mon Sep 17 00:00:00 2001
From: Martin Heide <martin.heide@faro.com>
Date: Mon, 2 Nov 2020 14:05:47 +0000
Subject: [PATCH] No longer allow desktop/mobile redirect URIs implicitly if
 RedirectURIs is set

Signed-off-by: Martin Heide <martin.heide@faro.com>
---
 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.