Skip to content

Set-DbaPrivilege - Write secedit's working database to temp, not cwd - #58

Open
potatoqualitee wants to merge 1 commit into
mainfrom
fix/set-dbaprivilege-secedit-tempdir
Open

Set-DbaPrivilege - Write secedit's working database to temp, not cwd#58
potatoqualitee wants to merge 1 commit into
mainfrom
fix/set-dbaprivilege-secedit-tempdir

Conversation

@potatoqualitee

Copy link
Copy Markdown
Member

Summary

Companion fix to dataplat/dbatools's PR for the same bug report: Set-DbaPrivilege leaves secedit.sdb/secedit.jfm behind in the current working directory.

SetDbaPrivilegeCommand.cs's MainScript constant calls secedit /configure /cfg $tempfile /db secedit.sdb /areas USER_RIGHTS /overwrite /quiet - the same relative /db argument as the PowerShell source it was ported from. secedit.exe resolves a bare filename for /db against the process's current directory, not $env:TEMP, so the working database and its .jfm journal file leak into whatever directory the caller's shell was running in.

  • MainScript: point /db at $temp\secedit.sdb ($temp is already set earlier in the same scriptblock).
  • CleanupScript: remove $temp\secedit.sdb and $temp\secedit.jfm alongside the existing secpolByDbatools.cfg cleanup.

The retired PowerShell source (dbatools/private/retired/Set-DbaPrivilege.ps1) still carries the original bug, but it's dead code on the libmigration branch, not shipping - not touched here.

Verification

  • dotnet build dbatools.sln -c Debug compiles dbatools.computer cleanly with this change (verified both with and without the patch to confirm a pre-existing, unrelated XmlDoc2CmdletDoc post-build failure in a fresh worktree - missing SMO assembly resolution on net8.0, across many unrelated projects - is not caused by this change; the actual dbatools.computer.dll output for both net472 and net8.0 built successfully).
  • No dedicated MSTest coverage exists for SetDbaPrivilegeCommand currently, so none was added here; the behavioral regression test lives in the PowerShell repo's tests/Set-DbaPrivilege.Tests.ps1.
  • Could not exercise this end-to-end locally - secedit.exe requires Administrator rights for a localhost target and my shell isn't elevated.

Test plan

  • dotnet build dbatools.sln -c Debug succeeds for dbatools.computer (both target frameworks)
  • Elevated integration run confirms no secedit.sdb/secedit.jfm leak into cwd

🤖 Generated with Claude Code

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>
@claude

claude Bot commented Aug 8, 2026

Copy link
Copy Markdown

Review

Solid, well-scoped fix — the root cause analysis (secedit /configure /db resolving a bare filename against cwd, not $env:TEMP) is correct and the fix follows the exact same $temp\<file> unquoted-argument pattern already used for /cfg elsewhere in ExportScript/MainScript, so it's consistent with the surrounding code rather than introducing a new idiom.

Correctness

  • $temp\secedit.sdb as the /db value is right, and reuses $temp, which is already computed earlier in the same MainScript scriptblock — no new variable needed.
  • CleanupScript's added Remove-Item $temp\secedit.sdb, $temp\secedit.jfm -Force -ErrorAction SilentlyContinue correctly scopes -ErrorAction SilentlyContinue to just the new files (the .jfm journal isn't guaranteed to exist), while leaving the pre-existing secpolByDbatools.cfg cleanup's error behavior untouched. Good restraint — didn't widen the blast radius of the existing line.
  • Comments are why-focused (secedit's cwd-resolution quirk), matching the repo's comment style guidance.

Missing version bump

CLAUDE.md is explicit: "Bump the version whenever you change runtime behavior — package upgrades, connection/auth logic, cmdlet behavior, anything beyond docs or tests — in the same PR." This is a cmdlet behavior fix (Set-DbaPrivilege no longer leaks files into cwd), but neither dbatools.library.psd1's ModuleVersion (still 2026.8.2) nor project/dbatools/dbatools.csproj's AssemblyVersion/FileVersion/Version (still 0.10.0.84) were touched. Since consumers pin this package, an unbumped merge makes the fix invisible until an unrelated release drags it along. Worth running the bump skill before merge.

Test coverage

The PR is upfront that there's no MSTest coverage for SetDbaPrivilegeCommand and that end-to-end verification needs elevation this environment doesn't have — reasonable given the constraints. One cheap, non-elevated addition that would still catch a future regression: a reflection-based test asserting the private MainScript/CleanupScript const strings contain secedit.sdb alongside $temp (and don't contain the old bare /db secedit.sdb pattern). It wouldn't validate secedit's actual behavior, but it would fail loudly if someone reverts or copy-pastes the bug back in later.

Minor / no action needed

  • Pre-existing (not introduced by this PR): $temp = ([System.IO.Path]::GetTempPath()).TrimEnd("") trims an empty string, so it's a no-op — GetTempPath()'s trailing backslash survives, producing a harmless double-backslash (...Temp\\secedit.sdb) that Windows accepts fine. Out of scope here, but if anyone touches this block again it's worth a second look.
  • Fixed $temp\secedit.sdb filename means concurrent Set-DbaPrivilege runs against the same host could theoretically collide on the same working-database name — but that risk already existed pre-fix (just with a cwd-relative name), so this PR doesn't make it worse.

Nice, tightly-scoped fix. Main blocker before merge is the version bump per repo convention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants