Skip to content

The upgrade and installer accept a hand-named darling.json.bak-* backup in the install folder (#5649) - #5659

Merged
erikdarlingdata merged 18 commits into
devfrom
fix/5649-backup-trust
Oct 11, 2026
Merged

erikdarlingdata merged 18 commits into
devfrom
fix/5649-backup-trust

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Oct 11, 2026 •

Copy link
Copy Markdown
Owner

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.json and on stamped backup names only (darling.json.bak-yyyyMMdd-HHmmss with an optional -N). A backup named by hand, such as darling.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 root

  • The service account is trusted, as grantee and as owner, on every plain file in the install root whose name starts with darling.json.bak- (case-insensitive).
  • It is narrow. A folder with that name is not trusted. Neither is a link, a file with more than one name (a hard link) or a file below the root. Any other grantee on such a file still refuses. The service account on any other file still refuses.
  • Whether an item is a file or a folder, and whether it is a link, come from the item the walk already listed. The number of names a file has is read live: Get-DarlingFileNameCount opens the file (for attributes only) each time the path test calls it.
  • One definition per language, and one way to test it:
    • PowerShell: BackupPrefix = 'darling.json.bak-' in Get-DarlingServiceWrittenNames. It sits in the block both scripts keep byte-identical, and a test pins that.
    • Every PowerShell site tests it with StartsWith($prefix, [StringComparison]::OrdinalIgnoreCase). -like and -Filter fold case by the current culture (a Turkish I). Windows PowerShell 5.1 also matches some look-alike characters. Either way they can differ from the service.
    • C#: DarlingConfigBackup.Prefix and DarlingConfigBackup.IsBackupName in Darling/PerformanceMonitor.Darling.Service/DarlingConfigBackup.cs. The directory listing's pattern only narrows the list, and the ordinal name test decides.
  • Every site reads that definition:
    • The pre-lock walk, through Test-DarlingServiceWrittenFileName and Test-DarlingServiceWrittenPath.
    • The install-tree lock and the install-time hardening, through the new Get-DarlingServiceWrittenRootFiles. The hardening calls it with -BackupsOnly.
    • The upgrade's manifest guard.
    • DarlingWorker.TryHardenConfigBackups, through DarlingConfigBackup.ListBackups.
    • The CLI permission check, through DarlingCliCommands.ConfigBackupFiles.
  • The anchored BackupPattern of 3.10.0 is gone from the scripts.

2. Files with more than one name

  • A root file the service writes (darling.json or 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-DarlingServiceWrittenRootFiles leaves 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.
  • The lock's own walk reports such a file and leaves it as it is (see the third pass below for files elsewhere in the tree).
  • A file's names are counted by Get-DarlingFileNameCount (second pass below), never from LinkType. The C# side already refuses these files (DarlingFileSecurity, NumberOfLinks > 1).

3. The refusal advice

  • Get-UntrustedWriteGrantees takes -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 to Get-DarlingRefusalAdvice.
  • Get-DarlingBackupFileFindingNames decides 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.
  • When every finding is a backup file, a note comes first. It names the files. It says a backup can hold the secrets that were in darling.json. It says the files can be moved to a folder only Administrators can read. The standard advice follows it unchanged.
  • The standard advice is unchanged and follows the note. A backup that is a finding belongs to, or is writable by, some account other than the service.

4. The CLI check for a config with another name

  • HardenFiles listed darling.json.bak-* beside any config. It now lists Path.GetFileName(resolvedConfig) plus .bak-, as TryHardenConfigBackups does.

5. CI

  • .github/scripts/darling-upgrade-shapes.ps1 puts a hand-named darling.json.bak-before-edit into 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.
  • The leg fails at setup if the owner is not NT SERVICE\PerformanceMonitor Darling, so it cannot pass without exercising the check. After the run it checks the file is still there.
  • U1 has an install leg and an upgrade leg, so the file is present for both scripts. No other leg changed.
  • The hosted legs did not run here. They run on this PR.

Changes after the first pass

  • Backup note: it comes first and the standard advice follows it unchanged.
  • Hard links: a hard-linked root file the service writes is a finding, and the lock and install loops skip it.
  • Advice decision: made from the walk's records (kind, path, file or not), not from the finding text.
  • Name match: one BackupPrefix, tested with an ordinal case-insensitive StartsWith at every PowerShell site and in C#.
  • CLI check: lists the given config's own backups.
  • File test: file-or-folder and link come from the item the walk listed (the name count is a live read, see the third pass).

Changes after the second pass

  • Counting a file's names. On Windows PowerShell 5.1, LinkType is 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 calls DarlingFileSecurity.HardenWithoutFollowingLinks makes. 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.
  • Every LinkType read is gone. That covers the pre-lock walk's hard-link finding, Get-DarlingServiceWrittenRootFiles, Test-DarlingServiceWrittenPath and the lock's verify walk. The type is compiled with Add-Type the 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.
  • darling.json with more than one name, or a link. install-darling.ps1 now stops before the hardening loop. The message names the file, says the service is installed but not started, and gives the fix: copy it to a new file, delete the old darling.json, rename the copy, run the script again. upgrade-darling.ps1 never adds darling.json to a hardening loop by name. Its owner step reads the root-file list, which now drops any file with more than one name. So it needed no change.
  • Verify walk in the lock. A root file the service writes that has more than one name, or whose names cannot be counted, is reported and skipped the way a junction is. Before, a backup-named hard link with an untrusted write grant was reset. That changed the permissions of its other name too. That pass limited it to root files with a service-written name. The third pass below widens it to every file.
  • CHANGELOG: the entry is still true, so it is unchanged.

Tests, all in DarlingInstallLocationTests, with the result of running each against the code before this pass:

  • TheFileNameCount_IsOneForAPlainFile_AndSeesAHardLinkHeldOpenWithNoReadSharing has 4 cases: both scripts, each under powershell.exe 5.1 and pwsh 7. 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 empty LinkType. It listed the held backup as a service-written file, trusted it, and the walk reported no hard link.
  • TheInstaller_RefusesADarlingJsonWithMoreThanOneName_BeforeItHardensAnything passes 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 on hardLink.refused=True.
  • TheInstallTreeLock_ReportsAHardLinkedBackupWithAnUntrustedGrant_AndLeavesItsPermissionsAlone hard-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.
  • Updated for the new helper: the service-written-list test now uses real files in a temp root, because a path that does not exist is never trusted. The probes that run the walk now define the service-written list. The lock pin asserts the new clause and no 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 in GetFullPath. 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 pointing TEMP at the short alias of a long-named folder. The same five failures came back, with the same messages.

  • TheServiceWrittenList_MatchesOnlyTheNamesTheServiceWrites was a product defect. Test-DarlingServiceWrittenPath spelled 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-DarlingCanonicalPath gives one long, absolute spelling of a path. It runs GetFullPath, then GetLongPathName on the part that exists, and drops the trailing backslash. It returns $null when nothing can be worked out, and every caller then treats the path as not trusted. Get-DarlingSegmentsBelowRoot puts 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. So pg-runtime.\a.dll and pg-runtime\..\x still match nothing.

The walk, Test-DarlingServiceWrittenPath, Get-DarlingBackupFileFindingNames, Get-DarlingExtraServiceWriteDirectories and Lock-DarlingInstallTree use 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_AnswerTheSameForARootInShortFormWithATrailingBackslashOrInOtherCase runs 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 and TheServiceWrittenList_..., and the five CI failures reproduce. After the change all of them pass, with a normal TEMP and 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-PathIsAtOrUnder and Test-DarlingSamePath still use GetFullPath on both sides. They predate this PR and compare the service's registered folder, not the trust check.

Last round: two bot findings

  1. A name Windows folds is not a service backup. Test-DarlingServiceWrittenFileName matched the prefix darling.json.bak- alone, while Test-DarlingServiceWrittenPath refuses a name ending in a dot or a space (Windows resolves such a name to a different file). So for darling.json.bak-x. and darling.json.bak-x the 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, and darling.json.bak-x still matches. Commit 468afed (Get-DarlingCanonicalPath, Get-DarlingSegmentsBelowRoot) covers the path test only. It did not touch the name test.
    • The C# side decides the same thing: DarlingConfigBackup.IsBackupName is the one test behind ListBackups (the service sweep) and ConfigBackupFiles (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.
    • Tests: TheServiceWrittenFileName_RefusesANameWindowsFolds_AndTheNameAndPathTestsAgree has 4 cases (both scripts, powershell.exe 5.1 and pwsh 7). It gives the name test and the path test darling.json.bak-x and DARLING.JSON.BAK-X (both true) and six folded spellings (both false), plus the empty name. TheBackupNameTest_RefusesANameWindowsFolds_InTheNameTestAndBothListings sits in DarlingHardenFilesJunctionTests, the second class in DarlingHardenFilesVerbTests.cs. It checks IsBackupName, then makes real files through a \\?\ path. Both listings skip the folded names and list darling.json.bak-x.
  2. The walk's de-duplication ignores case. Get-UntrustedWriteGrantees made its HashSet[string] with the default ordinal comparer. It now passes ([StringComparer]::OrdinalIgnoreCase), the way the other HashSet lines in upgrade-darling.ps1 do, so one path spelled in two cases is one finding. The line is the same in both scripts.
    • The finding says the Select-Object -Unique it replaced was case-insensitive. The Microsoft docs say otherwise: -Unique is case-sensitive, and -CaseInsensitive was added in PowerShell 7.4. I also ran "C:\A","c:\a" | Select-Object -Unique in 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_TreatsTheSamePathInTwoCasesAsOneFinding has 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.
  3. The CHANGELOG entry changed in two places. The CLI check now lists the backups of the config it was given instead of always 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, DarlingHardenFilesVerbTests and DarlingHardenFilesJunctionTests, 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.

  • Backup name built in one place. DarlingConfigBackup.BackupPath(configPath, stamp, suffix) builds the name: config path, .bak-, stamp, and -N from 2 up. DarlingCliCommands.WriteWithBackupAsync calls it for both names instead of spelling .bak- by hand. The unused Glob constant 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_AndTheSameText pins the written names byte for byte: darling.json.bak-20260101-120000, the -2 form, and prod.json...-13. It also pins that the writer has no hand-written .bak-.
  • Lock and files with more than one name. The lock can reach a hard-linked file that is not a service-written root file. Any hard link in the tree with an explicit untrusted write grant went to the /reset arm. That arm writes the tree's inherited entries onto the file's other name. Now Lock-DarlingInstallTree checks the name count inside if ($explicit.Count -gt 0). A file whose count is not exactly 1, or cannot be read, goes on the open list and is skipped with continue, before either icacls arm. Both scripts carry the same text.
    • Test: TheInstallTreeLock_ReportsAnyHardLinkedFileWithAnUntrustedGrant_AndLeavesItsPermissionsAlone, both scripts, powershell.exe 5.1 and pwsh 7.
    • It makes four hard-linked files: a root vendor.dll, an inner.dll in a sub-folder, and two backup-named files in that sub-folder. Each gets an explicit write grant for the built-in Users group.
    • The lock must name each one, and both names of each must keep the grant. A plain file with the same grant is still reset.
  • UNC names. Get-DarlingSegmentsBelowRoot now 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.
    • Test: TheSegmentsBelowRoot_ChecksEveryNameOfAUncPath_TheServerAndShareToo, both scripts, both shells.
    • It covers a normal UNC path below a UNC root (names returned) and a server or share name with a trailing dot or space. It also covers an empty server name, a trailing-dot leaf, a .. step, and two drive-letter cases.
    • A UNC install root is not refused outright. install-darling.ps1 warns about it ($networkKind) and asks the operator to confirm. upgrade-darling.ps1 has no such question. So the function is fixed on its own terms.
  • Comment. The comment above Test-DarlingServiceWrittenPath in 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_AndTheSameText asserts the lock text has no LinkType.
  • Shape check wording. The header of .github/scripts/darling-upgrade-shapes.ps1 and the comment in .github/workflows/darling-upgrade-shapes.yml now say U2 and U3 are U1 without the backup file. Only U1 gets it.
  • CHANGELOG. The "also ... not only" wording was already changed to "instead of" in c6eb6dc. This pass adds one sentence: the lock leaves any other file with more than one name alone and lists it as still open.

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_AndTheSameText fails. With the changes on they pass.

Checks for this pass: Darling.Tests builds with 0 warnings. DarlingInstallLocationTests, DarlingHardenFilesVerbTests (with its junction class), DarlingUpgradeShapes*, DocCommentHygiene and every Census and Ratchet class: 692 total, 0 failed, 1 skipped. DarlingCliCommandsTests, DarlingFileSecurityTests and DarlingDeployStaleFileTests: 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.ps1 and Darling/tools/upgrade-darling.ps1. The two copies are byte-identical in Initialize-DarlingNativeOwner and in Lock-DarlingInstallTree. Only the wording of the printed advice differs.

  • What it did. The lock began with one 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.
  • What it does. The icacls /setowner /T call is gone. Lock-DarlingInstallTree walks the tree itself and calls DarlingNativeOwner.SetOwner for each object. SetOwner opens the object with FILE_READ_ATTRIBUTES | WRITE_OWNER, all sharing modes and FILE_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 by SetSecurityInfo for the owner on the same handle.
  • Staying inside the folder. Before the walk, OpenRoot opens the install root once, without sharing delete or rename. The lock keeps that handle open until the walk ends, and CloseRoot runs in the finally. The root's real path is read from that handle with GetFinalPathNameByHandleW (normalized, by volume GUID).
  • The check on each object. SetOwner reads 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 call SetSecurityInfo. 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.
  • One text per result. Each result is listed once, in its own words:
    • 1: has N names (a hard link): its permissions are the same under every one, so its owner and permissions were not changed
    • 2: could not be opened, error N
    • 3: its names could not be counted, error N
    • 4: its owner could not be set, error N
    • 5: its path resolved outside the install folder, so its owner was not changed
    • A folder with result 2, 3 or 5 also gets ; nothing below it was given an owner or checked. The walk keeps Get-ChildItem objects on its stack, so it knows a folder from a file.
  • What the permission walk skips. Results 1, 2, 3 and 5 go in the skip set, so the walk below does not list or touch them again. Result 4 does not. Its count was 1, so the permission walk still removes its untrusted grants and checks it, as before the owner step. The root is never put in the skip set. If it fails, it is listed as open and the permission walk still handles it.
  • Names that Windows folds. A name that ends in a dot or a space is listed as open and not acted on. It goes in the skip set. This is the rule Test-DarlingServiceWrittenFileName already uses.
  • Case. The skip set is a HashSet[string] with an ordinal comparer. In a folder with the case-sensitive flag, skipping a.dll does not skip A.DLL.
  • Links. The walk does not follow a junction or link. The link itself gets the call. The attribute bits read from the handle decide whether to list its contents. A folder that cannot be listed is reported, and nothing below it is given an owner.
  • Privileges. EnablePrivileges turns on SeTakeOwnershipPrivilege and SeBackupPrivilege on the process token. Backup semantics give read-attributes, so an object that denies Administrators read access still opens. SeRestorePrivilege is no longer used. The previous state that AdjustTokenPrivileges returns is kept. RestorePrivileges, in the finally, 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 is this elevated session does not hold SeTakeOwnershipPrivilege and SeBackupPrivilege (see whoami /priv); a policy may have removed them from Administrators.
  • The printed advice. Neither script says "run elevated" for a privilege failure, because both already require it. They say to run the script again and name the two privileges. A path marked as a hard link gets its own line. It says to delete this name if it is not part of the install, or extract the file again if it is, and not to run icacls on it. The per-path /setowner and /reset commands are said to be only for paths not marked as a hard link. The per-path reset is now icacls "<path>" /reset /C, with no /T. Both scripts print the same wording.
  • Upgrade comment. The comment on New-DarlingProtectedStagingFolder no longer says the lock's owner step needs no privilege. It says the step turns on its two privileges itself.
  • CHANGELOG. The The upgrade and installer accept a hand-named darling.json.bak-* backup in the install folder (#5649) #5659 entry now also describes the owner step. It opens the install folder once and changes an owner only on an object whose real path is inside it. It lists a dot or space name instead of acting on it. It puts the privileges back as it found them.

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-DarlingInstallTree as shipped: root open, Get-ChildItem per folder, and SetOwner with 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 one SetSecurityInfo per object, not a measure of an ownership change. Two runs each:

Windows PowerShell 5.1 PowerShell 7
Owner step, run 1 1,124 ms 2,117 ms
Owner step, run 2 669 ms 1,573 ms
Listing the tree alone 292 ms, 133 ms 25 ms, 36 ms

The earlier numbers (454 ms and 731 ms) timed a bare loop of SetOwner calls. These time the whole step, so they are not like for like. The old single icacls /setowner /T call 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 ordinal HashSet. The native code uses the pinned flags, the \\?\ spelling, the real-path read and the two privileges, and no SeRestorePrivilege.
  • TheNativeOwnerStep_SetsAPlainFileAndAFolder_AndLeavesAHardLinkedFileAlone (updated to open the root first).
  • TheNativeOwnerStep_ChangesNothingOutsideTheRoot_AndGivesEveryOtherOutcomeItsOwnResult: a folder a that is a junction to an outside folder holding a file. SetOwner on <root>\a\file returns 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) and dn (a hard link), made through \\?\ paths. dn. returns 0 with count 1. dn returns 1 with count 2.
  • TheNativeOwnerStep_PutsThePrivilegesBackAsItFoundThem: the two whoami /priv lines 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 a dn. beside a dn. The outside file keeps its owner, and the junction is listed. In an elevated session dn. is listed and keeps its owner, and dn is 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-linked a.dll and a plain A.DLL, each with an explicit grant. The hard link is listed once and keeps its grant. A.DLL loses its grant. The case is skipped with a stated reason where the flag cannot be set. It ran here.
  • TheOwnerStepAdvice_NamesThePrivileges_GivesHardLinksTheirOwnLine_AndDropsTheTreeWideReset and TheUpgradeStagingFolderComment_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 SetOwner directly on the old native class. On <root>\a\file through the junction it returned 0 (owner set). On dn. it returned count 2, the numbers for dn. 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 is WebExceptionTextCensusTests.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

  • RED against the first-pass scripts (the files at acafd47, with the new tests). The refusal-advice test (both scripts), the record-driven advice test, the root-files test, the prefix pin and the name-list test all fail. The hard-link walk case also ran with only the hard-link code switched off, and it fails on hardLink.count=3. The CLI test fails when the config name is ignored: it lists DARLING.JSON.BAK-Upper for prod.json.
  • New or changed tests in DarlingInstallLocationTests:
    • A hard-linked darling.json.bak-linked is 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 called darling.json.bak-x with 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, and DARLING.JSON.BAK-B is 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 -like really does miss the upper-case name under tr-TR here, so the culture took effect.
    • TheBackupPrefix_HasOneDefinitionPerLanguage_AndTheSameText pins the prefix text, the ordinal StartsWith spelling, 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.
  • New test in DarlingHardenFilesVerbTests, under en-US and tr-TR. prod.json finds prod.json.bak-* in any case and not darling.json.bak-*. The service sweep lists the same files.
  • Darling.Tests builds with 0 warnings.
  • DarlingInstallLocationTests, DarlingDeployStaleFileTests, DarlingHardenFiles*, DarlingUpgradeShapes* and every Census and Ratchet class: 631 total, 0 failed, 1 skipped (last round, head c6eb6dc).
  • Stage=Guard trait: 1481 total, 0 failed (last round).
  • Third pass: the numbers are in the section above (692, 71 and 1481 total, 0 failed).
  • Owner step (Install scripts: the lock's owner step skips a file with more than one name, as the permission step does #5677): numbers are in the section above (113 total in DarlingInstallLocationTests and 626 in the other classes, 0 failed, 1 skipped).
  • Not run here: an elevated session, the full suite, Installer.Tests, and the hosted upgrade-shape legs. CI decides.

Worth a second look

  • A refusal made only of backup files now carries the note and then the standard advice.
  • The pre-lock hard-link finding covers only the root files the service writes (darling.json and the backups). A hard link inside an extracted pg-runtime does not refuse on that ground. The lock, though, no longer changes the permissions of any file that has more than one name (third pass).
  • The lock's owner step no longer runs icacls <root> /setowner Administrators /T /C /L over 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 in Initialize-DarlingNativeOwner is the part to read most closely. The paths that need both privileges held (an elevated token) run only on CI's runner.

…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)
@erikdarlingdata
erikdarlingdata marked this pull request as ready for review October 11, 2026 15:33
Comment thread Darling/tools/install-darling.ps1 Outdated
# 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]'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
$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).

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 + "*";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking, two small gaps in "one definition per language":

  1. Glob has no consumer. Product code uses GlobSuffix (DarlingCliCommands.cs:4344) and Prefix is read only by the pin in DarlingInstallLocationTests.cs:1423; Glob is referenced nowhere except DarlingInstallLocationTests.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.

  2. 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 beside darling.json COUNT as its backups") is honest about this, but it means Tail is the single spelling of the read side only — change Tail and the writer keeps emitting .bak-, which is exactly the drift this file exists to prevent. Routing those two lines through DarlingConfigBackup.Tail would 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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread CHANGELOG.md Outdated

### 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-*`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 16 to 18
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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')).

Comment thread Darling/tools/install-darling.ps1 Outdated
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The pre-lock walk raises no finding when such a file is admin-owned with no untrusted write ACE, so the install proceeds.
  2. The lock then runs icacls.exe $secret.FullName /setowner "*$serviceSid" on it (line 991) and the install-time pass runs Set-Acl -LiteralPath $secretFile on it (line 1567). Win32 and Path.GetFullPath both strip a trailing dot or space from the last component, so both calls land on darling.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 on darling.json.bak-x. ($isSecretFile true) 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.)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, and darling.json.bak-x still 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-DarlingServiceWrittenRootFiles and 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 behind ListBackups (the service sweep, DarlingWorker.cs:2248) and ConfigBackupFiles (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) checks darling.json.bak-x and DARLING.JSON.BAK-X (true), darling.json.bak-x., darling.json.bak-x and four more folded spellings (false), and that the path test gives the same answer for each. TheBackupNameTest_RefusesANameWindowsFolds_InTheNameTestAndBothListings covers IsBackupName and 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-*.

