Fix Windows ipc tests: accept self-owned pipe in test binary only#24
Open
korjwl1 wants to merge 1 commit into
Open
Fix Windows ipc tests: accept self-owned pipe in test binary only#24korjwl1 wants to merge 1 commit into
korjwl1 wants to merge 1 commit into
Conversation
go test ./internal/ipc was broken on Windows: verifyPipeOwner accepts only SY/BA pipe owners, but an unelevated test binary owns its in-process listener pipe as the user SID, so every Dial was rejected. TestClientServerRPC and TestEventBroadcast failed, and TestMethodNotFound panicked on a nil client because it discarded NewClient's error. - transport_windows.go: add allowSelfOwnedPipes escape hatch, settable only from this package's tests (init in transport_windows_test.go). Production builds never set it, so the SY/BA requirement is unchanged. Verified empirically: an elevated process owns its pipe as BUILTIN\Administrators, so the real helper pipe passes the strict check — the SYSTEM-service theory from earlier debugging notes was wrong. - ipc_test.go: check NewClient's error in TestMethodNotFound instead of dereferencing a nil client on failure. Skipping when unelevated was rejected: no CI job runs go test at all, so a skip would leave the Windows ipc transport with zero coverage anywhere. 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.
Problem
go test ./internal/ipchas been broken on Windows (onmainand every branch):TestClientServerRPC,TestEventBroadcast— fail withpipe ownership verification failed: pipe owned by unexpected SID: S-1-5-21-...TestMethodNotFound— panics (nil deref inclient.Close()) because it discardedNewClient's errorRoot cause:
verifyPipeOwneraccepts only Local System / Built-in Administrators as the pipe owner. The tests create their own listener in-process, and an unelevatedgo testowns that pipe as the user SID → every dial rejected.Fix
allowSelfOwnedPipespackage var intransport_windows.go, set only by a_test.goinit(). Production builds never set it — the SY/BA requirement is byte-for-byte unchanged for real clients (this is deliberately NOT a loosening of the check that Windows: privileged helper pipe is writable by every interactive user #20 concerns).TestMethodNotFoundnow checksNewClient's error instead of dereferencing nil.Why not skip when unelevated?
There is no CI job that runs
go test(release.ymlis the only workflow — filed separately), sot.Skipwould leave the Windows ipc transport with zero coverage anywhere, forever. With this fix the tests exercise the real named-pipe transport unelevated.Empirical note
Measured with a throwaway probe using the helper's exact SDDL: an elevated process owns its pipe as
BUILTIN\Administrators(S-1-5-32-544), so the real helper pipe passes the strict production check. This also disproves the earlier "helper must run as SYSTEM to own objects as Administrators" theory from the 2026-05-23 debugging notes — the UAPI protected-namespace failure has some other cause.Verification (Windows 11, real run)
go test -count=1 ./internal/ipc— all pass, via the real transport (not skipped)go vet ./...,go build ./...,go build -tags production ./...— cleanGOOS=darwin/GOOS=linuxbuilds of the package — unaffected (file is//go:build windows)grep -rn allowSelfOwnedPipes— only assignment is intransport_windows_test.go🤖 Generated with Claude Code