Skip to content

fix(gateway): harden WeCom callback against XML DoS and XXE - #8686

Closed
Dusk1e wants to merge 1 commit into
NousResearch:mainfrom
Dusk1e:fix/wecom-xml-dos-protection
Closed

fix(gateway): harden WeCom callback against XML DoS and XXE#8686
Dusk1e wants to merge 1 commit into
NousResearch:mainfrom
Dusk1e:fix/wecom-xml-dos-protection

Conversation

@Dusk1e

@Dusk1e Dusk1e commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR hardens the WeCom callback endpoint against XML-based Denial of Service (DoS) and XML External Entity (XXE) attacks.

Why

The previous implementation directly parsed raw callback XML using ElementTree before validation. This exposed a vulnerability where large payloads or malicious DOCTYPE/ENTITY definitions could lead to resource exhaustion or information leakage.

Changes

  • Payload Sanitization: Added a 256 KB body size limit for WeCom callbacks.
  • Pre-parse Validation: Introduced regex-based extraction for the Encrypt field, avoiding full XML parsing for raw messages.
  • Parser Security: Added explicit checks to reject XML containing DOCTYPE or ENTITY tags before they reach the parser.
  • Decryption Safety: Only validated and decrypted content is processed, significantly reducing the attack surface.

Verification

Added comprehensive security tests in tests/gateway/test_wecom_callback.py:

  • Verified rejection of unsafe XML (XXE/Billion Laughs patterns).
  • Confirmed that large payloads (>256 KB) are blocked.
  • Validated that decryption only proceeds after pre-parse checks pass.

@LonggTeng

Copy link
Copy Markdown

Independent audit confirms the underlying issue this PR is addressing.

Two validation notes that may help review:

  1. The core risk is real because gateway/platforms/wecom_callback.py calls ET.fromstring(body) on the raw callback body before decryption or trust establishment.
  2. On the current Python runtime, stdlib xml.etree.ElementTree does expand internal entities. A small local test expanded a 349-byte XML payload into a 1,000,000-character text node, so the parser-amplification or DoS angle is not just theoretical.

One nuance: this endpoint is running under aiohttp, whose default client_max_size is 1 MiB unless overridden. So the impact is better described as pre-auth XML parser amplification and resource pressure, not unlimited raw body upload. The mitigation direction in this PR looks aligned with that threat model: avoid full XML parse on the untrusted outer body, reject dangerous constructs early, and apply a tighter body limit.

@alt-glitch alt-glitch added type/security Security vulnerability or hardening P1 High — major feature broken, no workaround platform/wecom WeCom / WeChat Work adapter comp/gateway Gateway runner, session dispatch, delivery labels Apr 28, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Related to #10192 — both harden WeCom callback XML parsing. #10192 uses defusedxml + 64KB limit; this PR uses regex extraction + DOCTYPE/ENTITY rejection + 256KB limit. Consider consolidating.

@egilewski

Copy link
Copy Markdown
Contributor

merge conflicts

This PR does not merge cleanly with the base branch. Please rebase or merge current main and resolve the conflicts if it's still relevant.

Signed: GPT-5.5-low in Codex

@alt-glitch alt-glitch added P2 Medium — degraded but workaround exists sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data and removed P1 High — major feature broken, no workaround labels Jun 26, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #10192 / #28585 / #35413 (competing WeCom callback XML-hardening PRs). Note: the core XXE/billion-laughs protection already landed on main via the merged #32442 (defusedxml), and the WeCom adapter has since relocated to plugins/platforms/wecom/callback_adapter.py — which is the source of the reported merge conflicts. This PR's regex/DOCTYPE/256KB approach is a different mechanism; marked related_to, not a duplicate, for a maintainer to consolidate or close.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the security hardening work. This is an automated hermes-sweeper review; the reported protection is already implemented on current main through stronger, merged safeguards.

  • 5744b17579492f48d1436418509da084bd4e7fd7 (merged via harden(dashboard,wecom): restrict markdown link schemes; safe-parse untrusted XML #32442) changed the untrusted WeCom callback parser to defusedxml.ElementTree, blocking entity declarations, XXE, and entity-expansion attacks. The live implementation is now plugins/platforms/wecom/callback_adapter.py:20-30 and is used by _decrypt_request and _build_event at lines 344-355.
  • 74541beb9ce32fcb1b8d89d698120d4b59d34ca8 (merged via fix(security): cap WeCom callback body size before pre-auth XML parse #54615) added a stricter 64 KB client_max_size plus a defense-in-depth pre-parse handler limit at plugins/platforms/wecom/callback_adapter.py:57-61, :140-142, and :283-289.
  • The body-limit regression coverage is present at tests/gateway/test_wecom_callback.py:312-347.
  • The old gateway/platforms/wecom_callback.py target in this PR was relocated to the bundled WeCom plugin, so the PR cannot apply to the current implementation.

Closing as implemented on main.

@teknium1 teknium1 closed this Jul 12, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jul 12, 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 P2 Medium — degraded but workaround exists platform/wecom WeCom / WeChat Work adapter sweeper:implemented-on-main Sweeper: behavior already present on current main 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.

5 participants