From 6d21a62f11341f12baec0b98b9789fdbedfeee40 Mon Sep 17 00:00:00 2001 From: Andreas Jordan Date: Sun, 16 Aug 2026 15:55:01 +0200 Subject: [PATCH 1/4] Connect-DbaInstance - Give the cloned server ownership of its database connection When an existing server object is passed in together with a different -Database, the connection context is copied and the database connection was then created with GetDatabaseConnection. That opens the connection on the intermediate copy and returns a different ConnectionContext, so the server object we hand back never owns the connection. Disconnect-DbaInstance can only reach the context of the server it is given, so nothing ever closed it. The session therefore stayed open for the life of the process, sitting in the target database and holding a shared lock on it. On model that is enough to make a later CREATE DATABASE on the same instance fail with "Could not obtain exclusive lock on database model", which showed up as an intermittent failure in whatever test file happened to run next. Setting DatabaseName on the copy keeps the connection with the context that the returned server owns, so Disconnect-DbaInstance closes it. This is also what the connection string paths of this command already do, and it does not reset StatementTimeout, so the save and restore around the old call is no longer needed. Measured against one instance, connecting to a database and disconnecting again: before, four sessions were opened and one closed, leaving three behind including one parked in the database. Now two are opened and the database one is closed again. (do Connect-DbaInstance) Co-Authored-By: Claude Opus 5 (1M context) --- public/Connect-DbaInstance.ps1 | 16 ++++++++++++---- tests/Connect-DbaInstance.Tests.ps1 | 15 +++++++++++++++ 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/public/Connect-DbaInstance.ps1 b/public/Connect-DbaInstance.ps1 index bf14528bc45..f6e50fe9091 100644 --- a/public/Connect-DbaInstance.ps1 +++ b/public/Connect-DbaInstance.ps1 @@ -777,10 +777,18 @@ function Connect-DbaInstance { $connContext.NonPooledConnection = $true } if ($Database) { - # Save StatementTimeout because it might be reset on GetDatabaseConnection - $savedStatementTimeout = $connContext.StatementTimeout - $connContext = $connContext.GetDatabaseConnection($Database, $false) - $connContext.StatementTimeout = $savedStatementTimeout + # We set DatabaseName instead of calling GetDatabaseConnection on purpose. + # GetDatabaseConnection opens the connection on the copy we are holding and returns + # a *different* ConnectionContext, so the server we build below never owns that + # connection. Disconnect-DbaInstance can only reach the context of the server it is + # given, so nothing ever closed it: the session stayed open for the life of the + # process, holding a shared lock on the database. On model that is enough to make a + # later CREATE DATABASE on the same instance fail on the exclusive lock. + # Setting DatabaseName keeps the connection with the context we hand to the server, + # which is also what the connection string code paths of this command already do. + # It does not reset StatementTimeout either, so the save and restore that + # GetDatabaseConnection needed is gone with it. + $connContext.DatabaseName = $Database } $server = New-Object -TypeName Microsoft.SqlServer.Management.Smo.Server -ArgumentList $connContext if ($Database -and $server.ConnectionContext.CurrentDatabase -ne $Database) { diff --git a/tests/Connect-DbaInstance.Tests.ps1 b/tests/Connect-DbaInstance.Tests.ps1 index 28c83a7dd92..ae6e1a71bd0 100644 --- a/tests/Connect-DbaInstance.Tests.ps1 +++ b/tests/Connect-DbaInstance.Tests.ps1 @@ -540,6 +540,21 @@ Describe $CommandName -Tag IntegrationTests { $serverClone.ConnectionContext.ExecuteScalar("select db_name()") | Should -Be "tempdb" } + It "hands the clone a connection that Disconnect-DbaInstance can close" { + # The clone used to be built with GetDatabaseConnection, which opened the connection on an + # intermediate copy and returned a different context. The clone therefore did not own its + # connection and Disconnect-DbaInstance closed nothing, so every call left a session parked + # in the database holding a shared lock on it. + $countBefore = @(Get-DbaProcess -SqlInstance $server -Database tempdb | Where-Object Program -match "dbatools").Count + + $serverParked = Connect-DbaInstance -SqlInstance $server -Database tempdb + $serverParked.ConnectionContext.ExecuteScalar("select db_name()") | Should -Be "tempdb" + $null = $serverParked | Disconnect-DbaInstance + + $countAfter = @(Get-DbaProcess -SqlInstance $server -Database tempdb | Where-Object Program -match "dbatools").Count + $countAfter | Should -Be $countBefore + } + It "clones when using parameter ApplicationIntent" { $serverClone = Connect-DbaInstance -SqlInstance $server -ApplicationIntent ReadOnly $server.ConnectionContext.ApplicationIntent | Should -BeNullOrEmpty From c5a3f738f5d2500ea4525429bf24383b2932f6d2 Mon Sep 17 00:00:00 2001 From: Andreas Jordan Date: Fri, 21 Aug 2026 21:18:06 +0200 Subject: [PATCH 2/4] Connect-DbaInstance - Put settings a fixed connection string blocks into that connection string A ServerConnection built from a SqlConnection has its connection string set explicitly, and SMO then refuses to let DatabaseName, NonPooledConnection and ServerInstance be assigned. The previous commit started to rely on the DatabaseName assignment, which broke SqlConnection -> Server -> -Database. The other two were already broken on development: -Database -NonPooledConnection and -DedicatedAdminConnection both threw for such a server. All three now fall back to the connection string, where they are Initial Catalog, Pooling and Data Source. Falling back to GetDatabaseConnection was not an option because that is the leak this branch is about. Initial Catalog is part of the pool key, so the reason #9505 forced a non pooled connection still holds. The copy is disconnected before its connection string is set, because setting it on an open connection does not move it. The copy is a session of its own, so the caller is not affected. Tests: a Context for cloning from a server that was created from a SqlConnection, which is the shape CI never covered - it tests SqlConnection to Server and Server to another Database, but never chained. The leak regression now runs five cycles, because with pooling a single sleeping session is legitimate and only growth proves an orphaned connection. Against development it fails with "Expected 4, but got 7". (do Connect-DbaInstance) --- public/Connect-DbaInstance.ps1 | 64 +++++++++++++++++++++++---- tests/Connect-DbaInstance.Tests.ps1 | 67 ++++++++++++++++++++++++++--- 2 files changed, 116 insertions(+), 15 deletions(-) diff --git a/public/Connect-DbaInstance.ps1 b/public/Connect-DbaInstance.ps1 index f6e50fe9091..7ca0d6ae851 100644 --- a/public/Connect-DbaInstance.ps1 +++ b/public/Connect-DbaInstance.ps1 @@ -752,29 +752,52 @@ function Connect-DbaInstance { } elseif ($copyContext) { $isNewConnection = $true $connContext = $inputObject.ConnectionContext.Copy() + # A ServerConnection that was built from a SqlConnection has its connection string set + # explicitly, and SMO then refuses to let some of its properties be assigned: + # Property cannot be changed or read after a connection string has been set. + # DatabaseName, NonPooledConnection and ServerInstance are all in that group, which is + # why -Database, -NonPooledConnection and -DedicatedAdminConnection used to fail on a + # server that was created from a SqlConnection. Those settings are collected here and + # put into the connection string further down instead, where the same three are + # Initial Catalog, Pooling and Data Source. No readable property tells the two kinds of + # context apart - every one of them reads fine on both - so we find out by trying. + $connectionStringKeyword = @{ } if ($ApplicationIntent) { $connContext.ApplicationIntent = $ApplicationIntent } if ($NonPooledConnection) { - $connContext.NonPooledConnection = $true + try { + $connContext.NonPooledConnection = $true + } catch { + $connectionStringKeyword["Pooling"] = $false + } } if (Test-Bound -Parameter StatementTimeout) { $connContext.StatementTimeout = $StatementTimeout } - if ($DedicatedAdminConnection -and $inputObject.ConnectionContext.ServerInstance -notmatch '^ADMIN:') { + if ($DedicatedAdminConnection -and $inputObject.ConnectionContext.ServerInstance -notmatch "^ADMIN:") { if ($instance.IsLocalHost) { # Use localhost to avoid multiple IP resolution on multi-homed servers (issue #10151) - if ($instance.InstanceName -ne 'MSSQLSERVER') { - $connContext.ServerInstance = "ADMIN:localhost\$($instance.InstanceName)" + if ($instance.InstanceName -ne "MSSQLSERVER") { + $dedicatedAdminServerInstance = "ADMIN:localhost\$($instance.InstanceName)" } else { - $connContext.ServerInstance = "ADMIN:localhost" + $dedicatedAdminServerInstance = "ADMIN:localhost" } # Trust the server certificate because 'localhost' may not match the certificate CN (e.g., FQDN), issue #10254 $connContext.TrustServerCertificate = $true } else { - $connContext.ServerInstance = 'ADMIN:' + $connContext.ServerInstance + $dedicatedAdminServerInstance = "ADMIN:$($connContext.ServerInstance)" + } + try { + $connContext.ServerInstance = $dedicatedAdminServerInstance + } catch { + $connectionStringKeyword["Data Source"] = $dedicatedAdminServerInstance + } + try { + $connContext.NonPooledConnection = $true + } catch { + $connectionStringKeyword["Pooling"] = $false } - $connContext.NonPooledConnection = $true } if ($Database) { # We set DatabaseName instead of calling GetDatabaseConnection on purpose. @@ -788,7 +811,32 @@ function Connect-DbaInstance { # which is also what the connection string code paths of this command already do. # It does not reset StatementTimeout either, so the save and restore that # GetDatabaseConnection needed is gone with it. - $connContext.DatabaseName = $Database + try { + $connContext.DatabaseName = $Database + } catch { + # Falling back to GetDatabaseConnection here would bring back the very leak this + # change is about, so the database goes into the connection string instead. As + # Initial Catalog it is part of the pool key, which is what setting DatabaseName + # achieves as well, so the reason #9505 forced a non pooled connection still holds. + $connectionStringKeyword["Initial Catalog"] = $Database + } + } + if ($connectionStringKeyword.Count -gt 0) { + $changedKeywords = $connectionStringKeyword.Keys -join ", " + Write-Message -Level Debug -Message "Connection context does not accept property assignments, using the connection string for: $changedKeywords" + # The copy has to be disconnected first: setting the connection string of an open + # connection does not move it, and CurrentDatabase would silently stay where it was. + # The copy is a session of its own - a different SPID than the server that was passed + # in - so this does not touch the connection of the caller. + $connContext.Disconnect() + $connectionStringBuilder = New-Object -TypeName Microsoft.Data.SqlClient.SqlConnectionStringBuilder -ArgumentList $connContext.ConnectionString + foreach ($keyword in $connectionStringKeyword.Keys) { + # PowerShell routes property assignments on a connection string builder through its + # dictionary indexer, so the keyword has to be used rather than the property name: + # $connectionStringBuilder.InitialCatalog fails with "Keyword not supported". + $connectionStringBuilder[$keyword] = $connectionStringKeyword[$keyword] + } + $connContext.ConnectionString = $connectionStringBuilder.ConnectionString } $server = New-Object -TypeName Microsoft.SqlServer.Management.Smo.Server -ArgumentList $connContext if ($Database -and $server.ConnectionContext.CurrentDatabase -ne $Database) { diff --git a/tests/Connect-DbaInstance.Tests.ps1 b/tests/Connect-DbaInstance.Tests.ps1 index ae6e1a71bd0..c152ebf1486 100644 --- a/tests/Connect-DbaInstance.Tests.ps1 +++ b/tests/Connect-DbaInstance.Tests.ps1 @@ -525,6 +525,46 @@ Describe $CommandName -Tag IntegrationTests { } } + Context "connection is properly cloned from a connection that was created from a SqlConnection" { + BeforeAll { + # A ServerConnection that was built from a SqlConnection has its connection string set, and + # SMO then refuses assignments to DatabaseName, NonPooledConnection and ServerInstance, so + # cloning such a server has to go through the connection string instead. CI never noticed + # because it covers SqlConnection to Server and Server to another Database, but never the + # two of them chained. See #10584. + [Microsoft.Data.SqlClient.SqlConnection]$sqlConnectionToClone = "Data Source=$($TestConfig.InstanceMulti1);Integrated Security=True;Encrypt=False;Trust Server Certificate=True" + $serverFromSqlConnection = Connect-DbaInstance -SqlInstance $sqlConnectionToClone + } + + AfterAll { + $null = $serverFromSqlConnection | Disconnect-DbaInstance + } + + It "clones when using parameter Database" { + $serverClone = Connect-DbaInstance -SqlInstance $serverFromSqlConnection -Database tempdb + $serverClone.ConnectionContext.ExecuteScalar("select db_name()") | Should -Be "tempdb" + $serverFromSqlConnection.ConnectionContext.ExecuteScalar("select db_name()") | Should -Be "master" + $null = $serverClone | Disconnect-DbaInstance + } + + It "clones when using parameter Database together with NonPooledConnection" { + $serverClone = Connect-DbaInstance -SqlInstance $serverFromSqlConnection -Database tempdb -NonPooledConnection + $serverClone.ConnectionContext.ExecuteScalar("select db_name()") | Should -Be "tempdb" + $serverClone.ConnectionContext.ConnectionString | Should -Match "Pooling=False" + $null = $serverClone | Disconnect-DbaInstance + } + + It "clones when using parameter DedicatedAdminConnection" { + $serverClone = Connect-DbaInstance -SqlInstance $serverFromSqlConnection -DedicatedAdminConnection + $serverClone.ConnectionContext.ConnectionString | Should -Match "ADMIN:" + # The connection context cannot be asked here, because ServerInstance is one of the properties + # that cannot be assigned on such a context, so ask the session which endpoint it is on. + $dacQuery = "SELECT COUNT(*) FROM sys.dm_exec_sessions AS s JOIN sys.endpoints AS e ON e.endpoint_id = s.endpoint_id WHERE e.is_admin_endpoint = 1 AND s.session_id = @@SPID" + $serverClone.ConnectionContext.ExecuteScalar($dacQuery) | Should -Be 1 + $null = $serverClone | Disconnect-DbaInstance + } + } + Context "connection is properly cloned from an existing connection" { BeforeAll { $server = Connect-DbaInstance -SqlInstance $TestConfig.InstanceMulti1 @@ -545,14 +585,20 @@ Describe $CommandName -Tag IntegrationTests { # intermediate copy and returned a different context. The clone therefore did not own its # connection and Disconnect-DbaInstance closed nothing, so every call left a session parked # in the database holding a shared lock on it. - $countBefore = @(Get-DbaProcess -SqlInstance $server -Database tempdb | Where-Object Program -match "dbatools").Count - - $serverParked = Connect-DbaInstance -SqlInstance $server -Database tempdb - $serverParked.ConnectionContext.ExecuteScalar("select db_name()") | Should -Be "tempdb" - $null = $serverParked | Disconnect-DbaInstance + # Repeated on purpose. With pooling, a sleeping session that stays behind is legitimate - it + # belongs to the pool and the next call reuses it. What must not happen is that their number + # grows with every call, which is what an orphaned connection looks like: nothing can reuse + # it and nothing can close it. Against the old implementation the count went up on almost + # every cycle, so a single cycle is not enough to tell the two apart. + $countPerCycle = @() + foreach ($cycle in 1..5) { + $serverParked = Connect-DbaInstance -SqlInstance $server -Database tempdb + $serverParked.ConnectionContext.ExecuteScalar("select db_name()") | Should -Be "tempdb" + $null = $serverParked | Disconnect-DbaInstance + $countPerCycle += @(Get-DbaProcess -SqlInstance $server -Database tempdb | Where-Object Program -match "dbatools").Count + } - $countAfter = @(Get-DbaProcess -SqlInstance $server -Database tempdb | Where-Object Program -match "dbatools").Count - $countAfter | Should -Be $countBefore + $countPerCycle[-1] | Should -Be $countPerCycle[0] } It "clones when using parameter ApplicationIntent" { @@ -561,6 +607,13 @@ Describe $CommandName -Tag IntegrationTests { $serverClone.ConnectionContext.ApplicationIntent | Should -Be "ReadOnly" } + It "clones when using parameter Database together with NonPooledConnection" { + $serverClone = Connect-DbaInstance -SqlInstance $server -Database tempdb -NonPooledConnection + $serverClone.ConnectionContext.ExecuteScalar("select db_name()") | Should -Be "tempdb" + $serverClone.ConnectionContext.NonPooledConnection | Should -Be $true + $null = $serverClone | Disconnect-DbaInstance + } + It "clones when using parameter NonPooledConnection" { $serverClone = Connect-DbaInstance -SqlInstance $server -NonPooledConnection $server.ConnectionContext.NonPooledConnection | Should -Be $false From 00ed91722d7dd6d2a0d70b7ca817a8cf645a9562 Mon Sep 17 00:00:00 2001 From: Andreas Jordan Date: Sat, 22 Aug 2026 19:29:30 +0200 Subject: [PATCH 3/4] Connect-DbaInstance - Switch the database on the connection instead of rebuilding its string Answers the review on #10584. All three findings were reproduced in the lab. The password loss is real and it was a regression. Once a SqlConnection has been opened with Persist Security Info=False, SqlClient hides the password, so the connection string that can be read back no longer carries one. Rebuilding from it produced a clone that failed on first use with "Login failed for user". Verified against a real SQL Server login. So -Database no longer touches the connection string at all. The copy already holds a working connection of its own - a different SPID from the server that was passed in - and the database is switched on it with ChangeDatabase. Nothing has to be rebuilt, so nothing that lives on the SqlConnection rather than in its string can be lost, and the copy stays the context the returned server owns, which is what this branch is about. The two settings that genuinely need a new connection, -NonPooledConnection and -DedicatedAdminConnection, still rebuild the string, and now refuse rather than hand back a server that cannot log in when the password is no longer readable. The caller is told to use the instance name with -SqlCredential instead. TrustServerCertificate does not reach a fixed connection string either. Assigning the property succeeds and reads back as True while the string still says False, so the localhost DAC would have lost the trust that #10254 added. It is put into the string now, and ApplicationIntent with it, which had the same silent mismatch. Tests: a Context for an already open SQL authenticated SqlConnection, which asserts that the password really is hidden, that the clone works and runs as that login, and that the case needing a new connection is refused with a readable message. Both fail against the previous revision with "Login failed for user". The leak regression now requires every cycle to report the same number of sessions rather than only the first and the last, so a sequence like 4, 5, 4, 5, 4 fails. Not covered: the localhost DAC path, because every instance in the lab used for this is remote. The keyword is added from the same branch that sets the property, and the non-local DAC path is tested. (do Connect-DbaInstance) --- public/Connect-DbaInstance.ps1 | 57 ++++++++++++++++++++++---- tests/Connect-DbaInstance.Tests.ps1 | 62 ++++++++++++++++++++++++++++- 2 files changed, 111 insertions(+), 8 deletions(-) diff --git a/public/Connect-DbaInstance.ps1 b/public/Connect-DbaInstance.ps1 index 7ca0d6ae851..adcff010838 100644 --- a/public/Connect-DbaInstance.ps1 +++ b/public/Connect-DbaInstance.ps1 @@ -762,6 +762,7 @@ function Connect-DbaInstance { # Initial Catalog, Pooling and Data Source. No readable property tells the two kinds of # context apart - every one of them reads fine on both - so we find out by trying. $connectionStringKeyword = @{ } + $databaseNeedsSwitch = $false if ($ApplicationIntent) { $connContext.ApplicationIntent = $ApplicationIntent } @@ -792,6 +793,14 @@ function Connect-DbaInstance { $connContext.ServerInstance = $dedicatedAdminServerInstance } catch { $connectionStringKeyword["Data Source"] = $dedicatedAdminServerInstance + if ($instance.IsLocalHost) { + # TrustServerCertificate was assigned above, and on a context with a fixed + # connection string that assignment does not reach the string - the property + # reads back as True while the string still says False. Since the string is + # what the new connection is built from, the same has to be put in there, or + # the localhost DAC fails on a certificate that does not match. See #10254. + $connectionStringKeyword["Trust Server Certificate"] = $true + } } try { $connContext.NonPooledConnection = $true @@ -815,21 +824,44 @@ function Connect-DbaInstance { $connContext.DatabaseName = $Database } catch { # Falling back to GetDatabaseConnection here would bring back the very leak this - # change is about, so the database goes into the connection string instead. As + # change is about. The database is switched on the connection of the copy further + # down instead, which needs no new connection and therefore cannot lose anything. + $databaseNeedsSwitch = $true + } + } + if ($connectionStringKeyword.Count -gt 0) { + # Rebuilding the connection string means opening a new connection from it, and that is + # only safe when the string still carries what it takes to log in. Once a SqlConnection + # has been opened with Persist Security Info=False, SqlClient hides the password, so + # the string we can read back no longer has one and the new connection would fail with + # "Login failed". Rejecting that is better than handing back a server that cannot be + # used - and the caller can always pass the instance name and a SqlCredential instead. + $connectionStringBuilder = New-Object -TypeName Microsoft.Data.SqlClient.SqlConnectionStringBuilder -ArgumentList $connContext.ConnectionString + $usesSqlLogin = -not $connectionStringBuilder["Integrated Security"] -and $connectionStringBuilder["User ID"] + if ($usesSqlLogin -and -not $connectionStringBuilder["Password"] -and -not $connContext.SqlConnectionObject.Credential) { + Stop-Function -Message "Cannot apply the requested settings to [$instance]: they need a new connection, and the password of the SQL Server login is no longer readable from the connection that was passed in. Connect with the instance name and -SqlCredential instead, or pass a SqlConnection that uses Persist Security Info=True." -Target $instance -Continue + } + + if ($databaseNeedsSwitch) { + # A new connection is opened anyway, so the database belongs in the string. As # Initial Catalog it is part of the pool key, which is what setting DatabaseName # achieves as well, so the reason #9505 forced a non pooled connection still holds. $connectionStringKeyword["Initial Catalog"] = $Database + $databaseNeedsSwitch = $false } - } - if ($connectionStringKeyword.Count -gt 0) { + if ($ApplicationIntent) { + # Assigned as a property above, which does not reach a fixed connection string + # either, so the context would report ReadOnly while the connection routes as it + # always did. + $connectionStringKeyword["ApplicationIntent"] = $ApplicationIntent + } + $changedKeywords = $connectionStringKeyword.Keys -join ", " Write-Message -Level Debug -Message "Connection context does not accept property assignments, using the connection string for: $changedKeywords" # The copy has to be disconnected first: setting the connection string of an open - # connection does not move it, and CurrentDatabase would silently stay where it was. - # The copy is a session of its own - a different SPID than the server that was passed - # in - so this does not touch the connection of the caller. + # connection does not move it. The copy is a session of its own - a different SPID + # than the server that was passed in - so this does not touch the caller. $connContext.Disconnect() - $connectionStringBuilder = New-Object -TypeName Microsoft.Data.SqlClient.SqlConnectionStringBuilder -ArgumentList $connContext.ConnectionString foreach ($keyword in $connectionStringKeyword.Keys) { # PowerShell routes property assignments on a connection string builder through its # dictionary indexer, so the keyword has to be used rather than the property name: @@ -838,6 +870,17 @@ function Connect-DbaInstance { } $connContext.ConnectionString = $connectionStringBuilder.ConnectionString } + if ($databaseNeedsSwitch) { + # Nothing else needs a new connection, so the database is switched on the one the copy + # already holds. That keeps every part of the authentication that lives on the + # SqlConnection rather than in its string, which rebuilding the string would lose. + # The copy is a session of its own, so this is not the leak of #10555 - the caller + # keeps its own database. + if ($connContext.SqlConnectionObject.State -ne "Open") { + $connContext.Connect() + } + $connContext.SqlConnectionObject.ChangeDatabase($Database) + } $server = New-Object -TypeName Microsoft.SqlServer.Management.Smo.Server -ArgumentList $connContext if ($Database -and $server.ConnectionContext.CurrentDatabase -ne $Database) { Write-Message -Level Warning -Message "Changing connection context to database $Database was not successful. Current database is $($server.ConnectionContext.CurrentDatabase). Please open an issue on https://github.com/dataplat/dbatools/issues." diff --git a/tests/Connect-DbaInstance.Tests.ps1 b/tests/Connect-DbaInstance.Tests.ps1 index c152ebf1486..8fc81fac3fa 100644 --- a/tests/Connect-DbaInstance.Tests.ps1 +++ b/tests/Connect-DbaInstance.Tests.ps1 @@ -565,6 +565,64 @@ Describe $CommandName -Tag IntegrationTests { } } + Context "connection is properly cloned from an open SQL authenticated SqlConnection" { + BeforeAll { + $PSDefaultParameterValues["*-Dba*:EnableException"] = $true + + # Once a SqlConnection has been opened with Persist Security Info=False, SqlClient hides the + # password: the connection string that can be read back no longer carries one. Anything that + # rebuilds the connection string from it therefore produces a connection that cannot log in, + # so the database has to be switched on the connection that already exists. See #10584. + $sqlAuthLogin = "dbatoolsci_clone_$(Get-Random)" + $sqlAuthPassword = "dbatools.IO_$(Get-Random)" + $splatSqlAuthLogin = @{ + SqlInstance = $TestConfig.InstanceMulti1 + Login = $sqlAuthLogin + Password = (ConvertTo-SecureString -String $sqlAuthPassword -AsPlainText -Force) + Force = $true + } + $null = New-DbaLogin @splatSqlAuthLogin + $null = Set-DbaLogin -SqlInstance $TestConfig.InstanceMulti1 -Login $sqlAuthLogin -AddRole sysadmin + + $sqlAuthConnectionString = "Data Source=$($TestConfig.InstanceMulti1);Initial Catalog=master;User ID=$sqlAuthLogin;Password=$sqlAuthPassword;Persist Security Info=False;Encrypt=False;Trust Server Certificate=True" + $sqlAuthConnection = New-Object -TypeName Microsoft.Data.SqlClient.SqlConnection -ArgumentList $sqlAuthConnectionString + $sqlAuthConnection.Open() + $serverFromSqlAuth = Connect-DbaInstance -SqlInstance $sqlAuthConnection + + $PSDefaultParameterValues.Remove("*-Dba*:EnableException") + } + + AfterAll { + $PSDefaultParameterValues["*-Dba*:EnableException"] = $true + + $null = $serverFromSqlAuth | Disconnect-DbaInstance + $sqlAuthConnection.Close() + $null = Remove-DbaLogin -SqlInstance $TestConfig.InstanceMulti1 -Login $sqlAuthLogin -Force -ErrorAction SilentlyContinue + + $PSDefaultParameterValues.Remove("*-Dba*:EnableException") + } + + It "hides the password from the connection string, which is what makes this case hard" { + $serverFromSqlAuth.ConnectionContext.ConnectionString | Should -Not -Match "Password=" + } + + It "clones when using parameter Database" { + $serverClone = Connect-DbaInstance -SqlInstance $serverFromSqlAuth -Database tempdb + $serverClone.ConnectionContext.ExecuteScalar("select db_name()") | Should -Be "tempdb" + $serverClone.ConnectionContext.ExecuteScalar("select suser_sname()") | Should -Be $sqlAuthLogin + $serverFromSqlAuth.ConnectionContext.ExecuteScalar("select db_name()") | Should -Be "master" + $null = $serverClone | Disconnect-DbaInstance + } + + It "refuses instead of returning a server that cannot log in" { + # This one needs a new connection, and the password for it is gone, so the command has to say + # so rather than hand back something that fails on first use. Connect-DbaInstance throws by + # default, which is why this is not a warning. + { Connect-DbaInstance -SqlInstance $serverFromSqlAuth -Database tempdb -NonPooledConnection } | + Should -Throw -ExpectedMessage "*no longer readable*" + } + } + Context "connection is properly cloned from an existing connection" { BeforeAll { $server = Connect-DbaInstance -SqlInstance $TestConfig.InstanceMulti1 @@ -598,7 +656,9 @@ Describe $CommandName -Tag IntegrationTests { $countPerCycle += @(Get-DbaProcess -SqlInstance $server -Database tempdb | Where-Object Program -match "dbatools").Count } - $countPerCycle[-1] | Should -Be $countPerCycle[0] + # Every cycle has to land on the same number, not just the first and the last. A sequence like + # 4, 5, 4, 5, 4 starts and ends the same way and still means the pool is churning. + ($countPerCycle | Select-Object -Unique).Count | Should -Be 1 } It "clones when using parameter ApplicationIntent" { From 0b904db43e2acd96c3e5c09fe2eece3e2bcb22a1 Mon Sep 17 00:00:00 2001 From: Andreas Jordan Date: Sat, 22 Aug 2026 19:41:06 +0200 Subject: [PATCH 4/4] Connect-DbaInstance - Cover the local dedicated admin connection, which CI can reach The CI instances are on the machine running the tests, so the local DAC path can be exercised there even though it cannot be in a lab of remote instances. The test skips where the instance is not local and runs on the runners, which is where the coverage was missing. It starts from Trust Server Certificate=False on purpose. With True it would pass even if the command did nothing, which is what made the earlier DAC test blind to this: on a context whose connection string is fixed, assigning TrustServerCertificate succeeds and reads back as True while the string still says False, and the string is what the new connection is built from. The assertion reads the setting back through a connection string builder rather than matching the string. A builder keeps the spelling it was given, so the same setting comes out as "Trust Server Certificate=True" or "TrustServerCertificate=True" depending on how the caller wrote it, and matching the second one against a string built from the first fails for no good reason. (do Connect-DbaInstance) --- tests/Connect-DbaInstance.Tests.ps1 | 32 +++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/Connect-DbaInstance.Tests.ps1 b/tests/Connect-DbaInstance.Tests.ps1 index 8fc81fac3fa..c8218b95304 100644 --- a/tests/Connect-DbaInstance.Tests.ps1 +++ b/tests/Connect-DbaInstance.Tests.ps1 @@ -7,6 +7,12 @@ param( BeforeDiscovery { $script:hasCredentialSspiProvider = $null -ne ("Dataplat.Dbatools.Connection.NetworkCredentialSspiContextProvider" -as [type]) + # A dedicated admin connection to an instance on the machine running the tests takes a different path + # than one to a remote instance: it goes to ADMIN:localhost and forces TrustServerCertificate, because + # the certificate of the instance does not have to match "localhost" (#10254). That path can only be + # exercised where the instance really is local, which is the case on the CI runners and not in a lab + # of remote instances. The value decides a Skip, so it has to exist at discovery time. + $script:instanceIsLocalHost = ([DbaInstanceParameter]$TestConfig.InstanceMulti1).IsLocalHost } Describe $CommandName -Tag UnitTests { @@ -563,6 +569,32 @@ Describe $CommandName -Tag IntegrationTests { $serverClone.ConnectionContext.ExecuteScalar($dacQuery) | Should -Be 1 $null = $serverClone | Disconnect-DbaInstance } + + It "keeps the forced certificate trust of a local dedicated admin connection" -Skip:(-not $script:instanceIsLocalHost) { + # A local DAC goes to ADMIN:localhost and forces TrustServerCertificate, because the + # certificate of the instance does not have to match "localhost" (#10254). On a context whose + # connection string is fixed, assigning that property succeeds and reads back as True while + # the string still says False - and the string is what the new connection is built from. So + # the trust has to be put into the string as well, or the local DAC fails on the certificate. + # Starts from Trust Server Certificate=False on purpose: with True the assertion would pass + # even if the command did nothing at all. + $localDacConnectionString = "Data Source=$($TestConfig.InstanceMulti1);Integrated Security=True;Encrypt=False;Trust Server Certificate=False" + [Microsoft.Data.SqlClient.SqlConnection]$localDacConnection = $localDacConnectionString + $serverForLocalDac = Connect-DbaInstance -SqlInstance $localDacConnection + try { + $serverClone = Connect-DbaInstance -SqlInstance $serverForLocalDac -DedicatedAdminConnection + + # Read back through a builder rather than matching the string: a connection string builder + # keeps the spelling it was given, so the same setting reads as "Trust Server Certificate" + # or "TrustServerCertificate" depending on how the caller wrote it. + $cloneStringBuilder = New-Object -TypeName Microsoft.Data.SqlClient.SqlConnectionStringBuilder -ArgumentList $serverClone.ConnectionContext.ConnectionString + $cloneStringBuilder["Trust Server Certificate"] | Should -BeTrue + $cloneStringBuilder["Data Source"] | Should -Match "^ADMIN:localhost" + $null = $serverClone | Disconnect-DbaInstance + } finally { + $null = $serverForLocalDac | Disconnect-DbaInstance + } + } } Context "connection is properly cloned from an open SQL authenticated SqlConnection" {