From 3638ea57b4d53bcd666f8296cd05ee0d8232b28f Mon Sep 17 00:00:00 2001 From: apstndb <803393+apstndb@users.noreply.github.com> Date: Thu, 2 Jul 2026 00:38:36 +0900 Subject: [PATCH 1/2] Validate resource IDs at option boundaries Reject path-breaking project, instance, and database IDs during option finalization so invalid resource paths and CREATE DATABASE statements fail before bootstrap. Co-authored-by: Cursor --- omni.go | 3 + options.go | 63 +++++++++++++++++ options_test.go | 183 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 249 insertions(+) diff --git a/omni.go b/omni.go index 4655484..9038648 100644 --- a/omni.go +++ b/omni.go @@ -230,6 +230,9 @@ func finalizeOmniOptions(opts *emulatorOptions) (*emulatorOptions, error) { opts.projectID = cmp.Or(opts.projectID, defaultOmniProjectID) opts.instanceID = cmp.Or(opts.instanceID, defaultOmniInstanceID) opts.databaseID = cmp.Or(opts.databaseID, DefaultDatabaseID) + if err := validateResourceIDs(opts); err != nil { + return nil, err + } opts.clientConfig = finalizeManagedOmniClientConfig(opts.clientConfig, opts.disableBackendGuardrails) if err := applyContainerProviderEnv(opts); err != nil { return nil, err diff --git a/options.go b/options.go index 90e4b41..5b86f71 100644 --- a/options.go +++ b/options.go @@ -559,6 +559,10 @@ func finalizeOptions(opts *emulatorOptions) (*emulatorOptions, error) { opts.instanceID = cmp.Or(opts.instanceID, DefaultInstanceID) opts.databaseID = cmp.Or(opts.databaseID, DefaultDatabaseID) + if err := validateResourceIDs(opts); err != nil { + return nil, err + } + // Disable native metrics by default for emulator connections. // Without SPANNER_EMULATOR_HOST, the Spanner client tries to create a real // Cloud Monitoring exporter and contacts the GCP metadata server, adding @@ -588,6 +592,65 @@ func validateSetupFileDescriptorSet(opts *emulatorOptions) error { return fmt.Errorf("setup file descriptor set requires WithSetupDDLs when database auto-creation is disabled") } +const ( + maxProjectIDLength = 30 + maxInstanceIDLength = 64 + maxDatabaseIDLength = 30 +) + +func validateResourceIDs(opts *emulatorOptions) error { + for _, resource := range []struct { + kind string + id string + maxLen int + }{ + {kind: "project", id: opts.projectID, maxLen: maxProjectIDLength}, + {kind: "instance", id: opts.instanceID, maxLen: maxInstanceIDLength}, + {kind: "database", id: opts.databaseID, maxLen: maxDatabaseIDLength}, + } { + if err := validateResourceID(resource.kind, resource.id, resource.maxLen); err != nil { + return err + } + } + return nil +} + +func validateResourceID(kind, id string, maxLen int) error { + if id == "" { + return fmt.Errorf("%s ID is empty after option finalization; use a non-empty ID or the default", kind) + } + if strings.TrimSpace(id) == "" { + return fmt.Errorf("%s ID %q is empty after trimming whitespace; use a non-empty ID or the default", kind, id) + } + if len(id) > maxLen { + return fmt.Errorf("%s ID %q is too long: got %d characters, max %d", kind, id, len(id), maxLen) + } + if !isLowercaseLetter(id[0]) { + return fmt.Errorf("%s ID %q must start with a lowercase letter and use only lowercase letters, digits, and hyphens", kind, id) + } + if !isLowercaseLetterOrDigit(id[len(id)-1]) { + return fmt.Errorf("%s ID %q must end with a lowercase letter or digit and use only lowercase letters, digits, and hyphens", kind, id) + } + for i := 1; i < len(id)-1; i++ { + if !isResourceIDChar(id[i]) { + return fmt.Errorf("%s ID %q contains invalid character %q at byte %d; use only lowercase letters, digits, and hyphens so it is safe in resource paths and CREATE DATABASE statements", kind, id, id[i], i) + } + } + return nil +} + +func isResourceIDChar(b byte) bool { + return isLowercaseLetterOrDigit(b) || b == '-' +} + +func isLowercaseLetterOrDigit(b byte) bool { + return isLowercaseLetter(b) || '0' <= b && b <= '9' +} + +func isLowercaseLetter(b byte) bool { + return 'a' <= b && b <= 'z' +} + func applyContainerProviderEnv(opts *emulatorOptions) error { if opts.containerProviderSet { return nil diff --git a/options_test.go b/options_test.go index b97806d..3c50dfa 100644 --- a/options_test.go +++ b/options_test.go @@ -2,6 +2,7 @@ package spanemuboost import ( "bytes" + "strings" "testing" "google.golang.org/protobuf/proto" @@ -114,6 +115,188 @@ func TestWithSetupRawFileDescriptorSetSnapshotsBeforeApply(t *testing.T) { } } +func TestValidateResourceIDsRejectsInvalidOptions(t *testing.T) { + tests := []struct { + name string + options []Option + want string + }{ + { + name: "project slash", + options: []Option{WithProjectID("bad/project")}, + want: "project ID", + }, + { + name: "project whitespace", + options: []Option{WithProjectID(" \t ")}, + want: "project ID", + }, + { + name: "project overlong", + options: []Option{WithProjectID(strings.Repeat("a", maxProjectIDLength+1))}, + want: "too long", + }, + { + name: "instance backtick", + options: []Option{WithInstanceID("bad`instance")}, + want: "instance ID", + }, + { + name: "instance overlong", + options: []Option{WithInstanceID(strings.Repeat("a", maxInstanceIDLength+1))}, + want: "too long", + }, + { + name: "database quote", + options: []Option{WithDatabaseID(`bad"database`)}, + want: "database ID", + }, + { + name: "database backtick", + options: []Option{WithDatabaseID("bad`database")}, + want: "database ID", + }, + { + name: "database uppercase", + options: []Option{WithDatabaseID("Database")}, + want: "database ID", + }, + { + name: "database overlong", + options: []Option{WithDatabaseID(strings.Repeat("a", maxDatabaseIDLength+1))}, + want: "too long", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := applyOptions(tt.options...) + if err == nil { + t.Fatal("applyOptions() error = nil, want non-nil") + } + if !strings.Contains(err.Error(), tt.want) { + t.Fatalf("applyOptions() error = %q, want substring %q", err, tt.want) + } + }) + } +} + +func TestValidateResourceIDsRejectsInvalidOmniOptions(t *testing.T) { + tests := []struct { + name string + options []Option + want string + }{ + { + name: "database slash", + options: []Option{WithDatabaseID("bad/database")}, + want: "database ID", + }, + { + name: "project slash with guardrails disabled", + options: []Option{ + DisableBackendGuardrails(), + WithProjectID("bad/project"), + }, + want: "project ID", + }, + { + name: "instance backtick with guardrails disabled", + options: []Option{ + DisableBackendGuardrails(), + WithInstanceID("bad`instance"), + }, + want: "instance ID", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := applyOmniOptions(tt.options...) + if err == nil { + t.Fatal("applyOmniOptions() error = nil, want non-nil") + } + if !strings.Contains(err.Error(), tt.want) { + t.Fatalf("applyOmniOptions() error = %q, want substring %q", err, tt.want) + } + }) + } +} + +func TestValidateResourceIDRejectsEmptyFinalID(t *testing.T) { + err := validateResourceID("database", "", maxDatabaseIDLength) + if err == nil { + t.Fatal("validateResourceID() error = nil, want non-nil") + } + if !strings.Contains(err.Error(), "empty after option finalization") { + t.Fatalf("validateResourceID() error = %q, want empty-finalization message", err) + } +} + +func TestValidateResourceIDsAcceptsDefaultsAndRandomIDs(t *testing.T) { + t.Run("emulator defaults", func(t *testing.T) { + opts, err := applyOptions() + if err != nil { + t.Fatalf("applyOptions() error = %v, want nil", err) + } + if opts.projectID != DefaultProjectID { + t.Fatalf("projectID = %q, want %q", opts.projectID, DefaultProjectID) + } + if opts.instanceID != DefaultInstanceID { + t.Fatalf("instanceID = %q, want %q", opts.instanceID, DefaultInstanceID) + } + if opts.databaseID != DefaultDatabaseID { + t.Fatalf("databaseID = %q, want %q", opts.databaseID, DefaultDatabaseID) + } + }) + + t.Run("emulator random IDs", func(t *testing.T) { + opts, err := applyOptions( + WithRandomProjectID(), + WithRandomInstanceID(), + WithRandomDatabaseID(), + ) + if err != nil { + t.Fatalf("applyOptions() error = %v, want nil", err) + } + for name, id := range map[string]string{ + "project": opts.projectID, + "instance": opts.instanceID, + "database": opts.databaseID, + } { + if len(id) != idRange { + t.Fatalf("%s ID length = %d, want %d", name, len(id), idRange) + } + } + }) + + t.Run("omni defaults", func(t *testing.T) { + opts, err := applyOmniOptions() + if err != nil { + t.Fatalf("applyOmniOptions() error = %v, want nil", err) + } + if opts.projectID != defaultOmniProjectID { + t.Fatalf("projectID = %q, want %q", opts.projectID, defaultOmniProjectID) + } + if opts.instanceID != defaultOmniInstanceID { + t.Fatalf("instanceID = %q, want %q", opts.instanceID, defaultOmniInstanceID) + } + if opts.databaseID != DefaultDatabaseID { + t.Fatalf("databaseID = %q, want %q", opts.databaseID, DefaultDatabaseID) + } + }) + + t.Run("omni random database", func(t *testing.T) { + opts, err := applyOmniOptions(WithRandomDatabaseID()) + if err != nil { + t.Fatalf("applyOmniOptions() error = %v, want nil", err) + } + if len(opts.databaseID) != idRange { + t.Fatalf("databaseID length = %d, want %d", len(opts.databaseID), idRange) + } + }) +} + func TestHasSetupDDLWork(t *testing.T) { t.Run("ddl only", func(t *testing.T) { opts, err := applyOptions(WithSetupDDLs([]string{"CREATE TABLE t (id INT64) PRIMARY KEY (id)"})) From 15225402dd0dfc6f428438610167a3f21a0dc6d8 Mon Sep 17 00:00:00 2001 From: apstndb <803393+apstndb@users.noreply.github.com> Date: Thu, 2 Jul 2026 01:16:43 +0900 Subject: [PATCH 2/2] Handle resource ID validation review feedback Allow valid database ID underscores and reject too-short resource IDs at option finalization so invalid fixed IDs fail before backend setup. Co-authored-by: Cursor --- options.go | 60 ++++++++++++++++++++++++++++++++++++++++--------- options_test.go | 37 +++++++++++++++++++++++++++++- 2 files changed, 85 insertions(+), 12 deletions(-) diff --git a/options.go b/options.go index 5b86f71..439caa1 100644 --- a/options.go +++ b/options.go @@ -593,6 +593,10 @@ func validateSetupFileDescriptorSet(opts *emulatorOptions) error { } const ( + minProjectIDLength = 6 + minInstanceIDLength = 2 + minDatabaseIDLength = 2 + maxProjectIDLength = 30 maxInstanceIDLength = 64 maxDatabaseIDLength = 30 @@ -602,45 +606,79 @@ func validateResourceIDs(opts *emulatorOptions) error { for _, resource := range []struct { kind string id string + minLen int maxLen int }{ - {kind: "project", id: opts.projectID, maxLen: maxProjectIDLength}, - {kind: "instance", id: opts.instanceID, maxLen: maxInstanceIDLength}, - {kind: "database", id: opts.databaseID, maxLen: maxDatabaseIDLength}, + { + kind: "project", + id: opts.projectID, + minLen: minProjectIDLength, + maxLen: maxProjectIDLength, + }, + { + kind: "instance", + id: opts.instanceID, + minLen: minInstanceIDLength, + maxLen: maxInstanceIDLength, + }, + { + kind: "database", + id: opts.databaseID, + minLen: minDatabaseIDLength, + maxLen: maxDatabaseIDLength, + }, } { - if err := validateResourceID(resource.kind, resource.id, resource.maxLen); err != nil { + if err := validateResourceID(resource.kind, resource.id, resource.minLen, resource.maxLen); err != nil { return err } } return nil } -func validateResourceID(kind, id string, maxLen int) error { +func validateResourceID(kind, id string, minLen, maxLen int) error { if id == "" { return fmt.Errorf("%s ID is empty after option finalization; use a non-empty ID or the default", kind) } if strings.TrimSpace(id) == "" { return fmt.Errorf("%s ID %q is empty after trimming whitespace; use a non-empty ID or the default", kind, id) } + if len(id) < minLen { + return fmt.Errorf("%s ID %q is too short: got %d characters, min %d", kind, id, len(id), minLen) + } if len(id) > maxLen { return fmt.Errorf("%s ID %q is too long: got %d characters, max %d", kind, id, len(id), maxLen) } + allowedChars := resourceIDAllowedCharsDescription(kind) if !isLowercaseLetter(id[0]) { - return fmt.Errorf("%s ID %q must start with a lowercase letter and use only lowercase letters, digits, and hyphens", kind, id) + return fmt.Errorf("%s ID %q must start with a lowercase letter and use only %s", kind, id, allowedChars) } if !isLowercaseLetterOrDigit(id[len(id)-1]) { - return fmt.Errorf("%s ID %q must end with a lowercase letter or digit and use only lowercase letters, digits, and hyphens", kind, id) + return fmt.Errorf("%s ID %q must end with a lowercase letter or digit and use only %s", kind, id, allowedChars) } for i := 1; i < len(id)-1; i++ { - if !isResourceIDChar(id[i]) { - return fmt.Errorf("%s ID %q contains invalid character %q at byte %d; use only lowercase letters, digits, and hyphens so it is safe in resource paths and CREATE DATABASE statements", kind, id, id[i], i) + if !isResourceIDChar(id[i], kind == "database") { + return fmt.Errorf( + "%s ID %q contains invalid character %q at byte %d; use only %s so it is safe in resource paths and CREATE DATABASE statements", + kind, + id, + id[i], + i, + allowedChars, + ) } } return nil } -func isResourceIDChar(b byte) bool { - return isLowercaseLetterOrDigit(b) || b == '-' +func resourceIDAllowedCharsDescription(kind string) string { + if kind == "database" { + return "lowercase letters, digits, hyphens, and underscores" + } + return "lowercase letters, digits, and hyphens" +} + +func isResourceIDChar(b byte, allowUnderscore bool) bool { + return isLowercaseLetterOrDigit(b) || b == '-' || (allowUnderscore && b == '_') } func isLowercaseLetterOrDigit(b byte) bool { diff --git a/options_test.go b/options_test.go index 3c50dfa..79766b4 100644 --- a/options_test.go +++ b/options_test.go @@ -131,11 +131,21 @@ func TestValidateResourceIDsRejectsInvalidOptions(t *testing.T) { options: []Option{WithProjectID(" \t ")}, want: "project ID", }, + { + name: "project too short", + options: []Option{WithProjectID("abcde")}, + want: "too short", + }, { name: "project overlong", options: []Option{WithProjectID(strings.Repeat("a", maxProjectIDLength+1))}, want: "too long", }, + { + name: "instance too short", + options: []Option{WithInstanceID("a")}, + want: "too short", + }, { name: "instance backtick", options: []Option{WithInstanceID("bad`instance")}, @@ -161,6 +171,11 @@ func TestValidateResourceIDsRejectsInvalidOptions(t *testing.T) { options: []Option{WithDatabaseID("Database")}, want: "database ID", }, + { + name: "database too short", + options: []Option{WithDatabaseID("a")}, + want: "too short", + }, { name: "database overlong", options: []Option{WithDatabaseID(strings.Repeat("a", maxDatabaseIDLength+1))}, @@ -224,7 +239,7 @@ func TestValidateResourceIDsRejectsInvalidOmniOptions(t *testing.T) { } func TestValidateResourceIDRejectsEmptyFinalID(t *testing.T) { - err := validateResourceID("database", "", maxDatabaseIDLength) + err := validateResourceID("database", "", minDatabaseIDLength, maxDatabaseIDLength) if err == nil { t.Fatal("validateResourceID() error = nil, want non-nil") } @@ -270,6 +285,16 @@ func TestValidateResourceIDsAcceptsDefaultsAndRandomIDs(t *testing.T) { } }) + t.Run("emulator database underscores", func(t *testing.T) { + opts, err := applyOptions(WithDatabaseID("my_database")) + if err != nil { + t.Fatalf("applyOptions() error = %v, want nil", err) + } + if opts.databaseID != "my_database" { + t.Fatalf("databaseID = %q, want %q", opts.databaseID, "my_database") + } + }) + t.Run("omni defaults", func(t *testing.T) { opts, err := applyOmniOptions() if err != nil { @@ -286,6 +311,16 @@ func TestValidateResourceIDsAcceptsDefaultsAndRandomIDs(t *testing.T) { } }) + t.Run("omni database underscores", func(t *testing.T) { + opts, err := applyOmniOptions(WithDatabaseID("my_database")) + if err != nil { + t.Fatalf("applyOmniOptions() error = %v, want nil", err) + } + if opts.databaseID != "my_database" { + t.Fatalf("databaseID = %q, want %q", opts.databaseID, "my_database") + } + }) + t.Run("omni random database", func(t *testing.T) { opts, err := applyOmniOptions(WithRandomDatabaseID()) if err != nil {