Set-DbaPrivilege - Write secedit's working database to temp, not cwd - #10544
Open
potatoqualitee wants to merge 4 commits into
Open
Set-DbaPrivilege - Write secedit's working database to temp, not cwd#10544potatoqualitee wants to merge 4 commits into
potatoqualitee wants to merge 4 commits into
Conversation
secedit /configure /db resolves a bare filename against the process's current directory, not $env:TEMP. The relative "secedit.sdb" argument left secedit.sdb and its secedit.jfm journal file behind wherever the caller happened to be running the command. Point /db at an absolute $temp path instead, and clean up the database and journal file alongside the exported cfg. (do Set-DbaPrivilege) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Make the secedit database name unique per invocation (Get-Random token) so two concurrent Set-DbaPrivilege runs against the same computer can no longer collide on, or delete, each other's temp database/journal files. Test suite fixes: - Fix the Invoke-Command2 mock in the regressions Describe block, which discriminated calls by argument count and started misrouting the cleanup call once it gained an -ArgumentList; it now matches on the scriptblock body instead. - Harden the integration test's AfterAll revert: check secedit's exit code instead of ignoring failures, use a Get-Random-suffixed cfg/db/jfm set instead of fixed names, fix a SID-removal regex that could strip a different SID sharing the same prefix, and clean up in a finally block. - Replace the two vacuous "artifact absent" It blocks with one test that proves the database is actually written under temp during the run (via a FileSystemWatcher on Path/Filter/EnableRaisingEvents, not a splat, since splatting New-Object applies the keys to New-Object itself rather than the constructed object) instead of only inferring it from post-hoc absence, and asserts with EnableException instead of swallowing warnings. (do Set-DbaPrivilege)
- Tokenize the shared secpolByDbatools.cfg name too, not just the .sdb/.jfm pair: the cfg file was still fixed, so two concurrent Set-DbaPrivilege runs against the same computer could still overwrite or delete each other's cfg mid-flight and apply the wrong privileges. The token is now generated once before the export step and threaded through export, configure, and cleanup. - Splat all three Invoke-Command2 calls (each now has 4-6 named parameters) with aligned, purpose-named hashtables, and give the token parameter a distinct, purpose-specific name in each scope (ExportRunToken/ ConfigureRunToken/CleanupRunToken at the remote scriptblocks, seceditRunToken at the caller) instead of reusing $dbToken/$DbToken across all of them. - Fix the regressions unit test's Invoke-Command2 mock, which still wrote/read a fixed secpolByDbatools.cfg while production code now uses a tokenized name; the mock now derives the same token from $ArgumentList. - Guard the integration test's AfterAll revert on a $preTestStateCaptured flag set only after BeforeAll successfully determines whether the user already held the privilege, so a BeforeAll failure can no longer be misread as "didn't have it" and trigger an unwanted revert. - Replace the immediate post-call Get-Event with a bounded Wait-Event, since FileSystemWatcher delivers Created events asynchronously and a same-tick poll can race the event queue. - Track the exact file path(s) the watcher observed being created and assert those specific paths are gone afterward, instead of re-scanning temp with the same wildcard (which can't distinguish this invocation's artifact from a leftover or a concurrent run). Add an explicit check for the legacy hardcoded secedit.sdb/secedit.jfm names in the working directory, so a regression back to the pre-fix filename would still be caught. - Splat Register-ObjectEvent and combine the four Get-ChildItem existence checks into one splatted call with -Include covering both the tokenized and legacy filenames. (do Set-DbaPrivilege)
AfterAll set $PSDefaultParameterValues["*-Dba*:EnableException"] and never removed it, so the enable leaked into whatever test file ran next in the same session. The file's own tests pass either way; the repo-wide check in dbatools.Tests.ps1 caught it as "2 enabled, 1 removed" during a full suite run. (do Set-DbaPrivilege) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A user reported that
Set-DbaPrivilegeleavessecedit.sdbandsecedit.jfmbehind in the current working directory after it runs.Root cause: the command calls
secedit /configure /cfg $tempfile /db secedit.sdb /areas USER_RIGHTS /overwrite /quiet./dbtakes a bare, relative filename, whichsecedit.exeresolves against the process's current directory - not$env:TEMP- so the working database (and its.jfmjournal file) land wherever the caller's shell happened to be.public/Set-DbaPrivilege.ps1:secedit's three working files - the exported.cfg, the/dbdatabase.sdb, and its.jfmjournal - all now live under an absolute$temppath, named with a singleGet-Randomtoken generated once per invocation and threaded through the export, configure, and cleanupInvoke-Command2calls via splatted-ArgumentList. Tokenizing all three (not just the.sdb/.jfmpair) matters: twoSet-DbaPrivilegeruns against the same computer at the same time would otherwise still collide on, or delete, each other's shared.cfgfile and apply the wrong privileges. Cleanup removes all three tokenized files alongside the working database.tests/Set-DbaPrivilege.Tests.ps1: added a real (non-mocked)IntegrationTestsDescribeblock. A single combined test grantsCreateGlobalObjects, watches temp with aFileSystemWatcher(via a boundedWait-Event) to positively confirm the tokenized.sdbdatabase is actually created under temp during the run, tracks the exact path(s) observed and confirms they're gone afterward, and separately checks the working directory for both the tokenized pattern and the legacy hardcodedsecedit.sdb/secedit.jfmnames (so a regression back to the pre-fix filename would still be caught).AfterAllreverts the granted privilege unless the user already held it, only doing so onceBeforeAllhas actually confirmed the prior state, validatingsecedit's exit code, and cleaning up its own scratch files in afinallyblock.This same bug also exists in the compiled C# port (
dbatools.library,SetDbaPrivilegeCommand.cs) - see the companion PR at dataplat/dbatools.library.Follow-up fixes from automated review (2 rounds)
Invoke-Command2mock in theSet-DbaPrivilege regressionsunit test, which discriminated "set privileges" vs. "cleanup" calls by$ArgumentList.Count, and later derives the same per-run token dbatools now uses so its fabricated.cfgpath matches what the real scriptblock reads/writes.AfterAllthat could match a SID as a string-prefix of a different, longer SID sharing the same prefix.Invoke-Command2/Register-ObjectEvent/Get-ChildItemcall that grew to 3+ parameters, with purpose-specific variable names for the per-run token in each scope (seceditRunToken,ExportRunToken,ConfigureRunToken,CleanupRunToken) instead of reusing one name across scopes.AfterAllrevert on a$preTestStateCapturedflag so aBeforeAllfailure can't be misread as "user didn't already have the privilege" and trigger an unwanted revert.Get-Eventwith a boundedWait-Event, sinceFileSystemWatcherdeliversCreatedevents asynchronously and a same-tick poll can race the event queue.Verification limitation
My local shell isn't elevated, and both
secedit.exeand dbatools' ownTest-ElevationRequirementcheck require Administrator rights for a localhost target. I confirmed:UnitTests-tagged tests (parameter validation + theMock-based regression test) pass locally.IntegrationTeststest fails locally, but only for the expected/honest reason:Set-DbaPrivilegethrowsConsole not elevated, but elevation is required...via its ownTest-ElevationRequirementcheck, and theAfterAllrevert fails withsecedit /export failed with exit code 740(ERROR_ELEVATION_REQUIRED) for the same reason. Neither theFileSystemWatcherassertion nor the/dbpath fix has been exercised end-to-end on my machine - that needs a run on the elevated self-hosted CI runner to be conclusive.Test plan
Set-DbaPrivilegeIntegrationTestson the elevated self-hosted runner and the new test passesUnitTestspass locally馃 Generated with Claude Code