Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 78 additions & 8 deletions packages/channels/weixin/src/WeixinAdapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import { loadAccount, DEFAULT_BASE_URL } from './accounts.js';
import { startPollLoop, getContextToken } from './monitor.js';
import type { CdnRef, FileCdnRef } from './monitor.js';
import { sendText } from './send.js';
import { sendText, sendImage } from './send.js';
import { downloadAndDecrypt } from './media.js';
import { getConfig, sendTyping } from './api.js';
import { TypingStatus } from './types.js';
Expand All @@ -43,6 +43,22 @@
}

async connect(): Promise<void> {
// Default channel instructions: tell the AI it can send images via WeChat

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] Default instructions only set when falsy — custom instructions lose image capability

if (!this.config.instructions) means any channel with custom instructions silently loses the image-sending guide. The [IMAGE: ...] parsing still works unconditionally, so the mismatch creates inconsistent behavior.

Suggested change
// Default channel instructions: tell the AI it can send images via WeChat
const imageInstructions = [
'## WeChat Channel Capabilities',
'',
'You are communicating with users through WeChat. You CAN send images to the user.',
'To send an image, include it in your response using this EXACT format:',
'[IMAGE: 图片文件的完整绝对路径]',
'',
'Example: 如果你想发送 /tmp/cat.png 给用户, 在回复中写 [IMAGE: /tmp/cat.png]',
'This marker will be automatically removed from the text and the image will be uploaded and sent.',
'You can include multiple [IMAGE: ...] markers in one response.',
'',
'Users can also send you images, which you can see and analyze.',
].join('\n');
if (!this.config.instructions) {
this.config.instructions = imageInstructions;
} else if (!this.config.instructions.includes('[IMAGE:')) {
this.config.instructions += '\n\n' + imageInstructions;
}

— pai/glm-5 via Qwen Code /review

if (!this.config.instructions) {
this.config.instructions = [
'## WeChat Channel Capabilities',
'',
'You are communicating with users through WeChat. You CAN send images to the user.',
'To send an image, include it in your response using this EXACT format:',
'[IMAGE: 图片文件的完整绝对路径]',
'',
'Example: 如果你想发送 /tmp/cat.png 给用户, 在回复中写 [IMAGE: /tmp/cat.png]',
'This marker will be automatically removed from the text and the image will be uploaded and sent.',
'You can include multiple [IMAGE: ...] markers in one response.',
'',
'Users can also send you images, which you can see and analyze.',
].join('\n');
}
const account = loadAccount();
if (!account) {
throw new Error(
Expand Down Expand Up @@ -153,18 +169,72 @@
}
}

// Always remind the AI about image-sending capability on every message
const IMAGE_INSTRUCTION =
'[WeChat Channel] 你可以通过微信发送图片。在回复中使用 [IMAGE: 文件绝对路径] 发送图片,例如 [IMAGE: /tmp/cat.png]。标记会被自动移除。';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] Per-message IMAGE_INSTRUCTION is redundant and increases attack surface

IMAGE_INSTRUCTION is prepended to every inbound message, but config.instructions already set in connect() tells the AI about image capability. This doubles token cost (~80 Chinese chars per message) and explicitly tells the AI the [IMAGE: ...] syntax on every turn, making prompt injection easier for an attacker.

Consider removing the per-message injection and relying solely on config.instructions. If a reminder is needed, keep it in the system prompt rather than the user message.

— pai/glm-5 via Qwen Code /review

envelope.text = `${IMAGE_INSTRUCTION}\n\n${envelope.text}`;

await super.handleInbound(envelope);
}

async sendMessage(chatId: string, text: string): Promise<void> {
async sendMessage(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] imagePaths parameter is dead code

The base class ChannelBase.sendMessage(chatId, text) only passes 2 arguments. No caller ever provides imagePaths. This parameter gives the misleading impression that the ACP pipeline passes image paths, but it never does. Consider removing it until the pipeline actually supports it, or add a comment marking it as a future integration point.

— pai/glm-5 via Qwen Code /review

chatId: string,
text: string,
imagePaths?: string[],
): Promise<void> {
const contextToken = getContextToken(chatId) || '';
await sendText({
to: chatId,
text,
baseUrl: this.baseUrl,
token: this.token,
contextToken,

// Parse [IMAGE: /path/to/file.png] markers from text
const imageRegex = /\[IMAGE:\s*([^\]]+)\]/gi;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] [IMAGE:] regex matches inside code blocks and produces empty paths

Two issues with the regex \[IMAGE:\s*([^\]]+)\]:

  1. If the AI explains the syntax inside a code block (`[IMAGE: /tmp/example.png]`), the marker is extracted and the code block text is corrupted.
  2. [IMAGE: ] captures a space, which .trim() converts to '', then readFileSync('') throws a confusing error.

For (2), filter empty paths after trimming:

Suggested change
const imageRegex = /\[IMAGE:\s*([^\]]+)\]/gi;
let cleanedText = text.replace(imageRegex, (_, path: string) => {
const trimmed = path.trim();
if (trimmed) parsedImages.push(trimmed);
return '';
});

For (1), consider stripping code blocks before running the regex, or extracting the parsing into a testable pure function.

— pai/glm-5 via Qwen Code /review

const parsedImages: string[] = [];
let cleanedText = text.replace(imageRegex, (_, path: string) => {
parsedImages.push(path.trim());
return '';
});

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data High

This
regular expression
that depends on
library input
may run slow on strings starting with '[image:' and with many repetitions of '\t'.

// Merge with any imagePaths from the ACP pipeline
const allImages = [...(imagePaths || []), ...parsedImages];

// Clean up double blank lines left by removed markers
cleanedText = cleanedText.replace(/\n{3,}/g, '\n\n').trim();

// Send text first if non-empty
if (cleanedText) {
await sendText({
to: chatId,
text: cleanedText,
baseUrl: this.baseUrl,
token: this.token,
contextToken,
});
}

// Send images
if (allImages.length) {
for (const imagePath of allImages) {
try {
await sendImage({
to: chatId,
imagePath,
baseUrl: this.baseUrl,
token: this.token,
contextToken,
});
} catch (err) {
const errMsg = err instanceof Error ? err.message : String(err);
process.stderr.write(
`[Weixin:${this.name}] Failed to send image ${imagePath}: ${errMsg}\n`,
);
await sendText({

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Critical] Fallback sendText not wrapped in try/catch — unhandled rejection

When sendImage fails, the catch block calls await sendText(...) to notify the user, but this fallback call is itself unguarded. If sendText also throws (e.g., expired auth token), the unhandled promise rejection can crash the process under Node.js's default --unhandled-rejections=throw.

Also, errMsg is sent directly to the user, leaking internal file paths (e.g., ENOENT: no such file or directory, open '/etc/shadow').

Suggested change
await sendText({
} catch (err) {
const errMsg = err instanceof Error ? err.message : String(err);
process.stderr.write(
`[Weixin:${this.name}] Failed to send image ${imagePath}: ${errMsg}\n`,
);
try {
await sendText({
to: chatId,
text: '图片发送失败,请稍后重试',
baseUrl: this.baseUrl,
token: this.token,
contextToken,
});
} catch (fallbackErr) {
process.stderr.write(
`[Weixin:${this.name}] Fallback text also failed: ${fallbackErr instanceof Error ? fallbackErr.message : String(fallbackErr)}\n`,
);
}
}

— pai/glm-5 via Qwen Code /review

to: chatId,
text: `图片发送失败: ${errMsg}`,
baseUrl: this.baseUrl,
token: this.token,
contextToken,
});
}
}
}
}

disconnect(): void {
Expand Down
98 changes: 98 additions & 0 deletions packages/channels/weixin/src/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,3 +141,101 @@ export async function sendTyping(
const body: SendTypingReq = { ...req, base_info: baseInfo() };
return post<SendTypingResp>(baseUrl, '/ilink/bot/sendtyping', body, token);
}

interface GetUploadUrlReq {
filekey: string;
media_type: number;
to_user_id: string;
rawsize: number;
rawfilemd5: string;
filesize: number;
no_need_thumb: boolean;
aeskey: string;
base_info: BaseInfo;
}

interface GetUploadUrlResp {
ret?: number;
errmsg?: string;
upload_full_url?: string;
upload_param?: string;
thumb_upload_param?: string;
}

/**
* Request an upload URL and CDN credentials for media.
* @param aeskeyHex 16-byte AES key as 32-char hex string (e.g. "00112233445566778899aabbccddeeff")
* @returns Either the full CDN upload URL or the upload_param string
*/
export async function getUploadUrl(
baseUrl: string,
token: string,
toUserId: string,
filekey: string,
rawsize: number,
rawfilemd5: string,
encryptedSize: number,
aeskeyHex: string,
): Promise<string> {
const body: GetUploadUrlReq = {
filekey,
media_type: 1,
to_user_id: toUserId,
rawsize,
rawfilemd5,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] 错误消息中包含字面量 ${undefined}

`getuploadurl failed: ret=${resp.ret} errcode=${undefined} errmsg=...` 产生 errcode=undefined,误导排查人员。

Suggested change
rawfilemd5,
`getuploadurl failed: ret=${resp.ret} errmsg=${resp.errmsg || '(none)'}`

— deepseek-v4-pro via Qwen Code /review

filesize: encryptedSize,
no_need_thumb: true,
aeskey: aeskeyHex,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Critical] getUploadUrl 重试永不生效 — isRetryableError 检查 errcode 但此端点使用 ret

GetUploadUrlResp 接口没有 errcode 字段,WeixinApiError 构造函数只传入 ret(第 3 参数)未传 errcode(第 4 参数)。isRetryableError 只检查 err.errcode(=== -14、=== -1、=== 45011),因此来自 getuploadurl 的所有 API 错误都直接传播,不重试。

Suggested change
aeskey: aeskeyHex,
// 在 GetUploadUrlResp 中添加 errcode?: number
// 映射 resp.errcode 并传入 WeixinApiError 构造函数
if (resp.ret !== undefined && resp.ret !== 0 || resp.errcode !== undefined && resp.errcode !== 0) {
throw new WeixinApiError(
`getuploadurl failed: ret=${resp.ret} errcode=${resp.errcode ?? '(none)'} errmsg=${resp.errmsg || '(none)'}`,
200,
resp.ret,
resp.errcode,
);
}
// 同时在 isRetryableError 中增加对 err.ret 的检查

— deepseek-v4-pro via Qwen Code /review

base_info: baseInfo(),
};
const resp = await post<GetUploadUrlResp>(
baseUrl,
'/ilink/bot/getuploadurl',
body,
token,
);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] getUploadUrl doesn't verify ret === 0

