Skip to content

fix(security): use constant-time comparison for BlueBubbles webhook auth - #44025

Open
zapabob wants to merge 1 commit into
NousResearch:mainfrom
zapabob:codex/sec-fix-bluebubbles-timing-20260611
Open

fix(security): use constant-time comparison for BlueBubbles webhook auth#44025
zapabob wants to merge 1 commit into
NousResearch:mainfrom
zapabob:codex/sec-fix-bluebubbles-timing-20260611

Conversation

@zapabob

@zapabob zapabob commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Summary

The BlueBubbles webhook handler authenticated inbound requests with a plain string inequality check against the configured shared password. A plain != comparison short-circuits on the first differing byte, so the time taken to reject a request correlates with how many leading bytes of the supplied credential matched. This is a classic timing side channel (CWE-208) that, over many requests, can let an attacker recover the secret byte-by-byte.

Root cause

gateway/platforms/bluebubbles.py_handle_webhook:

python token = request.query.get(password) or ... or request.headers.get(x-bluebubbles-guid) if token != self.password: return web.json_response({error: unauthorized}, status=401)

Fix

Compare with hmac.compare_digest (constant-time). token may be None when no credential is supplied, so it is coerced to an empty string before encoding; unauthenticated requests are still rejected and no exception is raised.

Tests

tests/gateway/test_bluebubbles.py::TestBlueBubblesWebhookAuth — wrong credential rejected, missing credential rejected (no exception), and an invariant test asserting both operands flow through hmac.compare_digest (i.e. no plain == fast path remains). Full existing test_bluebubbles.py suite (59 tests) still passes.

Impact

Inbound webhook authentication only. No change to message handling, config, prompt cache, or role alternation. Behavior is identical for valid/invalid credentials apart from the comparison being constant-time.

The inbound webhook handler validated the shared password with a plain != comparison, which short-circuits on the first differing byte and leaks the matched prefix length through response timing (CWE-208). Switch to hmac.compare_digest and coerce a missing credential to an empty string so unauthenticated requests are still rejected without raising.
@liuhao1024

Copy link
Copy Markdown
Contributor

Verification: LGTM

Reviewed the diff and tests. The webhook auth check correctly handles the None token case with (token or "").encode("utf-8") — a missing credential header/query param would otherwise cause a TypeError on .encode().

Checked:

  • hmac.compare_digest used for constant-time comparison (CWE-208)
  • None token coerced to empty string before encoding (prevents TypeError)
  • Test covers: wrong password rejected, missing credential rejected, constant-time compare verified via spy
  • The test_missing_credential_rejected test is important — it catches the None edge case that a plain != would silently handle but hmac.compare_digest would crash on

Clean implementation, no issues found.

@liuhao1024

Copy link
Copy Markdown
Contributor

Verified: constant-time webhook auth is correctly implemented.

Reviewed the full diff — the fix replaces plain != with hmac.compare_digest(), coerces None tokens via (token or "").encode("utf-8"), and uses consistent UTF-8 encoding on both operands. Tests cover the three critical cases: wrong password, missing credential header, and verification that the constant-time comparator is actually invoked (spy pattern). Clean implementation.

Note: this is part of a cluster with #44026 (Slack SSRF guard) and #44027 (Google Meet constant-time token) — all three follow the same CWE-208 pattern and are independently clean.

@alt-glitch alt-glitch added type/security Security vulnerability or hardening P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery duplicate This issue or pull request already exists labels Jun 11, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #9219 — same hmac.compare_digest constant-time fix for the BlueBubbles webhook token in the same file (gateway/platforms/bluebubbles.py).

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the focused security hardening. The current-main premise is valid: gateway/platforms/bluebubbles.py:883 still uses token != self.password, and the proposed UTF-8 byte hmac.compare_digest replacement addresses that path.

Problems

  • The added tests verify rejection and that the comparator is invoked, but do not verify a valid credential is accepted. Add a successful-auth assertion for _handle_webhook.
  • Add non-ASCII credential coverage. The byte-based implementation supports it, and the related duplicate PR #9219 already demonstrates the useful correct/wrong credential cases.

Suggested changes

  • Test a valid token returning 200 using a non-message webhook payload.
  • Test a non-ASCII configured password with both matching and non-matching tokens.

Automated hermes-sweeper review.

"""Webhook credential check must be timing-safe (CWE-208)."""

def test_wrong_password_rejected(self, monkeypatch):
adapter = _make_adapter(monkeypatch)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a positive-auth regression case. These tests establish 401 behavior and comparator invocation, but none verifies that a request with the configured token proceeds to the normal 200 webhook path.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants