-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(qqbot): security hardening — gateway validation, atomic state, sanitized logging #6200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
43b3fd0
9c248ba
8774eb9
9786cd9
46910de
3ec3877
231f7c7
ebd6f30
2ccbfe6
14ea726
a1367e0
3e8f713
707b752
483305e
3da730f
eff0084
9eff75f
2acf178
a9bd70c
548512a
86615ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,14 @@ import type { | |
| ChannelAgentBridge, | ||
| } from '@qwen-code/channel-base'; | ||
| import WebSocket from 'ws'; | ||
| import { readFileSync, writeFileSync, existsSync, mkdirSync } from 'node:fs'; | ||
| import { | ||
| readFileSync, | ||
| writeFileSync, | ||
| existsSync, | ||
| mkdirSync, | ||
| renameSync, | ||
| unlinkSync, | ||
| } from 'node:fs'; | ||
| import { join } from 'node:path'; | ||
| import { OpCode, Intent } from './types.js'; | ||
| import type { | ||
|
|
@@ -128,6 +135,8 @@ export class QQChannel extends ChannelBase { | |
| private lastHeartbeatAck: number = 0; | ||
| /** Debounce timer for saveQQState to avoid blocking event loop. */ | ||
| private saveTimer: ReturnType<typeof setTimeout> | null = null; | ||
| /** beforeExit hook to flush state when the event loop drains naturally. Does NOT fire for SIGKILL, OOM kills, or uncaughtException. */ | ||
| private beforeExitHook: (() => void) | null = null; | ||
| /** Timer for reconnectWithRetry fallback (unref'd so it doesn't block exit). */ | ||
| private reconnectTimer: ReturnType<typeof setTimeout> | null = null; | ||
| /** Guard against parallel reconnectWithRetry chains from stale close events. */ | ||
|
|
@@ -199,16 +208,30 @@ export class QQChannel extends ChannelBase { | |
| try { | ||
| await this.fetchToken(); | ||
| await this.connectGateway(); | ||
| // Register beforeExit hook so the unref'd debounce timer's unflushed | ||
| // state is persisted when the event loop drains naturally. Does NOT | ||
| // fire for SIGKILL, OOM kills, or uncaughtException. | ||
| if (this.beforeExitHook) { | ||
| process.off('beforeExit', this.beforeExitHook); | ||
| } | ||
| this.beforeExitHook = () => this.flushQQState(); | ||
| process.on('beforeExit', this.beforeExitHook); | ||
| return; | ||
| } catch (e: unknown) { | ||
| if (attempt < 2) { | ||
| const msg = e instanceof Error ? e.message : String(e); | ||
| process.stderr.write( | ||
| `[QQ:${this.name}] Connect attempt ${attempt + 1} failed: ${msg}, retrying...\n`, | ||
| `[QQ:${this.name}] Connect attempt ${attempt + 1} failed: ${sanitizeLogText(msg, 200)}, retrying...\n`, | ||
| ); | ||
| await this.sleep(2000); | ||
| } else { | ||
| throw e; | ||
| // Final attempt: wrap the connection error with sanitized text. | ||
| // The sanitizeLogText path is exercised by the existing connect gateway | ||
| // retry tests in send.test.ts (gateway reconnect timer block). | ||
| throw new Error( | ||
|
Eric-GoodBoy-Tech marked this conversation as resolved.
Eric-GoodBoy-Tech marked this conversation as resolved.
|
||
| sanitizeLogText(e instanceof Error ? e.message : String(e), 200), | ||
|
Eric-GoodBoy-Tech marked this conversation as resolved.
|
||
| { cause: e }, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -253,7 +276,7 @@ export class QQChannel extends ChannelBase { | |
| if (!resp.ok && useMarkdown) { | ||
| const errBody = await resp.text().catch(() => ''); | ||
| process.stderr.write( | ||
| `[QQ:${this.name}] Markdown rejected (HTTP ${resp.status}: ${errBody.slice(0, 100)}), retrying as plain text\n`, | ||
| `[QQ:${this.name}] Markdown rejected (HTTP ${resp.status}: ${sanitizeLogText(errBody, 200)}), retrying as plain text\n`, | ||
| ); | ||
| const plainBody: Record<string, unknown> = { | ||
| content: chunk, | ||
|
|
@@ -275,14 +298,16 @@ export class QQChannel extends ChannelBase { | |
| // Drain response body to avoid socket leak | ||
| const errBody = await resp.text().catch(() => ''); | ||
| process.stderr.write( | ||
| `[QQ:${this.name}] Send HTTP ${resp.status} (msg_seq=${body['msg_seq'] ?? '-'}): ${errBody.slice(0, 200)}\n`, | ||
| `[QQ:${this.name}] Send HTTP ${resp.status} (msg_seq=${body['msg_seq'] ?? '-'}): ${sanitizeLogText(errBody, 200)}\n`, | ||
| ); | ||
| break; // stop sending on failure to avoid msg_seq gaps | ||
| } | ||
| // Only persist seq on success | ||
| if (msgId) this.msgSeqMap.set(msgId, nextSeq); | ||
| } catch (e) { | ||
| process.stderr.write(`[QQ:${this.name}] Send error: ${e}\n`); | ||
| process.stderr.write( | ||
| `[QQ:${this.name}] Send error: ${sanitizeLogText(e instanceof Error ? e.message : String(e), 200)}\n`, | ||
| ); | ||
| break; | ||
| } | ||
| } | ||
|
|
@@ -326,6 +351,10 @@ export class QQChannel extends ChannelBase { | |
| clearTimeout(this.reconnectTimer); | ||
| this.reconnectTimer = null; | ||
| } | ||
| if (this.beforeExitHook) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: disconnect(): void {
if (this.disposed) return;
this.disposed = true;
// ... rest
}
|
||
| process.off('beforeExit', this.beforeExitHook); | ||
| this.beforeExitHook = null; | ||
| } | ||
| this.flushQQState(); | ||
| this.backupGlobalSessions(); | ||
| if (this.ws) { | ||
|
|
@@ -362,22 +391,36 @@ export class QQChannel extends ChannelBase { | |
|
|
||
| /** Debounced state persistence to avoid blocking event loop. */ | ||
| private saveQQState(): void { | ||
| // NOTE: guarded here; flushQQState() is intentionally NOT — disconnect() | ||
| // sets disposed=true *before* calling it, so it must still write final state. | ||
| if (this.disposed) return; | ||
|
Eric-GoodBoy-Tech marked this conversation as resolved.
|
||
| if (this.saveTimer) clearTimeout(this.saveTimer); | ||
| const tmpPath = this.qqStatePath + '.tmp'; | ||
| this.saveTimer = setTimeout(() => { | ||
|
Eric-GoodBoy-Tech marked this conversation as resolved.
|
||
| if (this.disposed) return; | ||
| try { | ||
| writeFileSync( | ||
| this.qqStatePath, | ||
| tmpPath, | ||
| JSON.stringify({ | ||
| chatTypeMap: Array.from(this.chatTypeMap.entries()), | ||
| replyMsgId: Array.from(this.replyMsgId.entries()), | ||
| msgSeqMap: Array.from(this.msgSeqMap.entries()), | ||
| }), | ||
| { mode: 0o600 }, | ||
| ); | ||
| } catch { | ||
| /* best-effort */ | ||
| renameSync(tmpPath, this.qqStatePath); | ||
| } catch (e) { | ||
| try { | ||
| unlinkSync(tmpPath); | ||
| } catch { | ||
| /* best-effort */ | ||
| } | ||
| process.stderr.write( | ||
|
Eric-GoodBoy-Tech marked this conversation as resolved.
|
||
| `[QQ:${this.name}] saveQQState write failed: ${sanitizeLogText(e instanceof Error ? e.message : String(e), 200)}\n`, | ||
| ); | ||
| } | ||
| }, 500); | ||
| this.saveTimer.unref(); | ||
| } | ||
|
Eric-GoodBoy-Tech marked this conversation as resolved.
Eric-GoodBoy-Tech marked this conversation as resolved.
|
||
|
|
||
| /** Flush pending state writes immediately (called on disconnect). */ | ||
|
|
@@ -386,37 +429,104 @@ export class QQChannel extends ChannelBase { | |
| clearTimeout(this.saveTimer); | ||
| this.saveTimer = null; | ||
| } | ||
| const tmpPath = this.qqStatePath + '.tmp'; | ||
| try { | ||
| writeFileSync( | ||
| this.qqStatePath, | ||
| tmpPath, | ||
| JSON.stringify({ | ||
| chatTypeMap: Array.from(this.chatTypeMap.entries()), | ||
| replyMsgId: Array.from(this.replyMsgId.entries()), | ||
| msgSeqMap: Array.from(this.msgSeqMap.entries()), | ||
| }), | ||
| { mode: 0o600 }, | ||
| ); | ||
| renameSync(tmpPath, this.qqStatePath); | ||
| } catch (e) { | ||
| try { | ||
| unlinkSync(tmpPath); | ||
| } catch { | ||
| /* best-effort */ | ||
| } | ||
| process.stderr.write( | ||
| `[QQ:${this.name}] flushQQState write failed: ${sanitizeLogText(e instanceof Error ? e.message : String(e), 200)}\n`, | ||
| ); | ||
| } catch { | ||
| /* best-effort */ | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Restore QQ routing state from disk. | ||
| * Trusts persisted JSON — if the file is corrupt, new Map() may create | ||
| * entries with undefined values, causing get()===undefined to fall through | ||
| * to default routing (C2C). This is acceptable for a rare edge case. | ||
| * Validates and filters every entry on restore — corrupt or unexpected | ||
| * entries (e.g. unknown chat types, oversized replyMsgIds, negative seqs) | ||
| * are silently dropped so they don't propagate into runtime routing. | ||
| */ | ||
| private restoreQQState(): boolean { | ||
| try { | ||
| if (!existsSync(this.qqStatePath)) return false; | ||
| const raw = JSON.parse(readFileSync(this.qqStatePath, 'utf-8')); | ||
| if (raw.chatTypeMap) this.chatTypeMap = new Map(raw.chatTypeMap); | ||
| if (raw.replyMsgId) this.replyMsgId = new Map(raw.replyMsgId); | ||
| if (raw.msgSeqMap) this.msgSeqMap = new Map(raw.msgSeqMap); | ||
| if (typeof raw !== 'object' || raw === null || Array.isArray(raw)) { | ||
| process.stderr.write( | ||
| `[QQ:${this.name}] Invalid QQ state file (not an object), ignoring\n`, | ||
| ); | ||
| return false; | ||
| } | ||
| if (raw.chatTypeMap && Array.isArray(raw.chatTypeMap)) { | ||
| const rawCT = raw.chatTypeMap as Array<[string, unknown]>; | ||
|
Eric-GoodBoy-Tech marked this conversation as resolved.
|
||
| // Validate: only accept 'c2c' | 'group' values | ||
| this.chatTypeMap = new Map( | ||
| rawCT.filter( | ||
| ([k, v]) => | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: The filter callback uses destructuring The same pattern repeats for Suggested fix — add an rawCT.filter(
(e) => {
if (!Array.isArray(e) || e.length !== 2) return false;
const [k, v] = e;
return typeof k === 'string' && k.length <= 256 && (v === 'c2c' || v === 'group');
},
)This also affects the existing tests — the 'filters chatTypeMap' test only uses well-formed tuples, so this crash path isn't exercised. |
||
| typeof k === 'string' && | ||
| k.length <= 256 && | ||
| (v === 'c2c' || v === 'group'), | ||
| ), | ||
| ) as Map<string, 'c2c' | 'group'>; | ||
| const dropped = rawCT.length - this.chatTypeMap.size; | ||
| if (dropped > 0) | ||
| process.stderr.write( | ||
|
Eric-GoodBoy-Tech marked this conversation as resolved.
|
||
| `[QQ:${this.name}] Dropped ${dropped} invalid chatTypeMap entries during restore\n`, | ||
| ); | ||
| } | ||
| if (raw.replyMsgId && Array.isArray(raw.replyMsgId)) { | ||
| const rawRM = raw.replyMsgId as Array<[string, unknown]>; | ||
| // Validate: entries must be strings ≤ 128 chars | ||
| this.replyMsgId = new Map( | ||
| rawRM.filter( | ||
| ([k, v]) => | ||
| typeof k === 'string' && | ||
| k.length <= 256 && | ||
| typeof v === 'string' && | ||
| v.length <= 128, | ||
| ), | ||
| ) as Map<string, string>; | ||
| const dropped = rawRM.length - this.replyMsgId.size; | ||
| if (dropped > 0) | ||
| process.stderr.write( | ||
| `[QQ:${this.name}] Dropped ${dropped} invalid replyMsgId entries during restore\n`, | ||
| ); | ||
| } | ||
| if (raw.msgSeqMap && Array.isArray(raw.msgSeqMap)) { | ||
| const rawMS = raw.msgSeqMap as Array<[string, unknown]>; | ||
| // Validate: entries must be non-negative safe integers | ||
| this.msgSeqMap = new Map( | ||
| rawMS.filter( | ||
| ([k, v]) => | ||
| typeof k === 'string' && | ||
| k.length <= 256 && | ||
| typeof v === 'number' && | ||
| Number.isSafeInteger(v) && | ||
| v >= 0, | ||
| ), | ||
| ) as Map<string, number>; | ||
| const dropped = rawMS.length - this.msgSeqMap.size; | ||
| if (dropped > 0) | ||
| process.stderr.write( | ||
| `[QQ:${this.name}] Dropped ${dropped} invalid msgSeqMap entries during restore\n`, | ||
| ); | ||
| } | ||
| return true; | ||
| } catch (e) { | ||
| process.stderr.write( | ||
| `[QQ:${this.name}] Failed to restore QQ state: ${e instanceof Error ? e.message : String(e)}\n`, | ||
| `[QQ:${this.name}] Failed to restore QQ state: ${sanitizeLogText(e instanceof Error ? e.message : String(e), 200)}\n`, | ||
| ); | ||
| return false; | ||
| } | ||
|
|
@@ -549,7 +659,7 @@ export class QQChannel extends ChannelBase { | |
| this.fetchToken().catch((e) => { | ||
| if (this.disposed) return; | ||
| process.stderr.write( | ||
| `[QQ:${this.name}] Token refresh failed: ${e}, retrying in 60s\n`, | ||
| `[QQ:${this.name}] Token refresh failed: ${sanitizeLogText(e instanceof Error ? e.message : String(e), 200)}, retrying in 60s\n`, | ||
| ); | ||
| this.scheduleTokenRefreshRetry(); | ||
| }); | ||
|
|
@@ -564,7 +674,7 @@ export class QQChannel extends ChannelBase { | |
| this.fetchToken().catch((e) => { | ||
| if (this.disposed) return; | ||
| process.stderr.write( | ||
| `[QQ:${this.name}] Token refresh failed: ${e}, retrying in 60s\n`, | ||
| `[QQ:${this.name}] Token refresh failed: ${sanitizeLogText(e instanceof Error ? e.message : String(e), 200)}, retrying in 60s\n`, | ||
| ); | ||
| this.scheduleTokenRefreshRetry(); | ||
| }); | ||
|
|
@@ -611,7 +721,7 @@ export class QQChannel extends ChannelBase { | |
| this.handleGatewayMessage(msg, resolve); | ||
| } catch (e) { | ||
| process.stderr.write( | ||
| `[QQ:${this.name}] Malformed gateway message: ${e instanceof Error ? e.message : String(e)}\n`, | ||
| `[QQ:${this.name}] Malformed gateway message: ${sanitizeLogText(e instanceof Error ? e.message : String(e), 200)}\n`, | ||
| ); | ||
| } | ||
| }); | ||
|
|
@@ -680,7 +790,9 @@ export class QQChannel extends ChannelBase { | |
| }); | ||
|
|
||
| this.ws.on('error', (e: Error) => { | ||
| process.stderr.write(`[QQ:${this.name}] WebSocket error: ${e.message}\n`); | ||
| process.stderr.write( | ||
| `[QQ:${this.name}] WebSocket error: ${sanitizeLogText(e.message, 200)}\n`, | ||
| ); | ||
| if (!this.ws || this.ws.readyState !== WebSocket.OPEN) { | ||
| reject(e); | ||
| } | ||
|
|
@@ -853,7 +965,7 @@ export class QQChannel extends ChannelBase { | |
| const msg = e instanceof Error ? e.message : String(e); | ||
| const backoff = Math.min(1000 * 2 ** (attempt + 1), 30000); | ||
| process.stderr.write( | ||
| `[QQ:${this.name}] RC: ${msg} (retry in ${backoff}ms, attempt ${attempt + 1}/${maxGwRetries})\n`, | ||
| `[QQ:${this.name}] RC: ${sanitizeLogText(msg, 200)} (retry in ${backoff}ms, attempt ${attempt + 1}/${maxGwRetries})\n`, | ||
| ); | ||
| if (attempt < maxGwRetries - 1) await this.sleep(backoff); | ||
| } | ||
|
|
@@ -944,7 +1056,9 @@ export class QQChannel extends ChannelBase { | |
| isMentioned: true, | ||
| isReplyToBot: false, | ||
| }).catch((e) => | ||
| process.stderr.write(`[QQ:${this.name}] C2C handler error: ${e}\n`), | ||
| process.stderr.write( | ||
| `[QQ:${this.name}] C2C handler error: ${sanitizeLogText(e instanceof Error ? e.message : String(e), 200)}\n`, | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -1013,7 +1127,9 @@ export class QQChannel extends ChannelBase { | |
| isReplyToBot: true, | ||
| ...(isSlash ? {} : { alreadyPrefixed: true as const }), | ||
| }).catch((e) => | ||
| process.stderr.write(`[QQ:${this.name}] Group handler error: ${e}\n`), | ||
| process.stderr.write( | ||
| `[QQ:${this.name}] Group handler error: ${sanitizeLogText(e instanceof Error ? e.message : String(e), 200)}\n`, | ||
| ), | ||
| ); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.