fix(bluebubbles): redact the password embedded in API URLs from error output - #65705
Open
Frowtek wants to merge 2 commits into
Open
fix(bluebubbles): redact the password embedded in API URLs from error output#65705Frowtek wants to merge 2 commits into
Frowtek wants to merge 2 commits into
Conversation
… output BlueBubbles authenticates by putting ?password=<pw> in the request URL (its REST/webhook API has no header-based auth path — see _api_url). httpx raises HTTPStatusError/RequestError whose str() includes that URL, so the send paths that returned SendResult(error=str(exc)) — and the connect() 'cannot reach server' log — leaked the password on any BlueBubbles API failure (server offline, wrong-password 401, chat-not-found 404, timeout). The global secret redactor does not catch this: it intentionally passes URL query strings through unchanged (NousResearch#34029, to preserve OAuth callbacks / magic links / presigned URLs), and its webhook-access-log guard only matches 'METHOD /path?...' request-target lines — neither masks the "for url '...?password=...'" shape an httpx error produces. This is the same class as telegram's token-in-URL leak (NousResearch#58594): mask it at the adapter boundary with _redact_bb_error_text, mirroring _redact_telegram_error_text, and route the send-result + connect error sites through it. Adds regression tests: the redactor masks the password (raw, url-encoded, and &password= forms) while leaving non-secret query params and benign errors intact, and a failed send() no longer returns the password in SendResult.error.
Collaborator
…error logs too
Two remaining raw-exception sites in the same leak class: _register_webhook
and _unregister_webhook log the httpx error verbatim on failure
('failed to register/unregister webhook: %s' % exc). The exception comes
from _api_post / client.delete on _api_url(...), which carries
?password=<pw>, so raise_for_status()'s HTTPStatusError.str() leaks the
password into those logs — the status logs already use the redacted
_webhook_register_url_for_log, but the exception logs did not.
Route both through _redact_bb_error_text, completing the redaction across
every BlueBubbles error/log site (mirrors telegram's 'all remaining raw
exception sites' pass). Adds a caplog regression test asserting the
register-failure log no longer contains the password.
Contributor
|
Thanks for addressing the BlueBubbles error-string leak. The main premise is verified on current Problems
Suggested changes
Automated hermes-sweeper review. |
Contributor
|
suggesting changes
Security evidence:
Not checked:
Signed: GPT-5.6-sol-xhigh in Codex |
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.
What does this PR do?
BlueBubbles authenticates by putting
?password=<pw>in the request URL — its REST/webhook API has no header-based auth path (see_api_url). httpx raisesHTTPStatusError/RequestErrorwhosestr()includes that URL, so every path that logged or returned the raw exception leaked the password on any BlueBubbles API failure (server offline, wrong-password401, chat-not-found404, timeout):SendResult(error=str(exc)),connect()"cannot reach server" log,_register_webhook/_unregister_webhookfailure logs (the status logs already used the redacted_webhook_register_url_for_log, but the exception logs interpolated the raw error).The global secret redactor does not catch this: it intentionally passes URL query strings through unchanged (#34029, to preserve OAuth callbacks / magic links / presigned URLs), and the webhook-access-log guard added in #31690 only matches
METHOD /path?... HTTP/1.1request-target lines. Neither masks thefor url '...?password=...'shape an httpx error produces. Verified end-to-end: a failed_api_postreturnsSendResult.errorcontaining the password, and it survivesredact_sensitive_text(..., force=True).Same class as Telegram's token-in-URL leak (#58594). Add an adapter-local
_redact_bb_error_text(mirroring_redact_telegram_error_text) and route every BlueBubbles error/log site through it — without touching the global "web URLs pass through" behavior.Related Issue
Fixes # — no existing issue. Parity sibling of the merged Telegram transport-error redaction (#58594); complements #31690 (inbound webhook access-log / registration URL) with the outbound send/connect/register error paths it did not cover.
Type of Change
Changes Made
gateway/platforms/bluebubbles.py:_redact_bb_error_text(error)+_BB_URL_PASSWORD_RE— mask a?password=/&password=value (raw or URL-encoded) in any error string.SendResult(error=str(exc/e))send failures, theconnect()unreachable log, and the_register_webhook/_unregister_webhookfailure logs.tests/gateway/test_bluebubbles.py:TestBlueBubblesPasswordRedaction(4 tests) +TestBlueBubblesWebhookErrorRedaction(1 test).How to Test
pytest tests/gateway/test_bluebubbles.py -q→ 65 passed (60 existing + 5 new).?password=(raw, URL-encoded, and&password=forms) while leaving non-secret query params (guid=x) and benign errors intact;send()(mocked_api_postraising anHTTPStatusErrorbuilt from the real_api_url) returns aSendResultwhose.errorhas no password;_register_webhookno longer writes the password into its log (asserted viacaplog).str(exc)verbatim) to confirm the password leaks there.Checklist
Code
fix(scope):)pytest tests/ -qand all tests passDocumentation & Housekeeping
cli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/ASecurity
Closes a credential leak: the BlueBubbles auth password, embedded in the API request URL, reached
SendResult.errorand logs on any API failure. Masked at the adapter boundary (mirroring the existing Telegram token-in-URL redaction); the global URL-passthrough behavior other features rely on is unchanged.Screenshots / Logs
Before (send failure, current code):
After (this branch):