From c85de6e75d905befa5038ae84ef27aa636b36a89 Mon Sep 17 00:00:00 2001 From: Matt Castelaz Date: Tue, 20 Oct 2026 15:53:57 +0000 Subject: [PATCH 1/2] fix(auth): validate tenant ID and provider ID URL path segments --- auth/provider_config.go | 14 +++++++-- auth/provider_config_test.go | 10 +++++++ auth/tenant_mgt.go | 26 +++++++++++------ auth/tenant_mgt_test.go | 55 ++++++++++++++++++++++++++++++++++++ 4 files changed, 95 insertions(+), 10 deletions(-) diff --git a/auth/provider_config.go b/auth/provider_config.go index c33eaf0d..7ebe2379 100644 --- a/auth/provider_config.go +++ b/auth/provider_config.go @@ -20,6 +20,7 @@ import ( "fmt" "net/http" "net/url" + "regexp" "strconv" "strings" @@ -967,8 +968,17 @@ 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.") || id == "oidc." || !isSafeURLSegment(id) { return fmt.Errorf("invalid OIDC provider id: %q", id) } @@ -976,7 +986,7 @@ func validateOIDCConfigID(id string) error { } func validateSAMLConfigID(id string) error { - if !strings.HasPrefix(id, "saml.") { + if !strings.HasPrefix(id, "saml.") || id == "saml." || !isSafeURLSegment(id) { return fmt.Errorf("invalid SAML provider id: %q", id) } diff --git a/auth/provider_config_test.go b/auth/provider_config_test.go index 4e2e2551..455c4466 100644 --- a/auth/provider_config_test.go +++ b/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) { diff --git a/auth/tenant_mgt.go b/auth/tenant_mgt.go index 35b27de1..b034a349 100644 --- a/auth/tenant_mgt.go +++ b/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, diff --git a/auth/tenant_mgt_test.go b/auth/tenant_mgt_test.go index 574c91f1..05ccbb61 100644 --- a/auth/tenant_mgt_test.go +++ b/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() From 40e3e81596b5ef1d166f1a7de7822c97170910ee Mon Sep 17 00:00:00 2001 From: Matt Castelaz Date: Tue, 20 Oct 2026 20:24:36 +0000 Subject: [PATCH 2/2] fix(auth): simplify provider ID validation and expand update tests --- auth/provider_config.go | 4 ++-- auth/provider_config_test.go | 6 ++---- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/auth/provider_config.go b/auth/provider_config.go index 7ebe2379..164be2a2 100644 --- a/auth/provider_config.go +++ b/auth/provider_config.go @@ -978,7 +978,7 @@ func isSafeURLSegment(segment string) bool { } func validateOIDCConfigID(id string) error { - if !strings.HasPrefix(id, "oidc.") || id == "oidc." || !isSafeURLSegment(id) { + if !strings.HasPrefix(id, "oidc.") || !isSafeURLSegment(id) { return fmt.Errorf("invalid OIDC provider id: %q", id) } @@ -986,7 +986,7 @@ func validateOIDCConfigID(id string) error { } func validateSAMLConfigID(id string) error { - if !strings.HasPrefix(id, "saml.") || id == "saml." || !isSafeURLSegment(id) { + if !strings.HasPrefix(id, "saml.") || !isSafeURLSegment(id) { return fmt.Errorf("invalid SAML provider id: %q", id) } diff --git a/auth/provider_config_test.go b/auth/provider_config_test.go index 455c4466..d5cd5621 100644 --- a/auth/provider_config_test.go +++ b/auth/provider_config_test.go @@ -493,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) @@ -1113,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)