Repository navigation
The upgrade and installer accept a hand-named darling.json.bak-* backup in the install folder (#5649) - #5659
Conversation
…up in the install folder (#5649) The pre-lock check trusted the service account only on darling.json and on stamped backup names, while the hardening makes the service the owner of every darling.json.bak-* file. One glob per language now drives the check, the install-time hardening, the manifest guard, the service sweep and the CLI check. A refusal that lists only such files names them and gives the one-file fix. The U1 upgrade-shape legs carry a service-owned hand-named backup.
…ackups are findings, the backup note keeps the standard advice (#5649)
| # Each finding once: a folder made under C:\ inherits BUILTIN\Users as two ACEs (one grants append, one | ||
| # grants write), and without this every caller named that principal twice for the same path. | ||
| return @($result | Select-Object -Unique) | ||
| $seen = New-Object 'System.Collections.Generic.HashSet[string]' |
There was a problem hiding this comment.
Non-blocking: this dedupe is case-SENSITIVE where the Select-Object -Unique it replaces was case-insensitive. New-Object 'System.Collections.Generic.HashSet[string]' with no comparer uses EqualityComparer<string>.Default, i.e. ordinal; Select-Object -Unique compares with PowerShell's case-insensitive semantics. The repo's own idiom three functions down in the same pair of scripts passes the comparer explicitly (upgrade-darling.ps1:1530, :1605, :1621, :1673 all use ([StringComparer]::OrdinalIgnoreCase)).
In practice the two findings this is meant to collapse (the two inherited BUILTIN\Users ACEs on one path) produce byte-identical Text, so nothing observable changes — which is also why nothing would fail if the comparer were wrong. The PR body says "the text form is unchanged"; that is true of content and order, but not of the comparer, and it is the one behavioural edge the body does not mention.
| $seen = New-Object 'System.Collections.Generic.HashSet[string]' | |
| $seen = New-Object 'System.Collections.Generic.HashSet[string]' ([StringComparer]::OrdinalIgnoreCase) |
(and the same line at upgrade-darling.ps1:274, since the census keeps the function byte-identical).
There was a problem hiding this comment.
Claude posting for Erik Darling.
Fixed in c6eb6dc. The line is now $seen = New-Object 'System.Collections.Generic.HashSet[string]' ([StringComparer]::OrdinalIgnoreCase) at install-darling.ps1:234 and upgrade-darling.ps1:287, the same form the other HashSet lines in upgrade-darling.ps1 use (:1696, :1771, :1787, :1839). Windows paths compare without regard to case, so one path spelled in two cases is one finding, the first kept.
One correction to the finding's premise. The Microsoft docs for Select-Object say -Unique "is case-sensitive", and that -CaseInsensitive was added in PowerShell 7.4 (https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.utility/select-object). I also ran "C:\A","c:\a" | Select-Object -Unique in Windows PowerShell 5.1.26100 and PowerShell 7.6.6, and both return 2 items. So the code before this change was case-sensitive too, and the new comparer is a small behaviour change, not a restoration. The fix is the same either way.
Test: TheWalksDeDuplication_TreatsTheSamePathInTwoCasesAsOneFinding takes the $seen and $unique lines out of each script and runs them as shipped, in both scripts under both shells (4 cases), over four findings where two differ only in case. It expects 2 findings with the first spelling kept, and asserts the comparer is named on the line. With the comparer removed, all 4 cases fail. The New-Object ... (comparer) form works in both shells. The PR body now says the text form changes in this one way.
| public const string Prefix = "darling.json" + Tail; | ||
|
|
||
| /// <summary>The search pattern for every backup of the default <c>darling.json</c>.</summary> | ||
| public const string Glob = Prefix + "*"; |
There was a problem hiding this comment.
Non-blocking, two small gaps in "one definition per language":
-
Globhas no consumer. Product code usesGlobSuffix(DarlingCliCommands.cs:4344) andPrefixis read only by the pin inDarlingInstallLocationTests.cs:1423;Globis referenced nowhere exceptDarlingInstallLocationTests.cs:1442, which asserts its own text. A constant whose only reader is an assertion about itself is a pin that cannot fail on anything. -
The side that WRITES the name still spells the tail by hand, twice:
DarlingCliCommands.cs:3219(resolvedPath + ".bak-" + stamp) and:3222($"{resolvedPath}.bak-{stamp}-{suffix}"). The class summary's scope ("which files besidedarling.jsonCOUNT as its backups") is honest about this, but it meansTailis the single spelling of the read side only — changeTailand the writer keeps emitting.bak-, which is exactly the drift this file exists to prevent. Routing those two lines throughDarlingConfigBackup.Tailwould close it.
While you are in there: the doc comment at DarlingCliCommands.cs:3258 still says "The only references to the .bak- name in non-test code are the two lines above that CONSTRUCT it." That was already stale before this PR (DarlingWorker.TryHardenConfigBackups, the CLI harden check) and this PR adds another reader.
There was a problem hiding this comment.
Claude posting for Erik Darling. Fixed in 75d192f. DarlingConfigBackup.BackupPath(configPath, stamp, suffix) now builds the name (DarlingConfigBackup.cs), and DarlingCliCommands.WriteWithBackupAsync calls it at both places (lines 3219 and 3222) instead of spelling .bak- by hand. The unused Glob constant and its self-pin are removed. The written names are unchanged and pinned byte for byte in TheBackupPrefix_HasOneDefinitionPerLanguage_AndTheSameText (darling.json.bak-20260101-120000, the -2 form, prod.json...-13), which also asserts the writer has no hand-written .bak-. I put the old hand-written suffix line back once and that test failed. The stale doc comment at the old line 3258 now says where the name is built and listed.
|
|
||
| ### Fixed | ||
|
|
||
| - **The upgrade and installer accept a hand-named `darling.json.bak-*` backup in the install folder** ([#5659]) - In 3.10.0 the check that runs before the lock refused an install folder holding a backup such as `darling.json.bak-before-edit`, because the service owns every `darling.json.bak-*` file after a start and the check trusted it only on the stamped names the CLI writes. The refusal said the folder was writable by ordinary users and told the operator to move the whole install. The check now trusts the service account on every file in the install root whose name starts with `darling.json.bak-`, the same prefix the hardening uses, and nowhere else. Any other grantee on such a file, the service on another file, a folder with that name, a link, a file with more than one name (a hard link) and a file below the root still refuse. A backup-named hard link is now reported as a finding of its own, and the lock and the install pass leave it alone, so it never gets the service as owner. When a refusal lists only backup files, it now starts with a note that names them, says they can hold the secrets that were in darling.json and can be moved to a folder only Administrators can read, and then gives the usual advice unchanged. The check, the install-time hardening, the upgrade's manifest guard, the service sweep and the CLI check now match one prefix per language by ordinal, case-insensitive name, so a Turkish-locale host reads the names the way the service does. The CLI permission check also covers the backups of the config it was given, not only `darling.json.bak-*`. |
There was a problem hiding this comment.
Non-blocking wording: the last sentence reads additive where the change is substitutive. "The CLI permission check also covers the backups of the config it was given, not only darling.json.bak-*" invites the reading that darling.json.bak-* is still covered alongside the given config's backups. It is not: DarlingCliCommands.cs:4124 replaces the "darling.json.bak-*" enumeration with ConfigBackupFiles(…, Path.GetFileName(resolvedConfig)), and DarlingHardenFilesVerbTests.cs:270 pins that prod.json returns none of the darling.json.bak-* files in the same folder.
That substitution is the right call — it makes the verb agree with TryHardenConfigBackups, which has always keyed on the config it was handed — and the PR body's §4 states it correctly ("It now lists Path.GetFileName(resolvedConfig) plus .bak-"). Only the changelog's phrasing is ambiguous, and an operator running --harden-files --config prod.json on a folder that also holds darling.json.bak-* would read it the wrong way. Something like "now covers the backups of the config it was given rather than only darling.json.bak-*" says it unambiguously.
There was a problem hiding this comment.
Claude posting for Erik Darling. The wording was already changed in c6eb6dc. The current CHANGELOG entry (the #5659 line under Fixed) reads: "The CLI permission check now lists the backups of the config it was given (prod.json.bak-* for prod.json) instead of always darling.json.bak-*." It says instead of, not also.
| The folder also holds a hand-named darling.json.bak-before-edit, service-owned after the | ||
| base install and its service start (#5649); it must not refuse either script. | ||
| U2 as U1, service logon changed to a non-default local account and started once on it. must PASS |
There was a problem hiding this comment.
Non-blocking doc nit: U2 and U3 are both defined as "as U1", and U1's description now includes the hand-named backup — but New-Base … ($shape -eq 'U1') at line 199 gives the file to U1 only, so "as U1" is no longer literally true of the base those two legs get. The call-site comment ("U1 (both its install and its upgrade leg) carries a hand-named backup in the root") is the accurate statement; the header is the one a reader hits first. Either qualify U1's new sentence as U1-only or say so on the U2/U3 lines.
Same wording in .github/workflows/darling-upgrade-shapes.yml:8-10.
There was a problem hiding this comment.
Claude posting for Erik Darling. Fixed in 75d192f. The U2 and U3 lines in the header of .github/scripts/darling-upgrade-shapes.ps1 and the comment in .github/workflows/darling-upgrade-shapes.yml now say "as U1 without the backup file". Only U1 gets it (New-Base ... ($shape -eq 'U1')).
| return @(Get-ChildItem -LiteralPath $root -Force -File -ErrorAction SilentlyContinue | Where-Object { | ||
| (Test-DarlingServiceWrittenFileName $_.Name) -and | ||
| (-not $BackupsOnly -or $_.Name.StartsWith($names.BackupPrefix, [StringComparison]::OrdinalIgnoreCase)) -and | ||
| $_.LinkType -ne 'HardLink' -and |
There was a problem hiding this comment.
Non-blocking, but this is the one line in the diff whose correctness the test suite cannot reach for the host it actually runs on.
LinkType is an ETS property the FileSystem provider attaches, not a BCL property, and the hard-link half of it is the engine's, not the filesystem's. Every pin for it here goes through RunWindowsPowerShell (DarlingInstallLocationTests.cs → powershell.exe, i.e. Windows PowerShell 5.1), including setup.hardLinksMade in TheServiceWrittenRootFiles_LeaveOutHardLinks_… and setup.hardLinkMade in the two walk tests. install-darling.ps1 and upgrade-darling.ps1 are scripts an operator launches by hand, so they also get run under pwsh 7, where nothing in this repo asserts that LinkType still comes back 'HardLink' for a multiply-named file. If it comes back $null there, the exclusion here, the matching $target.LinkType -ne 'HardLink' at :1037, and the HardLink finding at :209 all silently stop firing — and they fail OPEN: the file goes back to being treated as a secret file the lock hands the service ownership and FullControl on, which is the exposure #5649's hard-link arm exists to close.
The PR body says "It was checked on a real hard link in Windows PowerShell 5.1 and in PowerShell 7.6", which is exactly the right check and is not something a read-only tree can confirm — so this is a request to keep that result, not a claim it is wrong. A #Requires -Version 5.1-style note in the header, or one Fail-guarded probe, would make the scripts say which engines they have been measured on. (Carrying it in a test is harder: the suite has no pwsh-7 runner helper.)
There was a problem hiding this comment.
Claude posting for Erik Darling. No trust decision reads LinkType any more. In both scripts every remaining occurrence is a comment: install-darling.ps1 lines 213 (pre-lock walk), 328 (above Get-DarlingFileNameCount) and 1748 (the darling.json refusal), and upgrade-darling.ps1 lines 265 and 380. The name count comes from Get-DarlingFileNameCount, which opens the file for attributes only. TheFileNameCount_IsOneForAPlainFile_AndSeesAHardLinkHeldOpenWithNoReadSharing runs it under powershell.exe 5.1 and pwsh 7 for both scripts, and TheBackupPrefix_HasOneDefinitionPerLanguage_AndTheSameText asserts the lock text has no LinkType.
| if ([string]::Equals($name, $file, [StringComparison]::OrdinalIgnoreCase)) { return $true } | ||
| } | ||
| return [bool]($name -match $names.BackupPattern) | ||
| return [bool]$name.StartsWith($names.BackupPrefix, [StringComparison]::OrdinalIgnoreCase) |
There was a problem hiding this comment.
The prefix match admits names Test-DarlingServiceWrittenPath deliberately rejects, so the one list is now read two ways.
The block above this line still says the list exists so the lock and the pre-lock walk "cannot drift apart" (install-darling.ps1:274-289). The old BackupPattern was anchored (…(-[0-9]+)?$), so a name Windows folds into another could not match it. StartsWith(prefix, OrdinalIgnoreCase) is unanchored, and Test-DarlingServiceWrittenPath keeps its own guard at install-darling.ps1:335:
if ($segment -eq '' -or $segment -eq '.' -or $segment -eq '..' -or $segment.EndsWith('.') -or $segment.EndsWith(' ')) { return $false }So for a root file literally named darling.json.bak-x. (or darling.json.bak-x , both creatable through a \\?\ path — the same oddity the 'pg-runtime.' case in that guard exists for):
| reader | answer |
|---|---|
Test-DarlingServiceWrittenPath (step 1a pre-lock walk, line 179) |
false — trailing dot, service not trusted |
Test-DarlingServiceWrittenFileName (lock's $isSecretFile, line 1037; Get-DarlingServiceWrittenRootFiles, line 313) |
true |
Before this change both said false. Two consequences:
- The pre-lock walk raises no finding when such a file is admin-owned with no untrusted write ACE, so the install proceeds.
- The lock then runs
icacls.exe $secret.FullName /setowner "*$serviceSid"on it (line 991) and the install-time pass runsSet-Acl -LiteralPath $secretFileon it (line 1567). Win32 andPath.GetFullPathboth strip a trailing dot or space from the last component, so both calls land ondarling.json.bak-x— a different file than the one the loop listed — making the service its owner and replacing its DACL. The lock's walk also trusts the service ondarling.json.bak-x.($isSecretFiletrue) where the pre-lock walk did not.
Reachability is low: creating the file needs write access to the install root, which on a locked tree is administrators only. But it is the exact name-confusion shape the surrounding code already guards against, and it is the invariant this diff's own comment claims.
The cheapest fix is to put the fold guard on the name test itself, so every reader of the list gets it:
# True when a file NAME is one of the root files on the list above.
function Test-DarlingServiceWrittenFileName([string]$name) {
$names = Get-DarlingServiceWrittenNames
# A name Windows would fold into another ('darling.json.bak-x.', a trailing space) is not one the service
# wrote: it resolves to a DIFFERENT file, and Test-DarlingServiceWrittenPath already refuses it.
if ($name -eq '' -or $name.EndsWith('.') -or $name.EndsWith(' ')) { return $false }
foreach ($file in @($names.Files)) {
if ([string]::Equals($name, $file, [StringComparison]::OrdinalIgnoreCase)) { return $true }
}
return [bool]$name.StartsWith($names.BackupPrefix, [StringComparison]::OrdinalIgnoreCase)
}(Byte-identically in upgrade-darling.ps1:353-358, and TheServiceWrittenList_MatchesOnlyTheNamesTheServiceWrites has no case for a trailing dot or space after a backup prefix — name:darling.json.=False only covers the config's own name.)
There was a problem hiding this comment.
Claude posting for Erik Darling.
Fixed in c6eb6dc. The disagreement was real: the name test matched the prefix alone, and the path test refuses a name ending in a dot or a space.
Test-DarlingServiceWrittenFileName(install-darling.ps1:318,upgrade-darling.ps1:371) now returns false for an empty name and for a name ending in.or, before it tests the file list or the prefix. The prefix match is unchanged, anddarling.json.bak-xstill matches. Both copies are byte-identical. This puts the guard on the name test, so every reader of the list gets it: the lock, the install-time hardening,Get-DarlingServiceWrittenRootFilesand the walk's hard-link report.- Commit 468afed (
Get-DarlingCanonicalPath,Get-DarlingSegmentsBelowRoot) did not cover this. It changed how the path test spells the root and the candidate. The name test was not touched. - The C# side decides the same thing, so I changed it too.
DarlingConfigBackup.IsBackupName(DarlingConfigBackup.cs:45) is the one test behindListBackups(the service sweep,DarlingWorker.cs:2248) andConfigBackupFiles(the CLI check,DarlingCliCommands.cs:4343). It now returns false for a name ending in a dot or a space. - Tests:
TheServiceWrittenFileName_RefusesANameWindowsFolds_AndTheNameAndPathTestsAgree(both scripts, Windows PowerShell 5.1 and PowerShell 7) checksdarling.json.bak-xandDARLING.JSON.BAK-X(true),darling.json.bak-x.,darling.json.bak-xand four more folded spellings (false), and that the path test gives the same answer for each.TheBackupNameTest_RefusesANameWindowsFolds_InTheNameTestAndBothListingscoversIsBackupNameand both listings, with real files made through a\\?\path. With the three guards switched off, all 9 new cases fail. - The CHANGELOG entry now names the dot-or-space rule, and its "also / not only" wording is replaced: the CLI check lists the given config's backups instead of always
darling.json.bak-*.
| # above makes it the owner, so the service is trusted on those files too. Without this the walk strips the | ||
| # service's own grant from its config (the explicit-ACE branch below) and the service cannot start. | ||
| $isSecretFile = (-not $target.PSIsContainer) -and ([IO.Path]::GetDirectoryName($path).TrimEnd('\') -ieq $root.TrimEnd('\')) -and (Test-DarlingServiceWrittenFileName $target.Name) | ||
| $isSecretFile = (-not $target.PSIsContainer) -and ([IO.Path]::GetDirectoryName($path).TrimEnd('\') -ieq $root.TrimEnd('\')) -and (Test-DarlingServiceWrittenFileName $target.Name) -and ($target.LinkType -ne 'HardLink') |
There was a problem hiding this comment.
Non-blocking, but the hard-link exclusion stops one step short of its own argument.
Dropping a hard link out of $isSecretFile sends it to the else arm thirteen lines down instead:
$null = & icacls.exe $path /reset /L /Q 2>&1/reset replaces the object's DACL with the inherited one, and /L has nothing to act on for a hard link — a hard link has no "link itself" distinct from the file, which is the premise of the finding text this PR adds ("its permissions are the same under every one"). So a hard-linked darling.json.bak-x in the root carrying one untrusted explicit write ACE now has the install root's inheritable ACEs written onto the file's other name, where before this change a stamped-name one took the narrower /remove:g arm.
This is a pre-existing path (any hard link in the tree with an explicit untrusted write grant already reached it, including a hand-named backup, which never matched the old BackupPattern), and it is reachable only past the refusal this PR adds or with -AcceptWritableExtraction. Worth noting because the walk at line 1025 already gives reparse points the continue + report treatment and hard links get no equivalent; the same $target.LinkType -ne 'HardLink' test you added here would read naturally beside it.
There was a problem hiding this comment.
Claude posting for Erik Darling. Fixed in 75d192f. Yes, the lock could reach one. The pre-lock walk reports a HardLink finding only for the root files the service writes (Get-UntrustedWriteGrantees), and the lock's earlier hard-link arm has the same limit. Any other hard-linked file with an explicit untrusted write grant (a file in a sub-folder, a backup-named file below the root, a shipped file) went to the else arm and its icacls /reset /L. Now Lock-DarlingInstallTree, inside if ($explicit.Count -gt 0), checks Get-DarlingFileNameCount for every file. One whose count is not exactly 1, or cannot be read, is added to the open list and skipped before either icacls arm. Both scripts have the same text. TheInstallTreeLock_ReportsAnyHardLinkedFileWithAnUntrustedGrant_AndLeavesItsPermissionsAlone runs both scripts under 5.1 and 7 with four hard-linked files (one in the root, three in a sub-folder). Each must be named, and both names of each must keep the grant. A plain file is still reset. With the check switched off all four cases fail. One thing left alone: the lock's first step, icacls <root> /setowner ... /T /C /L, still sets the owner of every file, hard-linked ones included. That is an owner change that predates this PR.
|
|
||
| ### Fixed | ||
|
|
||
| - **The upgrade and installer accept a hand-named `darling.json.bak-*` backup in the install folder** ([#5659]) - In 3.10.0 the check that runs before the lock refused an install folder holding a backup such as `darling.json.bak-before-edit`, because the service owns every `darling.json.bak-*` file after a start and the check trusted it only on the stamped names the CLI writes. The refusal said the folder was writable by ordinary users and told the operator to move the whole install. The check now trusts the service account on every file in the install root whose name starts with `darling.json.bak-`, the same prefix the hardening uses, and nowhere else. Any other grantee on such a file, the service on another file, a folder with that name, a link, a file with more than one name (a hard link) and a file below the root still refuse. A backup-named hard link is now reported as a finding of its own, and the lock and the install pass leave it alone, so it never gets the service as owner. When a refusal lists only backup files, it now starts with a note that names them, says they can hold the secrets that were in darling.json and can be moved to a folder only Administrators can read, and then gives the usual advice unchanged. The check, the install-time hardening, the upgrade's manifest guard, the service sweep and the CLI check now match one prefix per language by ordinal, case-insensitive name, so a Turkish-locale host reads the names the way the service does. The CLI permission check also covers the backups of the config it was given, not only `darling.json.bak-*`. |
There was a problem hiding this comment.
Non-blocking wording: the last sentence reads as an addition where the diff makes a substitution.
The CLI permission check also covers the backups of the config it was given, not only
darling.json.bak-*.
DarlingCliCommands.cs:4125 replaces the enumeration outright:
foreach (var backup in ConfigBackupFiles(Path.GetDirectoryName(resolvedConfig), Path.GetFileName(resolvedConfig)))So with --config prod.json, HardenFiles now hardens prod.json.bak-* instead of darling.json.bak-* — any darling.json.bak-* sitting in that folder for another install drops off the target list. That is the right call (it is what TryHardenConfigBackups has always done, and the new TheVerb_HardensTheBackupsOfTheConfigItWasGiven_ByAnOrdinalNameTest asserts exactly that exclusion), and the PR body states it correctly — "It now lists Path.GetFileName(resolvedConfig) plus .bak-". Only the changelog sentence says "also / not only", which an operator reading the release notes would take as "both".
There was a problem hiding this comment.
Claude posting for Erik Darling. Same as the other thread on this wording: c6eb6dc changed it. The current entry reads "now lists the backups of the config it was given (prod.json.bak-* for prod.json) instead of always darling.json.bak-*", which matches the substitution in DarlingCliCommands.ConfigBackupFiles and the pin in DarlingHardenFilesVerbTests.
There was a problem hiding this comment.
Verdict: CHANGES REQUESTED
One finding, and it is this diff's own stated invariant rather than anything the body gets wrong. Test-DarlingServiceWrittenFileName's new StartsWith is unanchored where BackupPattern ended at $, and Test-DarlingServiceWrittenPath keeps its own fold guard (install-darling.ps1:335), so for a root file named darling.json.bak-x. or darling.json.bak-x the pre-lock walk says "not the service's" while the lock's $isSecretFile and Get-DarlingServiceWrittenRootFiles say "it is". The walk raises nothing when that file is admin-owned, and the lock's icacls /setowner (:991) and the hardening's Set-Acl (:1567) then land on darling.json.bak-x instead, because Win32 strips a trailing dot or space from the last component. Reachability is admin-only, so this is robustness and consistency rather than a privilege path, but it is the name-confusion shape the surrounding code already guards against and the comment this diff rewrites promises "the two cannot drift apart". Details and a fix inline, plus two non-blocking notes (the lock's /reset arm still acts on hard links; the changelog's "also / not only" reads additive where the CLI change substitutes).
The rest holds up. The mechanism matches the reported failure, the narrowing cases are real, the file-or-folder answer does come from the listed item, and the CI leg fails at setup rather than passing for nothing.
Claims
- VERIFIED — the 3.10.0 check trusted stamped backup names only — the removed
BackupPattern = '^darling\.json\.bak-[0-9]{8}-[0-9]{6}(-[0-9]+)?$', replaced byBackupPrefixat install-darling.ps1:295 / upgrade-darling.ps1:348 - VERIFIED — every site reads that one definition — install-darling.ps1:305, :317, :343, :556, :990, :1037, :1541 and upgrade-darling.ps1:358, :370, :396, :609, :829, :876, :1358; no second spelling survives in either script
- VERIFIED — it is narrow: a folder, a link, a hard link, a nested path and any other grantee still refuse — install-darling.ps1:343 and :547-558, probed by
ThePreLockWalk_TrustsTheServiceOnAnyHandNamedBackupInTheRoot_AndNothingWider(handNamed.count=0,handNamedUsers.count=1,folder.count=1,nested.count=2,hardLink.count=3) - VERIFIED — the file-or-folder answer comes from the item the walk listed —
$itemthreads from :212 through :179 into :343;item:none=False,item:plainFile=True,item:directoryNeedsNoItem=True - VERIFIED — a hard-linked root file the service writes is a finding, and the lock and install loops skip it — install-darling.ps1:209, :318, :1037
- VERIFIED —
-AsRecordsreturns Kind/Path/Text/IsFile, the text form is unchanged, both refusals pass records — :167-168, :222-224, :1148/:1156 and upgrade-darling.ps1:1903/:1917 - VERIFIED — the backup note comes first and the standard advice follows unchanged; fourteen other shapes keep the standard advice alone —
Get-DarlingRefusalAdviceat :581-586, and the 15-label list (14 plus thenoFindingsbaseline) at DarlingInstallLocationTests.cs:506-507 - VERIFIED — the CI leg fails at setup unless the hand-named backup is service-owned, checks it survives, and only U1 carries it — darling-upgrade-shapes.ps1:148, :258, :199
- VERIFIED —
HardenFilesnow lists the given config's own backups — DarlingCliCommands.cs:4125 viaConfigBackupFiles(:4171) andDarlingConfigBackup.IsBackupName(DarlingConfigBackup.cs:25), the same helper DarlingWorker.cs:2248 calls - UNVERIFIED —
LinkTypewas checked on a real hard link in Windows PowerShell 5.1 and PowerShell 7.6 — needs a Windows host and a real hard link; the only engine the repo drives ispowershell.exe(darling-upgrade-shapes.ps1:91) and neither script carries a#Requires - UNVERIFIED — 0 build warnings; 823 and 1481 tests passing — needs a build and a Windows run, since every new probe goes through Windows PowerShell
- UNVERIFIED — RED at acafd47 — a single merge commit is checked out, so the first-pass tree is not reachable
- UNVERIFIED —
BackupGloband "You do not need to move the install" are gone —BackupPatternreally is removed, but neither of those two is in this PR's base scripts; both survive only as negative pins (DarlingInstallLocationTests.cs:1425, :1255), so the removals describe an intermediate pass
Riskiest lines
- Darling/tools/install-darling.ps1:305 — the unanchored prefix IS the fix and is read by the lock, the hardening and the manifest guard; it is where those readers diverge from the path test's fold guard — pinned by
TheServiceWrittenList_MatchesOnlyTheNamesTheServiceWritesandTheBackupPrefix_HasOneDefinitionPerLanguage_AndTheSameTextfor case folding, UNPINNED for a name Windows folds - Darling/tools/install-darling.ps1:343 — a root file is trusted only when the caller hands in the item it listed, so a caller that omits it silently stops trusting the service on its own config — pinned by
TheServiceWrittenList_MatchesOnlyTheNamesTheServiceWrites(item:none=False,item:directoryNeedsNoItem=True) - Darling/tools/install-darling.ps1:222-224 — one function now returns records or texts off a switch and de-duplicates on an ordinal
HashSet[string]of.Text; every refusal body in both scripts comes through it — pinned for records byTheWalksRecords_DecideTheBackupNote_AndNoTextIsParsed, for texts by the pre-existingThePreLockTrustedSet_…probes
Traps near this diff
- "A pin that encodes the lie … Ask what a pin would fail ON." — APPLIES — the name-list pin moved
darling.json.bak-x.dllanddarling.json.bak-20260101from False to True (DarlingInstallLocationTests.cs:285-295) and added no case for a name Windows folds, so nothing can fail on the inline finding - "Lite and Darling ship in parity (CONTRIBUTING's two-store rule): a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply — nothing in
Lite/orLite.Tests/names adarling.jsonor a.bak-backup; the binding contract here is install/upgrade byte-identity, extended to all five new or changed functions at DarlingInstallLocationTests.cs:596-605 - "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — does not apply as a defect — no cross-language alias exists, and the pin at DarlingInstallLocationTests.cs:1421-1423 compares the script literal against the C# const, so a one-sided drift fails
- "A roster-based census does not see the file it does not roster (#3699)" — does not apply — both rosters were extended: DarlingInstallLocationTests.cs:598-601 and DarlingDeployStaleFileTests.cs:65
- "
InternalsVisibleToon PerformanceMonitor.Analysis names Lite.Tests only — a helper Darling.Tests must call has to be public (#3688)" — does not apply —DarlingConfigBackupandConfigBackupFilessit in PerformanceMonitor.Darling.Service, whose csproj has<InternalsVisibleTo Include="Darling.Tests" />(:87) - "
claude-code-actionrefuses to run when.github/workflows/claude-review.ymldiffers from the DEFAULT branch's copy (#2229 cause 1)" — does not apply — the only workflow touched is darling-upgrade-shapes.yml, and that edit is one comment line (:8)
Divergence (blind read vs PR body)
- The body never mentions anchoring. Removing the
$is what lets the lock and the pre-lock walk answer differently, while the comment at install-darling.ps1:274-289 still promises one list "so the two cannot drift apart" — divergence by silence rather than by error. - "It is no longer a second
Test-Pathcall on the disk": the end state is in the tree (:173, :212, :343), but this PR's base made no disk call at all — it decided purely by name. Same for "BackupGlob… gone" and "the old sentence … is gone", neither of which is in the base scripts. All three grade a change between this PR's own passes. - The changelog says the CLI check "also covers … not only
darling.json.bak-*", while DarlingCliCommands.cs:4125 substitutes: with--config prod.json,darling.json.bak-*siblings drop off the target list. The body's own "It now lists …" is the accurate account, so the two disagree with each other.
Reviewer checklist
- Read the hosted
U1-install/U1-upgradelogs for theowner of …darling.json.bak-before-edit: NT SERVICE\PerformanceMonitor Darlingline — without it the leg passes without exercising the check - Build and run
Darling.Testson Windows; every new probe needs Windows PowerShell, so the body's counts are unobserved here - Confirm
(Get-Item <hard link>).LinkType -eq 'HardLink'on a real hard link under 5.1 and pwsh 7 — the whole hard-link defense rests on that one property - Plant
darling.json.bak-x.in a scratch root through a\\?\path, runinstall-darling.ps1, and check withicaclswhich file ends up service-owned - Run
HardenFiles --config prod.jsonin a folder holding bothprod.json.bak-*anddarling.json.bak-*and confirm leaving the latter unhardened is intended
…d darling.json, report hard links in the lock's verify walk (#5649) Windows PowerShell 5.1 leaves LinkType empty when a file cannot be opened for read, so a hard link held open with no read sharing read as an ordinary file. One shared helper now counts the names through a handle opened for attributes only, and a count that cannot be read is treated as more than one name. install-darling.ps1 refuses a darling.json with more than one name or a link before it hardens it. The lock's verify walk reports a hard-linked root file the service writes and leaves its permissions alone.
# Conflicts: # CHANGELOG.md
… switch (#5649) PowerShell variable names ignore case, so inside New-Base the [bool]$handNamedBackup parameter hid the script's $HandNamedBackup file name: the leg wrote and then checked a file called True instead of darling.json.bak-before-edit. The file name is now $HandNamedBackupName.
…g spelling (#5649) On a GitHub Windows runner the temp folder is an 8.3 short path (C:\Users\RUNNER~1\...). Windows PowerShell 5.1 and PowerShell 7 spell short names out differently in GetFullPath (5.1 leaves them when part of the path is missing or has a '..' step), so a root typed one way and a path joined or listed the other compared unequal: the service-written path test trusted none of the service's folders, and a hard link probe printed the wrong name. - Get-DarlingCanonicalPath: one long, absolute spelling of a path (GetFullPath, then GetLongPathName on the part that exists, trailing backslash dropped). $null when nothing can be worked out; every caller then treats the path as not trusted. - Get-DarlingSegmentsBelowRoot: the names from the root down to a candidate, both in that spelling, with the fold check (a '.' or '..' step, an empty name, a trailing dot or space) run on the candidate as handed in. Test-DarlingServiceWrittenPath and Get-DarlingBackupFileFindingNames use it. - The walk, the lock and Get-DarlingExtraServiceWriteDirectories compare through the same helper; the lock puts its root in that spelling first and reports a path it cannot work out. - Tests: a root in short form, short with a trailing backslash, and the long path in upper case, lower case and with a trailing backslash give the same walk, root-file, path-test, backup-name and key-folder answers in both scripts and both shells. The hard link probe printed a leaf by slicing at the typed root's length; it prints the file name instead. Both scripts carry the same helpers and call sites.
…d the walk's de-duplication ignores case (#5649) Test-DarlingServiceWrittenFileName now answers false for a name that ends in a dot or a space, the same answer Test-DarlingServiceWrittenPath already gave, so the pre-lock walk, the lock and the install-time hardening agree about darling.json.bak-x. and darling.json.bak-x followed by a space. DarlingConfigBackup.IsBackupName does the same, so the service sweep and the CLI check skip those names. Get-UntrustedWriteGrantees de-duplicates its findings with an OrdinalIgnoreCase comparer, because Windows compares paths without regard to case. Tests run both scripts under Windows PowerShell 5.1 and PowerShell 7.
| # other file or folder in it are not on the list. $item is what the caller's own walk listed for $candidate: a | ||
| # listed file is trusted by name only when it is a plain file (not a folder, a link or a file with more than one | ||
| # name), and with no item a root file is not trusted, so the answer never rests on a second look at the disk. | ||
| function Test-DarlingServiceWrittenPath([string]$root, [string]$candidate, [string[]]$extraDirectories = @(), $item = $null) { |
There was a problem hiding this comment.
Non-blocking: this comment ("the answer never rests on a second look at the disk") predates this diff but is now inaccurate — Test-DarlingServiceWrittenPath's return statement two lines below adds (Get-DarlingFileNameCount $candidate) -eq 1, which opens a fresh handle via CreateFileW on every call. That's the right fix (the old $item.LinkType -ne 'HardLink' was exactly the unreliable read this PR's commit message calls out), but the comment should say the file-or-folder/link answer comes from the listed item while the name-count answer is a live re-check, so a future reader doesn't rely on the stale "no second disk look" invariant. Same comment, same fix needed, in upgrade-darling.ps1:537.
There was a problem hiding this comment.
Claude posting for Erik Darling. Fixed in 75d192f, in both scripts. The comment above Test-DarlingServiceWrittenPath now says file-or-folder and link are read from the listed item, and the number of names is read live because Get-DarlingFileNameCount opens the file (for attributes only) each time it is called. The sentence "the answer never rests on a second look at the disk" is gone. The same claim in the PR body is corrected too.
| if ([string]::IsNullOrWhiteSpace($candidate) -or $candidate -notmatch '^([A-Za-z]:[\\/]|\\\\)') { return $null } | ||
| $given = @($candidate.TrimEnd('\', '/') -split '[\\/]') | ||
| $first = if ($candidate.StartsWith('\\')) { 3 } else { 1 } | ||
| for ($i = $first; $i -lt $given.Count; $i++) { |
There was a problem hiding this comment.
Non-blocking: for a UNC candidate (\\server\share\...), splitting on [\\/] gives ["", "", "server", "share", ...], so $first = 3 starts the fold-check loop (the ./../empty/trailing-dot-or-space scan two lines below) at share, skipping server (index 2) entirely — it's never checked for a folding name. That's asymmetric with the drive-letter branch ($first = 1), which checks every segment after the drive. In practice this doesn't change the trust outcome (the leaf segment that actually matters for Test-DarlingServiceWrittenFileName is always at or past index 3, since a UNC path always has at least share before it), but the function's own contract ("every name on the way … is not an absolute path, has a name on the way that Windows folds into another … or either path cannot be worked out") isn't quite met for the server component. No test exercises a UNC root/candidate here, so this is unpinned either way. Same code, same gap, in upgrade-darling.ps1:504.
There was a problem hiding this comment.
Claude posting for Erik Darling. Fixed in 75d192f, in both scripts. Get-DarlingSegmentsBelowRoot now starts its fold check at index 2 when the candidate begins with two backslashes, so the server name is checked as well. The drive-letter branch is unchanged. TheSegmentsBelowRoot_ChecksEveryNameOfAUncPath_TheServerAndShareToo (both scripts, 5.1 and 7) covers a normal UNC path below a UNC root (names returned), a server or share name with a trailing dot or space ($null), an empty server name, a trailing-dot leaf, a .. step and two drive-letter cases. With the index put back to 3 all four cases fail. A UNC install root is not refused earlier: install-darling.ps1 warns about it ($networkKind) and asks the operator to confirm, and upgrade-darling.ps1 has no such question. So the function is fixed on its own terms.
There was a problem hiding this comment.
Verdict: LGTM
Claims
- VERIFIED —
Test-DarlingServiceWrittenFileNamenow returns false for a name ending in.or' '(and for empty), agreeing withTest-DarlingServiceWrittenPath's fold guard —Darling/tools/install-darling.ps1andupgrade-darling.ps1, theif ([string]::IsNullOrEmpty($name) -or $name.EndsWith('.') -or $name.EndsWith(' ')) { return $false }line added in c6eb6dc; this is exactly the finding the prior full-pass review (head=45aa7c0) raised against the unanchoredStartsWith - VERIFIED —
DarlingConfigBackup.IsBackupName(C#) got the matching fold exclusion, so the service sweep (ListBackups) and the CLI check (ConfigBackupFiles) agree with the PowerShell side —Darling/PerformanceMonitor.Darling.Service/DarlingConfigBackup.cs:49-51, pinned byTheBackupNameTest_RefusesANameWindowsFolds_InTheNameTestAndBothListingsinDarlingHardenFilesVerbTests.cs - VERIFIED — a file's name-count is now read through an attribute-only native handle (
Get-DarlingFileNameCount, never$item.LinkType), and every caller (Get-UntrustedWriteGrantees,Get-DarlingServiceWrittenRootFiles,Test-DarlingServiceWrittenPath,Lock-DarlingInstallTree, install-darling.ps1's new pre-hardening darling.json check) treats -1 (unreadable) as "more than one name" — confirmed identical across both scripts and exercised byTheFileNameCount_IsOneForAPlainFile_AndSeesAHardLinkHeldOpenWithNoReadSharing(4 cases) andTheInstaller_RefusesADarlingJsonWithMoreThanOneName_BeforeItHardensAnything - VERIFIED — the lock's verify walk now reports a hard-linked root file the service writes and leaves its permissions alone (the prior review's "the lock's
/resetarm still acts on hard links" note), via the newcontinue-guarded branch inLock-DarlingInstallTree, pinned byTheInstallTreeLock_ReportsAHardLinkedBackupWithAnUntrustedGrant_AndLeavesItsPermissionsAlone - VERIFIED — paths are compared through one canonical long spelling (
Get-DarlingCanonicalPath/Get-DarlingSegmentsBelowRoot), fixing the 8.3-short-path mismatch under GitHub Windows runners (C:\Users\RUNNER~1\...) — both helpers are in the byte-identical-block test (ThePreLockWritableExtractionCheck_ShipsIdenticallyInTheInstallAndUpgradeScripts) and exercised byTheWalkAndThePathTests_AnswerTheSameForARootInShortFormWithATrailingBackslashOrInOtherCase - VERIFIED —
Get-UntrustedWriteGrantees's de-duplication now uses anOrdinalIgnoreCaseHashSet[string]comparer (the second "last round" bot finding) —[StringComparer]::OrdinalIgnoreCaseadded to theNew-Objectcall in both scripts, pinned byTheWalksDeDuplication_TreatsTheSamePathInTwoCasesAsOneFinding - VERIFIED — the CHANGELOG's
#5659entry was reworded from "also … not only" (additive-sounding) to "now lists … instead of always …" (substitutive), matching the actualDarlingCliCommands.csbehavior — the prior review's third non-blocking note - VERIFIED —
.github/scripts/darling-upgrade-shapes.ps1's$HandNamedBackup→$HandNamedBackupNamerename fixes a real case-insensitive collision withNew-Base's[bool]$handNamedBackupparameter (PowerShell variable names ignore case), matching the commit message's stated bug - UNVERIFIED — the build/test counts in the PR body ("631 total, 0 failed, 1 skipped"; "1481 total, 0 failed") and the CI reproduction of the 8.3-short-path failures — need a Windows build/test run, unavailable in this read-only tree
Riskiest lines
Darling/tools/install-darling.ps1:485(andupgrade-darling.ps1:537) —Test-DarlingServiceWrittenPath's now-stale comment claims the answer "never rests on a second look at the disk," but the function's own return statement callsGet-DarlingFileNameCount $candidate, a fresh nativeCreateFileWper call — pinned functionally (the behavior is correct and tested) but the invariant stated in the comment is UNPINNED and now false; flagged inline, non-blockingDarling/tools/install-darling.ps1:452(andupgrade-darling.ps1:504) —Get-DarlingSegmentsBelowRoot's$first = 3for a UNC candidate skips theserversegment in the fold-check loop (onlyshareonward is scanned), asymmetric with the drive-letter branch's$first = 1— UNPINNED (no UNC test exists), but has no effect on the actual trust decision since the leaf name is always at or past index 3; flagged inline, non-blockingDarling/tools/install-darling.ps1— the new pre-hardening darling.json check ($configItem = Get-Item -LiteralPath $configPath -Force -ErrorAction SilentlyContinue; if ($null -ne $configItem -and (...))) — pinned byTheInstaller_RefusesADarlingJsonWithMoreThanOneName_BeforeItHardensAnythingfor plain/hard-link/held-hard-link/symlink cases
Traps near this diff
- "Lite and Darling ship in parity (CONTRIBUTING's two-store rule)" — does not apply — every changed symbol in this range (
Get-DarlingFileNameCount,Get-DarlingCanonicalPath,Get-DarlingSegmentsBelowRoot,Test-DarlingServiceWrittenFileName,DarlingConfigBackup.IsBackupName) is Darling-only Windows-service ACL machinery; Lite has no equivalent service-hardening surface to keep in parity with - "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — does not apply as a defect —
install-darling.ps1andupgrade-darling.ps1cannot import each other (each runs from wherever its own zip was extracted), so byte-identity is the only option, and it's enforced one-sided byExtractFunction(InstallScript, name)vsExtractFunction(upgrade, name)comparisons that now include all three new helpers (DarlingInstallLocationTests.cs, theThePreLockWritableExtractionCheck_ShipsIdenticallyInTheInstallAndUpgradeScriptslist) - "
InternalsVisibleToon PerformanceMonitor.Analysis names Lite.Tests only" — does not apply —DarlingConfigBackup.IsBackupNameis called directly fromDarlingHardenFilesVerbTests.cs(Darling.Tests), andPerformanceMonitor.Darling.Service's ownInternalsVisibleToalready covers Darling.Tests (unchanged by this diff)
Divergence (blind read vs PR body)
None — the body's "Last round: two bot findings" and "Changes after the second pass" sections describe exactly the four commits in this range (c022ad8, a029b0e, 468afed, c6eb6dc) and match what the diff does; the "Worth a second look" note ("the hard-link finding covers only the root files the service writes … a hard link inside an extracted pg-runtime does not refuse") is an accurate, honest scoping statement, not an overclaim. The merge commit 4fbb8a8 in this range brought in #5663 (deadlock-graph Copy button) and #5657/#5656 (CPU scheduler offline-count fix) from dev; those are not this PR's own changes and are out of this follow-up's scope.
Reviewer checklist
- Build and run
Darling.Testson Windows (bothpowershell.exe5.1 andpwsh7) to confirm the counts the PR body cites, since every new probe in this range needs a real Windows host - Confirm
Get-DarlingFileNameCount's nativeCreateFileW/GetFileInformationByHandlepair actually returns 2 for a real hard link and -1 for a file held open with no read sharing, on a real Windows box (the whole hard-link defense now rests on this one P/Invoke) - Confirm
Get-DarlingCanonicalPath'sGetLongPathNameWwalk resolves an 8.3 short%TEMP%path the same way on a real GitHub Windows runner, matching the CI failure this commit set was written to fix
# Conflicts: # CHANGELOG.md
…eaves hard-linked files alone, UNC server name is checked (#5649) - DarlingConfigBackup.BackupPath builds the backup name (config path, Tail, stamp, optional -N); the CLI writer calls it instead of spelling .bak- by hand. The unused Glob constant is removed. The written names are unchanged and pinned. - Lock-DarlingInstallTree: any file with more than one name (or whose names cannot be counted) that would have its permissions changed is reported as open and left untouched, wherever it is in the tree. - Get-DarlingSegmentsBelowRoot checks every name after the leading double backslash of a UNC path. - Comment in Test-DarlingServiceWrittenPath now says the name count is a live read. - Shape check header and workflow comment: U2 and U3 are U1 without the backup file.
…ves a file with more than one name alone (#5677)
…one name its owner (#5677)
…h outcome in its own words (#5677) The owner step opens the root once without sharing delete or rename, reads its real path from that handle, and sets an owner only on an object whose own real path is the root or below it. A name ending in a dot or a space is listed and not acted on, extended-length paths are used so Windows does not fold such a name, the skip set compares names exactly, and an object whose owner could not be set is still fixed and checked by the permission walk. The step uses SeTakeOwnershipPrivilege and SeBackupPrivilege and puts the previous state back. The printed advice names the privileges, gives a hard-linked path its own line and drops the tree-wide reset.
… answer when one fails (#5677) Three groups of lock tests failed only on an elevated runner: - The hard-link tests expected the permission walk's wording. An elevated session runs the owner step first, which lists the file in its own words and leaves it out of the walk. The tests now expect the owner step's text when the session is elevated, the walk's text otherwise, and require the file to be listed exactly once. - The owner test filtered the lock's lines by the probe's root, which is the 8.3 short form under a runner's temp folder, while the lock lists the long form, so no line matched. The probe now takes the long form of its root. - The dot-name test never gave dn. an owner. A file made by an elevated administrator starts owned by Administrators, so the check that the lock left dn. alone could not pass. The probe now sets the current account as its owner through a \\?\ path and checks that it took. A failed assertion on a probe's answers now prints every answer instead of the first five.
Fixes #5649.
Fixes #5677.
Why
The service hardens every file whose name starts with
darling.json.bak-. It makes the service account the owner and gives it FullControl. It does this at install (install-darling.ps1), at every service start (TryHardenConfigBackups) and in the CLI check.The 3.10.0 check that runs before the lock trusted the service account on
darling.jsonand on stamped backup names only (darling.json.bak-yyyyMMdd-HHmmsswith an optional-N). A backup named by hand, such asdarling.json.bak-before-edit, became service-owned at the next start. The check then listed it as an untrusted write grantee and refused. The advice told the operator to move the whole install.What changes
1. The check trusts every
darling.json.bak-*file in the install rootdarling.json.bak-(case-insensitive).Get-DarlingFileNameCountopens the file (for attributes only) each time the path test calls it.BackupPrefix = 'darling.json.bak-'inGet-DarlingServiceWrittenNames. It sits in the block both scripts keep byte-identical, and a test pins that.StartsWith($prefix, [StringComparison]::OrdinalIgnoreCase).-likeand-Filterfold case by the current culture (a TurkishI). Windows PowerShell 5.1 also matches some look-alike characters. Either way they can differ from the service.DarlingConfigBackup.PrefixandDarlingConfigBackup.IsBackupNameinDarling/PerformanceMonitor.Darling.Service/DarlingConfigBackup.cs. The directory listing's pattern only narrows the list, and the ordinal name test decides.Test-DarlingServiceWrittenFileNameandTest-DarlingServiceWrittenPath.Get-DarlingServiceWrittenRootFiles. The hardening calls it with-BackupsOnly.DarlingWorker.TryHardenConfigBackups, throughDarlingConfigBackup.ListBackups.DarlingCliCommands.ConfigBackupFiles.BackupPatternof 3.10.0 is gone from the scripts.2. Files with more than one name
darling.jsonor a backup) can have more than one name (a hard link). That is now reported as a finding, the way a junction or a link is.Get-DarlingServiceWrittenRootFilesleaves such a file, and a file that is a link, out of the lock's ownership loop and out of the install-time hardening loop. They never get the service as owner or a FullControl grant. Either one lands on the other names too.Get-DarlingFileNameCount(second pass below), never fromLinkType. The C# side already refuses these files (DarlingFileSecurity,NumberOfLinks > 1).3. The refusal advice
Get-UntrustedWriteGranteestakes-AsRecords, which returns one object per finding (Kind,Path,Text,IsFile). The text form is unchanged, except that two findings whose text differs only in case are one finding (see the last round below). Both refusals pass the records toGet-DarlingRefusalAdvice.Get-DarlingBackupFileFindingNamesdecides from each record's kind and path, not from its text. Only a grant or an owner finding on a file directly in the install folder, with a backup name, counts. A junction, a hard link, a path below the root, an unlistable folder and an unreadable path all keep the standard advice alone.darling.json. It says the files can be moved to a folder only Administrators can read. The standard advice follows it unchanged.4. The CLI check for a config with another name
HardenFileslisteddarling.json.bak-*beside any config. It now listsPath.GetFileName(resolvedConfig)plus.bak-, asTryHardenConfigBackupsdoes.5. CI
.github/scripts/darling-upgrade-shapes.ps1puts a hand-nameddarling.json.bak-before-editinto the U1 base before the release's own install script runs. That script and the service then harden it, so it is service-owned the way a real one is after a service start.NT SERVICE\PerformanceMonitor Darling, so it cannot pass without exercising the check. After the run it checks the file is still there.Changes after the first pass
BackupPrefix, tested with an ordinal case-insensitiveStartsWithat every PowerShell site and in C#.Changes after the second pass
LinkTypeis empty when the file cannot be opened for read. That happens when another process holds it with no read sharing, or when the account is denied read. A hard link then read as an ordinary file. One helper,Get-DarlingFileNameCount, now counts the names. It is identical in both scripts. It opens a handle for attributes only, with full sharing and without following links, the same callsDarlingFileSecurity.HardenWithoutFollowingLinksmakes. It returns -1 when the count cannot be read. Every caller treats that as more than one name: the file is never trusted, never made service-owned, never granted anything and never reset.LinkTyperead is gone. That covers the pre-lock walk's hard-link finding,Get-DarlingServiceWrittenRootFiles,Test-DarlingServiceWrittenPathand the lock's verify walk. The type is compiled withAdd-Typethe first time the helper runs. In this pass the helper is only called for root files that carry a service-written name. The third pass below also calls it in the lock for any file about to have its permissions changed.Tests, all in
DarlingInstallLocationTests, with the result of running each against the code before this pass:TheFileNameCount_IsOneForAPlainFile_AndSeesAHardLinkHeldOpenWithNoReadSharinghas 4 cases: both scripts, each underpowershell.exe5.1 andpwsh7. A plain file counts 1, a hard link 2, a folder 1 and a missing file -1. A hard link held open with no sharing still counts 2. It is dropped from the root-file list, the path test does not trust it and the walk reports it. Before this pass, the same held-open probe under 5.1 returned an emptyLinkType. It listed the held backup as a service-written file, trusted it, and the walk reported no hard link.TheInstaller_RefusesADarlingJsonWithMoreThanOneName_BeforeItHardensAnythingpasses a plain file. It refuses a hard link, a hard link held open with no sharing, and a symbolic link when the account can make one. The message names the file and the fix. With the refusal switched off, the test fails onhardLink.refused=True.TheInstallTreeLock_ReportsAHardLinkedBackupWithAnUntrustedGrant_AndLeavesItsPermissionsAlonehard-links a backup to a file outside the root and gives it an explicit BUILTIN\Users Modify grant. The lock must return it as a hard link and keep the grant. With the new branch removed, the test fails because nothing is reported.LinkType. The two byte-identical pins include the new helper.Checks for this pass are in the last round below.
CI fix
On the GitHub Windows runner the temp folder is an 8.3 short path (
C:\Users\RUNNER~1\...). Everything the tests list under it comes back in the long form. The two PowerShell versions also spell short names out differently inGetFullPath. Windows PowerShell 5.1 leaves them alone when part of the path is missing or has a..step. So a root typed one way and a path joined or listed the other way compared unequal. That failed two tests in the "Darling PG tests (3)" job. I reproduced both here by pointingTEMPat the short alias of a long-named folder. The same five failures came back, with the same messages.TheServiceWrittenList_MatchesOnlyTheNamesTheServiceWriteswas a product defect.Test-DarlingServiceWrittenPathspelled the root out but compared it with a candidate joined onto the root as typed. None of the service's folders or files matched. The product's own callers pass the listing's long path, so the walk was not hit. A root handed to the lock in short form (a script run from a short-named extraction folder) had the same hole, in the lock and in the key-folder test.TheFileNameCount_IsOneForAPlainFile_...(all four cases) was a test defect only. The product walk did report the held-open hard link. The probe cut the name out of the finding's path by the typed root's length (Substring($root.Length + 1)), and the short root is three characters shorter than the long path. The probe now prints the file name. The product was not affected there.The change:
Get-DarlingCanonicalPathgives one long, absolute spelling of a path. It runsGetFullPath, thenGetLongPathNameon the part that exists, and drops the trailing backslash. It returns$nullwhen nothing can be worked out, and every caller then treats the path as not trusted.Get-DarlingSegmentsBelowRootputs the root and the candidate in that spelling and returns the names between them. The check for names Windows folds into another (.,.., an empty name, a trailing dot or space) runs on the candidate as it was handed in. Sopg-runtime.\a.dllandpg-runtime\..\xstill match nothing.The walk,
Test-DarlingServiceWrittenPath,Get-DarlingBackupFileFindingNames,Get-DarlingExtraServiceWriteDirectoriesandLock-DarlingInstallTreeuse these helpers. The lock puts its root in that spelling first, and reports a path it cannot work out instead of passing it. Both scripts carry byte-identical helpers, and the identical-block test lists them.Tests:
TheWalkAndThePathTests_AnswerTheSameForARootInShortFormWithATrailingBackslashOrInOtherCaseruns both scripts in both shells. It gives the walk, the root-file list, the path test, the backup-name test and the key-folder test one install folder in five spellings. Those are its short path, the short path with a trailing backslash, and the long path in upper case, lower case and with a trailing backslash. The path test is also tried with the candidate joined onto the root, onto the long spelling and onto the short one. Every spelling must give the same exact answers. The service's own folders and its two kinds of root file are trusted, and nothing else is. The test skips where the volume has no 8.3 names. A second test pins that the lock and the walk go through the helper.Red: with the short
TEMP, the scripts as pushed (a029b0e) fail the four new cases andTheServiceWrittenList_..., and the five CI failures reproduce. After the change all of them pass, with a normalTEMPand with the short one. The existing assertions are unchanged.Not run in that pass: the full suite,
Installer.Tests, and the scripts against a real install or service. Not changed:Test-PathIsAtOrUnderandTest-DarlingSamePathstill useGetFullPathon both sides. They predate this PR and compare the service's registered folder, not the trust check.Last round: two bot findings
Test-DarlingServiceWrittenFileNamematched the prefixdarling.json.bak-alone, whileTest-DarlingServiceWrittenPathrefuses a name ending in a dot or a space (Windows resolves such a name to a different file). So fordarling.json.bak-x.anddarling.json.bak-xthe lock and the hardening said yes and the pre-lock walk said no. The name test now returns false for an empty name and for a name ending in.or. The prefix match itself is unchanged, anddarling.json.bak-xstill matches. Commit 468afed (Get-DarlingCanonicalPath,Get-DarlingSegmentsBelowRoot) covers the path test only. It did not touch the name test.DarlingConfigBackup.IsBackupNameis the one test behindListBackups(the service sweep) andConfigBackupFiles(the CLI check). It now returns false for a name ending in a dot or a space, with the same reason. Those two names are neither hardened by the service nor listed by the CLI check. The script comment and the class summary say so.TheServiceWrittenFileName_RefusesANameWindowsFolds_AndTheNameAndPathTestsAgreehas 4 cases (both scripts,powershell.exe5.1 andpwsh7). It gives the name test and the path testdarling.json.bak-xandDARLING.JSON.BAK-X(both true) and six folded spellings (both false), plus the empty name.TheBackupNameTest_RefusesANameWindowsFolds_InTheNameTestAndBothListingssits inDarlingHardenFilesJunctionTests, the second class inDarlingHardenFilesVerbTests.cs. It checksIsBackupName, then makes real files through a\\?\path. Both listings skip the folded names and listdarling.json.bak-x.Get-UntrustedWriteGranteesmade itsHashSet[string]with the default ordinal comparer. It now passes([StringComparer]::OrdinalIgnoreCase), the way the otherHashSetlines inupgrade-darling.ps1do, so one path spelled in two cases is one finding. The line is the same in both scripts.Select-Object -Uniqueit replaced was case-insensitive. The Microsoft docs say otherwise:-Uniqueis case-sensitive, and-CaseInsensitivewas added in PowerShell 7.4. I also ran"C:\A","c:\a" | Select-Object -Uniquein Windows PowerShell 5.1.26100 and PowerShell 7.6.6: both return 2 items. So this changes behaviour a little: two findings whose text differs only in case are now one, the first one kept. - Test:TheWalksDeDuplication_TreatsTheSamePathInTwoCasesAsOneFindinghas 4 cases (both scripts, both shells). It takes the two shipped lines ($seen = ...and$unique = ...) from each script. It runs them over four findings, two of which differ only in case. It expects 2 findings with the first spelling kept, and asserts the comparer is named on the line.darling.json.bak-*, because the old "also ... not only" read as additive. The entry also says a name ending in a dot or a space is not treated as a backup.Red: with the three fixes switched off, all 9 new cases fail (4 + 4 + 1). With them on, all pass.
Checks for head c6eb6dc: Darling.Tests builds with 0 warnings.
DarlingInstallLocationTests,DarlingHardenFilesVerbTestsandDarlingHardenFilesJunctionTests,DarlingDeployStaleFileTests,DarlingUpgradeShapes*and every Census and Ratchet class: 631 total, 0 failed, 1 skipped.Stage=Guard: 1481 total, 0 failed. The changelog and guard-workflow classes: 19 total, 0 failed. Not run: the full suite,Installer.Tests, the scripts against a real install or service, the hosted legs. CI decides.Third pass: eight bot threads
Head after this pass: 677cebe (code in 75d192f). Each thread has its own reply.
DarlingConfigBackup.BackupPath(configPath, stamp, suffix)builds the name: config path,.bak-, stamp, and-Nfrom 2 up.DarlingCliCommands.WriteWithBackupAsynccalls it for both names instead of spelling.bak-by hand. The unusedGlobconstant is gone, with the assertion on its own text. The doc comment that said only two lines build the name now says where it is built and listed.TheBackupPrefix_HasOneDefinitionPerLanguage_AndTheSameTextpins the written names byte for byte:darling.json.bak-20260101-120000, the-2form, andprod.json...-13. It also pins that the writer has no hand-written.bak-./resetarm. That arm writes the tree's inherited entries onto the file's other name. NowLock-DarlingInstallTreechecks the name count insideif ($explicit.Count -gt 0). A file whose count is not exactly 1, or cannot be read, goes on the open list and is skipped withcontinue, before eithericaclsarm. Both scripts carry the same text.TheInstallTreeLock_ReportsAnyHardLinkedFileWithAnUntrustedGrant_AndLeavesItsPermissionsAlone, both scripts,powershell.exe5.1 andpwsh7.vendor.dll, aninner.dllin a sub-folder, and two backup-named files in that sub-folder. Each gets an explicit write grant for the built-in Users group.Get-DarlingSegmentsBelowRootnow starts its fold check at index 2 for a path that begins with two backslashes, so the server name is checked too. The drive-letter branch is unchanged.TheSegmentsBelowRoot_ChecksEveryNameOfAUncPath_TheServerAndShareToo, both scripts, both shells...step, and two drive-letter cases.install-darling.ps1warns about it ($networkKind) and asks the operator to confirm.upgrade-darling.ps1has no such question. So the function is fixed on its own terms.Test-DarlingServiceWrittenPathin both scripts now says what the code does. File-or-folder and link come from the listed item. The name count is a live read.LinkType. No trust decision reads it. In both scripts the word appears only in comments.TheBackupPrefix_HasOneDefinitionPerLanguage_AndTheSameTextasserts the lock text has noLinkType..github/scripts/darling-upgrade-shapes.ps1and the comment in.github/workflows/darling-upgrade-shapes.ymlnow say U2 and U3 are U1 without the backup file. Only U1 gets it.Red: with the lock check switched off, all 4 lock cases fail. With the UNC start index put back to 3, all 4 UNC cases fail. With the writer's hand-written name put back,
TheBackupPrefix_HasOneDefinitionPerLanguage_AndTheSameTextfails. With the changes on they pass.Checks for this pass: Darling.Tests builds with 0 warnings.
DarlingInstallLocationTests,DarlingHardenFilesVerbTests(with its junction class),DarlingUpgradeShapes*,DocCommentHygieneand every Census and Ratchet class: 692 total, 0 failed, 1 skipped.DarlingCliCommandsTests,DarlingFileSecurityTestsandDarlingDeployStaleFileTests: 71 total, 0 failed.Stage=Guard: 1481 total, 0 failed. Not run: the full suite,Installer.Tests, the scripts against a real install or service, the hosted legs. CI decides.Owner step: a file with more than one name keeps its owner, and nothing outside the install folder gets one (#5677)
Head after this change: 094a571. Code:
Darling/tools/install-darling.ps1andDarling/tools/upgrade-darling.ps1. The two copies are byte-identical inInitialize-DarlingNativeOwnerand inLock-DarlingInstallTree. Only the wording of the printed advice differs.icacls <root> /setowner Administrators /T /C /L. A file with more than one name has one owner that every name shares. So that call changed the owner under its other names too, which can be outside the install folder.icacls /setowner /Tcall is gone.Lock-DarlingInstallTreewalks the tree itself and callsDarlingNativeOwner.SetOwnerfor each object.SetOwneropens the object withFILE_READ_ATTRIBUTES | WRITE_OWNER, all sharing modes andFILE_FLAG_OPEN_REPARSE_POINT | FILE_FLAG_BACKUP_SEMANTICS. It uses a\\?\path (\\?\UNC\for a UNC path). It reads the link count from that handle. Only a count of exactly 1 is followed bySetSecurityInfofor the owner on the same handle.OpenRootopens the install root once, without sharing delete or rename. The lock keeps that handle open until the walk ends, andCloseRootruns in thefinally. The root's real path is read from that handle withGetFinalPathNameByHandleW(normalized, by volume GUID).SetOwnerreads the real path of the handle it opened, right after opening it and before anything else. Unless that path is the root's path or starts with it plus a backslash, it returns 5 and does not callSetSecurityInfo. The comparison is ordinal and case-sensitive, because both paths come from the file system. So a folder swapped for a junction while the walk runs is never given an owner, and no timing is involved. If the root cannot be opened or its real path cannot be read, the step does not run and the root is listed.has N names (a hard link): its permissions are the same under every one, so its owner and permissions were not changedcould not be opened, error Nits names could not be counted, error Nits owner could not be set, error Nits path resolved outside the install folder, so its owner was not changed; nothing below it was given an owner or checked. The walk keepsGet-ChildItemobjects on its stack, so it knows a folder from a file.Test-DarlingServiceWrittenFileNamealready uses.HashSet[string]with an ordinal comparer. In a folder with the case-sensitive flag, skippinga.dlldoes not skipA.DLL.EnablePrivilegesturns onSeTakeOwnershipPrivilegeandSeBackupPrivilegeon the process token. Backup semantics give read-attributes, so an object that denies Administrators read access still opens.SeRestorePrivilegeis no longer used. The previous state thatAdjustTokenPrivilegesreturns is kept.RestorePrivileges, in thefinally, puts back exactly that and forces nothing off. If either privilege cannot be turned on, the step does not run and the root is listed. The text isthis elevated session does not hold SeTakeOwnershipPrivilege and SeBackupPrivilege (see whoami /priv); a policy may have removed them from Administrators./setownerand/resetcommands are said to be only for paths not marked as a hard link. The per-path reset is nowicacls "<path>" /reset /C, with no/T. Both scripts print the same wording.New-DarlingProtectedStagingFolderno longer says the lock's owner step needs no privilege. It says the step turns on its two privileges itself.Timing, on a scratch tree of 5,000 files in 50 folders plus one file with a second name outside the tree (5,052 objects). It runs the owner-step block of
Lock-DarlingInstallTreeas shipped: root open,Get-ChildItemper folder, andSetOwnerwith the real-path read on every object. The laptop is not elevated, so the privilege call is replaced by$true. The owner is set to the account that already owned everything. So the cost is the walk plus oneSetSecurityInfoper object, not a measure of an ownership change. Two runs each:The earlier numbers (454 ms and 731 ms) timed a bare loop of
SetOwnercalls. These time the whole step, so they are not like for like. The old singleicacls /setowner /Tcall took about 260 ms. The step is slower by roughly half a second to two seconds on 5,000 files, and it runs once per lock. The hard link was the one object listed.Tests are in
DarlingInstallLocationTests. Each case runs under both scripts. A case that runs a probe also runs under Windows PowerShell 5.1 and PowerShell 7.TheInstallTreeLockOwnerStep_SetsTheOwnerPerObjectOnTheCountedHandle_AndHasNoSubtreeCall(updated): the lock opens and closes the root and enables and restores the privileges. It uses the ordinalHashSet. The native code uses the pinned flags, the\\?\spelling, the real-path read and the two privileges, and noSeRestorePrivilege.TheNativeOwnerStep_SetsAPlainFileAndAFolder_AndLeavesAHardLinkedFileAlone(updated to open the root first).TheNativeOwnerStep_ChangesNothingOutsideTheRoot_AndGivesEveryOtherOutcomeItsOwnResult: a folderathat is a junction to an outside folder holding a file.SetOwneron<root>\a\filereturns 5, and the outside file keeps its owner. The junction itself, a plain file, a folder, the root and an upper-case spelling of the root return 0. A deleted path returns 2 with error 2. An owner the system refuses returns 4. With no root open, every path returns 5.TheNativeOwnerStep_OpensANameEndingInADot_AsThatName_NotAsTheNameWithoutIt:dn.(one name) anddn(a hard link), made through\\?\paths.dn.returns 0 with count 1.dnreturns 1 with count 2.TheNativeOwnerStep_PutsThePrivilegesBackAsItFoundThem: the twowhoami /privlines are the same before and after, also after a second turn-on and restore. This laptop is not elevated, so turning them on reports false. In an elevated session the test also expects true and a visible change.TheInstallTreeLock_LeavesAnOutsideFolderAlone_AndListsANameEndingInADotWithoutActingOnIt: the lock over a tree with the junction and adn.beside adn. The outside file keeps its owner, and the junction is listed. In an elevated sessiondn.is listed and keeps its owner, anddnis owned by Administrators. Otherwise the root is listed with the privilege names.TheInstallTreeLock_StillFixesAndChecksAnObjectWhoseOwnerCouldNotBeSet: the privilege call is replaced by$true, so the step runs without elevation and setting Administrators is refused (result 4). The object is listed with its error. Its explicit grant for ordinary users is still removed by the permission walk.TheInstallTreeLock_TreatsTwoNamesThatDifferOnlyInCaseAsTwoObjects: a folder with the case-sensitive flag (fsutil file setCaseSensitiveInfo). It holds a hard-linkeda.dlland a plainA.DLL, each with an explicit grant. The hard link is listed once and keeps its grant.A.DLLloses its grant. The case is skipped with a stated reason where the flag cannot be set. It ran here.TheOwnerStepAdvice_NamesThePrivileges_GivesHardLinksTheirOwnLine_AndDropsTheTreeWideResetandTheUpgradeStagingFolderComment_SaysTheOwnerStepTurnsItsPrivilegesOnItself: the texts above.TheInstallTreeLock_GivesAdministratorsTheOwnerOfPlainFilesAndFolders_AndLeavesAHardLinkedFilesOwner(updated): the new hard-link text and the privilege text.Red: the new and updated tests against the scripts at 7b8fb0b gave 37 failed cases. I also ran
SetOwnerdirectly on the old native class. On<root>\a\filethrough the junction it returned 0 (owner set). Ondn.it returned count 2, the numbers fordn. With the changes they return 5 and count 1. Green:DarlingInstallLocationTests, 113 total, 0 failed.Checks: Darling.Tests builds with 0 warnings. The other classes run were
DarlingHardenFiles*,DarlingUpgradeShapes*,DocCommentHygiene, the changelog classes and every Census and Ratchet class. They gave 626 total, 0 failed, 1 skipped. The skipped case isWebExceptionTextCensusTests.RealToolCaughtStatementTimeout_OnTheRig_Returns503_NoExceptionText_OneLogLine, which needs a live database this laptop does not have. Not run: the full suite,Installer.Tests, the scripts against a real install or service, and an elevated session. The paths where both privileges are held and the owner becomes Administrators run only on CI's runner. CI decides.Test plan
hardLink.count=3. The CLI test fails when the config name is ignored: it listsDARLING.JSON.BAK-Upperforprod.json.DarlingInstallLocationTests:darling.json.bak-linkedis reported with its own line and is not trusted, even with the service as owner and grantee.TheRefusalAdvice_PutsTheBackupNoteAheadOfTheStandardAdvice_OnlyWhenEveryFindingIsABackup, both scripts: the note comes first and the standard advice follows, for a service that runs from this folder and one that does not. Fourteen other shapes keep the standard advice alone, including four findings that sit beside a file calleddarling.json.bak-xwith text that looks like its own.TheWalksRecords_DecideTheBackupNote_AndNoTextIsParsed: the real walk, then the real advice decision, with a nested owner, a folder, a junction and a hard link.TheServiceWrittenRootFiles_...: real files and real hard links in a temp folder. Hard links are left out, andDARLING.JSON.BAK-Bis found, under en-US and under tr-TR. A look-alike with U+0130 is not a backup under either. The test also checks that-likereally does miss the upper-case name under tr-TR here, so the culture took effect.TheBackupPrefix_HasOneDefinitionPerLanguage_AndTheSameTextpins the prefix text, the ordinalStartsWithspelling, the lock and hardening calls, and the C# call sites.TheServiceWrittenList_...now passes the walk's item to the path test, and covers a folder item, a hard-link item, a link item and no item.DarlingHardenFilesVerbTests, under en-US and tr-TR.prod.jsonfindsprod.json.bak-*in any case and notdarling.json.bak-*. The service sweep lists the same files.DarlingInstallLocationTests,DarlingDeployStaleFileTests,DarlingHardenFiles*,DarlingUpgradeShapes*and every Census and Ratchet class: 631 total, 0 failed, 1 skipped (last round, head c6eb6dc).Stage=Guardtrait: 1481 total, 0 failed (last round).DarlingInstallLocationTestsand 626 in the other classes, 0 failed, 1 skipped).Installer.Tests, and the hosted upgrade-shape legs. CI decides.Worth a second look
darling.jsonand the backups). A hard link inside an extractedpg-runtimedoes not refuse on that ground. The lock, though, no longer changes the permissions of any file that has more than one name (third pass).icacls <root> /setowner Administrators /T /C /Lover the tree. It sets the owner of each object in process (section "Owner step" above), only on objects whose real path is inside the root. That is the part of this PR written last, and the native code inInitialize-DarlingNativeOwneris the part to read most closely. The paths that need both privileges held (an elevated token) run only on CI's runner.