fix(weixin): refuse to send when contextToken is missing (avoid silent-drop) - #60
Conversation
…t-drop) Adapted from Tencent/openclaw-weixin#247 (authored by stwhwing). Community edition (NewFuture/openclaw-weixin) still had 5 send entry points that warned-then-sent without contextToken, causing iLink to return HTTP 200 with empty body and silently drop the message while the caller saw a fake success. Changes: - send.ts: throw on missing contextToken in all 5 send entry points (sendMessageWeixin, sendMessageItemWeixin, sendImageMessageWeixin, sendVideoMessageWeixin, sendFileMessageWeixin); error messages carry no raw token or full recipient id (redacted via redactToken). - send.test.ts: flip 4 existing 'no throw' tests to assert throw + backend not called; add focused regression + valid-token counterexample. - CHANGELOG: add Unreleased entry (zh + en); no version bump. Verified: send.test.ts 23/23, tsc clean, biome 0 errors.
73db044 to
6864808
Compare
There was a problem hiding this comment.
Pull request overview
Prevents false-success sends when contextToken is missing and improves recipient-log redaction.
Changes:
- Rejects five outbound send paths without a context token.
- Adds regression/privacy tests and resets mocks between tests.
- Updates bilingual changelogs.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/messaging/send.ts |
Adds token guards and recipient redaction. |
src/messaging/send.test.ts |
Tests rejection, API suppression, and valid-token sending. |
CHANGELOG.md |
Documents the fix in Chinese. |
CHANGELOG_EN.md |
Documents the fix in English. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
协作备注(资深/最小改动视角,非合并)总评:方向对,核心改动够小,值得合;建议先用很小补丁收口调用面合同,再 squash。 认同
请用最小补丁收口(非重构)
结论支持修这个 bug。上述 1–3 是小收口,不是要求扩大 scope。收口后我这边没有反对合入的理由。 |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
src/messaging/send.test.ts:346
- This privacy test checks only
Error.message; it never exercises or inspects the changed logger calls atsend.ts:95,138,188,193,220. A regression that logs the complete recipient on API failure or media success would still pass. Add logger assertions for the text failure and media success/failure paths, verifying that combined log messages exclude the full synthetic ID and contain only its redacted form.
it("does not leak the full recipient id in the thrown error (privacy)", async () => {
src/messaging/send.test.ts:285
- This opaque value has the shape of a live Weixin account identifier and is not clearly synthetic.
CONTRIBUTING.md:111-112requires removing account identifiers from tests; use an unmistakably synthetic recipient value so a real identifier cannot be published as a privacy fixture.
const FULL_ID = "o9cq806PLhqoC5-fjuN63zCyAInQ@im.wechat";
src/messaging/send.ts:214
- This guard runs only after the production media path has downloaded/encrypted and uploaded the file:
sendWeixinMediaFileperforms each CDN upload before invoking these image/video/file functions (src/messaging/send-media.ts:28-67, andsrc/channel.ts:273-291may download first). A missing-token media request therefore still creates an unnecessary sensitive CDN artifact before being rejected. ValidatecontextTokenat the media boundary before any download/upload, and test that the upload APIs remain untouched.
if (!opts.contextToken) {
logger.error("sendImageMessageWeixin: contextToken missing — refusing to send (silent-drop risk; upstream #247)");
CHANGELOG_EN.md:18
- This claim is broader than the implementation: recipient IDs are still logged verbatim by the same outbound paths, for example
src/channel.ts:121,src/messaging/send-media.ts:29-58, andsrc/messaging/process-message.ts:369,424,435,449,461. Either redact those call sites too or scope the changelog statement to the diagnostics changed in these five functions.
generated fake-success `messageId`. These helpers redact recipient IDs in their
own logs via `redactToken`
(see Tencent/openclaw-weixin#247).
CHANGELOG.md:16
- 这里的表述超出了实际改动范围:相同出站链路仍会在
src/channel.ts:121、src/messaging/send-media.ts:29-58和src/messaging/process-message.ts:369,424,435,449,461明文记录接收方 ID。请同时脱敏这些调用点,或把 changelog 限定为本次修改的五个函数内部日志。
token 时返回本地生成的「假成功」`messageId`。这些入口自身的接收方日志改用
`redactToken` 脱敏(见 Tencent/openclaw-weixin#247)。
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/messaging/send.test.ts:285
- This value resembles a real Weixin recipient identifier, and recipient IDs are sensitive. Tests must use unmistakably synthetic identifiers so private account data cannot be committed; the redaction assertion remains valid with a synthetic
@im.wechatvalue.
const FULL_ID = "o9cq806PLhqoC5-fjuN63zCyAInQ@im.wechat";
src/messaging/send.test.ts:356
- This privacy test checks only the thrown error, while the PR also changes several diagnostic log sites. The repository contract requires tests to prove sensitive values are absent from diagnostics. Add assertions against the mocked logger for representative text failure and media success/failure paths, verifying the synthetic recipient never appears verbatim.
it("does not leak the full recipient id in the thrown error (privacy)", async () => {
let thrown: unknown;
try {
await sendMessageWeixin({ to: FULL_ID, text: "hi", opts: { baseUrl: "https://api.com" } });
} catch (e) {
thrown = e;
}
expect(thrown).toBeInstanceOf(Error);
const msg = thrown instanceof Error ? thrown.message : String(thrown);
// the recipient peer id must never appear verbatim in the error surface
expect(msg).not.toContain(FULL_ID);
src/messaging/error-notice.ts:19
- This warning still writes the complete recipient ID, contradicting the redaction goal and the repository privacy contract. The recipient is unnecessary to explain this skip, so omit it (or redact it).
logger.warn(`sendWeixinErrorNotice: no contextToken for to=${params.to}, skipping error notice`);
src/messaging/send.ts:217
- This guard is reached only after the media pipeline has already downloaded and uploaded the file:
sendWeixinMediaFileperforms its CDN upload before calling this helper (src/messaging/send-media.ts:28-53). A missing token therefore still consumes network/CDN resources and uploads content that cannot be delivered. Validate immediately after the sending hook and before remote download/CDN upload in both outbound paths, and test that upload mocks remain untouched.
if (!opts.contextToken) {
logger.error("sendImageMessageWeixin: contextToken missing — refusing to send (silent-drop risk; upstream #247)");
throw new Error(
"[openclaw-weixin] sendImageMessageWeixin: contextToken missing — refusing to send to avoid silent-drop (upstream issue #247)",
);
PR 描述正文(适配 NewFuture/openclaw-weixin 社区版)
问题来源
iLink 通道在
contextToken缺失时,旧代码仅logger.warn后仍然调用sendMessageApi。iLink 后端对该情况返回 HTTP 200 + 空 body,消息被后端丢弃,但调用方拿到的是本地生成的messageId(伪成功)。这导致微信推送失败在日志侧“看起来成功”,实际用户收不到消息,且难以排查。上游 #247 已认定该 bug 真实存在,社区版
main当前仍复现(5 处logger.warn → sendMessageApi)。本 PR 的改动
1.
src/messaging/send.ts—— 5 个发送入口缺 token 时直接抛错(而非 warn 后发送)涉及:
sendMessageWeixin/sendImageMessageWeixin/sendVideoMessageWeixin/sendFileMessageWeixin(及媒体项聚合路径)。2. 错误信息 / 日志脱敏(隐私合规,社区硬性要求)
Error.message不含contextToken,也不含 完整收件人 ID(to)。to=${to}改为to=${redactToken(to)},复用社区既有src/util/redact.ts的redactToken(显示前 6 字符 +(len=N)),避免泄露用户标识。3. 测试
refuses to send without contextToken (throws),断言rejects.toThrow(/contextToken missing/)且mockSendMessageApi未被调用(保证真的没发出去,而非发了被丢弃)。describe("missing contextToken refuses to send (silent-drop fix, upstream #247)"):o9cq806PLhqoC5-fjuN63zCyAInQ@im.wechat);contextToken时sendMessageWeixin正常发送成功。beforeEach改用vi.resetAllMocks()),避免「re-throws API errors」误判为通过。4. Changelog(不 bump 版本)
CHANGELOG.md(中文)/CHANGELOG_EN.md(英文)的## [未发布] / ## [Unreleased]段新增修复条目,注明静默丢弃修复与溯源 #247。验证
npx vitest run src/messaging/send.test.ts:23/23 通过npx tsc --noEmit -p tsconfig.json:改动文件无类型错误npx biome check:0 错误(已自动格式化超长行)已知的无关失败
全量测试时
src/messaging/inbound-dedupe.test.ts有 1 个失败,但经在 pristinemain上复跑确认该失败为仓库既有问题(涉及文件系统状态、与本次改动无关)。本 PR 未改动该模块。提交信息(建议)