Comment thread Darling/tools/install-darling.ps1 Outdated
# 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')

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread CHANGELOG.md Outdated

### 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-*`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

claude[bot]
claude Bot previously requested changes Oct 11, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 by BackupPrefix at 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 — $item threads 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 — -AsRecords returns 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-DarlingRefusalAdvice at :581-586, and the 15-label list (14 plus the noFindings baseline) 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 — HardenFiles now lists the given config's own backups — DarlingCliCommands.cs:4125 via ConfigBackupFiles (:4171) and DarlingConfigBackup.IsBackupName (DarlingConfigBackup.cs:25), the same helper DarlingWorker.cs:2248 calls
  • UNVERIFIED — LinkType was 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 is powershell.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 — BackupGlob and "You do not need to move the install" are gone — BackupPattern really 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

  1. 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_MatchesOnlyTheNamesTheServiceWrites and TheBackupPrefix_HasOneDefinitionPerLanguage_AndTheSameText for case folding, UNPINNED for a name Windows folds
  2. 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)
  3. 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 by TheWalksRecords_DecideTheBackupNote_AndNoTextIsParsed, for texts by the pre-existing ThePreLockTrustedSet_… 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.dll and darling.json.bak-20260101 from 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/ or Lite.Tests/ names a darling.json or 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
  • "InternalsVisibleTo on PerformanceMonitor.Analysis names Lite.Tests only — a helper Darling.Tests must call has to be public (#3688)" — does not apply — DarlingConfigBackup and ConfigBackupFiles sit in PerformanceMonitor.Darling.Service, whose csproj has <InternalsVisibleTo Include="Darling.Tests" /> (:87)
  • "claude-code-action refuses to run when .github/workflows/claude-review.yml differs 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-Path call 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-upgrade logs for the owner of …darling.json.bak-before-edit: NT SERVICE\PerformanceMonitor Darling line — without it the leg passes without exercising the check
  • Build and run Darling.Tests on 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, run install-darling.ps1, and check with icacls which file ends up service-owned
  • Run HardenFiles --config prod.json in a folder holding both prod.json.bak-* and darling.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.
