fix(tests): stop leaking test values into developer keyring#615
Merged
Conversation
test_redacted_logging was missing @patch for SecureStorage.set_password, so every run wrote the literal string "secret_key" to the real system keyring under pirate_weather_api_key — which then surfaced in the Settings dialog on fresh installs. Fix the immediate test and add an autouse conftest fixture that routes SecureStorage through an in-memory fake keyring for all tests. Explicit @patch of _get_keyring or SecureStorage.*_password in individual tests still wins, so existing secure-storage tests pass unchanged.
Orinks
added a commit
that referenced
this pull request
Apr 21, 2026
Routine sync of `dev` into `main`. Includes: - fix(tests): stop leaking test values into developer keyring ([#615](#615)) - feat(products): Forecast Products dialog + HWO/SPS notifications ([#613](#613)) - feat(zones): NWS zone metadata on saved locations ([#612](#612)) - chore: remove unused html_formatters module - feat(ui): add preference for location buttons near dropdown ([#611](#611)) --------- Co-authored-by: Claude Opus 4.7 (1M context) <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
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.
New AccessiWeather installs on a machine that had run the test suite showed
secret_keypre-filled in the Pirate Weather provider field. The Settings dialog was fine — the developer's Windows Credential Manager actually contained the literal stringsecret_keyunder serviceaccessiweather, usernamepirate_weather_api_key, and the production load path (LazySecureStorage→ keyring) faithfully read it back on every new install.The source was tests/test_settings_operations.py:
test_redacted_loggingcalledoperations.update_settings(pirate_weather_api_key="secret_key")without patchingSecureStorage.set_password, so every localpytestrun committedsecret_keyto the real keyring. Its siblingtest_update_secure_settingright above it already used@patch("accessiweather.config.settings.SecureStorage.set_password")— the redaction test was a copy-paste miss. Visual Crossing and OpenRouter escaped only because no equivalent unmocked test ever ran for them.The existing autouse
_mock_keyring_availablefixture in tests/conftest.py faked the availability probe but never intercepted writes, so it offered no protection.This PR fixes both layers:
@patch("accessiweather.config.settings.SecureStorage.set_password")totest_redacted_logging._isolate_keyring_from_real_backendfixture intests/conftest.pythat swapsaccessiweather.config.secure_storage._keyring_modulewith an in-memory fake for every test. Explicit@patchof_get_keyringorSecureStorage.*_passwordin individual tests still wins, sotest_secure_storage.py,test_portable_secrets.py, and the startup-guidance suite keep passing unchanged.Impact is limited to developers who ran the test suite locally. CI runners use ephemeral backends and end users never run these tests, so published installers and fresh user installs were never affected. The already-polluted
pirate_weather_api_key(and an unrelatedavwx_api_keywith a pasted `gh pr checkout 480` string) were cleared from the developer's keyring during investigation.Test plan