feat: wechat channel#1666
Conversation
github-actions
Bot
added
scope: channel/web
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the platform's messaging capabilities by adding a new WeChat channel. It introduces a seamless interactive QR code login experience for channels, improving user onboarding and ensuring that channels requiring only authentication (rather than explicit pairing) are correctly reflected in the UI. The changes also include backend logic for managing interactive login sessions and persisting dynamic channel settings. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new WeChat channel extension for IronClaw, enabling direct-message chat via long polling. The changes include the implementation of WeChat API interactions, state management, and a build script for the WASM component. It also updates the feature parity documentation and extension registry. Significant modifications were made to the core application to support interactive login flows, including new API routes and UI components for handling QR code-based authentication. A new requires_binding field was added to channel capabilities to better categorize activation statuses. Review comments highlight a potential issue where unwrap_or(0) for the ret field in WeChat API responses could mask actual errors, suggesting a more explicit check for Some(0) to prevent silent failures.
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Security-Focused Review: WeChat Channel (iLink Bot)
Large PR from new contributor. Extra scrutiny applied. Important note: this is the iLink Bot protocol (JSON over HTTPS, long-polling), NOT the WeChat Official Account platform (XML, webhooks). No XML parsing, no webhook signature verification needed.
Critical
random_wechat_uin()is predictable (channels-src/wechat/src/api.rs): Usesbase64(timestamp_ms % u32::MAX)-- completely guessable. IfX-WECHAT-UINhas any security purpose in the iLink protocol, this is trivially spoofable. Use actual randomness or document as non-security-relevant.
High
-
baseurlfrom QR response stored without validation (src/extensions/wechat_login.rs): The iLink server's response can setbaseurlto any string, which gets persisted and used for all subsequent API calls (including those carrying the bot_token). An attacker who controls the QR response (MITM, rogue QR) could redirect all authenticated traffic to a malicious server. Validate that baseurl is HTTPS and matches known WeChat domains (e.g.,*.weixin.qq.com,*.wechat.com). -
Bot token received without provenance verification: QR login receives
bot_tokenfrom iLink server with no certificate pinning. Acceptable for most channels but WeChat operates in environments where MITM is a real concern. Document the trust assumption.
Medium
-
Client-side poll loop with 0ms delay (
src/channels/web/static/app.js):setTimeout(fn, 0)onpending/scanned/refreshedstatus creates a tight loop hammering the server. Use 2-5 second backoff. -
No message deduplication: Messages from
getupdatesare emitted without checking if already processed. Trackmessage_idin persistent state to prevent replay. -
CSP relaxed globally (
src/channels/web/server.rs):img-srcaddsliteapp.weixin.qq.comfor all pages, not just WeChat setup. Acceptable for MVP but should be documented.
Architecture -- Good
- Correctly follows WASM channel pattern (wit_bindgen, Guest trait, all callbacks)
- Module-owned initialization per CLAUDE.md
requires_bindingaddition for QR-login-based channels is clean- No new dependencies beyond standard ones (wit-bindgen, serde, base64)
- No
.unwrap()/.expect()in production code
Minor
inject_channel_secrets_into_configscope change (src/channels/wasm/setup.rs): Changed from hardcoded"default"toowner_id. Verify Feishu secrets still load correctly -- they may have been stored under the old"default"scope.
Verdict
Request changes. The baseurl validation (item 2) is the most important -- it's an open redirect for all authenticated API traffic. The client-side polling backoff (item 4) is also needed to prevent self-DoS. The rest are lower priority but should be addressed.
Good first contribution overall -- architecture is sound, code quality is high, and the iLink Bot protocol choice is well-suited to the WASM channel model.
github-actions
Bot
added
scope: agent
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Re-review: WeChat channel (3 new commits)
One fix, three findings still open:
| # | Finding | Status |
|---|---|---|
| 1 | random_wechat_uin predictable |
Not addressed -- still timestamp-based |
| 2 | base_url stored without validation |
Not addressed -- no URL validation |
| 3 | Bot token without provenance check | Not addressed |
| 4 | Client poll 0ms delay | Fixed -- poll_interval_ms defaults 30s with .max(30_000) enforcement |
| 5 | No message dedup | Partially -- cursor persistence helps, no client-side ID dedup |
New image messaging code interpolates user-influenced params (encrypted_query_param, filekey) directly into CDN URLs without sanitization -- minor additional concern.
Items 1-2 remain blocking. The base_url validation is the highest priority -- it's an open redirect for all authenticated API traffic.
# Conflicts: # Cargo.lock
think-in-universe
left a comment
think-in-universe
left a comment
There was a problem hiding this comment.
I'd like to approve this PR as testing work well.
I didn't go through all the code changes by myself due to time constraint, but don't want to block the feature.
…lper Move all SILK decoding into a workspace-excluded crate (`crates/ironclaw_silk_decoder/`) and invoke it from the host as a subprocess over stdin/stdout. The main `cargo build` no longer pulls `silk-rs` (and its `bindgen`/`libclang` build chain), addressing the review concern flagged on PR #1666 by zmanian and Copilot. Behavior is unchanged when the helper is installed: WeChat voice attachments are transcoded SILK→WAV and `mime_type` rewritten to `audio/wav`. Without the helper, the existing graceful-degrade path preserves raw `audio/silk` bytes and logs that the decoder is missing. The host looks up the helper in this order: 1. `IRONCLAW_SILK_DECODER` env var 2. Sibling of the running `ironclaw` executable 3. `ironclaw-silk-decoder` on `$PATH` Caller-level tests use a Unix shell stub binary (under `tempfile`) to drive the spawn-binary path through `maybe_transcode_wechat_silk_attachment`, covering success, decoder-failure (caller fallback), non-RIFF rejection, and empty-input cases. WIP: this is the native-binary variant. A WASM-target follow-up may replace the native binary with a single `silk-decoder.wasm` artifact matching the WeChat channel distribution pattern. https://claude.ai/code/session_01C5cjqPY1PkVRexraKWmmsz
Add an explicit empty `[workspace]` table so cargo treats the crate as its own root rather than trying to inherit the IronClaw workspace. Without this, `cargo build --manifest-path crates/ironclaw_silk_decoder/Cargo.toml` errors with "current package believes it's in a workspace when it's not". Commit the standalone Cargo.lock for reproducible builds of the helper. https://claude.ai/code/session_01C5cjqPY1PkVRexraKWmmsz
…tants
Two lints the Clippy job flagged on the silk-decoder host integration:
1. `question_mark` — replace `if let Err(error) = writer.await.map_err(...)?
{ return Err(error); }` with `writer.await.map_err(...)??;`. Same
semantics, idiomatic.
2. `assertions_on_constants` — promote the runtime
`assert!(MAX_DECODED_WAV_BYTES >= MAX_ATTACHMENT_BYTES)` invariant
to a `const _: () = assert!(...)` so it's checked at compile time
and doesn't burn a test slot.
No behavioral change.
https://claude.ai/code/session_01C5cjqPY1PkVRexraKWmmsz
Three async stub-binary tests and two resolve-command tests all mutate the same `IRONCLAW_SILK_DECODER` env var. Under cargo's parallel test threads they trampled each other, causing flaky failures (each test passed in isolation, two failed when run together). The CI "Run Tests" job hit this on the latest commit. Adopt the existing `crate::config::helpers::lock_env()` mutex pattern already used by shell.rs, llm/recording.rs, pairing/approval.rs, restart.rs, etc. EnvGuard now holds the lock as a field so the env var stays restored before another test takes the lock; ordering is guaranteed because Drop runs the custom restoration body before field-drop releases the mutex. Verified locally: all 13 channels::wasm::attachment_hydration tests pass with the default parallel test runner. https://claude.ai/code/session_01C5cjqPY1PkVRexraKWmmsz
silk-codec is a sibling fork of the same upstream Skype SILK SDK sources used by silk-rs, but newer and better-maintained: - bindgen 0.72 (vs silk-rs's 0.59 from 2021) - cc 1.2 (vs older) - thiserror 2.0 - Identical decode_silk(src, sample_rate) -> Result<Vec<u8>, SilkError> signature; both implementations skip the 0x02 Tencent flag and require the "#!SILK_V3" header, so behavior on WeChat voice notes is unchanged. This is purely a tooling refresh in the standalone helper crate. The main IronClaw build still has zero SILK / libclang dependency. The host's subprocess invocation, capability gating, size caps, and graceful-degrade-to-raw-SILK fallback are all unchanged. Verified locally: - crates/ironclaw_silk_decoder: 6/6 unit tests pass - ironclaw lib: 13/13 attachment_hydration tests pass (parallel runner) - cargo fmt --check: clean Note: PR #1666's spec-compatibility comment in Cargo.toml previously named silk-rs; the renamed comment in this commit reflects the new dependency. No other host code references either crate by name. https://claude.ai/code/session_01C5cjqPY1PkVRexraKWmmsz
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Approving — the silk-decoder isolation from #3341 has been folded into this branch, so the main cargo build no longer requires libclang while WeChat voice memos are still transcribed end-to-end via the standalone crates/ironclaw_silk_decoder/ helper. Verified the silk-decoder crate, build.sh, and the host-side subprocess invocation are all present at HEAD 204bd3e23.
Summary
channels-src/wechatWASM channel for IronClaw with single-account WeChat DM support: QR login, long-pollgetupdates, outboundsendmessage,context_tokenpersistence, and typing indicators.wechat_bot_tokenautomatically.weixinidentifiers towechatfor consistency with the product-facing channel name.Activeafter QR login, stepper labels localize correctly, and the QR flow uses a separate-tab CTA instead of a broken inline image.Change Type
Linked Issue
#1584
Validation
cargo fmtcargo clippy --all --benches --tests --examples --all-featurescargo test test_extensions_setup_returns_interactive_login_for_wechat --lib --quiet,cargo test test_extensions_wechat_login_poll_broadcasts_auth_completed_and_activates --lib --quiet,cargo test test_wechat_interactive_login_poll_persists_state_and_activates --lib --quiet,cargo test test_inject_channel_settings_uses_owner_scope --lib --quiet,cargo test test_requires_binding_detects_dm_owner_fields --lib --quiet,cargo test test_wechat_active_channel_does_not_require_pairing_status --lib --quiet,cargo test --lib wechat --quiet,cargo check -q,cargo check --manifest-path channels-src/wechat/Cargo.toml -q,cargo check --manifest-path channels-src/wechat/Cargo.toml --target wasm32-wasip2 -q,cargo clippy --all-targets -- -D warnings,cargo clippy --manifest-path channels-src/wechat/Cargo.toml --target wasm32-wasip2 -- -D warningsSecurity Impact
Touches authenticated extension setup/login flows, secret persistence, and outbound HTTP from the WeChat WASM channel to
ilinkai.weixin.qq.comand the WeChat CDN. No auth model, sandbox boundary, or secret exposure guarantees were weakened; the channel continues to rely on existing secret injection and HTTP allowlist enforcement.Database Impact
None. No schema changes or migrations. This reuses existing settings/secrets persistence paths and remains backend-agnostic for both PostgreSQL and libSQL.
Blast Radius
Touches the WASM channel host path, extension manager setup/login flow, web gateway extension UI, and the new WeChat channel implementation. Regressions would most likely affect extension setup state classification, interactive login UX, or WeChat channel activation/polling.
Rollback Plan
Revert the WeChat-specific extension manager/web gateway changes and remove the
channels-src/wechatregistration/artifacts. If a partial rollback is needed, disable thewechatregistry entry and remove the installedwechat.wasm/wechat.capabilities.jsonartifacts so existing channels continue unaffected while the host/UI changes are reverted.Review track: C