@erikdarlingdata
erikdarlingdata enabled auto-merge (squash) October 11, 2026 16:16
… 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) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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++) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: LGTM

Claims

  • VERIFIED — Test-DarlingServiceWrittenFileName now returns false for a name ending in . or ' ' (and for empty), agreeing with Test-DarlingServiceWrittenPath's fold guard — Darling/tools/install-darling.ps1 and upgrade-darling.ps1, the if ([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 unanchored StartsWith
  • 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 by TheBackupNameTest_RefusesANameWindowsFolds_InTheNameTestAndBothListings in DarlingHardenFilesVerbTests.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 by TheFileNameCount_IsOneForAPlainFile_AndSeesAHardLinkHeldOpenWithNoReadSharing (4 cases) and TheInstaller_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 /reset arm still acts on hard links" note), via the new continue-guarded branch in Lock-DarlingInstallTree, pinned by TheInstallTreeLock_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 by TheWalkAndThePathTests_AnswerTheSameForARootInShortFormWithATrailingBackslashOrInOtherCase
  • VERIFIED — Get-UntrustedWriteGrantees's de-duplication now uses an OrdinalIgnoreCase HashSet[string] comparer (the second "last round" bot finding) — [StringComparer]::OrdinalIgnoreCase added to the New-Object call in both scripts, pinned by TheWalksDeDuplication_TreatsTheSamePathInTwoCasesAsOneFinding
  • VERIFIED — the CHANGELOG's #5659 entry was reworded from "also … not only" (additive-sounding) to "now lists … instead of always …" (substitutive), matching the actual DarlingCliCommands.cs behavior — the prior review's third non-blocking note
  • VERIFIED — .github/scripts/darling-upgrade-shapes.ps1's $HandNamedBackup → $HandNamedBackupName rename fixes a real case-insensitive collision with New-Base's [bool]$handNamedBackup parameter (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

  1. Darling/tools/install-darling.ps1:485 (and upgrade-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 calls Get-DarlingFileNameCount $candidate, a fresh native CreateFileW per call — pinned functionally (the behavior is correct and tested) but the invariant stated in the comment is UNPINNED and now false; flagged inline, non-blocking
  2. Darling/tools/install-darling.ps1:452 (and upgrade-darling.ps1:504) — Get-DarlingSegmentsBelowRoot's $first = 3 for a UNC candidate skips the server segment in the fold-check loop (only share onward 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-blocking
  3. Darling/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 by TheInstaller_RefusesADarlingJsonWithMoreThanOneName_BeforeItHardensAnything for 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.ps1 and upgrade-darling.ps1 cannot 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 by ExtractFunction(InstallScript, name) vs ExtractFunction(upgrade, name) comparisons that now include all three new helpers (DarlingInstallLocationTests.cs, the ThePreLockWritableExtractionCheck_ShipsIdenticallyInTheInstallAndUpgradeScripts list)
  • "InternalsVisibleTo on PerformanceMonitor.Analysis names Lite.Tests only" — does not apply — DarlingConfigBackup.IsBackupName is called directly from DarlingHardenFilesVerbTests.cs (Darling.Tests), and PerformanceMonitor.Darling.Service's own InternalsVisibleTo already 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.Tests on Windows (both powershell.exe 5.1 and pwsh 7) to confirm the counts the PR body cites, since every new probe in this range needs a real Windows host
  • Confirm Get-DarlingFileNameCount's native CreateFileW/GetFileInformationByHandle pair 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's GetLongPathNameW walk 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

…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.
…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.
@erikdarlingdata
erikdarlingdata dismissed claude[bot]’s stale review October 11, 2026 21:06

Claude posting for Erik Darling. Every finding in this review is answered: each inline thread has a fix commit (c6eb6dc, 75d192f) or a reply, the summary items are fixed in c6eb6dc and 75d192f or pinned, and the later review on c6eb6dc is LGTM.

@erikdarlingdata
erikdarlingdata merged commit ea8b243 into dev Oct 11, 2026
38 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/5649-backup-trust branch October 11, 2026 21:06
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.

1 participant