From 67482c4d5d4ee1b610c58248e17ad77ca4c2f81f Mon Sep 17 00:00:00 2001 From: Yakir Gibraltar Date: Mon, 9 Feb 2026 16:05:22 +0200 Subject: [PATCH 1/6] fix: remove double-transformation in trigger length validation ValidateGhostTriggerLengthBelowMaxLength was calling GetGhostTriggerName on an already-transformed name, adding the suffix twice. This caused valid trigger names (ghost name <= 64 chars) to be falsely rejected. The caller in inspect.go:627 already transforms the name via GetGhostTriggerName before passing it, so the validation function should check the length as-is. Unit tests updated to reflect the correct call pattern: transform first with GetGhostTriggerName, then validate the result. Added boundary tests for exactly 64 and 65 char names. --- go/base/context.go | 7 +++---- go/base/context_test.go | 43 +++++++++++++++++++++++++++++++++++------ 2 files changed, 40 insertions(+), 10 deletions(-) diff --git a/go/base/context.go b/go/base/context.go index c6ccb800c..1e96cbcc1 100644 --- a/go/base/context.go +++ b/go/base/context.go @@ -970,9 +970,8 @@ func (this *MigrationContext) GetGhostTriggerName(triggerName string) string { return triggerName + this.TriggerSuffix } -// validateGhostTriggerLength check if the ghost trigger name length is not more than 64 characters +// ValidateGhostTriggerLengthBelowMaxLength checks if the given trigger name (already transformed +// by GetGhostTriggerName) does not exceed the maximum allowed length. func (this *MigrationContext) ValidateGhostTriggerLengthBelowMaxLength(triggerName string) bool { - ghostTriggerName := this.GetGhostTriggerName(triggerName) - - return utf8.RuneCountInString(ghostTriggerName) <= mysql.MaxTableNameLength + return utf8.RuneCountInString(triggerName) <= mysql.MaxTableNameLength } diff --git a/go/base/context_test.go b/go/base/context_test.go index f87bc9f13..b37e5f097 100644 --- a/go/base/context_test.go +++ b/go/base/context_test.go @@ -86,38 +86,69 @@ func TestGetTriggerNames(t *testing.T) { } func TestValidateGhostTriggerLengthBelowMaxLength(t *testing.T) { + // Tests simulate the real call pattern: GetGhostTriggerName first, then validate the result. { + // Short trigger name with suffix appended: well under 64 chars context := NewMigrationContext() context.TriggerSuffix = "_gho" - require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength("my_trigger")) + ghostName := context.GetGhostTriggerName("my_trigger") // "my_trigger_gho" = 14 chars + require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName)) } { + // 64-char original + "_ghost" suffix = 70 chars → exceeds limit context := NewMigrationContext() context.TriggerSuffix = "_ghost" - require.False(t, context.ValidateGhostTriggerLengthBelowMaxLength(strings.Repeat("my_trigger_ghost", 4))) // 64 characters + "_ghost" + ghostName := context.GetGhostTriggerName(strings.Repeat("my_trigger_ghost", 4)) // 64 + 6 = 70 + require.False(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName)) } { + // 48-char original + "_ghost" suffix = 54 chars → valid context := NewMigrationContext() context.TriggerSuffix = "_ghost" - require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength(strings.Repeat("my_trigger_ghost", 3))) // 48 characters + "_ghost" + ghostName := context.GetGhostTriggerName(strings.Repeat("my_trigger_ghost", 3)) // 48 + 6 = 54 + require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName)) } { + // RemoveTriggerSuffix: 64-char name ending in "_ghost" → suffix removed → 58 chars → valid context := NewMigrationContext() context.TriggerSuffix = "_ghost" context.RemoveTriggerSuffix = true - require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength(strings.Repeat("my_trigger_ghost", 4))) // 64 characters + "_ghost" removed + ghostName := context.GetGhostTriggerName(strings.Repeat("my_trigger_ghost", 4)) // suffix removed → 58 + require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName)) } { + // RemoveTriggerSuffix: name doesn't end in suffix → suffix appended → 65 + 6 = 71 chars → exceeds context := NewMigrationContext() context.TriggerSuffix = "_ghost" context.RemoveTriggerSuffix = true - require.False(t, context.ValidateGhostTriggerLengthBelowMaxLength(strings.Repeat("my_trigger_ghost", 4)+"X")) // 65 characters + "_ghost" not removed + ghostName := context.GetGhostTriggerName(strings.Repeat("my_trigger_ghost", 4) + "X") // no match, appended → 71 + require.False(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName)) } { + // RemoveTriggerSuffix: 70-char name ending in "_ghost" → suffix removed → 64 chars → exactly at limit → valid context := NewMigrationContext() context.TriggerSuffix = "_ghost" context.RemoveTriggerSuffix = true - require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength(strings.Repeat("my_trigger_ghost", 4)+"_ghost")) // 70 characters + last "_ghost" removed + ghostName := context.GetGhostTriggerName(strings.Repeat("my_trigger_ghost", 4) + "_ghost") // suffix removed → 64 + require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName)) + } + { + // Edge case: exactly 64 chars after transformation → valid (boundary test) + context := NewMigrationContext() + context.TriggerSuffix = "_ght" + originalName := strings.Repeat("x", 60) // 60 chars + ghostName := context.GetGhostTriggerName(originalName) // 60 + 4 = 64 + require.Equal(t, 64, len(ghostName)) + require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName)) + } + { + // Edge case: 65 chars after transformation → exceeds (boundary test) + context := NewMigrationContext() + context.TriggerSuffix = "_ght" + originalName := strings.Repeat("x", 61) // 61 chars + ghostName := context.GetGhostTriggerName(originalName) // 61 + 4 = 65 + require.Equal(t, 65, len(ghostName)) + require.False(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName)) } } From 105b41a647a3eb0db67e3392918f929ea3890853 Mon Sep 17 00:00:00 2001 From: Yakir Gibraltar Date: Mon, 9 Feb 2026 16:05:28 +0200 Subject: [PATCH 2/6] fix: return error from trigger creation during atomic cut-over During atomic cut-over, if CreateTriggersOnGhost failed, the error was logged but not returned. The migration continued and completed without triggers, silently losing them. The two-step cut-over (line 793) already correctly returns the error. This aligns the atomic cut-over to do the same. --- go/logic/migrator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/logic/migrator.go b/go/logic/migrator.go index 714b42f1d..6c915ea62 100644 --- a/go/logic/migrator.go +++ b/go/logic/migrator.go @@ -842,7 +842,7 @@ func (this *Migrator) atomicCutOver() (err error) { // If we need to create triggers we need to do it here (only create part) if this.migrationContext.IncludeTriggers && len(this.migrationContext.Triggers) > 0 { if err := this.applier.CreateTriggersOnGhost(); err != nil { - this.migrationContext.Log.Errore(err) + return this.migrationContext.Log.Errore(err) } } From b0fad4269585287cd5198d3a4ccc85451b4c1822 Mon Sep 17 00:00:00 2001 From: Yakir Gibraltar Date: Mon, 9 Feb 2026 16:05:34 +0200 Subject: [PATCH 3/6] fix: check trigger name uniqueness per schema, not per table validateGhostTriggersDontExist was filtering by event_object_table, only checking if the ghost trigger name existed on the original table. MySQL trigger names are unique per schema, so a trigger with the same name on any other table would block CREATE TRIGGER but pass validation. Remove the event_object_table filter to check trigger_name + trigger_schema only, matching MySQL's uniqueness constraint. --- go/logic/inspect.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/go/logic/inspect.go b/go/logic/inspect.go index 044360153..97895890d 100644 --- a/go/logic/inspect.go +++ b/go/logic/inspect.go @@ -596,7 +596,7 @@ func (this *Inspector) validateGhostTriggersDontExist() error { var foundTriggers []string for _, trigger := range this.migrationContext.Triggers { triggerName := this.migrationContext.GetGhostTriggerName(trigger.Name) - query := "select 1 from information_schema.triggers where trigger_name = ? and trigger_schema = ? and event_object_table = ?" + query := "select 1 from information_schema.triggers where trigger_name = ? and trigger_schema = ?" err := sqlutils.QueryRowsMap(this.db, query, func(rowMap sqlutils.RowMap) error { triggerExists := rowMap.GetInt("1") if triggerExists == 1 { @@ -606,7 +606,6 @@ func (this *Inspector) validateGhostTriggersDontExist() error { }, triggerName, this.migrationContext.DatabaseName, - this.migrationContext.OriginalTableName, ) if err != nil { return err From 59911543d10f22fb3b75e4d65ac0763ea1c4b846 Mon Sep 17 00:00:00 2001 From: Yakir Gibraltar Date: Mon, 9 Feb 2026 16:05:39 +0200 Subject: [PATCH 4/6] fix: use parameterized query in GetTriggers to prevent SQL injection GetTriggers used fmt.Sprintf with string interpolation for database and table names, causing SQL syntax errors with special characters and potential SQL injection. Switched to parameterized query with ? placeholders, matching the safe pattern already used in inspect.go:553-559. --- go/mysql/utils.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/go/mysql/utils.go b/go/mysql/utils.go index 7d57afbf1..9619e1e9f 100644 --- a/go/mysql/utils.go +++ b/go/mysql/utils.go @@ -248,9 +248,9 @@ func Kill(db *gosql.DB, connectionID string) error { // GetTriggers reads trigger list from given table func GetTriggers(db *gosql.DB, databaseName, tableName string) (triggers []Trigger, err error) { - query := fmt.Sprintf(`select trigger_name as name, event_manipulation as event, action_statement as statement, action_timing as timing - from information_schema.triggers - where trigger_schema = '%s' and event_object_table = '%s'`, databaseName, tableName) + query := `select trigger_name as name, event_manipulation as event, action_statement as statement, action_timing as timing + from information_schema.triggers + where trigger_schema = ? and event_object_table = ?` err = sqlutils.QueryRowsMap(db, query, func(rowMap sqlutils.RowMap) error { triggers = append(triggers, Trigger{ @@ -260,7 +260,7 @@ func GetTriggers(db *gosql.DB, databaseName, tableName string) (triggers []Trigg Timing: rowMap.GetString("timing"), }) return nil - }) + }, databaseName, tableName) if err != nil { return nil, err } From e95e7c07cc75422ca89d79bd8657d7e670168bd5 Mon Sep 17 00:00:00 2001 From: Yakir Gibraltar Date: Mon, 9 Feb 2026 19:02:21 +0200 Subject: [PATCH 5/6] test: add regression tests for trigger handling bugs Add two integration tests: - trigger-long-name-validation: verifies 60-char trigger names (64-char ghost name) are not falsely rejected by double-transform - trigger-ghost-name-conflict: verifies validation detects ghost trigger name conflicts on other tables in the same schema --- .../trigger-ghost-name-conflict/create.sql | 35 +++++++++++++++++ .../trigger-ghost-name-conflict/destroy.sql | 2 + .../expect_failure | 1 + .../trigger-ghost-name-conflict/extra_args | 1 + .../trigger-long-name-validation/create.sql | 38 +++++++++++++++++++ .../trigger-long-name-validation/extra_args | 1 + 6 files changed, 78 insertions(+) create mode 100644 localtests/trigger-ghost-name-conflict/create.sql create mode 100644 localtests/trigger-ghost-name-conflict/destroy.sql create mode 100644 localtests/trigger-ghost-name-conflict/expect_failure create mode 100644 localtests/trigger-ghost-name-conflict/extra_args create mode 100644 localtests/trigger-long-name-validation/create.sql create mode 100644 localtests/trigger-long-name-validation/extra_args diff --git a/localtests/trigger-ghost-name-conflict/create.sql b/localtests/trigger-ghost-name-conflict/create.sql new file mode 100644 index 000000000..32980daa0 --- /dev/null +++ b/localtests/trigger-ghost-name-conflict/create.sql @@ -0,0 +1,35 @@ +-- Bug #3 regression test: validateGhostTriggersDontExist must check whole schema +-- MySQL trigger names are unique per SCHEMA, not per table. +-- The validation must detect a trigger with the ghost name on ANY table in the schema. + +drop trigger if exists gh_ost_test_ai_ght; +drop trigger if exists gh_ost_test_ai; +drop table if exists gh_ost_test_other; +drop table if exists gh_ost_test; + +create table gh_ost_test ( + id int auto_increment, + i int not null, + primary key(id) +) auto_increment=1; + +-- This trigger has the _ght suffix (simulating a previous migration left it). +-- Ghost name = "gh_ost_test_ai" (suffix removed). +create trigger gh_ost_test_ai_ght + after insert on gh_ost_test for each row + set @dummy = 1; + +-- Create ANOTHER table with a trigger named "gh_ost_test_ai" (the ghost name). +-- Validation must detect this conflict even though the trigger is on a different table. +create table gh_ost_test_other ( + id int auto_increment, + primary key(id) +); + +create trigger gh_ost_test_ai + after insert on gh_ost_test_other for each row + set @dummy = 1; + +insert into gh_ost_test values (null, 11); +insert into gh_ost_test values (null, 13); +insert into gh_ost_test values (null, 17); diff --git a/localtests/trigger-ghost-name-conflict/destroy.sql b/localtests/trigger-ghost-name-conflict/destroy.sql new file mode 100644 index 000000000..5b5e1e4f7 --- /dev/null +++ b/localtests/trigger-ghost-name-conflict/destroy.sql @@ -0,0 +1,2 @@ +drop trigger if exists gh_ost_test_ai; +drop table if exists gh_ost_test_other; diff --git a/localtests/trigger-ghost-name-conflict/expect_failure b/localtests/trigger-ghost-name-conflict/expect_failure new file mode 100644 index 000000000..1a4c997f3 --- /dev/null +++ b/localtests/trigger-ghost-name-conflict/expect_failure @@ -0,0 +1 @@ +Found gh-ost triggers \ No newline at end of file diff --git a/localtests/trigger-ghost-name-conflict/extra_args b/localtests/trigger-ghost-name-conflict/extra_args new file mode 100644 index 000000000..128ce0a66 --- /dev/null +++ b/localtests/trigger-ghost-name-conflict/extra_args @@ -0,0 +1 @@ +--include-triggers --trigger-suffix=_ght --remove-trigger-suffix-if-exists \ No newline at end of file diff --git a/localtests/trigger-long-name-validation/create.sql b/localtests/trigger-long-name-validation/create.sql new file mode 100644 index 000000000..526713b74 --- /dev/null +++ b/localtests/trigger-long-name-validation/create.sql @@ -0,0 +1,38 @@ +-- Bug #1: Double-transformation in trigger length validation +-- A trigger with a 60-char name should be valid: ghost name = 60 + 4 (_ght) = 64 chars (max allowed). +-- But validateGhostTriggersLength() applies GetGhostTriggerName() twice, +-- computing 60 + 4 + 4 = 68, which falsely exceeds the 64-char limit. + +drop table if exists gh_ost_test; + +create table gh_ost_test ( + id int auto_increment, + i int not null, + primary key(id) +) auto_increment=1; + +-- Trigger name is exactly 60 characters (padded to 60). +-- Ghost name with _ght suffix = 64 chars = exactly at the MySQL limit. +-- 60 chars: trigger_long_name_padding_aaaaaaaaaaaaaaaaaaaaa_60chars_xxxx +create trigger trigger_long_name_padding_aaaaaaaaaaaaaaaaaaaaa_60chars_xxxx + after insert on gh_ost_test for each row + set @dummy = 1; + +insert into gh_ost_test values (null, 11); +insert into gh_ost_test values (null, 13); +insert into gh_ost_test values (null, 17); + +drop event if exists gh_ost_test; +delimiter ;; +create event gh_ost_test + on schedule every 1 second + starts current_timestamp + ends current_timestamp + interval 60 second + on completion not preserve + enable + do +begin + insert into gh_ost_test values (null, 23); + update gh_ost_test set i=i+1 where id=1; +end ;; +delimiter ; diff --git a/localtests/trigger-long-name-validation/extra_args b/localtests/trigger-long-name-validation/extra_args new file mode 100644 index 000000000..128ce0a66 --- /dev/null +++ b/localtests/trigger-long-name-validation/extra_args @@ -0,0 +1 @@ +--include-triggers --trigger-suffix=_ght --remove-trigger-suffix-if-exists \ No newline at end of file From 059962557e159af0a0d2a6a3341e0883a259f817 Mon Sep 17 00:00:00 2001 From: Yakir Gibraltar Date: Tue, 10 Feb 2026 10:06:51 +0200 Subject: [PATCH 6/6] style: gofmt context_test.go --- go/base/context_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/base/context_test.go b/go/base/context_test.go index b37e5f097..f8bce6f27 100644 --- a/go/base/context_test.go +++ b/go/base/context_test.go @@ -136,7 +136,7 @@ func TestValidateGhostTriggerLengthBelowMaxLength(t *testing.T) { // Edge case: exactly 64 chars after transformation → valid (boundary test) context := NewMigrationContext() context.TriggerSuffix = "_ght" - originalName := strings.Repeat("x", 60) // 60 chars + originalName := strings.Repeat("x", 60) // 60 chars ghostName := context.GetGhostTriggerName(originalName) // 60 + 4 = 64 require.Equal(t, 64, len(ghostName)) require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName)) @@ -145,7 +145,7 @@ func TestValidateGhostTriggerLengthBelowMaxLength(t *testing.T) { // Edge case: 65 chars after transformation → exceeds (boundary test) context := NewMigrationContext() context.TriggerSuffix = "_ght" - originalName := strings.Repeat("x", 61) // 61 chars + originalName := strings.Repeat("x", 61) // 61 chars ghostName := context.GetGhostTriggerName(originalName) // 61 + 4 = 65 require.Equal(t, 65, len(ghostName)) require.False(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName))