Invoke-DbaDbDecryptObject - Add DataPages to decrypt without a dedicated admin connection

[howarthcd]

(do Invoke-DbaDbDecryptObject)

Adds -NoDAC, which reads the encrypted definition straight from the raw data pages with DBCC PAGE instead of opening a dedicated admin connection and altering each object inside a rolled back transaction. Nothing is written to the database on this path. Omitting the switch keeps the original behaviour.

The reader lives in four new private functions. Get-EncryptedObjectImageValue is the engine, seeking the sysobjvalues clustered index and falling back to a page scan; ConvertFrom-DbccPageDump, ConvertFrom-EncryptedObjectChunk and Get-EncryptedObjectKeystream are split out so they can be unit tested without an instance.

Also fixed while here:

• A dedicated admin connection this command opens is now closed even when the run fails. The instance loop body is wrapped in try/finally, because an instance allows only one and a leaked session blocked every later run.
• Encrypted INSTEAD OF triggers on views are now found, and can only be decrypted with -NoDAC. The default method derives a known plaintext by rewriting the trigger as AFTER INSERT, which a view rejects.
• Multi database runs no longer carry objects between databases.
• Trigger discovery no longer costs one query per table, and IsEncrypted is added to the SMO init fields, which takes the test suite from 490s to 92s.
• -EncodingType warns when bound with -NoDAC, because it is ignored there.

Type of Change

• Bug fix (non-breaking change)
• New feature (non-breaking change, adds functionality)
• Breaking change (affects multiple commands or functionality, fixes # )
• Ran manual Pester test and has passed (Invoke-ManualPester -Path <command> -ScriptAnalyzer -Compliance)
• Adding code coverage to existing functionality
• Pester test is included
• If new file reference added for test, has is been added to github.com/dataplat/appveyor-lab ?
• Unit test is included
• Documentation
• Build system

Purpose

The existing command can only reach an encrypted definition through a dedicated admin connection, and it obtains the known plaintext it needs by altering every object inside a transaction that is rolled back. That rules the command out where a DAC is unavailable or where writing to the database, even transiently, is unacceptable, and an instance allows only one DAC at a time.

It also cannot decrypt an encrypted INSTEAD OF trigger defined on a view at all, because the known plaintext it builds rewrites the object as an AFTER trigger and a view rejects that.

Approach

-NoDAC derives the RC4 key from public metadata rather than obtaining a known plaintext, so it needs no dedicated admin connection and writes nothing. The scheme is set out under Learning below.

The ciphertext lives in sys.sysobjvalues, which is DAC-only through T-SQL, so the raw pages are read with DBCC PAGE ... WITH TABLERESULTS and the family GUID with DBCC DBINFO WITH TABLERESULTS. Both need sysadmin, checked up front so the failure is a clear message rather than a permission error midway through reading pages.

Rows are found by seeking the sysobjvalues clustered index, about five page reads whatever the size of the database, with a full page scan as the fallback and as the test oracle.

Comment-based help was updated throughout, including a note that a view trigger requires -NoDAC.

Commands to test

The help examples cover the normal paths. Beyond those:

# read the definitions without a DAC and without writing anything
Invoke-DbaDbDecryptObject -SqlInstance sql01 -Database db1 -NoDAC

# the case the default method cannot do at all
Invoke-DbaDbDecryptObject -SqlInstance sql01 -Database db1 -ObjectName MyViewTrigger -NoDAC

# the default method still behaves exactly as before
Invoke-DbaDbDecryptObject -SqlInstance sql01 -Database db1 -ObjectName MyProc

Learning

The part worth writing down is the obfuscation scheme itself. WITH ENCRYPTION is widely described as "not really encryption", but the actual construction does not appear to be written up anywhere, so it was reverse engineered for this change and is documented here in case it is useful to anyone else.

SQL Server stores the module text in sys.sysobjvalues.imageval, keyed on the object id with valclass = 1. The bytes are the UCS-2 (UTF-16LE) source text XORed with an RC4 keystream. There is no secret: the RC4 key is a SHA1 over 22 bytes of metadata that any sysadmin can already read.

seed(22)  = familyGuid(16) + objectId(4, little endian) + colId(2, little endian)
key       = SHA1(seed)
keystream = RC4(key)
plaintext = ciphertext XOR keystream          then decode as UTF-16LE

Four details are load bearing, and each of them fails in a way that is quiet rather than obvious:

• The GUID byte order is the .NET System.Guid layout, where the first three fields are little endian, not the order the GUID prints in. Using the string order produces a valid looking key and complete garbage. The value is dbi_familyGUID from DBCC DBINFO, and it is a property of the database family rather than of the object.
• colId is an input to the key, so a definition that spans more than one sysobjvalues row needs a separate keystream per row. Deriving one keystream for the whole object leaves the first chunk perfectly readable and everything after it mojibake, which reads like an encoding bug rather than a key bug. The chunks also have to be concatenated in colId order rather than in the order the rows were read.
• Because the key includes the object id, two objects with identical source text produce completely different ciphertext. That rules out any approach based on recognising repeated ciphertext.
• RC4 is a stream cipher, so the keystream depends only on the key and not on the data. Exactly as many bytes are generated as there is ciphertext, and the ciphertext length must be even, because UCS-2 is two bytes per character. An odd length silently decodes with the trailing byte dropped, which produces a plausible looking definition that is subtly truncated, so it is refused instead.

This is also why the existing method works at all. It never derives the key: it alters the object to a placeholder of exactly the same length inside a transaction that is rolled back, which yields a known plaintext and its matching ciphertext, and XORing the three values together recovers the original. That is a clever way around not knowing the key, but it costs a DAC, a write, and it cannot be applied to an object whose definition it is unable to legally rewrite, which is exactly the INSTEAD OF trigger on a view case. Deriving the key directly removes all three constraints.

[andreasjordan]

Nice piece of work. The reverse engineering in the PR description is the most useful thing I have read
in a dbatools PR for a while, and the comments in the reader explain the reasoning rather than the
syntax, which is exactly right for code where a wrong constant returns plausible rubbish instead of
failing. I went looking for problems in the new code and did not find any.

What I verified

Test suite, both editions, against the lab:

	PowerShell 7	Windows PowerShell 5.1
Result	40/40 passed	40/40 passed
Duration	125s	126s
Warnings, lab leftovers, module left loaded	none	none

Because the tests only ever run against one instance, I repeated the byte for byte check on every
version in the lab — SQL Server 2019, 2022 and 2025 across two hosts — with a small module, one stored
off row at 20 KB, one stored as a blob tree at 300 KB, and a view with non-ASCII characters. Every
definition came back exactly as submitted, the seek route was taken every time, and the
sys.sysobjvalues container id constant held everywhere.

I also checked three things I expected to be problems and they were not, so for the record:

• $db.Query() does not leave the connection in that database, so the reader is not a database
  context leak.
• The SetDefaultInitFields comment is right. GetDefaultInitFields for StoredProcedure goes from
  0 entries to 2 on a Connect-DbaInstance connection, and Connect-DbaInstance only ever sets
  fields for Database, Login and Agent.Job, so on any dbatools connection this really is
  additive.
• Not disconnecting the connection the -NoDAC path opens matches the rest of the module — 561 public
  commands call Connect-DbaInstance and 19 call Disconnect-DbaInstance.

Please fix in this PR

(You don't have to - I can also push those changes to this branch)

Object names are interpolated into T-SQL unescaped, in the DAC path.

This is not your bug — it is identical on development and predates the PR. I am asking for it here
because you are already rewriting this file, and we would rather not leave a known hole behind in a
command that has just been given a second code path.

An object name is allowed to contain a single quote, and a schema, object or parent name is allowed to
contain a closing bracket. $object.Schema, $object.Name and $object.Parent are interpolated
straight into three places:

• the OBJECT_ID('...') literal in the query that reads the secret
• the OBJECT_ID('...') literal in the known-secret query
• the EXEC ('...') that runs the known plaintext, where the name also sits inside [...], and
  $object.Parent is interpolated bare with no brackets at all

A name containing a quote therefore ends the literal early and whatever follows it runs as a further
statement in the same batch. I confirmed this against a lab instance: a procedure with a crafted name
caused an unintended statement to execute, and Invoke-DbaDbDecryptObject returned without error, so
nothing surfaces to the user. It runs over the DAC as sysadmin, which makes the shape a privilege
escalation — creating a procedure in one database is enough to have your statement run as sysadmin the
next time someone decrypts that database. I am deliberately not putting the name I used in this
comment.

The -NoDAC path is already clean and shows the pattern to follow: it resolves names to object ids in
PowerShell and only ever splices integers into SQL. Where the DAC path genuinely needs the name — the
ALTER statements — doubling ' and ] is enough.

-Force:$Force where there is no -Force parameter.

In the export block, New-Item -Path $destinationFolder -ItemType Directory -Force:$Force. This
command has no -Force parameter, so $Force is always $null and the switch is always off. Also
pre-existing, harmless in practice, and it would throw under StrictMode.

Minor, take or leave

• SetDefaultInitFields is never put back. It is additive as discussed, so this is only a note: a
  caller who had set their own init fields for StoredProcedure, View or UserDefinedFunction
  would have them replaced for the life of their connection.
• += inside loops appears 9 times in the reader and 6 in the command, while two other spots carry
  comments explaining why += was deliberately avoided there. $nextLevel += in the blob walk and
  $wantedObjectId += have the same shape as the ones you avoided. Probably fine at these sizes —
  just want to know it was a decision rather than an oversight.

Questions

• Which versions did you verify the on-disk format against? I have covered 2019, 2022 and 2025.
  Nothing between 2005 and 2016 has been exercised, and the chain-walk fallback and
  -MinimumVersion 9 both claim 2005.
• What happens on Azure SQL Database and Managed Instance? DBCC PAGE is not available there and
  -MinimumVersion 9 will not catch it, so I suspect the user gets a confusing failure rather than a
  clear one.
• Have you tried this against an AG readable secondary or a database snapshot — supported, refused, or
  simply not looked at yet?

On the name of the switch

Not something for you to decide, and not a blocker — this is for the maintainers, ultimately Chrissy.
Recording it here so the decision is not lost.

If the new path turns out to be strictly better — no write, no DAC, decrypts an INSTEAD OF trigger on
a view, and returns Unicode intact — then we will eventually want either to make it the only code path
or to let users switch the default without editing their scripts. dbatools already has the pattern for
the second one: Connect-DbaInstance defaults switches from configuration, for example
[switch]$EncryptConnection = (Get-DbatoolsConfigValue -FullName 'sql.connection.encrypt').

The thing worth thinking about now is that -NoDAC is a negative switch, and a negative switch makes
both of those awkward. A configuration item would read as "set nodac to true so that no DAC is used",
and opting back out at the call site would be -NoDAC:$false, which is a double negative. A positively
phrased parameter would leave both doors open at no cost today. I do not think this needs settling
before merge, but it is much cheaper to decide now than to deprecate a parameter later.

---

This text was created by Claude and reviewed by Andreas Jordan.

[howarthcd]

Thank you for this. The injection in particular is a much better catch than a rubber stamp, and taking
the time to reproduce it in the lab rather than just flagging the shape of it is what made it easy to
act on.

Everything under "Please fix in this PR" is done, along with both minor points. All three questions are
answered, and two of them turned into code changes rather than answers: Azure is now refused explicitly,
and the supported floor has moved to SQL Server 2008 on evidence rather than assumption. Where something
is still untested I have said so rather than reasoned about what should happen.

What I fixed

Object names are interpolated into T-SQL unescaped, in the DAC path.

The object id lookup that -NoDAC already did is now done for both methods, so each selects its rows by
id and only ever splices a number into the query text. Both OBJECT_ID('...') literals are gone.

Where the ALTER genuinely needs the name, three things changed:

• Every closing bracket in a schema or object name is doubled, so a name containing ] stays inside
  its identifier.
• The trigger's parent is written as [schema].[name] taken from the object, instead of interpolating
  the SMO object bare. It happens to render with brackets, so this was not broken, but it was relying
  on that rather than stating it.
• Every single quote in the finished statement is doubled in one place before it goes inside the
  EXEC literal, rather than the two quotes in the trigger case being doubled by hand. That is the
  change that actually closes the hole, because it escapes quotes that arrive in an object name and not
  only the ones written in the source.

I confirmed the same way you did, with a procedure whose name spells the end of the literal followed by
a CREATE TABLE. Against the previous code the command returned 1 of the 3 objects in the fixture and
created the table, without an error:

--- DAC ---   returned 1 objects
  FAIL  not returned exactly once: <name containing a quote, redacted>
  FAIL  not returned exactly once: dbatoolsci_bracket_]_proc
FAIL  the object name ran as a statement, table created

After the fix, 3 of 3 byte exact on both methods and no table. There are two integration tests for
this, in their own database because the counts asserted against the shared fixture are exact. They cover
a name containing a quote, a name containing a closing bracket, and a trigger whose parent name contains
one, on both code paths, and they assert the injected table does not exist.

I have kept the crafted name out of this comment for the same reason you did, but it is in the test
file, which seemed the right place for it once the hole is closed.

-Force:$Force where there is no -Force parameter.

Fixed, to plain -Force, matching the call in begin that creates ExportDestination. The comment
there now says why it must not be written as -Force:$Force, since it looks like a harmless tidy up.

Minor, take or leave

SetDefaultInitFields is never put back.

Now put back. You called it a note rather than a defect, but it turned out to be cheap and provably
correct, so it seemed worth doing rather than documenting.

What made it viable is that GetDefaultInitFields returns a copy rather than the live collection, so
capturing it before the set actually preserves something. The saved value goes back in the finally
that already exists for the dedicated admin connection, so it is restored whether the run succeeds or
fails, behind a null guard and in its own try/catch so it cannot mask the error that sent us there.

Measured on SQL Server 2025, reading IsEncrypted on 100 procedures on a fresh connection each time:

Init field state	Time
untouched	5955 ms
fields set	0 ms
set, then restored	6151 ms
untouched again, as a control	5897 ms

So the restore genuinely undoes the effect rather than just reporting a restored value. There is a test
that pins it: a caller sets their own fields, runs the command on their own connection, and gets them
back. It deliberately chooses a set that omits IsEncrypted so a missing restore leaves a visible
difference, checks all three types because View starts with ID and UserDefinedFunction starts
empty, and asserts the decrypt produced its script first so an early return cannot pass it.

+= inside loops.

A decision, and your instinct about where to look was right in one of the two places.

The rule I applied was per page, per dump line or per blob link versus per object, per chunk or per
batch. The three spots that avoid appending all iterate over something that grows with the database or
with the size of a definition: a page dump is about 410 lines, a blob root holds hundreds of links, and
the scan learns one next pointer per page read, which is 9,669 on the database I have been testing
against. The appends that remain are bounded by BatchSize, which is 32, or by the number of encrypted
objects, or by the rows of a single object, which is normally one.

$nextLevel += is not the same shape, and the fact that it reads like it is means the code was at
fault, not you. It appends $childNode, which is the whole array the @(for ...) immediately above it
built in one pass, so the per entry append has already been hoisted out. What remains fires once per
internal node, adding that node's entire child list. An internal node holds up to about 500 entries, so
a 4 MB definition reaches its roughly 1,000 data pages through a handful of internal nodes and therefore
a handful of appends. Its sibling $currentLevel += sits directly under a comment explaining exactly
this; that one had none, and now does.

$wantedObjectId += you are right about. It appends one item per iteration, exactly like the spots that
avoid it. The bound is the encrypted objects of one database, so it is tens of elements in practice, but
the inconsistency was real and it was a single line to remove, so it is collected in one pass now. I
have said in the comment that it is for consistency rather than because the append was measurably
costing anything, since claiming a performance motive for it would be inventing one.

The general rule is now stated once in the reader's .DESCRIPTION, so the remaining appends do not each
need defending.

One more, found while making the escaping changes.

$queryKnownPlain survived the object loop, so an object type matching no branch of the switch would
have been altered with the previous object's statement rather than reaching the check that reports a
missing known plain. Every type the discovery produces does match a branch, so it was unreachable, but
it is cleared per object now.

Questions

Which versions did you verify the on-disk format against?

The same three as you: SQL Server 2019 (15.0.4440.1), 2022 (16.0.4262.2) and 2025 (17.0.4060.2), all
four storage shapes byte exact, seek route asserted taken rather than silently falling back, and seek
and scan agreeing.

I went looking for the older end you named, and found something worth raising beyond this PR. SQL Server
2005 is unreachable for the module as a whole, not just for this command. On a real 2005 instance
(9.00.5000.00, Express with Advanced Services), the connection succeeds and Invoke-DbaQuery returns
results, but every SMO backed call fails:

	Result on SQL Server 2005
Invoke-DbaQuery	works
$server.VersionString, .Edition, .ProductLevel	all empty
$server.Databases, $server.Logins	throw 'CONNECTIONPROPERTY' is not a recognized built-in function name
Get-DbaDatabase	throws Invalid column name 'is_cdc_enabled'
Get-DbaLogin	throws CONNECTIONPROPERTY
Invoke-DbaDbDecryptObject -NoDAC	throws CONNECTIONPROPERTY, at $server.Databases, before a page is read

CONNECTIONPROPERTY and is_cdc_enabled both arrived in SQL Server 2008, and the failing queries are
SMO's own rather than ours. The SMO in dbatools.library 2026.5.3 is 18.100.0.0, and SqlManagementObjects
dropped 2005 well before that. So the command never reaches the page reading code on 2005, and I cannot
prove the on disk format there through the command whatever the format actually does.

This is not specific to the new path. The default method fails on 2005 one step sooner: its first action
is the RemoteDacConnectionsEnabled check, and Get-DbaSpConfigure throws Failed to retrieve data for this request enumerating the configuration through SMO. So 2005 is out of reach whichever method is
chosen, which is why raising the floor describes the command rather than only the switch.

It is worth setting this beside #9821, which is the nearest existing report and was closed as an
unsupported combination of PowerShell 7, TLS 1.0 and SqlClient. What I saw is a layer above that and a
different thing: the connection itself succeeded from PowerShell 7, TLS 1.0 and all, and Invoke-DbaQuery
returned results, so it is not environmental. SMO's own queries are what fail, and they would fail the
same way for anybody on 2005. Whether that deserves an issue of its own, a line in the README, or
nothing at all is a maintainer decision rather than mine, but it is a firmer answer than the one that
thread ended on, and I would rather hand it over than leave it in a PR comment. The repository guidance
that suggests -MinimumVersion 9 for SQL 2005 support is the other thing it touches.

That made -MinimumVersion 9 on this path a floor the dependency cannot deliver, so it is now 10, and
the help states SQL Server 2008 or later for the command as a whole rather than for the switch.

One caveat goes with it, and the comment in the code carries the same note, because the change otherwise
looks like a working guard when it is not. Raising the floor does not refuse a 2005 instance.
Connect-DbaInstance only applies MinimumVersion when it can read VersionMajor, and on 2005 that
property comes back empty for the same reason everything else does, so 10 admits the connection
exactly as 9 did and the failure still happens later inside SMO. I verified that rather than assuming
it. The floor is there to state what is supported, not because it can be enforced at the one version
where it would matter.

SQL Server 2008 is a different story, and it passes cleanly. On 10.0.5500.0, Express Edition 64-bit:

	Result on SQL Server 2008
SMO	enumerates databases normally, so the module works here
sys.dm_db_database_page_allocations	absent, as expected before 2012
Route taken	seek
In row definition, 76 characters	byte exact
Off row definition, 20,094 characters	byte exact
Three level blob tree, 300,093 characters	byte exact
View with non ASCII content	byte exact
Seek, page list and page chain	all three agree

Two things in there are worth more than the pass itself. The seek route was taken on 2008, so the 20
byte index record, the leading valclass in the key and the leftmost entry behaving as negative
infinity all decode correctly on a version seventeen years older than the newest one tested. And because
the page allocations DMV does not exist on 2008, the page chain walk is what a scan genuinely has to do
there rather than something a switch forced, which is the first time that fallback has run against a
version that actually needs it.

So the on disk format is now verified at both ends of the range the module can reach: 2008 at the
bottom, then 2019, 2022 and 2025. The versions in between are not covered, but they are bracketed rather
than beyond the evidence, which I think is a materially different position from the one we were both in
before.

One caveat on those two results. The 2005 and 2008 instances were stood up for this exercise and are
already gone. Neither is part of CI and neither can be re-run, so both are point in time measurements
rather than something the suite protects. I would rather say that plainly than let a version table imply
continuous coverage. If a 2008 instance is worth having permanently, that is a conversation about the CI
matrix rather than about this PR.

What happens on Azure SQL Database and Managed Instance?

You were right, and it is now refused explicitly. Neither platform supports DBCC PAGE, and
MinimumVersion does not catch either of them because both report a version this command is happy with,
so -NoDAC would have got as far as the first DBCC and failed on something that did not name the real
problem.

The check runs before anything else on that path:

if ($server.DatabaseEngineEdition -eq "SqlManagedInstance") {
    $azurePlatform = "Azure SQL Managed Instance"
} elseif ($server.DatabaseEngineType -eq "SqlAzureDatabase") {
    $azurePlatform = "Azure SQL Database"
}

Both properties are needed. Connect-DbaInstance -AzureUnsupported tests only
DatabaseEngineType -eq "SqlAzureDatabase", which would let a Managed Instance through, so it was not
usable here. The engine edition is tested first so the message names the right platform whichever way
the engine type reads, which is the same reason Copy-DbaLogin and Copy-DbaDatabase identify a
Managed Instance that way.

It is scoped to -NoDAC, so the default method is untouched, and it sits ahead of the sysadmin check so
an Azure user is not told to reconnect as a sysadmin, which would not help them. The limitation is in
the -NoDAC help as well, since that is where somebody looks before they hit the error.

Two things I want to be straight about rather than let the code imply otherwise:

• This is not verified against a live Azure instance, because I do not have one. The property values
  follow the existing patterns in the module rather than a measurement, and no test can reach the
  branch. It is written so that a wrong property name fails open, meaning the guard simply never fires,
  rather than blocking a normal instance. I confirmed the latter on SQL Server 2019, 2022 and 2025,
  which all report Enterprise and Standalone and evaluate the guard to false.
• I deliberately did not tell the user to drop -NoDAC in the message. The default method calls
  Get-DbaSpConfigure -ConfigName RemoteDacConnectionsEnabled first, and I have not established what
  that does on Azure SQL Database, so advising it could send somebody into a second and less clear
  failure. The message states the fact and stops there. If you know that the dedicated admin connection
  method does work on either platform, it is worth saying so in the help.

Have you tried this against an AG readable secondary or a database snapshot?

Both, and -NoDAC works against each.

A readable secondary was tested on SQL Server 2019. That is the result I would hope for rather than a
surprise: the path only ever issues DBCC PAGE, DBCC DBINFO and reads of system views, so there is
nothing in it that a read only replica can refuse.

A database snapshot was tested on SQL Server 2022, holding one in row definition and one 20 KB
definition stored off row. Both came back byte exact, so the blob walk follows into a snapshot as well
as the in row read. The part I did not want to assume is the family GUID, since it is an input to the
keystream and a snapshot reporting a different one from its source would break decryption outright
rather than subtly. It does not:

family GUID source   b81626bb-bfa0-44d8-93e1-07cfbab554d2
family GUID snapshot b81626bb-bfa0-44d8-93e1-07cfbab554d2

The same run shows the contrast with the default method, which fails on a snapshot for the reason you
would expect, since it obtains its known plaintext by altering the object:

Failed to update database "dbatoolsci_snapshot_..._snap" because the database is read-only.

So a read only database is a second case where -NoDAC is the only method that can work, alongside the
INSTEAD OF trigger on a view, and for the same underlying reason. Both are now in the help. The
snapshot half has a test; the readable secondary half cannot have one here, because no instance
available to me provides one.

On the name of the switch

Agreed on both counts: worth settling now rather than deprecating later, and not mine to decide. The
double negative you describe is real, and the configuration precedent you point at is the right one to
measure a name against.

I am deliberately not proposing an alternative, because a name offered by the author of the PR tends to
become the decision by default, and this one belongs with the maintainers. I am happy to rename it here
once they have picked, and it is cheap to do while nothing depends on the name.

What I verified

Test suite, against the lab:

	PowerShell 7
SQL Server 2022 (16.0.4262.2)	44/44
SQL Server 2025 (17.0.4060.2)	44/44

That is the previous 40 plus two for the quoting of object names, one for the init fields and one for
reading a database snapshot. Two things carry no test of their own: the Azure refusal, because the
branch cannot be reached without an Azure instance, and the availability group readable secondary,
because no instance available here can provide one.

Repository wide compliance checks pass on both runs, no ScriptAnalyzer findings on the changed files,
PSUseCompatibleSyntax at 3.0 reports none, and the lab is left with no leftover databases and no
dedicated admin connection sessions.

---

This text was created by Claude and reviewed by Chris Howarth.

[andreasjordan]

Thank you for this, and particularly for the parts where you went and measured something rather than
reasoned about what ought to happen. Standing up a 2005 and a 2008 instance to answer a question about
a version floor is well beyond what the question asked for, and saying plainly that both are gone and
neither is protected by CI is the right way to report it.

I have re-verified everything rather than taking the summary on trust.

Confirmed fixed

	How I checked	Result
Object name injection	A procedure whose name ends the literal and appends a CREATE TABLE, plus one whose name contains a closing bracket, both methods	3/3 objects returned on each method, no table created
-Force:$Force	Reading the export block	plain -Force, matching begin
Init fields restored	Caller sets their own field list on a connection, runs the command on it, reads the list back	restored exactly
Test suite	Harness, both editions	44/44 on PowerShell 7 and on Windows PowerShell 5.1, no warnings, module not left loaded
Both methods across versions	Small, off row at 20 KB, Unicode view and a scalar function, on 2019, 2022 and 2025	4/4 byte exact on each version, on both methods

The separation in the escaping is the part I looked hardest at, since it is where a fix like this
usually goes wrong: $knownPlain keeps the un-doubled statement, because that is what SQL Server
actually stores, and only $queryKnownPlainLiteral is doubled for the EXEC. Getting those the wrong
way round would still have decrypted, because the padding means the statement text never reaches the
XOR, and it would have been invisible. It is right.

Your point about doubling in one place rather than by hand is the part that actually closes the hole,
and it is worth the comment you gave it.

One more, and I think it needs fixing before merge

Two objects can share a key in $objectIdMap, and the wrong definition is returned.

The map is keyed "$schema.$name". Both halves may themselves contain a dot, so the key is ambiguous:

• schema [a], procedure [b.c]
• schema [a.b], procedure [c]

Both key as a.b.c, so the second row read overwrites the first, and the object that lost the race is
then decrypted with the other one's id. Fixture with both, all encrypted, on SQL Server 2022:

  [NoDAC] returned 2/2
      [a].[b.c] -> WRONG DEFINITION      (carries the definition of [a.b].[c])
      [a.b].[c] -> correct
  [DAC]   returned 2/2
      [a].[b.c] -> WRONG DEFINITION      (comes back as the space padding)
      [a.b].[c] -> correct

Nothing warns. The object is returned under its own schema and name, with another object's body in
Script, which is the failure class the rest of this PR goes to some length to avoid.

Two things about scope, so this is not read as worse than it is. It needs a colliding pair to exist —
a single object with a dot in its name is fine — and it is not a security issue, since a name cannot
reach the query text any more. But it now affects both methods rather than only -NoDAC, because the id
lookup became shared, and that is a direct consequence of the change I asked you to make.

For comparison, development returns 0 of 2 for the same fixture: OBJECT_ID('a.b.c') resolves to
nothing and both objects are silently skipped. So the behaviour moves from silently returning nothing to
silently returning the wrong thing. Both are wrong, but the second is the worse of the two, and I would
rather not trade one for the other in the same PR that removed the injection.

I am not going to prescribe a shape. The general point is only that a single string built from two
identifiers cannot be an unambiguous key when either may contain the separator, so the lookup needs to
keep the two parts apart — or to carry the id from the query the discovery already runs, so there is no
name-based lookup to get wrong.

SQL Server 2005

This is worth more than a paragraph in a PR thread, and I agree it should not stay in one. SMO's own
queries failing on 2005 describes the whole module, not this command, and it is a firmer answer than
#9821 ended on — the connection succeeding and Invoke-DbaQuery working is what separates the two.
Please open it as its own issue and link it here; the repository guidance recommending
-MinimumVersion 9 for 2005 support is the part that most needs correcting on the back of it.

The caveat you attached to raising the floor to 10 is the right one to state, and stating it in the
code as well as the comment is better than leaving a guard that reads as if it enforces something it
cannot.

Accepted without verification

Recording these so it is clear what the evidence does and does not cover:

• The Azure refusal. I have no Azure instance either, so I have read the branch rather than run it.
  Deciding it should fail open, so a wrong property name never blocks a normal instance, is the right
  way round, and I confirmed the guard evaluates false on 2019, 2022 and 2025.
• The availability group readable secondary. Reasonable on the face of it, and untestable here.
• The 2005 and 2008 measurements, which no longer exist to re-run.

Nit, take or leave

$savedInitFieldsProcedure and its two siblings are not reset between instances in the foreach. They
are reassigned at the top of the try every time, so the stale value can only be reached if
GetDefaultInitFields itself throws on the first line — which is close to unreachable. One line if you
think it is worth it.

On the name of the switch

Agreed, and leaving it with the maintainers is the right call. Nothing further from me.

---

This text was created by Claude and reviewed by Andreas Jordan.

[andreasjordan]

Picking the switch question back up, since you said you would happily rename once the maintainers chose
and it is free to do while nothing depends on the name.

The shape

Replace -NoDAC with a method parameter whose default comes from configuration:

[ValidateSet("DAC", "DataPages")]
[string]$DecryptionMethod = (Get-DbatoolsConfigValue -FullName "commands.Invoke-DbaDbDecryptObject.decryptionmethod" -Fallback "DataPages")

Set-DbatoolsConfig -FullName "commands.Invoke-DbaDbDecryptObject.decryptionmethod" -Value "DataPages" -Initialize -Validation string -Description "Which method Invoke-DbaDbDecryptObject uses to read an encrypted definition. DataPages reads the raw data pages and writes nothing. DAC uses a dedicated admin connection and briefly alters each object inside a rolled back transaction."

This is an existing pattern rather than a new one. Add-DbaAgReplica already carries [ValidateSet(...)]
and a configuration backed default on the same parameter, and command scoped settings live in
private\configurations\settings\commands.ps1 as commands.<Command-Name>.<setting>.

DataPages for the new method because it names the mechanism rather than the absence of the other one,
and because it is the language the help already uses. RawPages or PageRead would do as well.
Direct and Fast would not, since they imply a judgement nobody has made. DAC stays as it is,
because that is the term the documentation already uses.

Our suggestion: ship with DataPages as the default

Not the conservative choice, deliberately. The new method is better on every axis a user cares about:

• It writes nothing. The current default alters every object inside a rolled back transaction, which is
  a write against a production database, in a command people reach for during disaster recovery.
• It needs no dedicated admin connection, so no RemoteDacConnectionsEnabled, and no contention for the
  one DAC slot an instance has.
• It decrypts objects the DAC method cannot reach at all: an INSTEAD OF trigger on a view, and
  anything in a read only database or a snapshot.
• It returns the definition in the encoding SQL Server stored, so Unicode survives without the user
  having to guess at -EncodingType.

The evidence behind it is good. Byte exact on 2008, 2019, 2022 and 2025 in your testing, and I have
independently confirmed both methods byte exact on 2019, 2022 and 2025, with the seek route asserted
rather than assumed.

There is precedent in the module for exactly this move. sql.connection.encrypt ships as $true with
sql.connection.trustcert and sql.connection.allowtrustcert as the escape hatches: the stricter
behaviour became the default, the configuration was there for anyone it did not suit, and the change was
documented rather than hidden. Andreas made a comparable change in Connect-DbaInstance some years ago
and it worked out. If somebody complains, the answer is one line of configuration rather than a
rewrite of their scripts, which is the whole point of putting the default behind a setting.

That said, this is Chrissy's call, not ours. What we would ask either way is that the endpoint gets
decided once now, rather than shipping DAC as the default and letting the flip drift indefinitely.

One thing to settle before the default flips

Azure. -NoDAC is now explicitly refused on Azure SQL Database and Managed Instance, and you were
straight about not having established what the DAC method does there. If the DAC method does work on
either platform, then making DataPages the default takes a capability away from those users, and the
default should stay DAC until that is known. If neither method works on Azure, which is what we both
suspect, there is no regression and the decision is easy.

This is the only case we can find where the flip could remove something rather than add it. Everything
else is strictly better or neutral.

Two things that bite with a configuration backed default

