From a39fb027247ea665776ea22b1c10da3954957fd7 Mon Sep 17 00:00:00 2001 From: Vault Automation Date: Thu, 30 Apr 2026 01:38:30 -0600 Subject: [PATCH] VAULT-43451: batching query instead of splitting based on semi-colen in MSSQL while deletion (#13474) (#14418) * batching query instead of splitting based on semi colen * added tests * updated test * updated test * updated * added changelog * updated the query map Co-authored-by: suraj-simha --- changelog/_13474.txt | 3 ++ plugins/database/mssql/mssql.go | 23 +++++----- plugins/database/mssql/mssql_test.go | 69 ++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 12 deletions(-) create mode 100644 changelog/_13474.txt diff --git a/changelog/_13474.txt b/changelog/_13474.txt new file mode 100644 index 0000000000..9ff2282996 --- /dev/null +++ b/changelog/_13474.txt @@ -0,0 +1,3 @@ +```release-note:bug +database/mssql: Fix dynamic secret revocation by executing custom statements as a single batch instead of splitting on semicolons +``` \ No newline at end of file diff --git a/plugins/database/mssql/mssql.go b/plugins/database/mssql/mssql.go index 82d677ae70..17709e2dd6 100644 --- a/plugins/database/mssql/mssql.go +++ b/plugins/database/mssql/mssql.go @@ -189,20 +189,19 @@ func (m *MSSQL) DeleteUser(ctx context.Context, req dbplugin.DeleteUserRequest) merr := &multierror.Error{} - // Execute each query + // Execute each command string as a single batch rather than splitting on + // semicolons. Splitting on ";" destroys the batch boundary, causing subsequent + // statements (e.g. DROP USER) to run in the wrong database context and fail. for _, stmt := range req.Statements.Commands { - for _, query := range strutil.ParseArbitraryStringSlice(stmt, ";") { - query = strings.TrimSpace(query) - if len(query) == 0 { - continue - } + if strings.TrimSpace(stmt) == "" { + continue + } - m := map[string]string{ - "name": req.Username, - } - if err := dbtxn.ExecuteDBQueryDirect(ctx, db, m, query); err != nil { - merr = multierror.Append(merr, err) - } + m := map[string]string{ + "name": req.Username, + } + if err := dbtxn.ExecuteDBQueryDirect(ctx, db, m, stmt); err != nil { + merr = multierror.Append(merr, err) } } diff --git a/plugins/database/mssql/mssql_test.go b/plugins/database/mssql/mssql_test.go index e304bf6b45..87c2699a4a 100644 --- a/plugins/database/mssql/mssql_test.go +++ b/plugins/database/mssql/mssql_test.go @@ -405,6 +405,75 @@ func TestMSSQLDeleteUserContainedDB(t *testing.T) { assertContainedDBCredsDoNotExist(t, connURL, dbUser) } +// TestMSSQLDeleteUserWithBatch verifies that USE database context switches work correctly when statements are executed as a single batch. +func TestMSSQLDeleteUserWithBatch(t *testing.T) { + cleanup, connURL := mssqlhelper.PrepareMSSQLTestContainer(t) + defer cleanup() + + dbUser := "vaultuser" + initPassword := "p4$sw0rd" + + // Create a test database + setupDB, err := sql.Open("mssql", connURL) + if err != nil { + t.Fatalf("Failed to open connection for setup: %s", err) + } + defer setupDB.Close() + + _, err = setupDB.ExecContext(context.Background(), "CREATE DATABASE testdb") + if err != nil { + t.Fatalf("Failed to create test database: %s", err) + } + + // Create login in master and user in testdb + createSQL := ` +CREATE LOGIN [{{name}}] WITH PASSWORD = '{{password}}'; +USE [testdb]; +CREATE USER [{{name}}] FOR LOGIN [{{name}}];` + + err = createTestMSSQLUser(connURL, dbUser, initPassword, createSQL) + if err != nil { + t.Fatalf("Failed to create user: %s", err) + } + + assertCredsExist(t, connURL, dbUser, initPassword) + + initReq := dbplugin.InitializeRequest{ + Config: map[string]interface{}{ + "connection_url": connURL, + }, + VerifyConnection: true, + } + + db := new() + dbtesting.AssertInitializeCircleCiTest(t, db, initReq) + defer dbtesting.AssertClose(t, db) + + deleteReq := dbplugin.DeleteUserRequest{ + Username: dbUser, + Statements: dbplugin.Statements{ + Commands: []string{ + `USE [testdb]; DROP USER [{{name}}]; USE [master]; DROP LOGIN [{{name}}];`, + }, + }, + } + + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + + deleteResp, err := db.DeleteUser(ctx, deleteReq) + if err != nil { + t.Fatalf("Failed to delete user with batch statement: %s", err) + } + + expectedResp := dbplugin.DeleteUserResponse{} + if !reflect.DeepEqual(deleteResp, expectedResp) { + t.Fatalf("Fields missing from expected response: Actual: %#v", deleteResp) + } + + assertCredsDoNotExist(t, connURL, dbUser, initPassword) +} + func TestMSSQLContainedDBSQLSanitization(t *testing.T) { cleanup, connURL := mssqlhelper.PrepareMSSQLTestContainer(t) defer cleanup()