From f8b03acca378a11f14f9f8364e9b869fb475d3bc Mon Sep 17 00:00:00 2001 From: TIANYAO ZHANG <88520881+ZHANGTIANYAO1@users.noreply.github.com> Date: Sat, 3 Oct 2026 17:17:35 +0800 Subject: [PATCH] fix: fence profile updates and check TeamSpeak permission failures --- src/bot/profile.test.ts | 178 +++++++++++++++++++++++++++- src/bot/profile.ts | 210 ++++++++++++++++++---------------- src/ts-protocol/http-query.ts | 7 +- 3 files changed, 294 insertions(+), 101 deletions(-) diff --git a/src/bot/profile.test.ts b/src/bot/profile.test.ts index 2575e74..ad6433b 100644 --- a/src/bot/profile.test.ts +++ b/src/bot/profile.test.ts @@ -1,5 +1,7 @@ import { describe, it, expect, beforeEach, vi } from "vitest"; +import { Client, generateIdentity } from "@honeybbq/teamspeak-client"; import { BotProfileManager } from "./profile.js"; +import { TS6HttpQuery } from "../ts-protocol/http-query.js"; import type { TS3Client } from "../ts-protocol/client.js"; import type { QueuedSong } from "../audio/queue.js"; @@ -14,6 +16,9 @@ function makeMockTs(): TS3Client & { get clearCalls() { return clears; }, getHost: () => "127.0.0.1", getHttpQuery: () => null, + getClientId: () => 17, + getChannelId: () => 5n, + execCommand: vi.fn().mockResolvedValue(undefined), fileTransferInitUpload: vi.fn().mockResolvedValue({}), uploadFileData: vi.fn().mockImplementation(async (_h: any, _i: any, stream: any) => { const chunks: Buffer[] = []; @@ -213,7 +218,7 @@ describe("BotProfileManager channel description follows the bot (#159)", () => { ts.cid = 5n; (ts as any).getChannelId = () => ts.cid; channelEdits = () => - (ts.sendCommandNoWait as any).mock.calls + (ts.execCommand as any).mock.calls .map((c: any[]) => c[0] as string) .filter((cmd: string) => cmd.startsWith("channeledit")); }); @@ -264,3 +269,174 @@ describe("BotProfileManager channel description follows the bot (#159)", () => { expect(channelEdits()).toHaveLength(1); }); }); + +function deferred() { + let resolve!: (value: T) => void; + let reject!: (error: Error) => void; + const promise = new Promise((res, rej) => { resolve = res; reject = rej; }); + return { promise, resolve, reject }; +} + +function makeHttpProfile(partial: Partial = {}) { + const ts = makeMockTs() as any; + const state = { clid: 17, cid: 5n }; + const descriptions = new Map(); + const http = new TS6HttpQuery({ host: "127.0.0.1", port: 10080 }); + const request = vi.spyOn(http, "request").mockImplementation(async (_method, path, body) => { + if (path.includes("clientlist")) { + return { status: 200, body: { body: [{ clid: String(state.clid), cid: String(state.cid) }], status: { code: 0, message: "ok" } } }; + } + if (path.includes("channeledit")) descriptions.set(Number(body!.cid), String(body!.channel_description)); + return { status: 200, body: { status: { code: 0, message: "ok" } } }; + }); + ts.getHttpQuery = () => http; + ts.getClientId = () => state.clid; + ts.getChannelId = () => state.cid; + const logger: any = { child: () => logger, info: vi.fn(), debug: vi.fn(), warn: vi.fn(), error: vi.fn() }; + const pm = new BotProfileManager(ts, logger, { ...cfgOff, ...partial }, "Bot"); + return { pm, ts, state, http, request, descriptions, logger }; +} + +describe("BotProfileManager checked TS6 profile lifecycle", () => { + it("clears the old channel through HTTP Query when moved, even without full-client edit permission", async () => { + const { pm, ts, state, descriptions } = makeHttpProfile({ channelDescEnabled: true }); + ts.execCommand.mockRejectedValue(new Error("insufficient client permissions")); + await pm.onSongChange(fakeSong); + expect(descriptions.get(5)).toContain("X - Y"); + state.cid = 9n; + await pm.onChannelMoved(9n); + expect(descriptions.get(5)).toBe(""); + expect(descriptions.get(9)).toContain("X - Y"); + expect(ts.sendCommandNoWait).not.toHaveBeenCalled(); + expect(ts.execCommand).not.toHaveBeenCalled(); + }); + + it("discards an old client-list reply after reconnect", async () => { + const { pm, state, request } = makeHttpProfile({ channelDescEnabled: true }); + state.cid = 0n; + const lookup = deferred(); + request.mockImplementationOnce(() => lookup.promise); + const update = pm.onSongChange(fakeSong); + await flush(); + state.clid = 21; + state.cid = 9n; + pm.onConnect(); + lookup.resolve({ status: 200, body: { body: [{ clid: "17", cid: "5" }], status: { code: 0, message: "ok" } } }); + await update; + expect(request.mock.calls.filter((call) => call[1].includes("channeledit"))).toEqual([]); + await pm.onSongChange(null); + expect(request).toHaveBeenLastCalledWith("POST", "/1/channeledit?sid=1", { cid: 9, channel_description: "" }); + }); + + it("does not restore the previous remembered channel when a write completes after reconnect", async () => { + const { pm, state, request } = makeHttpProfile({ channelDescEnabled: true }); + const write = deferred(); + request.mockImplementationOnce(() => write.promise); + const update = pm.onSongChange(fakeSong); + await flush(); + state.clid = 21; + state.cid = 9n; + pm.onConnect(); + write.resolve({ status: 200, body: { status: { code: 0, message: "ok" } } }); + await update; + await pm.onSongChange(null); + expect(request).toHaveBeenLastCalledWith("POST", "/1/channeledit?sid=1", { cid: 9, channel_description: "" }); + }); + + it("discards a pending channel lookup when playback stops", async () => { + const { pm, ts, request, descriptions } = makeHttpProfile({ channelDescEnabled: true }); + ts.getChannelId = () => 0n; + const lookup = deferred(); + request.mockImplementationOnce(() => lookup.promise); + const update = pm.onSongChange(fakeSong); + await flush(); + await pm.onSongChange(null); + lookup.resolve({ status: 200, body: { body: [{ clid: "17", cid: "5" }], status: { code: 0, message: "ok" } } }); + await update; + expect(descriptions.get(5)).toBe(""); + }); + + it("discards a pending channel lookup when the bot is moved", async () => { + const { pm, ts, request, descriptions } = makeHttpProfile({ channelDescEnabled: true }); + ts.getChannelId = () => 0n; + const lookup = deferred(); + request.mockImplementationOnce(() => lookup.promise); + const update = pm.onSongChange(fakeSong); + await flush(); + await pm.onChannelMoved(9n); + lookup.resolve({ status: 200, body: { body: [{ clid: "17", cid: "5" }], status: { code: 0, message: "ok" } } }); + await update; + expect(descriptions.has(5)).toBe(false); + expect(descriptions.get(9)).toContain("X - Y"); + }); + + it("does not disable the new connection after an old write returns a permission failure", async () => { + const { pm, state, request } = makeHttpProfile({ channelDescEnabled: true }); + const write = deferred(); + request.mockImplementationOnce(() => write.promise); + const update = pm.onSongChange(fakeSong); + await flush(); + state.clid = 21; + state.cid = 9n; + pm.onConnect(); + write.resolve({ status: 403, body: { status: { code: 2568, message: "insufficient client permissions" } } }); + await update; + await pm.onSongChange(fakeSong); + expect(request).toHaveBeenLastCalledWith("POST", "/1/channeledit?sid=1", { cid: 9, channel_description: "♪ 正在播放: X - Y\n专辑: Z\n平台: netease" }); + }); + + it("resolves an unknown channel and sends raw newlines with one targeted description update", async () => { + const { pm, ts, state, request } = makeHttpProfile({ channelDescEnabled: true, descriptionEnabled: true }); + ts.getChannelId = () => 0n; + state.cid = 5n; + await pm.onSongChange(fakeSong); + expect(request.mock.calls.filter((call) => call[1].includes("clientedit"))).toEqual([ + ["POST", "/1/clientedit?sid=1", { clid: 17, client_description: "X - Y [Z]" }], + ]); + expect(request).toHaveBeenLastCalledWith("POST", "/1/channeledit?sid=1", { cid: 5, channel_description: "♪ 正在播放: X - Y\n专辑: Z\n平台: netease" }); + expect(ts.execCommand).not.toHaveBeenCalled(); + }); + + it("does not resolve or write a disconnected client", async () => { + const { pm, state, request } = makeHttpProfile({ channelDescEnabled: true, descriptionEnabled: true }); + state.clid = 0; + state.cid = 0n; + await pm.onSongChange(fakeSong); + expect(request).not.toHaveBeenCalled(); + }); + + it("reports HTTP lookup permission errors once and retries after reconnect", async () => { + const { pm, ts, request } = makeHttpProfile({ channelDescEnabled: true }); + ts.getChannelId = () => 0n; + request.mockResolvedValue({ status: 403, body: { status: { code: 2568, message: "insufficient client permissions" } } }); + await pm.onSongChange(fakeSong); + await pm.onSongChange(fakeSong); + expect(request).toHaveBeenCalledTimes(1); + pm.onConnect(); + await pm.onSongChange(fakeSong); + expect(request).toHaveBeenCalledTimes(2); + }); + + it("checks self clientupdate permission responses and retries only after reconnect", async () => { + const { pm, ts, request, logger } = makeHttpProfile({ nicknameEnabled: true, awayStatusEnabled: true }); + const client: any = new Client(generateIdentity(0), "127.0.0.1:9987", "Bot"); + const commands: string[] = []; + client.handler.sendPacket = vi.fn((_type, data: Buffer) => { + const command = data.toString(); + commands.push(command); + const returnCode = command.match(/return_code=(\d+)/)?.[1]; + client.handler.onPacket({ typeFlagged: 2, data: Buffer.from(`error id=2568 msg=insufficient\\sclient\\spermissions${returnCode ? ` return_code=${returnCode}` : ""}`) }); + }); + ts.sendCommandNoWait.mockImplementation((command: string) => client.sendCommandNoWait(command)); + ts.execCommand.mockImplementation((command: string) => client.execCommand(command)); + await pm.onSongChange(null); + await pm.onSongChange(null); + expect(commands).toHaveLength(1); + expect(commands[0]).toMatch(/^clientupdate client_nickname=Bot client_away=1 client_away_message=等待播放 return_code=\d+$/); + expect(request).not.toHaveBeenCalled(); + expect(logger.info.mock.calls.some((call: any[]) => call[1] === "Client properties updated (nickname + away)")).toBe(false); + pm.onConnect(); + await pm.onSongChange(null); + expect(commands).toHaveLength(2); + }); +}); diff --git a/src/bot/profile.ts b/src/bot/profile.ts index c29fd04..e0885e8 100644 --- a/src/bot/profile.ts +++ b/src/bot/profile.ts @@ -13,6 +13,13 @@ const AVATAR_MAX_BYTES = 200 * 1024; /** Timeout for file-transfer operations (upload / delete). */ const FILE_TRANSFER_TIMEOUT_MS = 6000; +interface ProfileUpdateContext { + generation: number; + channelGeneration: number; + clientId: number; + httpQuery: ReturnType; +} + /** * Manages the bot's TeamSpeak presence (avatar, description, nickname, * away status, channel description, now-playing messages). @@ -57,6 +64,8 @@ export class BotProfileManager { * the generation changed, a newer update has superseded them. */ private generation = 0; + /** Channel moves supersede channel writes without cancelling avatar work. */ + private channelGeneration = 0; constructor( tsClient: TS3Client, @@ -119,20 +128,25 @@ export class BotProfileManager { */ async onSongChange(song: QueuedSong | null): Promise { const gen = ++this.generation; + this.channelGeneration++; this.currentSong = song; + const context = this.createUpdateContext(); // 1. Avatar first — file transfer uses its own response tracker and // must run before sendCommandNoWait calls whose orphaned responses // could confuse the command matcher. await this.updateAvatar(song?.coverUrl ?? null, gen); - if (this.generation !== gen) return; // superseded + if (!this.isCurrentUpdate(context)) return; - // 2. Combined clientupdate (nickname + away in one fire-and-forget) - await this.updateClientProperties(song); + // 2. Checked clientupdate sent by the visible client itself. + await this.updateClientProperties(song, context); + if (!this.isCurrentUpdate(context)) return; // 3. Description (clientedit on TS3, httpQuery on TS6) - await this.updateDescription(song); - // 4. Channel description (fire-and-forget channeledit) - await this.updateChannelDescription(song); + await this.updateDescription(song, context); + if (!this.isCurrentUpdate(context)) return; + // 4. Checked channel description update. + await this.updateChannelDescription(song, context); + if (!this.isCurrentUpdate(context)) return; // 5. Now-playing chat message if (song) await this.sendNowPlayingMessage(song); } @@ -140,6 +154,7 @@ export class BotProfileManager { /** Reset permission-denied flags and bump generation on new connection. */ onConnect(): void { this.generation++; + this.channelGeneration++; this.currentSong = null; // Channel ids are per-server; never carry one across a (re)connect. this.channelDescCid = null; @@ -168,19 +183,20 @@ export class BotProfileManager { if (!this.config.channelDescEnabled || this.permDenied.channelDesc) return; const oldChannelId = this.channelDescCid; if (oldChannelId === newChannelId) return; + this.channelGeneration++; + const context = this.createUpdateContext(); + const song = this.currentSong; try { if (oldChannelId !== null) { - await this.tsClient.sendCommandNoWait( - `channeledit cid=${oldChannelId} channel_description=`, - ); + if (!await this.writeChannelDescription(oldChannelId, "", context)) return; this.channelDescCid = null; } } catch (err) { - this.handleFeatureError("channelDesc", err); + if (this.isCurrentChannelUpdate(context)) this.handleFeatureError("channelDesc", err); return; } - if (this.currentSong) { - await this.updateChannelDescription(this.currentSong, newChannelId); + if (song) { + await this.updateChannelDescription(song, context, newChannelId); } } @@ -286,18 +302,19 @@ export class BotProfileManager { } } - private async updateDescription(song: QueuedSong | null): Promise { + private async updateDescription(song: QueuedSong | null, context: ProfileUpdateContext): Promise { if (!this.config.descriptionEnabled || this.permDenied.description) return; + if (!this.isCurrentUpdate(context)) return; try { const text = song ? `${song.name} - ${song.artist} [${song.album}]` : ""; - const clid = this.tsClient.getClientId(); + const clid = context.clientId; if (clid <= 0) return; - const httpQuery = this.tsClient.getHttpQuery(); + const httpQuery = context.httpQuery; if (httpQuery) { // IMPORTANT: @@ -306,6 +323,7 @@ export class BotProfileManager { const result = await httpQuery.clientEdit(clid, { client_description: text, }); + if (!this.isCurrentUpdate(context)) return; this.logger.info( { status: result.status, clid }, @@ -318,27 +336,22 @@ export class BotProfileManager { ), 5000, ); + if (!this.isCurrentUpdate(context)) return; this.logger.info({ clid }, "Description updated"); } } catch (err) { - this.handleFeatureError("description", err); + if (this.isCurrentUpdate(context)) this.handleFeatureError("description", err); } } /** * Build and send a single `clientupdate` command that sets nickname - * and away status together, avoiding multiple round-trips that can - * cause command-queue timeouts on the TS3 protocol. - * - * Values are collected as raw strings/numbers. The TS6 HTTP path - * forwards them as JSON (the server expects real spaces, not `\s`); - * the TS3 wire path escapes them on the fly. Previously the code - * escaped upfront and then split the escaped string to build the - * JSON body, so TS6 received literal backslashes and silently - * rejected the update. + * and away status together. The full client sends this command on both + * TS3 and TS6, with a return code so permission failures are observable. */ - private async updateClientProperties(song: QueuedSong | null): Promise { + private async updateClientProperties(song: QueuedSong | null, context: ProfileUpdateContext): Promise { + if (!this.isCurrentUpdate(context) || context.clientId <= 0) return; const rawProps: Record = {}; // --- Nickname --- @@ -374,20 +387,23 @@ export class BotProfileManager { : `${key}=${value}`, ); - await this.tsClient.sendCommandNoWait( - `clientupdate ${parts.join(" ")}`, + await this.withTimeout( + this.tsClient.execCommand(`clientupdate ${parts.join(" ")}`), + 5000, ); + if (!this.isCurrentUpdate(context)) return; this.logger.info( { - clid: this.tsClient.getClientId(), + clid: context.clientId, props: Object.keys(rawProps), }, "Client properties updated (nickname + away)", ); } catch (err) { - this.handleFeatureError("nickname", err); - this.handleFeatureError("awayStatus", err); + if (!this.isCurrentUpdate(context)) return; + if (rawProps.client_nickname !== undefined) this.handleFeatureError("nickname", err); + if (rawProps.client_away !== undefined) this.handleFeatureError("awayStatus", err); } } @@ -438,28 +454,35 @@ export class BotProfileManager { private async updateChannelDescription( song: QueuedSong | null, + context: ProfileUpdateContext, targetChannelId?: bigint, ): Promise { if (!this.config.channelDescEnabled || this.permDenied.channelDesc) return; + if (!this.isCurrentChannelUpdate(context) || context.clientId <= 0) return; try { - let channelId = targetChannelId ?? this.tsClient.getChannelId(); + // A stop already knows which channel to clear if a write succeeded. + // Avoid a needless client-list lookup that could prevent that cleanup. + let channelId = !song && this.channelDescCid !== null + ? this.channelDescCid + : targetChannelId ?? this.tsClient.getChannelId(); // TS6 full-client may report channelID() as 0 even after the // visible music client has already joined a channel. // Fall back to HTTP Query and resolve our real clid -> cid. if (channelId === 0n) { - const httpQuery = this.tsClient.getHttpQuery(); - const clid = this.tsClient.getClientId(); + const httpQuery = context.httpQuery; + const clid = context.clientId; if (httpQuery && clid > 0) { const result = await httpQuery.clientList(); + if (!this.isCurrentChannelUpdate(context)) return; const payload = result.body as { body?: Array>; }; - const me = payload.body?.find( + const me = payload?.body?.find( (client) => Number(client.clid) === clid, ); @@ -478,43 +501,12 @@ export class BotProfileManager { } if (!song) { - const target = this.channelDescCid ?? channelId; - - if (target === 0n) return; - - const httpQuery = this.tsClient.getHttpQuery(); - - if (httpQuery) { - const result = await httpQuery.channelEdit(Number(target), { - channel_description: "", - }); - - this.logger.info( - { - status: result.status, - cid: target.toString(), - }, - "Channel description cleared", - ); - } else { - await this.withTimeout( - this.tsClient.execCommand( - `channeledit cid=${target} channel_description=`, - ), - 5000, - ); - - this.logger.info( - { cid: target.toString() }, - "Channel description cleared", - ); - } - - this.channelDescCid = null; + if (channelId <= 0n) return; + if (await this.writeChannelDescription(channelId, "", context)) this.channelDescCid = null; return; } - if (channelId === 0n) return; + if (channelId <= 0n) return; const lines = [ `♪ 正在播放: ${song.name} - ${song.artist}`, @@ -525,40 +517,41 @@ export class BotProfileManager { // HTTP Query uses a normal JSON string, so use real newlines here. const desc = lines.join("\n"); - const httpQuery = this.tsClient.getHttpQuery(); - - if (httpQuery) { - const result = await httpQuery.channelEdit(Number(channelId), { - channel_description: desc, - }); - - this.logger.info( - { - status: result.status, - cid: channelId.toString(), - }, - "Channel description updated", - ); - } else { - await this.withTimeout( - this.tsClient.execCommand( - `channeledit cid=${channelId} channel_description=${escapeTS3(desc)}`, - ), - 5000, - ); - - this.logger.info( - { cid: channelId.toString() }, - "Channel description updated", - ); - } - - this.channelDescCid = channelId; + if (await this.writeChannelDescription(channelId, desc, context)) this.channelDescCid = channelId; } catch (err) { - this.handleFeatureError("channelDesc", err); + if (this.isCurrentChannelUpdate(context)) this.handleFeatureError("channelDesc", err); } } + /** Both move cleanup and ordinary writes use the same checked transport. */ + private async writeChannelDescription( + channelId: bigint, + description: string, + context: ProfileUpdateContext, + ): Promise { + if (!this.isCurrentChannelUpdate(context)) return false; + let status: number | undefined; + if (context.httpQuery) { + const result = await context.httpQuery.channelEdit(Number(channelId), { + channel_description: description, + }); + status = result.status; + } else { + await this.withTimeout( + this.tsClient.execCommand( + `channeledit cid=${channelId} channel_description=${escapeTS3(description)}`, + ), + 5000, + ); + } + if (!this.isCurrentChannelUpdate(context)) return false; + this.logger.info( + { status, cid: channelId.toString() }, + description ? "Channel description updated" : "Channel description cleared", + ); + return true; + } + private async sendNowPlayingMessage(song: QueuedSong): Promise { if (!this.config.nowPlayingMsgEnabled || this.permDenied.nowPlayingMsg) return; try { @@ -571,6 +564,25 @@ export class BotProfileManager { // --- Helpers --- + private createUpdateContext(): ProfileUpdateContext { + return { + generation: this.generation, + channelGeneration: this.channelGeneration, + clientId: this.tsClient.getClientId(), + httpQuery: this.tsClient.getHttpQuery(), + }; + } + + private isCurrentUpdate(context: ProfileUpdateContext): boolean { + return context.generation === this.generation && + context.clientId === this.tsClient.getClientId() && + context.httpQuery === this.tsClient.getHttpQuery(); + } + + private isCurrentChannelUpdate(context: ProfileUpdateContext): boolean { + return this.isCurrentUpdate(context) && context.channelGeneration === this.channelGeneration; + } + /** * Append CDN resize parameters to get a thumbnail suitable for TS3 avatars. * NetEase and QQ Music CDNs support URL-based image resizing. diff --git a/src/ts-protocol/http-query.ts b/src/ts-protocol/http-query.ts index ec8f6cd..2a8373b 100644 --- a/src/ts-protocol/http-query.ts +++ b/src/ts-protocol/http-query.ts @@ -167,7 +167,12 @@ export class TS6HttpQuery { /** List clients on a virtual server */ async clientList(sid = 1): Promise { - return this.request("GET", `/1/clientlist?sid=${sid}`); + const path = `/1/clientlist?sid=${sid}`; + const result = await this.request("GET", path); + if (result.status < 200 || result.status >= 300) { + throw new HttpQueryError(path, result.status, result.body); + } + return result; } /** List channels on a virtual server */