fix(security): keep WeCom callback msg_signature out of access logs - #40072
briandevans wants to merge 2 commits into
Conversation
The WeCom callback URL-verification handshake (_handle_verify) and every inbound callback (_handle_callback) carry the msg_signature HMAC, plus timestamp/nonce and (on verification) the encrypted echostr, in the request query string. aiohttp's default access logger writes the full request target — including that query string — to agent.log verbatim, so the signature tuple is persisted in plaintext logs. Construct the WeCom callback AppRunner with access_log=None so the request target is never written to the access log, mirroring the BlueBubbles webhook fix in 514f502, which disabled the access log on its own runner for the same class of query-string-borne secret.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR prevents sensitive WeCom callback query-string parameters (notably msg_signature) from being written to logs by disabling aiohttp access logging for the WeCom callback server.
Changes:
- Disable aiohttp access logging for the WeCom callback
AppRunnerby settingaccess_log=None. - Add an async test that asserts
connect()constructs theAppRunnerwithaccess_log=Noneto avoid signature leakage.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| gateway/platforms/wecom_callback.py | Disables aiohttp access logging for the WeCom callback server to avoid leaking msg_signature in logs. |
| tests/gateway/test_wecom_callback.py | Adds coverage ensuring connect() explicitly disables aiohttp access logging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # The WeCom URL-verification handshake and every inbound callback | ||
| # carry the `msg_signature` HMAC (and `echostr`) in the request | ||
| # *query string*. aiohttp's default access logger would write that | ||
| # full request target to agent.log verbatim. Disable the access log | ||
| # so the signature is never persisted (mirrors the BlueBubbles | ||
| # webhook fix in 514f5020c). | ||
| self._runner = web.AppRunner(self._app, access_log=None) |
There was a problem hiding this comment.
Good call — fixed in e2fc75e. Instead of access_log=None (which silenced all request logging incl. /health), connect() now installs a custom AbstractAccessLogger subclass via access_log_class. It keeps access logging on for observability and redacts the sensitive query values (msg_signature, echostr, timestamp, nonce) to REDACTED before they reach agent.log, preserving the path and any non-sensitive params. Added regression tests for the redaction helper, the logger output, and that connect() installs the redacting class with logging still enabled.
| # so the signature is never persisted (mirrors the BlueBubbles | ||
| # webhook fix in 514f5020c). |
There was a problem hiding this comment.
Fixed in e2fc75e — dropped the raw commit-hash reference. The code comment now describes the leak and the redaction approach directly without pinning to a SHA.
| """The WeCom URL-verification handshake and inbound callbacks carry the | ||
| ``msg_signature`` HMAC (and ``echostr``) in the request *query string* | ||
| (see ``_handle_verify`` / ``_handle_callback``). aiohttp's default access | ||
| logger would write that full request target to agent.log verbatim, leaking | ||
| the signature. ``connect()`` must build the AppRunner with | ||
| ``access_log=None`` so it is never persisted -- mirroring the BlueBubbles | ||
| webhook fix in 514f5020c. | ||
| """ |
There was a problem hiding this comment.
Fixed in e2fc75e — the test docstring no longer references a commit SHA; it describes the rationale (sensitive query params must not be persisted to access logs) directly.
alt-glitch
added
type/security
…ure instead of disabling access logs Replace access_log=None with a redacting AbstractAccessLogger subclass so request access logging stays enabled (preserving /health and traffic/latency observability) while the msg_signature HMAC, echostr, timestamp and nonce query values are replaced with REDACTED before they reach agent.log. Drop the brittle raw-commit-hash references in the code comment and test docstring in favor of describing the prior fix directly. Add regression tests for the redaction helper, the access-logger output, and that connect() installs the redacting class with access logging still on.
|
Closing to keep the hardening queue focused — this is a mechanical sibling-widen of the 514f502 access-log family (mirrors |
This is a sibling follow-up to commit 514f502
?password=) rides the inbound request target into aiohttp's default access log. It addedaccess_log=Noneto the BlueBubblesweb.AppRunnerso the request target is never persisted toagent.log.gateway/platforms/wecom_callback.py), whosemsg_signatureHMAC (plustimestamp/nonce, and the encryptedechostron URL verification) rides the inbound request query string into the exact same aiohttp access log. Itsweb.AppRunnerwas still constructed with the default access logger enabled.access_log=Noneon the WeCom callbackAppRunner, closing the identical query-string-borne-secret leak at the second platform that exhibits it.What does this PR do?
The WeCom URL-verification handshake (
_handle_verify) and every inbound callback (_handle_callback) readmsg_signature,timestamp,nonce, andechostrfromrequest.query. aiohttp's default access logger writes the full request target — including that query string — toagent.logverbatim, persisting themsg_signatureHMAC (and the encryptedechostr) in plaintext logs.msg_signatureis an HMAC-SHA1 over (token, timestamp, nonce, payload); a log reader who recovers a(msg_signature, timestamp, nonce, echostr)tuple can replay the URL-verification handshake and weaken callback-source authentication.This disables the access log on the WeCom callback
AppRunner(access_log=None) so the request target is never written, mirroring the BlueBubbles platform-side change in 514f502 exactly.Note on scope: the central redaction denylist (
agent/redact.py::_SENSITIVE_QUERY_PARAMS) is intentionally not changed. The request-target / URL query-param redaction path was deliberately turned OFF onmainin5f66c3647 fix(redact): pass web URLs through unchanged (#34029)— adding keys there would be dead code that contradicts that decision. Disabling the access log at the runner (the same defense 514f502 chose for BlueBubbles) is the live, correct fix.Related Issue
N/A — sibling hardening follow-up to commit 514f502.
Type of Change
Changes Made
gateway/platforms/wecom_callback.py: construct the callbackweb.AppRunnerwithaccess_log=None(mirrors BlueBubbles 514f502).tests/gateway/test_wecom_callback.py: regression test assertingconnect()builds theAppRunnerwithaccess_log=None.How to Test
uv run --with pytest --with pytest-xdist --with pytest-asyncio python3 -m pytest tests/gateway/test_wecom_callback.py -vAppRunner must be constructed with an explicit access_log kwarg); restored, it passes. All 13 tests in the file pass.Checklist
Code
Documentation & Housekeeping
docs/, docstrings) — N/Acli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/AEscalation paths covered
The WeCom callback site exposed the query-string secret via aiohttp's access log. This PR closes that path at the runner. The redaction-denylist path is intentionally out of scope (request-target redaction is OFF on main per #34029); the access-log disable is the same defense the parent commit used for BlueBubbles.