Repository navigation
fix(adapters): stop logging raw webhook bodies and message content (#187) - #245
Conversation
) Port upstream log-hygiene hardening (fc7df9c4 / vercel/chat#500, logging parts of f485255b / vercel/chat#877). Webhook handlers now log request-shape metadata only (byte length, content type, event type, signature presence), never bodies or slices of them: - GitHub: {bodyBytes, contentType, eventType, signaturePresent} under "signature verification failed" / "request verified"; jsonParseStatus on invalid JSON. - GChat, Slack (post-verification), Teams bridge: "... webhook received" {bodyLength}. - Linear: drop the Python-only raw-body log; invalid-JSON logs bodyBytes. - Chat "Incoming message": drop author, add is_bot (upstream key set). Python-ahead divergences, recorded in docs/UPSTREAM_SYNC.md: - WhatsApp drops its raw-body / bodyPreview logs (upstream still has them). - GChat message-event / Pub/Sub logs and Chat/Slack/Discord slash-command logs carry textLength instead of message text / display names. Adds shared/log_utils.utf8_byte_length (Buffer.byteLength equivalent that never raises), ports upstream's three GitHub handleWebhook log tests, and adds per-adapter sentinel-based regression tests.
|
Warning Review limit reachedNext included review available in 3 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (15)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…-prefix previews (#187) - GitHub: parametrized test for signaturePresent — missing header -> False, empty header -> True (mirrors upstream `signature !== null`); kills the bool(signature) and hard-coded True mutants. - GChat: attachment-only message (no `text` key) returns 200 and logs textLength 0; kills the unguarded len(message_text) mutant. - assert_body_not_logged: also reject the body's leading/trailing 24-char slices (verbatim and JSON-escaped), so a body[:N] preview is caught even when the payload's first sentinel sits further in; kills the Slack body[:40] mutant.
|
Merge gate: CI green (Lint & Type Check, test (3.12), test (3.13), CodeQL, Analyze (python), Analyze (actions)); local Codex review (gpt-6-astra, xhigh, --base origin/main) on 7cfa6b4 (merge of origin/main incl. #234/#235, no conflicts): "No actionable regressions found."; 1 astra round after the main merge (prior clean review on 0e129fe); bot reviews: no submitted reviews, CodeRabbit rate-limited. Merging with --admin (Protect Main requires a code-owner approval). |
1 participant
Summary
Several adapters logged raw webhook bodies, or previews of them, at DEBUG. In some cases this happened before signature or JWT verification.
Chatalso logged author identity for every incoming message. With DEBUG enabled, unauthenticated input and user-authored content could be copied into log sinks. This PR ports upstream's log-hygiene hardening: handlers now log request-shape metadata only (byte length, content type, event type, whether a signature is present), never bodies or any slice of them.What changed
handle_webhookbody[:500]before verification;bodyPreviewon invalid JSON{bodyBytes, contentType, eventType, signaturePresent}under "GitHub webhook signature verification failed" / "GitHub webhook request verified";+ jsonParseStatus: "error"on invalid JSON (replaces the separate "event type" log)handle_webhook"GChat webhook received" {bodyLength}handle_webhookbody[:500]before verification"Slack webhook received" {bodyLength}afterverify_slack_requestsucceedsbody[:500]"Teams webhook received" {bodyLength}body[:500]before verification (Python-only);bodyPreview{bodyBytes, contentType}body[:500]before verification;bodyPreview{bodyBytes, contentType}(divergence)Chat"Incoming message"authoradapter, thread_id, message_id, is_bot, is_meChat/Slack/Discord slash-command logstextLengthonly; no text or author (divergence, the issue's recommended default)src/chat_sdk/shared/log_utils.pyaddsutf8_byte_length, the Python equivalent ofBuffer.byteLength. It counts UTF-8 bytes rather than code points, returns 0 forNone, and counts lone surrogates as 3 bytes, so computing a log field never raises.Upstream mapping
fc7df9c4fix(github): remove raw webhook payload logging (fix(github): remove raw webhook payload logging vercel/chat#500, chat@4.32.0). Ported exactly.f485255bfix(adapters): harden webhook tenant isolation (fix(adapters): harden webhook tenant isolation vercel/chat#877, chat@4.40.0). Logging parts only: GChat, Slack and Teams "webhook received" withbodyLength, and thechat.ts"Incoming message" key set. Tenant isolation and GChat identity are [4.41/SL0] Slack: installation-scoped caches, drop unresolved installs, strict response_url, bounded regexes #205, [4.41/G1] Google Chat: bind webhook JWT verification to configured identity #222 and [4.41/G2] Google Chat: explicit bot identity, media-API downloads, same-space message ids, native pagination #223.Tests
Ported: the three upstream
describe("handleWebhook")tests inadapter-github/src/index.test.ts, astests/test_github_webhook.py::TestGitHubWebhookLogHygiene:should not log raw payload content for invalid signatures... for invalid JSON... for valid webhooksThey use the same sentinels and the same exact metadata assertions. The invalid-signature case uses a multi-byte payload.
Python-specific:
tests/test_webhook_log_hygiene.py:utf8_byte_lengthedge cases.bodyLengthafter verification, and the slash command.Chat: "Incoming message" and slash command.tests/test_teams_bridge.py::TestLogging::test_logs_raw_body_at_debugwas replaced, not supplemented, per principle 3. It asserted onlylogger.debug.called.Shared helper:
tests/_log_capture.pystringifies every recorded call on aMockLoggerorMagicMocklogger. The absence check covers the body both verbatim and JSON-escaped, since a plain substring check passes vacuously for JSON bodies.Fail-first check: all 18 new or replaced tests fail against
origin/mainsource and pass with this change.Divergences (2, within budget)
Both have a row in
docs/UPSTREAM_SYNC.mdKnown Non-Parity, a breadcrumb at each code site, regression tests, and a CHANGELOG entry:Chat, Slack and Discord). Upstream still logs these. The action and reactionuser/user_namelogs stay at parity, per the issue's out-of-scope list.The Linear change restores parity, because upstream Linear never had these logs.
Consumer impact
This changes DEBUG log content only. No routing, response or status behavior changes.
"… webhook raw body"messages are gone, and GChat, Slack and Teams now emit"… webhook received"withbodyLengthin UTF-8 bytes."GitHub webhook event type"becomes"GitHub webhook request verified".bodyPreviewbecomesbodyBytes.author.Anything that parses these lines needs updating.
Validation
ruff checkandruff format --checkare clean.chat@4.31.0: 732/732.0e129fe).grep -rnE 'raw body"|bodyPreview' src/chat_sdkreturns nothing.Environment note (resolved on main by #251, which caps
microsoft-teams-*below 2.1):uv.lockis gitignored, and a freshuv syncnow resolvesmicrosoft-teams-*==2.1.0. Against 2.1.0, three existing Teams tests fail onmaintoo, withAppno longer havingactivity_sender:test_teams_adapter.py::TestOutboundServiceUrlRouting::test_edit_message_retargets_real_activities_clienttest_teams_native_streaming.py::TestCreateStreamerThe numbers above were run against the baseline SDK
2.0.13.4. The non-strict 4.41.1 fidelity count is not reported, because it waits on the #185 tooling.Closes #187
Part of #184
Review
Two independent reviews. Every fix was mutation-verified: the named mutant survived before commit
999b3c0and is killed after it.Fixed
signaturePresentwas untested (both reviewers). Added the parametrized testtest_signature_present_reflects_header_presence_not_truthiness. A missingx-hub-signature-256header givesFalse, and an empty header givesTrue, matching upstreamsignature !== null. The test asserts the 401 response and the exact "GitHub webhook signature verification failed" context. It kills thebool(signature)mutant and the hard-codedTruemutant.isinstance(message_text, str)guard was untested. Addedtest_message_event_without_text_logs_zero_text_length. It sends an attachment-only message with notextkey and asserts a 200 response,textLength: 0and dispatch toprocess_message. It kills the unguardedlen(message_text)mutant, which raised TypeError.assert_body_not_loggedmissed short prefix previews. The helper now also rejects the body's leading and trailing 24-character slices, both verbatim and JSON-escaped. Abody[:N]preview with N >= 24 is now caught wherever the payload's first sentinel sits. It kills the Slackbody[:40]mutant. I tried a full sliding window first. It gave false positives: a short JSON fragment legitimately matched a logged identifier (the Pub/Subce-typevalue). So the helper checks anchored slices, and the sentinels spread through each payload cover mid-body excerpts.Declined
fix(adapters)anddiverge(adapters)commits (docs/UPSTREAM_SYNC.md "How to land a divergence" / "Review signal"). The convention violation is real, but splitting182283bmeans rewriting published history and force-pushing. The wave's working rules say not to rebase or force-push, and another worktree currently has this branch checked out. main is squash-merged (every recent commit is one(#NNN)squash), so a per-commit split would not survive into main anyway. To give reviewers the same signal, both divergences are listed above under "Divergences (2, within budget)", each with its Known Non-Parity row, its CHANGELOG "Python-specific" entry and a# Divergence from upstreambreadcrumb at the code site. The maintainer can use adiverge(adapters)note in the squash message when merging.CI note (superseded, since #251 is merged into this branch and CI is now green): 3 Teams tests (
test_teams_adapter.py::TestOutboundServiceUrlRouting::test_edit_message_retargets_real_activities_clientand 2 intest_teams_native_streaming.py::TestCreateStreamer) fail with the lock-pinnedmicrosoft-teams-*2.1.0 SDK. The errors areApphaving noactivity_senderand an unexpectedservice_urlkwarg. This PR does not touchuv.lock, the Teams adapter or its streaming code, and the PR's earlier CI run failed the same way. This is outside #187. By topic it likely belongs to #216 (T1, per-service-URL clients) and #219 (T4, native streaming).Merge gate
origin/main(with fix(teams): cap microsoft-teams SDK below 2.1 #251) into the branch, which gives HEAD0e129fe. The only conflict was inCHANGELOG.md. It was resolved by keeping one## Unreleased (4.41 wave)heading with fix(teams): cap microsoft-teams SDK below 2.1 #251's Teams-cap bullet and this PR's Security and Python-specific sections. No source conflicts.0e129fe. Final verdict: "No actionable regressions found. Validation passed: 5,171 tests, lint, formatting, and test-quality audit; 13 tests were skipped. Upstream fidelity verification was unavailable because the pinned checkout was absent." The fidelity check it could not run was run locally against the pinnedchat@4.31.0checkout. See below.0e129fe:chat@4.31.0passes: all TS tests have Python equivalents.0e129fe: all green.ready_for_reviewrun. Thesynchronizerun was skipped because the PR was still a draft.