The function checks for upload_full_url/upload_param presence but never validates resp.ret === 0 first. This is inconsistent with getUpdates in monitor.ts which explicitly checks resp.ret. An error response with a non-zero ret but a non-empty upload_full_url would be used without validation.

Suggested change
if (resp.ret !== undefined && resp.ret !== 0) {
throw new Error(
`getuploadurl failed: ret=${resp.ret} errmsg=${resp.errmsg || '(none)'}`,
);
}

Place this check before the upload_full_url / upload_param checks.

— pai/glm-5 via Qwen Code /review

// upload_full_url: CDN upload URL with all params embedded
if (resp.upload_full_url) {
return resp.upload_full_url;
}

// upload_param: CDN upload params only (must construct URL with filekey)
if (resp.upload_param) {
return resp.upload_param;
}

throw new Error(
`getuploadurl failed: ret=${resp.ret} errmsg=${resp.errmsg || '(none)'}`,
);
}

/** Upload encrypted media to CDN.
* If urlOrParam is a full URL, use it directly.
* If it's just a param, construct the URL. */
export async function uploadToCdn(
urlOrParam: string,
filekey: string,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] SSRF — no URL host validation

uploadToCdn uses urlOrParam.startsWith('http') which accepts http:// URLs (sending encrypted data over cleartext) and does not validate the host. A compromised API response could direct the POST to an internal service (e.g., cloud metadata endpoint http://169.254.169.254/).

Suggested change
filekey: string,
const WECHAT_CDN_HOST = 'novac2c.cdn.weixin.qq.com';
let url: string;
if (urlOrParam.startsWith('https://')) {
const parsed = new URL(urlOrParam);
if (parsed.hostname !== WECHAT_CDN_HOST) {
throw new Error(`CDN upload URL has unexpected host: ${parsed.hostname}`);
}
url = urlOrParam;
} else {
url = `https://${WECHAT_CDN_HOST}/c2c/upload?encrypted_query_param=${encodeURIComponent(urlOrParam)}&filekey=${encodeURIComponent(filekey)}`;
}

— pai/glm-5 via Qwen Code /review

encryptedData: Buffer,
): Promise<string> {
const url = urlOrParam.startsWith('http')
? urlOrParam
: `https://novac2c.cdn.weixin.qq.com/c2c/upload?encrypted_query_param=${encodeURIComponent(urlOrParam)}&filekey=${encodeURIComponent(filekey)}`;

const resp = await fetch(url, {
method: 'POST',
headers: { 'Content-Type': 'application/octet-stream' },

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Critical] CDN upload has no timeout — can hang indefinitely

uploadToCdn uses bare fetch() with no AbortController, unlike the post() helper which has a 40-second timeout. A stalled CDN server will hang this promise forever, blocking the sequential image-sending loop and the entire message pipeline for that user.

Suggested change
headers: { 'Content-Type': 'application/octet-stream' },
export async function uploadToCdn(
urlOrParam: string,
filekey: string,
encryptedData: Buffer,
): Promise<string> {
const controller = new AbortController();
const timeout = setTimeout(() => controller.abort(), 40000);
try {
const url = urlOrParam.startsWith('http')
? urlOrParam
: `https://novac2c.cdn.weixin.qq.com/c2c/upload?encrypted_query_param=${encodeURIComponent(urlOrParam)}&filekey=${encodeURIComponent(filekey)}`;
const resp = await fetch(url, {
method: 'POST',
headers: { 'Content-Type': 'application/octet-stream' },
body: encryptedData,
signal: controller.signal,
});
if (!resp.ok) {
throw new Error(`CDN upload failed: HTTP ${resp.status}`);
}
const encryptParam = resp.headers.get('x-encrypted-param');
if (!encryptParam) {
throw new Error(
'CDN upload succeeded but missing x-encrypted-param header',
);
}
return encryptParam;
} finally {
clearTimeout(timeout);
}
}

— pai/glm-5 via Qwen Code /review

body: encryptedData,
});
if (!resp.ok) {
throw new Error(`CDN upload failed: HTTP ${resp.status}`);
}
// Extract x-encrypted-param from response header
const encryptParam = resp.headers.get('x-encrypted-param');
if (!encryptParam) {
throw new Error(
'CDN upload succeeded but missing x-encrypted-param header',
);
}
return encryptParam;
}
51 changes: 51 additions & 0 deletions packages/channels/weixin/src/media.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { describe, it, expect } from 'vitest';
import { createDecipheriv, createCipheriv } from 'node:crypto';
import { encryptAesEcb, computeMd5 } from './media.js';

