Prevent disclosure of Access Tokens in logs - #6
Open
negative-video wants to merge 3 commits into
Open
Conversation
The Gigya authentication flow logged whole API response bodies, which are
exactly the objects the surrounding code parses live credentials out of.
The worst two were in the Dominion token exchange:
- "Token exchange response: %s" logged the body holding the live
accessToken and refreshToken at DEBUG. Home Assistant users are
routinely asked to enable debug logging and paste the result into a
GitHub issue, which would publish a working refresh token - the
long-lived one. That is full account takeover.
- "Token exchange missing tokens. Response: %s" logged the same body at
ERROR, visible at default verbosity with no debug flag required. It
triggers when *either* token is missing, so a response carrying a valid
accessToken and an empty refreshToken printed a live access token into
every user's ordinary log.
The same pattern appeared elsewhere: the non-200 token-exchange branch
logged the raw error body and embedded it in the exception message that
Home Assistant surfaces in its UI, and the finalizeTFA and
finalizeRegistration error branches dumped whole Gigya bodies containing
regToken, id_token, sessionInfo.login_token, profile, emails and loginIDs.
Those two fire only when errorCode != 0 - precisely when a user is about
to file an issue. Credential submission also logged the account's raw
email address.
Log only what is provably non-secret and still diagnostic: HTTP status,
errorCode, errorMessage, callId, and the presence or field names of the
expected fields. This matches the style the file already used for
assertion lengths and TFATarget.obfuscated. The account email is now
obfuscated to u***@e***.com so accounts stay distinguishable in a log.
Nothing but logging changes: no wire changes, no control-flow changes, no
new exceptions, and the finalizeTFA/finalizeRegistration error handling
still falls through exactly as before.
The new tests drive each path with recognisable sentinel credentials and
assert both that no sentinel reaches caplog.text and that useful
diagnostics still do, so the fix cannot regress into a silent logger. All
six fail against the pre-change code.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`-vv` called logging.basicConfig(level=DEBUG), which sets the level on the *root* logger and so turns on DEBUG for every third-party library in the process as well as for dompower. That is a credential leak in its own right, independent of what dompower itself records. The Gigya flow passes regToken, gigyaAssertion, phvToken and the user's TFA code as URL *query parameters*, so any HTTP library that logs full request URLs at DEBUG prints them all in the clear. A user told to "run with -vv and paste the output" would publish a live TFA session. Keep the root logger at WARNING and set the level on the "dompower" logger instead. Records from dompower still reach the root handler by propagation, so -v/-vv are unchanged from the user's point of view: - dompower WARNING, third-party WARNING -v dompower INFO, third-party WARNING -vv dompower DEBUG, third-party WARNING This CLI's own records went through the root logger via bare logging.info()/logging.debug() calls, which the new configuration would have silenced at -v. They now go through a module logger, a child of "dompower", so they follow the verbosity flag exactly as before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
These two tests asserted the literal string "finalizeTFA failed" / "finalizeRegistration failed". That phrasing is an implementation detail: a non-zero errorCode that still carries a session is legitimately logged as a warning that the step continued, not as a failure. Pinning the wording makes the tests fail when combined with any change that refines it, even though the property under test - that no secret reaches the log, and that the error code, message and callId still do - is fully intact. Assert on the step name and those details instead. Co-Authored-By: Claude Opus 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.
As of current, the token-exchange response is logged in full, which the next two lines parse
accessTokenandrefreshTokenout of:gigya_auth.py:872— logged at DEBUGgigya_auth.py:891— same, logged at ERROR which is on by default and needs no flagSo asking a user for a debug log to diagnose a problem gets you their live refresh token, and the ERROR path can print an access token with no debug logging enabled at all.
finalizeTFAandfinalizeRegistrationlikewise dump whole Gigya bodies (regToken,id_token,sessionInfo.login_token) on their error paths.Separately, the CLI's
-vvuseslogging.basicConfig, which raises the root logger and Gigya passesregToken,gigyaAssertion,phvTokenand the TFA code as URL query parameters, so third-party HTTP debug logging prints them in the clear.This logs status, error codes,
callIdand field names instead of bodies, obfuscates the login email, and scopes CLI verbosity to thedompowerlogger.Each leak has a test asserting the secret is absent and a diagnostic still present; all fail without the fix. ruff, mypy --strict and the suite pass.
I'm not taking credit for this discovery or fix, I stumbled across it during some testing with Claude Code.