ValidateSet does not validate a default value. Verified rather than assumed: a parameter whose
default is a string outside the set passes straight through, while the same string supplied by a caller
is rejected. So a typo in the configuration reaches the switch unvalidated, matches no branch, and the
object is silently skipped. -Validation string only checks that it is a string. The command needs an
explicit check in begin that the resolved value is one of the two, with a message naming the setting
it came from. Worth knowing separately that this applies to the existing Add-DbaAgReplica usage too.

The -EncodingType warning needs rewording, and matters more with this default. It currently fires
when -EncodingType is bound together with -NoDAC. Once DataPages is the default, binding
-EncodingType warns by default, at a user who typed no switch to provoke it. The message should name
the method rather than the switch, and Test-Bound -ParameterName DecryptionMethod will distinguish a
user who chose the method from one who inherited it from configuration, if the wording wants to say so.

Documentation

If the default changes, it needs saying somewhere a user reads before they hit it, not only in the
parameter help: what the default now is, that it writes nothing where the old default wrote, that the
old behaviour is one configuration setting away, and that Azure is refused on the new path. The
sql.connection.encrypt change is the model for how much to say.

We would also like the command to state at verbose level which method it actually used. With the method
resolvable from configuration, "did this run write to my database" stops being answerable from the
command line alone, and that is a question somebody will eventually need to answer after the fact.

---

This text was created by Claude and reviewed by Andreas Jordan.

[howarthcd]

Thank you for this - the collision was a real gap in what looked like a complete fix, and picking the switch name back up rather than letting it drift was the right call.

Collision bug - fixed

$objectIdMap was keyed "$schema.$name". Either half can contain a dot, so schema [a], object [b.c] and schema [a.b], object [c] both keyed as "a.b.c", and whichever row the lookup read last silently overwrote the other's id. Fixed by removing the lookup entirely rather than patching the key: both methods now read the id straight from SMO's own ID property on each object, confirmed against a live instance to equal sys.objects.object_id for a stored procedure, a view, a user-defined function and a trigger. ID rides along with the init fields the command already forces for StoredProcedure, View and UserDefinedFunction, so reading it costs no extra round trip, and the query that built the old map is gone entirely.

Reproduced your exact fixture against the previous code on both methods (both returned the wrong definition for one of the pair), then confirmed 2 of 2 byte exact, each with its own definition, once the id came from SMO. Two new integration tests cover it, in their own database. Full suite: 46/46 passed, compliance 21/21, 0 parse/format/analyzer/PS3 findings, no leftover databases or DAC sessions afterward.

The nit - already fixed before it was raised

Checked against the current file rather than assumed: $savedInitFieldsProcedure and its two siblings are already reset to $null at the top of every instance iteration, ahead of the try that reassigns them, in the commit pushed the night before this comment. Nothing to change here.

Switch renamed to -DataPages

Adopted the positively-phrased name. -NoDAC is gone; the parameter is now [switch]$DataPages, same meaning, same default - omitting it still uses the DAC method, exactly as before. Every mention in the help, the messages and the test suite is updated to match; full suite passes with the new name.

What wasn't changed, and why

Deliberately narrower than the full shape proposed, because the rest is a product decision rather than a naming one:

• No DecryptionMethod / ValidateSet parameter, no configuration-backed default. The parameter is still a plain switch. The bigger design - a string parameter, a commands.invoke-dbadbdecryptobject.decryptionmethod setting, and everything that comes with a resolvable default - is the maintainers' call, not something to build ahead of that decision.
• The default is unchanged. Omitting -DataPages still uses the DAC method. Nothing about which method runs by default has moved.
• The ValidateSet-does-not-validate-a-default guard isn't needed yet. That risk only exists once a configuration-backed default is introduced; a plain switch has no default value to validate.
• The EncodingType warning text now says "DataPages" instead of "NoDAC", so it doesn't reference a removed parameter, but it doesn't yet use Test-Bound to distinguish a user who chose the method from one who inherited it from configuration - there's no configuration to inherit from yet.
• No verbose-level log of which method ran. Right now that's still fully answerable from the command line alone - it's exactly whether -DataPages was passed - so the stated reason for adding it doesn't apply until a configuration-backed default exists. Happy to add it in whichever change introduces that default.
• The Azure question is still open. Whether the DAC method works on Azure SQL Database or Managed Instance is still unestablished, which matters if the default ever flips in that direction.

Awaiting instruction

The default-method toggle - whether DataPages should eventually become the default over DAC - is not decided here and shouldn't be read into this rename. Renaming the switch only removes the double-negative naming problem; it says nothing about which behaviour should ship as the default. That question is explicitly parked for Chrissy to weigh in on, along with the rest of the shape above.

The SQL Server 2005 finding is filed as its own issue: #10583.

---

This text was created by Claude and reviewed by Chris Howarth.