diff --git a/auth/provider_config.go b/auth/provider_config.go index c33eaf0d..164be2a2 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.") || !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.") || !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..d5cd5621 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) { @@ -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) 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()