/**
* Test the AES key parsing and decryption logic used in media.ts.
Expand Down Expand Up @@ -89,3 +90,53 @@ describe('Weixin media crypto', () => {
});
});
});

describe('encryptAesEcb', () => {
it('encrypts data deterministically', () => {
const key = Buffer.alloc(16, 0xab);
const plaintext = Buffer.from('test data for encryption');
const ciphertext1 = encryptAesEcb(plaintext, key);
const ciphertext2 = encryptAesEcb(plaintext, key);
expect(ciphertext1).toEqual(ciphertext2);
});

it('encrypts then decrypts round-trip', () => {
const key = Buffer.alloc(16, 0x42);
const plaintext = Buffer.from('Hello, WeChat media upload!');

const ciphertext = encryptAesEcb(plaintext, key);
const decipher = createDecipheriv('aes-128-ecb', key, null);
const decrypted = Buffer.concat([
decipher.update(ciphertext),
decipher.final(),
]);
expect(decrypted.toString()).toBe(plaintext.toString());
});

it('handles empty plaintext', () => {
const key = Buffer.alloc(16, 0x01);
const ciphertext = encryptAesEcb(Buffer.alloc(0), key);
// ECB with empty input produces empty output (no padding block needed
// when input is exactly 0 bytes — behavior varies by implementation)
// At minimum the result should be decryptable
const decipher = createDecipheriv('aes-128-ecb', key, null);
const decrypted = Buffer.concat([
decipher.update(ciphertext),
decipher.final(),
]);
expect(decrypted.length).toBe(0);
});
});

describe('computeMd5', () => {
it('computes expected MD5', () => {
const data = Buffer.from('hello world');
expect(computeMd5(data)).toBe('5eb63bbbe01eeed093cb22bb8f5acdc3');
});

it('computes MD5 of empty buffer', () => {
expect(computeMd5(Buffer.alloc(0))).toBe(
'd41d8cd98f00b204e9800998ecf8427e',
);
});
});
15 changes: 13 additions & 2 deletions packages/channels/weixin/src/media.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* Ported from cc-weixin/plugins/weixin/src/media.ts (download path only).
*/

import { createDecipheriv } from 'node:crypto';
import { createCipheriv, createDecipheriv, createHash } from 'node:crypto';

const CDN_BASE_URL = 'https://novac2c.cdn.weixin.qq.com/c2c';

Expand All @@ -22,7 +22,7 @@ function decryptAesEcb(ciphertext: Buffer, key: Buffer): Buffer {
* - base64(raw 16 bytes) → images
* - base64(hex string of 16 bytes) → file/voice/video
*/
function parseAesKey(aesKeyBase64: string): Buffer {
export function parseAesKey(aesKeyBase64: string): Buffer {
const decoded = Buffer.from(aesKeyBase64, 'base64');
if (decoded.length === 16) {
return decoded;
Expand Down Expand Up @@ -54,3 +54,14 @@ export async function downloadAndDecrypt(
const keyBuf = parseAesKey(aesKey);
return decryptAesEcb(ciphertext, keyBuf);
}

/** AES-128-ECB encryption for CDN upload. */
export function encryptAesEcb(plaintext: Buffer, key: Buffer): Buffer {
const cipher = createCipheriv('aes-128-ecb', key, null);
return Buffer.concat([cipher.update(plaintext), cipher.final()]);
}

/** Compute MD5 hash of a buffer, returning hex string. */
export function computeMd5(data: Buffer): string {
return createHash('md5').update(data).digest('hex');
}
Loading
Loading