From fd63f537dfc8399330f16c47599ea2e9947972ee Mon Sep 17 00:00:00 2001
From: Tom Clegg <tom@curii.com>
Date: Mon, 27 Mar 2023 11:38:11 -0400
Subject: [PATCH] 20264: Ignore superfluous :443 and :80 in trusted origin
 check.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curii.com>
---
 lib/controller/localdb/login.go           | 32 ++++++++++----
 lib/controller/localdb/login_oidc_test.go |  2 +-
 lib/controller/localdb/login_test.go      | 54 +++++++++++++++++++++++
 3 files changed, 78 insertions(+), 10 deletions(-)
 create mode 100644 lib/controller/localdb/login_test.go

diff --git a/lib/controller/localdb/login.go b/lib/controller/localdb/login.go
index a1ac2c55b0..041336bcac 100644
--- a/lib/controller/localdb/login.go
+++ b/lib/controller/localdb/login.go
@@ -173,16 +173,14 @@ func validateLoginRedirectTarget(cluster *arvados.Cluster, returnTo string) erro
 	if err != nil {
 		return err
 	}
-	if u.Port() == "80" && u.Scheme == "http" {
-		u.Host = u.Hostname()
-	} else if u.Port() == "443" && u.Scheme == "https" {
-		u.Host = u.Hostname()
-	}
-	if _, ok := cluster.Login.TrustedClients[arvados.URL(*u)]; ok {
-		return nil
+	target := origin(*u)
+	for trusted := range cluster.Login.TrustedClients {
+		if origin(url.URL(trusted)) == target {
+			return nil
+		}
 	}
-	if u.String() == cluster.Services.Workbench1.ExternalURL.String() ||
-		u.String() == cluster.Services.Workbench2.ExternalURL.String() {
+	if target == origin(url.URL(cluster.Services.Workbench1.ExternalURL)) ||
+		target == origin(url.URL(cluster.Services.Workbench2.ExternalURL)) {
 		return nil
 	}
 	if cluster.Login.TrustPrivateNetworks {
@@ -199,3 +197,19 @@ func validateLoginRedirectTarget(cluster *arvados.Cluster, returnTo string) erro
 	}
 	return fmt.Errorf("requesting site is not listed in TrustedClients config")
 }
+
+// origin returns the canonical origin of a URL, e.g.,
+// origin("https://example:443/foo") returns "https://example/"
+func origin(u url.URL) string {
+	origin := url.URL{
+		Scheme: u.Scheme,
+		Host:   u.Host,
+		Path:   "/",
+	}
+	if origin.Port() == "80" && origin.Scheme == "http" {
+		origin.Host = origin.Hostname()
+	} else if origin.Port() == "443" && origin.Scheme == "https" {
+		origin.Host = origin.Hostname()
+	}
+	return origin.String()
+}
diff --git a/lib/controller/localdb/login_oidc_test.go b/lib/controller/localdb/login_oidc_test.go
index cf9cf30eca..9469fdfd31 100644
--- a/lib/controller/localdb/login_oidc_test.go
+++ b/lib/controller/localdb/login_oidc_test.go
@@ -44,7 +44,7 @@ type OIDCLoginSuite struct {
 }
 
 func (s *OIDCLoginSuite) SetUpTest(c *check.C) {
-	s.trustedURL = &arvados.URL{Scheme: "https", Host: "app.example.com", Path: "/"}
+	s.trustedURL = &arvados.URL{Scheme: "https", Host: "app.example.com:443", Path: "/"}
 
 	s.fakeProvider = arvadostest.NewOIDCProvider(c)
 	s.fakeProvider.AuthEmail = "active-user@arvados.local"
diff --git a/lib/controller/localdb/login_test.go b/lib/controller/localdb/login_test.go
new file mode 100644
index 0000000000..edd558a552
--- /dev/null
+++ b/lib/controller/localdb/login_test.go
@@ -0,0 +1,54 @@
+// Copyright (C) The Arvados Authors. All rights reserved.
+//
+// SPDX-License-Identifier: AGPL-3.0
+
+package localdb
+
+import (
+	"encoding/json"
+
+	"git.arvados.org/arvados.git/sdk/go/arvados"
+	check "gopkg.in/check.v1"
+)
+
+var _ = check.Suite(&loginSuite{})
+
+type loginSuite struct{}
+
+func (s *loginSuite) TestValidateLoginRedirectTarget(c *check.C) {
+	var cluster arvados.Cluster
+	for _, trial := range []struct {
+		pass    bool
+		wb1     string
+		wb2     string
+		trusted string
+		target  string
+	}{
+		{true, "https://wb1.example/", "https://wb2.example/", "", "https://wb2.example/"},
+		{true, "https://wb1.example:443/", "https://wb2.example:443/", "", "https://wb2.example/"},
+		{true, "https://wb1.example:443/", "https://wb2.example:443/", "", "https://wb2.example"},
+		{true, "https://wb1.example:443", "https://wb2.example:443", "", "https://wb2.example/"},
+		{true, "http://wb1.example:80/", "http://wb2.example:80/", "", "http://wb2.example/"},
+		{false, "https://wb1.example:80/", "https://wb2.example:80/", "", "https://wb2.example/"},
+		{false, "https://wb1.example:1234/", "https://wb2.example:1234/", "", "https://wb2.example/"},
+		{false, "https://wb1.example/", "https://wb2.example/", "", "https://bad.wb2.example/"},
+		{true, "https://wb1.example/", "https://wb2.example/", "https://good.wb2.example/", "https://good.wb2.example"},
+		{true, "https://wb1.example/", "https://wb2.example/", "https://good.wb2.example:443/", "https://good.wb2.example"},
+		{true, "https://wb1.example/", "https://wb2.example/", "https://good.wb2.example:443", "https://good.wb2.example/"},
+	} {
+		c.Logf("trial %+v", trial)
+		// We use json.Unmarshal() to load the test strings
+		// because we're testing behavior when the config file
+		// contains string X.
+		err := json.Unmarshal([]byte(`"`+trial.wb1+`"`), &cluster.Services.Workbench1.ExternalURL)
+		c.Assert(err, check.IsNil)
+		err = json.Unmarshal([]byte(`"`+trial.wb2+`"`), &cluster.Services.Workbench2.ExternalURL)
+		c.Assert(err, check.IsNil)
+		if trial.trusted != "" {
+			err = json.Unmarshal([]byte(`{"`+trial.trusted+`": {}}`), &cluster.Login.TrustedClients)
+			c.Assert(err, check.IsNil)
+		}
+		err = validateLoginRedirectTarget(&cluster, trial.target)
+		c.Check(err == nil, check.Equals, trial.pass)
+	}
+}
-- 
2.30.2