From 44bd70712c7e1fb9c2c33ac64fe1006f620c418b Mon Sep 17 00:00:00 2001
From: James Elliott <james-d-elliott@users.noreply.github.com>
Date: Fri, 8 Apr 2022 17:38:38 +1000
Subject: [PATCH] fix(configuration): sector identifier not parsed correctly
 (#3142)

This fixes an issue preventing the sector identifier for OpenID Connect clients from being parsed.
---
 .../validator/identity_providers.go           |  4 ++
 .../validator/identity_providers_test.go      | 28 +++++++++++
 internal/utils/strings.go                     | 21 ++++++++
 internal/utils/strings_test.go                | 50 +++++++++++++++++++
 4 files changed, 103 insertions(+)

diff --git a/internal/configuration/validator/identity_providers.go b/internal/configuration/validator/identity_providers.go
index 85f4b9db5..a51c820ed 100644
--- a/internal/configuration/validator/identity_providers.go
+++ b/internal/configuration/validator/identity_providers.go
@@ -171,6 +171,10 @@ func validateOIDCClients(config *schema.OpenIDConnectConfiguration, validator *s
 
 func validateOIDCClientSectorIdentifier(client schema.OpenIDConnectClientConfiguration, validator *schema.StructValidator) {
 	if client.SectorIdentifier.String() != "" {
+		if utils.IsURLHostComponent(client.SectorIdentifier) || utils.IsURLHostComponentWithPort(client.SectorIdentifier) {
+			return
+		}
+
 		if client.SectorIdentifier.Scheme != "" {
 			validator.Push(fmt.Errorf(errFmtOIDCClientInvalidSectorIdentifier, client.ID, client.SectorIdentifier.String(), client.SectorIdentifier.Host, "scheme", client.SectorIdentifier.Scheme))
 
diff --git a/internal/configuration/validator/identity_providers_test.go b/internal/configuration/validator/identity_providers_test.go
index 1b524ff27..13d138bba 100644
--- a/internal/configuration/validator/identity_providers_test.go
+++ b/internal/configuration/validator/identity_providers_test.go
@@ -245,6 +245,34 @@ func TestShouldRaiseErrorWhenOIDCServerClientBadValues(t *testing.T) {
 				fmt.Sprintf(errFmtOIDCClientRedirectURIAbsolute, "client-check-uri-abs", "google.com"),
 			},
 		},
+		{
+			Name: "ValidSectorIdentifier",
+			Clients: []schema.OpenIDConnectClientConfiguration{
+				{
+					ID:     "client-valid-sector",
+					Secret: "a-secret",
+					Policy: policyTwoFactor,
+					RedirectURIs: []string{
+						"https://google.com",
+					},
+					SectorIdentifier: mustParseURL("example.com"),
+				},
+			},
+		},
+		{
+			Name: "ValidSectorIdentifierWithPort",
+			Clients: []schema.OpenIDConnectClientConfiguration{
+				{
+					ID:     "client-valid-sector",
+					Secret: "a-secret",
+					Policy: policyTwoFactor,
+					RedirectURIs: []string{
+						"https://google.com",
+					},
+					SectorIdentifier: mustParseURL("example.com:2000"),
+				},
+			},
+		},
 		{
 			Name: "InvalidSectorIdentifierInvalidURL",
 			Clients: []schema.OpenIDConnectClientConfiguration{
diff --git a/internal/utils/strings.go b/internal/utils/strings.go
index 7fb6b8eec..d776eb7e3 100644
--- a/internal/utils/strings.go
+++ b/internal/utils/strings.go
@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"math/rand"
 	"net/url"
+	"strconv"
 	"strings"
 	"time"
 	"unicode"
@@ -287,6 +288,26 @@ func JoinAndCanonicalizeHeaders(sep []byte, headers ...string) (joined []byte) {
 	return joined
 }
 
+// IsURLHostComponent returns true if the provided url.URL that was parsed from a string to a url.URL via url.Parse is
+// just a hostname. This is needed because of the way this function parses such strings.
+func IsURLHostComponent(u url.URL) (isHostComponent bool) {
+	return u.Path != "" && u.Scheme == "" && u.Host == "" && u.RawPath == "" && u.Opaque == "" &&
+		u.RawQuery == "" && u.Fragment == "" && u.RawFragment == ""
+}
+
+// IsURLHostComponentWithPort returns true if the provided url.URL that was parsed from a string to a url.URL via
+// url.Parse is just a hostname with a port. This is needed because of the way this function parses such strings.
+func IsURLHostComponentWithPort(u url.URL) (isHostComponentWithPort bool) {
+	if u.Opaque != "" && u.Scheme != "" && u.Host == "" && u.Path == "" && u.RawPath == "" &&
+		u.RawQuery == "" && u.Fragment == "" && u.RawFragment == "" {
+		_, err := strconv.Atoi(u.Opaque)
+
+		return err == nil
+	}
+
+	return false
+}
+
 func init() {
 	rand.Seed(time.Now().UnixNano())
 }
diff --git a/internal/utils/strings_test.go b/internal/utils/strings_test.go
index 6b1b5e7cd..5d8e5f01a 100644
--- a/internal/utils/strings_test.go
+++ b/internal/utils/strings_test.go
@@ -262,3 +262,53 @@ func TestJoinAndCanonicalizeHeaders(t *testing.T) {
 
 	assert.Equal(t, []byte("X-Example-One, X-Egg-Two"), result)
 }
+
+func TestIsURLHostComponent(t *testing.T) {
+	testCases := []struct {
+		desc, have           string
+		expectedA, expectedB bool
+	}{
+		{
+			desc:      "ShouldBeFalseWithScheme",
+			have:      "https://google.com",
+			expectedA: false, expectedB: false,
+		},
+		{
+			desc:      "ShouldBeTrueForHostComponentButFalseForWithPort",
+			have:      "google.com",
+			expectedA: true, expectedB: false,
+		},
+		{
+			desc:      "ShouldBeFalseForHostComponentButTrueForWithPort",
+			have:      "google.com:8000",
+			expectedA: false, expectedB: true,
+		},
+		{
+			desc:      "ShouldBeFalseWithPath",
+			have:      "google.com:8000/path",
+			expectedA: false, expectedB: false,
+		},
+		{
+			desc:      "ShouldBeFalseWithFragment",
+			have:      "google.com:8000#test",
+			expectedA: false, expectedB: false,
+		},
+		{
+			desc:      "ShouldBeFalseWithQuery",
+			have:      "google.com:8000?test=1",
+			expectedA: false, expectedB: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			u, err := url.Parse(tc.have)
+
+			require.NoError(t, err)
+			require.NotNil(t, u)
+
+			assert.Equal(t, tc.expectedA, IsURLHostComponent(*u))
+			assert.Equal(t, tc.expectedB, IsURLHostComponentWithPort(*u))
+		})
+	}
+}