fix: stop tests leaking sensitive values into one another - #507
Merged
Conversation
josegonzalez
force-pushed
the
preflight-harden-global-test-state
branch
from
August 31, 2026 07:34
ff2fd03 to
61e2835
Compare
Four property tests register a secret and never clear it, so it stays registered for every test that runs afterwards and any of them reading that literal back would see `***` instead. Nothing fails today, because no test happens to assert on `token123` or `deploy-bot` - but the exposure is one unlucky fixture value away, and it becomes a real hazard the moment tests run in parallel and the set of tests that ran first stops being fixed. Production code is the source and is behaving correctly: `planProperty` registers a sensitive property's desired and probed values so a drift reason cannot echo a credential, and nothing in `tasks` clears the registry afterwards because a CLI process exits. The four tests that reach that path now take an empty registry and put the previous set back when they finish, through a shared `isolateMaskRegistry` helper, and the seven tests that were already managing the registry by hand use the same helper instead of clearing to nil. Restoring rather than clearing is what makes the leak observable at all. Clearing is why it went unnoticed: the tests that set the registry directly wiped it on the way out, so residue never survived far enough to be seen - an end-of-run check came back clean until those cleanups stopped wiping. With nothing wiping, `TestMain` can assert the registry is empty when the package finishes and fail the run when it is not, which is what pins this rather than leaving it to be rediscovered. Integration tests register through the same production path, and there the registration is the behaviour under test rather than a leak - the documented `dokku_letsencrypt_property` example sets a `dns-provider-*` credential. They are isolated at `skipIfNoDokkuT`, the one gate all 139 of them already pass through, so none has to know whether the task it applies happens to be sensitive. `commands` needs none of it: every command clears the registry on the way out of `Run`, so residue cannot accumulate there. Also saves and restores `color.NoColor` in the fmt colour test rather than assuming the previous value was `true`, matching `TestFormatterColorOnEmitsAnsi`.
josegonzalez
force-pushed
the
preflight-harden-global-test-state
branch
from
August 31, 2026 07:46
61e2835 to
05e20a9
Compare
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
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.
Four property tests register a secret and never clear it, so it stays registered for every test that runs afterwards and any of them reading that literal back would see
***instead. Nothing fails today, because no test happens to assert ontoken123ordeploy-bot- but the exposure is one unlucky fixture value away, and it becomes a real hazard the moment tests run in parallel and the set of tests that ran first stops being fixed.Production code is the source and is behaving correctly:
planPropertyregisters a sensitive property's desired and probed values so a drift reason cannot echo a credential, and nothing intasksclears the registry afterwards because a CLI process exits. The four tests that reach that path now take an empty registry and put the previous set back when they finish, through a sharedisolateMaskRegistryhelper, and the seven tests that were already managing the registry by hand use the same helper instead of clearing to nil.Restoring rather than clearing is what makes the leak observable at all. Clearing is why it went unnoticed: the tests that set the registry directly wiped it on the way out, so residue never survived far enough to be seen - an end-of-run check came back clean until those cleanups stopped wiping. With nothing wiping,
TestMaincan assert the registry is empty when the package finishes and fail the run when it is not, which is what pins this rather than leaving it to be rediscovered.Integration tests register through the same production path, and there the registration is the behaviour under test rather than a leak - the documented
dokku_letsencrypt_propertyexample sets adns-provider-*credential. They are isolated atskipIfNoDokkuT, the one gate all 139 of them already pass through, so none has to know whether the task it applies happens to be sensitive.commandsneeds none of it: every command clears the registry on the way out ofRun, so residue cannot accumulate there.Also saves and restores
color.NoColorin the fmt colour test rather than assuming the previous value wastrue, matchingTestFormatterColorOnEmitsAnsi.Prerequisite for #502.