fix(auth): validate tenant ID and provider ID URL path segments

auth/provider_config.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"net/http"
 	"net/url"
+	"regexp"
 	"strconv"
 	"strings"
 
@@ -967,16 +968,25 @@ func (dao *samlProviderConfigDAO) toSAMLProviderConfig() *SAMLProviderConfig {
 	}
 }
 
+var safeURLSegmentPattern = regexp.MustCompile(`^[a-zA-Z0-9\-_.~]+$`)
+
+func isSafeURLSegment(segment string) bool {
+	if segment == "" || strings.HasSuffix(segment, ".") {
+		return false
+	}
+	return safeURLSegmentPattern.MatchString(segment)
+}
+
 func validateOIDCConfigID(id string) error {
-	if !strings.HasPrefix(id, "oidc.") {
+	if !strings.HasPrefix(id, "oidc.") || !isSafeURLSegment(id) {
 		return fmt.Errorf("invalid OIDC provider id: %q", id)
 	}
 
 	return nil
 }
 
 func validateSAMLConfigID(id string) error {
-	if !strings.HasPrefix(id, "saml.") {
+	if !strings.HasPrefix(id, "saml.") || !isSafeURLSegment(id) {
 		return fmt.Errorf("invalid SAML provider id: %q", id)
 	}
 

auth/provider_config_test.go

@@ -98,12 +98,22 @@ var invalidOIDCConfigIDs = []string{
 	"",
 	"invalid.id",
 	"saml.config",
+	"oidc.",
+	"oidc.config.",
+	"oidc.config/sub",
+	"oidc.config?param=1",
+	"oidc.config#frag",
 }
 
 var invalidSAMLConfigIDs = []string{
 	"",
 	"invalid.id",
 	"oidc.config",
+	"saml.",
+	"saml.config.",
+	"saml.config/sub",
+	"saml.config?param=1",
+	"saml.config#frag",
 }
 
 func TestOIDCProviderConfig(t *testing.T) {
@@ -483,12 +493,11 @@ func TestUpdateOIDCProviderConfigZeroValues(t *testing.T) {
 }
 
 func TestUpdateOIDCProviderConfigInvalidID(t *testing.T) {
-	cases := []string{"", "saml.config"}
 	client := &baseClient{}
 	options := (&OIDCProviderConfigToUpdate{}).
 		DisplayName("")
 	want := "invalid OIDC provider id: "
-	for _, tc := range cases {
+	for _, tc := range invalidOIDCConfigIDs {
 		_, err := client.UpdateOIDCProviderConfig(context.Background(), tc, options)
 		if err == nil || !strings.HasPrefix(err.Error(), want) {
 			t.Errorf("UpdateOIDCProviderConfig(%q) = %v; want = %q", tc, err, want)
@@ -1103,14 +1112,13 @@ func TestUpdateSAMLProviderConfigZeroValues(t *testing.T) {
 }
 
 func TestUpdateSAMLProviderConfigInvalidID(t *testing.T) {
-	cases := []string{"", "oidc.config"}
 	client := &baseClient{}
 	options := (&SAMLProviderConfigToUpdate{}).
 		DisplayName("").
 		Enabled(false).
 		RequestSigningEnabled(false)
 	want := "invalid SAML provider id: "
-	for _, tc := range cases {
+	for _, tc := range invalidSAMLConfigIDs {
 		_, err := client.UpdateSAMLProviderConfig(context.Background(), tc, options)
 		if err == nil || !strings.HasPrefix(err.Error(), want) {
 			t.Errorf("UpdateSAMLProviderConfig(%q) = %v; want = %q", tc, err, want)

auth/tenant_mgt.go

@@ -94,8 +94,8 @@ func newTenantManager(client *internal.HTTPClient, conf *internal.AuthConfig, ba
 
 // AuthForTenant creates a new TenantClient scoped to a given tenantID.
 func (tm *TenantManager) AuthForTenant(tenantID string) (*TenantClient, error) {
-	if tenantID == "" {
-		return nil, errors.New("tenantID must not be empty")
+	if err := validateTenantID(tenantID); err != nil {
+		return nil, err
 	}
 
 	return &TenantClient{
@@ -105,8 +105,8 @@ func (tm *TenantManager) AuthForTenant(tenantID string) (*TenantClient, error) {
 
 // Tenant returns the tenant with the given ID.
 func (tm *TenantManager) Tenant(ctx context.Context, tenantID string) (*Tenant, error) {
-	if tenantID == "" {
-		return nil, errors.New("tenantID must not be empty")
+	if err := validateTenantID(tenantID); err != nil {
+		return nil, err
 	}
 
 	req := &internal.Request{
@@ -146,8 +146,8 @@ func (tm *TenantManager) CreateTenant(ctx context.Context, tenant *TenantToCreat
 
 // UpdateTenant updates an existing tenant with the given options.
 func (tm *TenantManager) UpdateTenant(ctx context.Context, tenantID string, tenant *TenantToUpdate) (*Tenant, error) {
-	if tenantID == "" {
-		return nil, errors.New("tenantID must not be empty")
+	if err := validateTenantID(tenantID); err != nil {
+		return nil, err
 	}
 	if tenant == nil {
 		return nil, errors.New("tenant must not be nil")
@@ -179,8 +179,8 @@ func (tm *TenantManager) UpdateTenant(ctx context.Context, tenantID string, tena
 
 // DeleteTenant deletes the tenant with the given ID.
 func (tm *TenantManager) DeleteTenant(ctx context.Context, tenantID string) error {
-	if tenantID == "" {
-		return errors.New("tenantID must not be empty")
+	if err := validateTenantID(tenantID); err != nil {
+		return err
 	}
 
 	req := &internal.Request{
@@ -191,6 +191,16 @@ func (tm *TenantManager) DeleteTenant(ctx context.Context, tenantID string) erro
 	return err
 }
 
+func validateTenantID(tenantID string) error {
+	if tenantID == "" {
+		return errors.New("tenantID must not be empty")
+	}
+	if !isSafeURLSegment(tenantID) {
+		return fmt.Errorf("invalid tenantID: %q", tenantID)
+	}
+	return nil
+}
+
 // Tenants returns an iterator over tenants in the project.
 //
 // If nextPageToken is empty, the iterator will start at the beginning. Otherwise,

auth/tenant_mgt_test.go

@@ -41,6 +41,28 @@ func TestAuthForTenantEmptyTenantID(t *testing.T) {
 	}
 }
 
+var invalidTenantIDs = []string{
+	".",
+	"..",
+	"tenant.",
+	"tenant/sub",
+	"tenant?param=1",
+	"tenant#frag",
+}
+
+func TestAuthForTenantInvalidTenantID(t *testing.T) {
+	s := echoServer(testGetUserResponse, t)
+	defer s.Close()
+
+	for _, id := range invalidTenantIDs {
+		wantErr := fmt.Sprintf("invalid tenantID: %q", id)
+		client, err := s.Client.TenantManager.AuthForTenant(id)
+		if client != nil || err == nil || err.Error() != wantErr {
+			t.Errorf("AuthForTenant(%q) = (%v, %v); want = (nil, %q)", id, client, err, wantErr)
+		}
+	}
+}
+
 func TestTenantID(t *testing.T) {
 	s := echoServer(testGetUserResponse, t)
 	defer s.Close()
@@ -1250,6 +1272,17 @@ func TestTenantEmptyID(t *testing.T) {
 	}
 }
 
+func TestTenantInvalidID(t *testing.T) {
+	tm := &TenantManager{}
+	for _, id := range invalidTenantIDs {
+		wantErr := fmt.Sprintf("invalid tenantID: %q", id)
+		tenant, err := tm.Tenant(context.Background(), id)
+		if tenant != nil || err == nil || err.Error() != wantErr {
+			t.Errorf("Tenant(%q) = (%v, %v); want = (nil, %q)", id, tenant, err, wantErr)
+		}
+	}
+}
+
 func TestTenantError(t *testing.T) {
 	s := echoServer([]byte(tenantNotFoundResponse), t)
 	defer s.Close()
@@ -1502,6 +1535,17 @@ func TestUpdateTenantEmptyID(t *testing.T) {
 	}
 }
 
+func TestUpdateTenantInvalidID(t *testing.T) {
+	tm := &TenantManager{}
+	options := (&TenantToUpdate{}).DisplayName("")
+	for _, id := range invalidTenantIDs {
+		wantErr := fmt.Sprintf("invalid tenantID: %q", id)
+		if _, err := tm.UpdateTenant(context.Background(), id, options); err == nil || err.Error() != wantErr {
+			t.Errorf("UpdateTenant(%q) = %v, want = %q", id, err, wantErr)
+		}
+	}
+}
+
 func TestUpdateTenantNilOptions(t *testing.T) {
 	tm := &TenantManager{}
 	want := "tenant must not be nil"
@@ -1548,6 +1592,17 @@ func TestDeleteTenantEmptyID(t *testing.T) {
 	}
 }
 
+func TestDeleteTenantInvalidID(t *testing.T) {
+	tm := &TenantManager{}
+	for _, id := range invalidTenantIDs {
+		wantErr := fmt.Sprintf("invalid tenantID: %q", id)
+		err := tm.DeleteTenant(context.Background(), id)
+		if err == nil || err.Error() != wantErr {
+			t.Errorf("DeleteTenant(%q) = %v; want = %q", id, err, wantErr)
+		}
+	}
+}
+
 func TestDeleteTenantError(t *testing.T) {
 	s := echoServer([]byte(tenantNotFoundResponse), t)
 	defer s.Close()