Skip to content

feat(hub): add WeCom bot push notification channel#553

Open
wzxjohn wants to merge 4 commits intotiann:mainfrom
wzxjohn:main
Open

feat(hub): add WeCom bot push notification channel#553
wzxjohn wants to merge 4 commits intotiann:mainfrom
wzxjohn:main

Conversation

@wzxjohn
Copy link
Copy Markdown

@wzxjohn wzxjohn commented May 1, 2026

Adds a WeCom (企业微信) notification channel parallel to the existing Telegram / ServerChan / WebPush channels, using the official @wecom/aibot-node-sdk for the WebSocket long-connection transport.

Close #600

Pushes every existing NotificationHub event (permission request, ready, task failure, session completion) as a template_card, and handles the button_interaction callbacks to approve/deny permission requests, replacing the original card via aibot_respond_update_msg with the original req_id threaded through by the SDK.

User binding: a WeCom user sends "<CLI_API_TOKEN>:" in a single chat with the bot; the handler validates the namespace against a conservative whitelist, rejects silent rebinds to a different namespace, and persists the mapping to store.users.

Configuration: WECOM_BOT_ID / WECOM_BOT_SECRET / WECOM_NOTIFICATION env vars mirror the existing TELEGRAM_* shape. Channel is enabled iff both credentials are present and the toggle is on.

Notable wire-format fix: the SDK's TypeScript declarations put event_key / task_id flat on the template_card_event, but the live wire nests them under event.template_card_event.*. callbacks.ts reads from the nested path first with a flat fallback.

Notable server-side requirement: WeCom rejects update reply cards without a card_action with errcode 42045. buildSystemReplyCard always attaches one pointing at the session URL (or publicUrl).

Notable post-kick behaviour: when the server pushes disconnected_event, the SDK marks the client as manually closed (no auto-reconnect); WecomBot re-arms connect() after a 30 s cooldown.

Includes an E2E smoke harness (hub/scripts/e2e-wecom.ts) that walks every notification type plus the binding + click flow against the real WeCom endpoint, gated on WECOM_BOT_ID / WECOM_BOT_SECRET.

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Findings

  • [Major] WeCom binding now rejects namespaces that the rest of HAPI already accepts. parseAccessToken() and the /api/auth flow allow any non-empty namespace without leading/trailing whitespace, but this regex only permits [A-Za-z0-9_-]{1,64}. That means deployments using an existing namespace like team alpha or prod.eu can authenticate everywhere else and still cannot bind WeCom, so the new channel is unusable for those users. Suggested fix: drop the hard whitelist and escape the namespace in the markdown reply, or validate with the same rule as parseAccessToken().

Summary

  • Review mode: initial. One compatibility issue found in the new WeCom binding path. I could not run Bun-based tests here because bun is not installed in this environment, so the new channel paths still carry some unverified runtime risk.

Testing

  • Not run (automation)

Comment thread hub/src/wecom/bot.ts Outdated
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Findings

  • [Major] Permission callbacks use truncated IDs — WeCom approval buttons only carry 8-character session/request prefixes, then handleTemplateCardEvent resolves the first matching active session/request. If two active sessions or pending requests in the same namespace share that prefix, tapping Allow/Deny can approve the wrong permission. This new WeCom path is not bound by Telegram's short callback-data limit, so use exact IDs in the button key and exact lookups. Evidence hub/src/wecom/sessionView.ts:66.
    Suggested fix:
    export function createCallbackData(action: string, sessionId: string, requestId?: string): string {
        return requestId ? `${action}:${sessionId}:${requestId}` : `${action}:${sessionId}`
    }
    
    export function findSessionById(sessions: Session[], id: string): Session | undefined {
        return sessions.find((session) => session.id === id)
    }
    
    const session = findSessionById(sessions, parsed.sessionId)
    const requestId = parsed.extra && session.agentState?.requests?.[parsed.extra]
        ? parsed.extra
        : null

Summary

  • Review mode: initial
  • Found one major issue in the WeCom permission callback routing. Residual risk/testing gap: add a regression test with two same-prefix sessions or requests to prove callbacks use exact IDs.

Testing

  • Not run (automation; bun is unavailable in this runner).

HAPI Bot

Comment thread hub/src/wecom/sessionView.ts Outdated
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Findings

  • [Major] WeCom binding accepts tokens the rest of HAPI cannot use — onTextMessage only checks startsWith("${cliApiToken}:") and then stores the remaining text after trim(). That accepts strings like TOKEN:foo:bar, but every normal HAPI auth path uses parseAccessToken(), where the last colon makes the base token TOKEN:foo, so CLI/web auth reject it. The bot can confirm a binding to a namespace that no session can authenticate into. Evidence hub/src/wecom/bot.ts:175.
    Suggested fix:
    import { parseAccessToken } from '../utils/accessToken'
    import { constantTimeEquals } from '../utils/crypto'
    
    const parsedToken = parseAccessToken(content)
    if (!parsedToken || !constantTimeEquals(parsedToken.baseToken, this.cliApiToken)) return
    
    const namespace = parsedToken.namespace

Summary

  • Review mode: follow-up after new commits
  • One major issue remains in the new WeCom binding path: it can persist namespaces that are impossible to use through the existing HAPI auth parser.

Testing

  • Not run (automation; bun is unavailable in this runner).

HAPI Bot

Comment thread hub/src/wecom/bot.ts Outdated
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Review mode: follow-up after new commits

No findings. Residual risk: the new WeCom path is only covered by unit tests in this diff, and I could not run Bun-based verification in this runner.

Testing: Not run (bun unavailable in this environment)

HAPI Bot

wzxjohn added 4 commits May 8, 2026 22:06
Adds a WeCom (企业微信) notification channel parallel to the existing
Telegram / ServerChan / WebPush channels, using the official
@wecom/aibot-node-sdk for the WebSocket long-connection transport.

Pushes every existing NotificationHub event (permission request, ready,
task failure, session completion) as a template_card, and handles the
button_interaction callbacks to approve/deny permission requests,
replacing the original card via aibot_respond_update_msg with the
original req_id threaded through by the SDK.

User binding: a WeCom user sends "<CLI_API_TOKEN>:<namespace>" in a
single chat with the bot; the handler validates the namespace against
a conservative whitelist, rejects silent rebinds to a different
namespace, and persists the mapping to store.users.

Configuration: WECOM_BOT_ID / WECOM_BOT_SECRET / WECOM_NOTIFICATION
env vars mirror the existing TELEGRAM_* shape. Channel is enabled iff
both credentials are present and the toggle is on.

Notable wire-format fix: the SDK's TypeScript declarations put
event_key / task_id flat on the template_card_event, but the live wire
nests them under event.template_card_event.*. callbacks.ts reads from
the nested path first with a flat fallback.

Notable server-side requirement: WeCom rejects update reply cards
without a card_action with errcode 42045. buildSystemReplyCard
always attaches one pointing at the session URL (or publicUrl).

Notable post-kick behaviour: when the server pushes disconnected_event,
the SDK marks the client as manually closed (no auto-reconnect);
WecomBot re-arms connect() after a 30 s cooldown.

Includes an E2E smoke harness (hub/scripts/e2e-wecom.ts) that walks
every notification type plus the binding + click flow against the real
WeCom endpoint, gated on WECOM_BOT_ID / WECOM_BOT_SECRET.
The bind handler's `[A-Za-z0-9_-]{1,64}` regex blocks namespaces that
the rest of HAPI happily accepts — parseAccessToken / /api/auth allow
any non-empty trimmed string. A user with a namespace containing spaces
or dots could authenticate elsewhere but not bind WeCom.

Drop the whitelist and instead escape markdown metacharacters
(`\`, `_`, `*`, `` ` ``) when interpolating namespace or userid into the
bind-reply markdown so an arbitrary namespace can't break the rendered
card. Storage uses the raw namespace; only the rendered reply is
escaped.
Approve/Deny callback button keys carried only 8-char prefixes of
session.id and request.id, and handleTemplateCardEvent resolved them
via prefix search. If two active sessions or pending requests in the
same namespace shared that prefix, a click could approve the wrong
permission.

WeCom button keys allow up to 1024 bytes (per the official SDK type),
so the Telegram-style truncation was never needed here. createCallbackData
now carries the full IDs and handleTemplateCardEvent resolves them by
exact match (findSessionById + hashmap lookup on agentState.requests).
The 8-char prefixes are kept only for task_id, which has WeCom's own
128-byte / [0-9A-Za-z_-@] constraints and where collisions are benign
(an update card landing on the wrong card, not the wrong syncEngine
call).

Adds a regression test that two sessions sharing an 8-char prefix do
not collide under the new exact-match lookup.
The naive prefix-strip accepted any text starting with `${cliApiToken}:`
and stored the suffix as the namespace, so `TOKEN:foo:bar` was
confirmed as namespace `foo:bar` — but parseAccessToken (used by the
CLI and /api/auth) splits on the LAST colon and would parse the same
input as baseToken=`TOKEN:foo`, namespace=`bar`, then reject it
because the base token doesn't match. Net effect: a persisted WeCom
binding for a namespace no client could ever authenticate into.

Replace the manual parse with parseAccessToken + constantTimeEquals
on the base token. Now WeCom accepts exactly what the rest of HAPI
accepts: bare TOKEN binds to `default`, TOKEN:<ns> binds to <ns>,
TOKEN:foo:bar is rejected.
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Findings

  • [Major] Restrict WeCom binding to single chats — onTextMessage parses and persists any valid CLI_API_TOKEN[:namespace] text callback as a WeCom binding, but the documented flow says binding happens in a single chat and the incoming frame already carries chattype. Accepting group-originated text makes an accidental token paste in a group create a persistent account binding instead of being ignored. Evidence hub/src/wecom/bot.ts:171.
    Suggested fix:
    const body = frame.body as {
        chattype?: string
        text?: { content?: string }
        from?: { userid?: string }
    } | undefined
    if (body?.chattype !== 'single') return

Summary
Review mode: follow-up after new commits

Current blocker/risk found in the latest WeCom bind handler: non-single chat messages can create bindings. Previous bot review had no findings; this pass still reviewed the full current PR diff.

Testing
Not run (automation; PR content not executed per review security policy)

HAPI Bot

Comment thread hub/src/wecom/bot.ts
@tiann
Copy link
Copy Markdown
Owner

tiann commented May 9, 2026

Sorry, I won't accept this PR since we've already supported https://sct.ftqq.com/ in #515.

@tiann tiann closed this May 9, 2026
@wzxjohn
Copy link
Copy Markdown
Author

wzxjohn commented May 9, 2026

Sorry, I won't accept this PR since we've already supported https://sct.ftqq.com/ in #515.

@tiann These two implementations are completely different. ServerChan’s WeCom push notifications are one-way only. In contrast, the WeCom feature in PR uses a smart bot WebSocket to enable two-way interaction, allowing approvals to be processed directly within the WeCom dialog box.

@tiann tiann reopened this May 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

WeCom push channel support

2 participants