fix: make the v2.0.1 release path actually installable on Windows - #5
Merged
Conversation
Test-NoUnauthorizedWriteAccess built its write mask by ORing in Modify and FullControl, both composite FileSystemRights values that also carry the read bits, so the check flagged the deliberate BUILTIN\Users ReadAndExecute grant as unauthorized write access. Build the mask from atomic write-only rights instead (Write, Delete, DeleteSubdirectoriesAndFiles, ChangePermissions, TakeOwnership) in both the real check and the duplicated mask in the installed-state CI harness, and rescope the static assertion test to the function body so it actually pins the atomic rights instead of matching Modify and FullControl elsewhere in the file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…cess RaiseException at ssPostInstall never changed Setup's exit code. By that step Inno has already copied the files and written the uninstall key, so the exception is caught, logged, shown as a runtime error, and Setup deinitializes with exit code 0. CI run 30760330669 recorded exactly that: service setup exited 1, the installer reported success, and no rollback ran. CurStepChanged now records the failure in a module-level flag and DeinitializeSetup, which runs late enough to control the real process exit code, calls kernel32 ExitProcess(1). The install is already committed at that point, so both failure messages tell the operator to uninstall before retrying. The guard test was a three-way substring or over the whole file, satisfied by the comment text alone, and would have passed with both RaiseException calls deleted. It now strips comments, pins the flag, the kernel32 import, the CurStepChanged assignment, and the exit code, and it fails when any of them is removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Narrowing the shared ACL mask to write-conferring bits was correct for the executable tree, which deliberately grants Users ReadAndExecute. It was wrong for the two secret paths that used the same helper. ReadAndExecute (0x200A9) -band the new mask (0xD0156) is 0, so a BUILTIN\Users read grant on the bearer token or the TLS private key started passing verification silently. Read access to either file is complete authentication or transport compromise, and catching a provisioning regression that leaks them is the whole job of this control. Test-NoUnauthorizedWriteAccess keeps the write mask for the executable paths. A new Test-NoUnauthorizedAccess rejects any allow ACE for Everyone, Authenticated Users, or Users on the token and the private key, with no rights mask at all. The duplicated block in the CI harness gets the same split, and the mask drift test now pins which call site uses which form. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…set window Two defects in Set-ProtectedApplicationAcl. The failed-file count was parsed out of the icacls summary line with an English-only regex. On a German, French, Japanese, Spanish, or Portuguese Windows the match missed, the count fell back to 0, and the fail-closed check degraded to exactly the exit-code-only test that /C makes unreliable. Dropping /C makes icacls stop at the first per-file failure and return nonzero in every locale, so the exit code alone is authoritative and the locale-dependent parse is removed rather than left as dead code. The trade is that one locked file now aborts the install instead of being reported at the end, which is the intended fail-closed behavior. The tree was also reset to inherited before the root was protected. C:\ grants Authenticated Users an inheritable Modify and the default install path sits directly under it, so for the length of two passes over roughly 3000 files every file in the tree was writable by any local user. On a fresh install that is not a regression. On an upgrade it un-hardens a protected tree that the service executes from. The root ACL now goes on first and the reset is scoped to the children, since resetting the root itself would drop the protection just applied. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
update.bat re-hardens the same tree the service executes from, so a user-writable file left anywhere under it is local privilege escalation. Two fixes this branch made in service-setup.ps1 never reached this copy. Set-HardenedAppAcl kept the fail-open icacls pattern: /C plus an exit-code-only check, with the output piped to Out-Null so a per-file failure was invisible. It also never set the owner, and an owner carries implicit WRITE_DAC and can re-grant itself write regardless of the DACL, which matters because a portable tree is unpacked by whatever account ran the unzip. It also reset the tree before protecting the root. All three now match Set-ProtectedApplicationAcl, and a new test pins the two copies together the way the ACL mask drift test already does. The script also called Get-Acl and Set-Acl with no PSModulePath rebuild. cmd hands powershell.exe the caller's environment, so running the updater from a pwsh 7 prompt gives 5.1 the 7.x module tree and Get-Acl fails to autoload Microsoft.PowerShell.Security. Same root cause the branch already fixed twice, left live in the third script with the pattern. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Repinning get-pip to an immutable commit URL pinned nothing. verify-downloads.ps1 reads Sha256 and MinimumBytes out of the manifest and lints the shape of Url, but no download path ever used Url, so the field was documentation and build-installer.ps1 kept fetching https://bootstrap.pypa.io/get-pip.py. That is a floating endpoint whose content changes on roughly every pip release, so the next rotation breaks the build with a digest mismatch. Neither guard could see it: the PowerShell floating-URL regex scans only the manifest, and the Python assertion read only asset-versions.ps1. The whole class is fixed rather than the one entry. build-installer.ps1 now dot-sources the manifest and resolves every download URL, and the Python, NSSM, LHM, and Inno Setup version strings, through Get-AssetUrl and Get-Asset. A -PythonVersion that disagrees with the pinned entry now fails immediately instead of several minutes later on a digest mismatch. A new test walks every Get-FileFromUrl call site and fails if the first argument is a literal http URL rather than a manifest-sourced value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Path.write_bytes creates the key at the process umask, which is 0644 on a
normal Linux box. The Windows installed path is covered, since
provision-security.ps1 hardens the TLS directory before calling this and
re-restricts the key right after, but the script also ships in {app}\tools and
in the portable release as a standalone entry point. Running it directly
produced a world-readable RSA private key.
The key is now created through os.open with mode 0600, so there is no window
where it exists with wider permissions rather than a mode fixed after the fact.
Any existing key file is unlinked first, because the mode argument only applies
to a file the call creates and a stale 0644 key would otherwise keep its
permissions through the rewrite.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…mentation Each of these asserted something weaker than it read. The advertised-identity test asserted "Where-Object" appears anywhere in service-setup.ps1, which Resolve-FirewallProfiles satisfies on its own, so it held with the fix reverted as long as the old line was not restored verbatim. It now pins the single $AdvertisedIdentity assignment. Its second half was an or over two mechanisms, pinning neither. Both filters are now required. The trap test asserted the bare word "trap", which a comment satisfies. It now requires the trap block itself. The workflow test asserted "if: always()" appeared somewhere in the file rather than on the step that dumps the installer and service setup logs, which is the step that has to survive a failed install. It now scopes to that step. The updater ACL test asserted the substring "readandexecute", which cannot tell the Users grant apart from the SYSTEM grant that predates it. It now pins the whole access rule. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73e026f23f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
cmd's caret continuation on the -Command fragments feeds each quoted line to powershell.exe as a separate argument, which PowerShell then joins with spaces into one logical line. A bare `#` inside any of those fragments comments out every fragment joined after it, and because the enclosing function's closing brace never runs, it is a parse error rather than a silent no-op. Convert the four bare `#` comments (three inside Set-HardenedAppAcl, one before the release secret scan) to `<# ... #>` block comment form, which is safe on a single joined line. No PowerShell logic or batch structure changed. Adds a regression test that reconstructs the joined -Command source and walks it tracking single-quote and block-comment state, so it flags a bare line-comment `#` without false-positiving on `#` inside a quoted string or regex character class. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Set-ProtectedApplicationAcl trimmed the trailing separator off its target
with $Path.TrimEnd('\'). For a drive root like C:\ that yields the
drive-relative C:, so the recursive child reset expanded to
"icacls C:\* /reset /T" and reset ACLs across the entire system drive,
and Get-Acl/Set-Acl against C: landed on the process current directory
instead of the drive root. HumWatch.iss leaves DisableDirPage=no and Inno
honours /DIR="C:\", so a scripted install could reach it. update.bat's
copied Set-HardenedAppAcl had the identical defect.
Both copies now normalize the path (fold forward slashes, strip an
extended-length prefix, trim the trailing separator) and refuse a volume
root before any Get-Acl, Set-Acl, or icacls call, so nothing is mutated
on the way to the error. Rejected: C:\, C:, c:/, anything that trims to a
bare drive, and a UNC share root such as \\server\share. Accepted:
C:\HumWatch, C:\HumWatch\, D:\Program Files\HumWatch, and
\\server\share\HumWatch. The root-ACL-before-child-reset ordering is
unchanged.
Covered behaviorally in the pwsh contract harness for both copies, with a
fake icacls so a regression can never reach the real binary, and at the
source level so the guard cannot be deleted without a failure on Linux.
The icacls fail-closed throw count is now scoped past the first icacls
call so the new refusal does not dilute it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d200805250
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Under Windows PowerShell 5.1 the 2>&1 redirection converts native stderr into error records, and the file-level $ErrorActionPreference = "Stop" turns the first one into a terminating error. The assignment died before $LASTEXITCODE was read, so the wide Out-String diagnostic never ran and the trap logged a bare NativeCommandError instead of the captured Python output, defeating the stderr capture this exists for. Bracket the one native call with the same save, silence, capture, restore sequence Invoke-Nssm and the pip bootstrap already use. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d511a1a79
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A pre-hardening tree is writable by ordinary users, so a local user can plant a junction or symlink inside it before the elevated installer, updater, or provisioner runs. The recursive icacls passes lacked /L and would follow the reparse point, resetting ACLs and ownership outside the install directory. Both passes in both copies now carry /L. Set-RestrictedAcl had the same hole twice over: Get-ChildItem -Recurse traverses junctions under PowerShell 5.1, and Get-Acl/Set-Acl resolve against the reparse target rather than the link. The walk is now a manual queue that refuses a reparse point as its root and neither descends into nor touches reparse-point children. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46c3e19f90
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A skipped link stays in the runtime tree, and the later already-provisioned branches would then let the service read its TLS material or token through a link a local user planted before the upgrade. Refusing the whole pass fails closed and surfaces the planted entry in the log. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
The v2.0.1 release path. The
Windows installed service securityCI job had never passed, not once, since it was written. It passes now, and it stayed green across three consecutive runs.Getting there took five root causes, found one at a time because each one hid the next.
The five causes
MsgBoxignores/SUPPRESSMSGBOXES. A headless runner sat on a modal dialog until the 20 minute step timeout. This is why the job looked like a hang.PSModulePath, sopowershell.exe5.1 inherited it, could not autoloadMicrosoft.PowerShell.Security, andGet-Aclfailed. An earlier commit had fixed this in the test harness only and explicitly ruled it "a harness artifact, not a product problem". It was a product problem.._pthsuppresses the script-directorysys.pathentry, sogenerate_certificate.pycould not import its sibling module. Certificate generation died with its stderr discarded.@($null)is a one-element array, so a dangling--advertised-identityflag reached argparse with no value.ModifyandFullControlare composite rights whose bitmasks include read bits.Test-NoUnauthorizedWriteAccessbuilt its write mask from them, so it flagged theBUILTIN\UsersReadAndExecute grant the installer deliberately creates. The check and the grant had contradicted each other since the day both were written.The plan this branch started from blamed a slow recursive ACL walk. That was wrong. The ACL rewrite in Task 2 is still worth having (the double walk was genuinely slow), it just was never the hang.
Also fixed
RaiseExceptionatssPostInstalldoes nothing useful, because Inno has already committed the install by then and swallows the exception. Silent and scripted installs were told they succeeded while the machine had files, an Add/Remove Programs entry, no service, and an unhardenedC:\HumWatch. Now a failure flag set inCurStepChangeddrivesExitProcess(1)fromDeinitializeSetup.get-pipto an immutable commit URL changed nothing, becausebuild-installer.ps1hardcodedbootstrap.pypa.ioand the manifestUrlfield was lint-only. All eight downloads now come from the manifest.update.bathad drifted fromservice-setup.ps1, missing both the hardened icacls pattern and thePSModulePathrebuild./Cmakes icacls stop on the first error, so the exit code alone is sufficient and the locale dependency is gone.Set-ProtectedApplicationAclreset the tree to inherited before protecting the root, briefly un-hardening an existing install mid-upgrade.__main__.(st_dev, st_ino)plus mtime and size, with an mtime-granularity guard so a same-tick rotation cannot serve a revoked token.config.jsonis a dpkg conffile, so Debian upgrades stop overwriting operator edits.Evidence
Verify-InstalledStateto throw. TheInstall HumWatch with default firewall taskstep went red and the downstream verification step was correctly skipped. That same step concludedsuccessin run 30760330669 while service setup exited 1.Known and deliberately deferred
update.bathas PowerShell#comments inside a cmd^-continued-Commandstring (lines 97, 98, 99, 149). If cmd joins those fragments onto one line, the first#comments out the rest of the script and the portable updater has never worked. Pre-existing onmain, flagged independently by two reviewers, and CI never executes the realupdate.bat. Wants ten minutes on a Windows box before release.update.batandservice-setup.ps1still grant SYSTEM different rights on the same tree.agent/__init__.pyis still2.0.0. The bump to2.0.1is the first step of release ops, and tagging before it would break every portableupdate.batrun.built_debfixture can pick a stale package,-ContractOnlynow has filesystem side effects.Review trail
Every task was reviewed independently, and the whole branch was reviewed twice at the end by two separate reviewers that converged on the same Critical finding with the same CI evidence. Every fix in the final wave was written test-first, observed red, and additionally mutation-tested by reverting each fix individually to confirm the intended test caught it.
🤖 Generated with Claude Code