From 5762cc321affd57e15bd2fdc69279e3de365b527 Mon Sep 17 00:00:00 2001 From: "openclaw-clownfish[bot]" <280122609+openclaw-clownfish[bot]@users.noreply.github.com> Date: Wed, 29 Apr 2026 01:31:49 -0700 Subject: [PATCH] Improve pairing diagnostics without unsafe formatting (#73933) Co-authored-by: openclaw-clownfish[bot] <280122609+openclaw-clownfish[bot]@users.noreply.github.com> --- src/pairing/allow-from-store-file.test.ts | 67 ++++++++++++++++++++ src/pairing/allow-from-store-file.ts | 75 +++++++++++++++++------ src/pairing/pairing-store.test.ts | 26 ++++++++ src/pairing/pairing-store.ts | 7 ++- 4 files changed, 155 insertions(+), 20 deletions(-) create mode 100644 src/pairing/allow-from-store-file.test.ts diff --git a/src/pairing/allow-from-store-file.test.ts b/src/pairing/allow-from-store-file.test.ts new file mode 100644 index 00000000000..31341d675e4 --- /dev/null +++ b/src/pairing/allow-from-store-file.test.ts @@ -0,0 +1,67 @@ +import { describe, expect, it } from "vitest"; +import { + resolveAllowFromAccountId, + resolveAllowFromFilePath, + safeChannelKey, +} from "./allow-from-store-file.js"; +import type { PairingChannel } from "./pairing-store.types.js"; + +function expectInvalidPairingKey(params: { + run: () => unknown; + message: string; + leaked?: string; +}): void { + try { + params.run(); + } catch (err) { + expect(err).toBeInstanceOf(Error); + const message = (err as Error).message; + expect(message).toBe(params.message); + if (params.leaked) { + expect(message).not.toContain(params.leaked); + } + return; + } + throw new Error("expected invalid pairing key error"); +} + +describe("allow-from store file keys", () => { + it("formats invalid channel diagnostics without stringifying unsafe values", () => { + const circular: Record = { label: "private-channel-value" }; + circular.self = circular; + + expectInvalidPairingKey({ + run: () => safeChannelKey(circular as unknown as PairingChannel), + message: "invalid pairing channel: expected non-empty string; got object", + leaked: "private-channel-value", + }); + }); + + it("formats invalid account diagnostics without stringifying unsafe values", () => { + expectInvalidPairingKey({ + run: () => resolveAllowFromFilePath("telegram", process.env, 10n as unknown as string), + message: "invalid pairing account id: expected non-empty string; got bigint", + leaked: "10", + }); + + expectInvalidPairingKey({ + run: () => resolveAllowFromAccountId(10n as unknown as string), + message: "invalid pairing account id: expected non-empty string; got bigint", + leaked: "10", + }); + }); + + it("reports sanitized-empty filename keys without exposing the raw key", () => { + expectInvalidPairingKey({ + run: () => safeChannelKey(".." as PairingChannel), + message: "invalid pairing channel: sanitized filename key is empty; got string length 2", + leaked: "..", + }); + + expectInvalidPairingKey({ + run: () => resolveAllowFromFilePath("telegram", process.env, "/" as string), + message: "invalid pairing account id: sanitized filename key is empty; got string length 1", + leaked: "/", + }); + }); +}); diff --git a/src/pairing/allow-from-store-file.ts b/src/pairing/allow-from-store-file.ts index af62a1f006a..fe9c5feab82 100644 --- a/src/pairing/allow-from-store-file.ts +++ b/src/pairing/allow-from-store-file.ts @@ -34,29 +34,68 @@ export function resolvePairingCredentialsDir(env: NodeJS.ProcessEnv = process.en return resolveOAuthDir(env, stateDir); } -/** Sanitize channel ID for use in filenames (prevent path traversal). */ -export function safeChannelKey(channel: PairingChannel): string { - const raw = normalizeLowercaseStringOrEmpty(String(channel)); +type PairingFilenameKeyKind = "channel" | "account id"; + +function describePairingFilenameKeyInput(value: unknown): string { + if (value === null) { + return "null"; + } + if (Array.isArray(value)) { + return "array"; + } + if (typeof value === "string") { + const trimmed = value.trim(); + return trimmed ? `string length ${trimmed.length}` : "empty string"; + } + if (typeof value === "number" && !Number.isFinite(value)) { + return "non-finite number"; + } + return typeof value; +} + +function invalidPairingFilenameKeyError( + kind: PairingFilenameKeyKind, + reason: string, + value: unknown, +): Error { + return new Error( + `invalid pairing ${kind}: ${reason}; got ${describePairingFilenameKeyInput(value)}`, + ); +} + +function normalizePairingFilenameKey(value: unknown, kind: PairingFilenameKeyKind): string { + if (typeof value !== "string") { + throw invalidPairingFilenameKeyError(kind, "expected non-empty string", value); + } + const raw = normalizeLowercaseStringOrEmpty(value); if (!raw) { - throw new Error("invalid pairing channel"); + throw invalidPairingFilenameKeyError(kind, "expected non-empty string", value); } const safe = raw.replace(/[\\/:*?"<>|]/g, "_").replace(/\.\./g, "_"); if (!safe || safe === "_") { - throw new Error("invalid pairing channel"); + throw invalidPairingFilenameKeyError(kind, "sanitized filename key is empty", value); } return safe; } +/** Sanitize channel ID for use in filenames (prevent path traversal). */ +export function safeChannelKey(channel: PairingChannel): string { + return normalizePairingFilenameKey(channel, "channel"); +} + function safeAccountKey(accountId: string): string { - const raw = normalizeLowercaseStringOrEmpty(accountId); - if (!raw) { - throw new Error("invalid pairing account id"); + return normalizePairingFilenameKey(accountId, "account id"); +} + +function resolveOptionalAccountFilenameKey(accountId: unknown): string | null { + if (accountId == null) { + return null; } - const safe = raw.replace(/[\\/:*?"<>|]/g, "_").replace(/\.\./g, "_"); - if (!safe || safe === "_") { - throw new Error("invalid pairing account id"); + if (typeof accountId !== "string") { + throw invalidPairingFilenameKeyError("account id", "expected non-empty string", accountId); } - return safe; + const normalizedAccountId = normalizeOptionalString(accountId) ?? ""; + return normalizedAccountId ? safeAccountKey(normalizedAccountId) : null; } export function resolveAllowFromFilePath( @@ -65,14 +104,11 @@ export function resolveAllowFromFilePath( accountId?: string, ): string { const base = safeChannelKey(channel); - const normalizedAccountId = normalizeOptionalString(accountId) ?? ""; - if (!normalizedAccountId) { + const accountKey = resolveOptionalAccountFilenameKey(accountId); + if (!accountKey) { return path.join(resolvePairingCredentialsDir(env), `${base}-allowFrom.json`); } - return path.join( - resolvePairingCredentialsDir(env), - `${base}-${safeAccountKey(normalizedAccountId)}-allowFrom.json`, - ); + return path.join(resolvePairingCredentialsDir(env), `${base}-${accountKey}-allowFrom.json`); } export function dedupePreserveOrder(entries: string[]): string[] { @@ -94,6 +130,9 @@ export function shouldIncludeLegacyAllowFromEntries(normalizedAccountId: string) } export function resolveAllowFromAccountId(accountId?: string): string { + if (accountId != null && typeof accountId !== "string") { + throw invalidPairingFilenameKeyError("account id", "expected non-empty string", accountId); + } return normalizeLowercaseStringOrEmpty(accountId) || DEFAULT_ACCOUNT_ID; } diff --git a/src/pairing/pairing-store.test.ts b/src/pairing/pairing-store.test.ts index 94a4238da29..8242748c239 100644 --- a/src/pairing/pairing-store.test.ts +++ b/src/pairing/pairing-store.test.ts @@ -424,6 +424,32 @@ describe("pairing store", () => { }); }); + it("reports unique code exhaustion without exposing reserved codes", async () => { + await withTempStateDir(async () => { + await withMockRandomInt({ + initialValue: 0, + run: async () => { + const first = await upsertChannelPairingRequest({ + channel: "telegram", + id: "123", + accountId: DEFAULT_ACCOUNT_ID, + }); + expect(first.code).toBe("AAAAAAAA"); + + await expect( + upsertChannelPairingRequest({ + channel: "telegram", + id: "456", + accountId: DEFAULT_ACCOUNT_ID, + }), + ).rejects.toThrow( + "failed to generate unique pairing code after 500 attempts; existing code count: 1", + ); + }, + }); + }); + }); + it("keeps allowFrom account-scoped across manual and pairing-code approvals", async () => { await withTempStateDir(async () => { await addChannelAllowFromStoreEntry({ diff --git a/src/pairing/pairing-store.ts b/src/pairing/pairing-store.ts index d7e6c3a17dd..ed923e8f7a9 100644 --- a/src/pairing/pairing-store.ts +++ b/src/pairing/pairing-store.ts @@ -30,6 +30,7 @@ export type { PairingChannel } from "./pairing-store.types.js"; const PAIRING_CODE_LENGTH = 8; const PAIRING_CODE_ALPHABET = "ABCDEFGHJKLMNPQRSTUVWXYZ23456789"; +const PAIRING_CODE_MAX_ATTEMPTS = 500; const PAIRING_PENDING_TTL_MS = 60 * 60 * 1000; const PAIRING_PENDING_MAX = 3; const PAIRING_STORE_LOCK_OPTIONS = { @@ -201,13 +202,15 @@ function randomCode(): string { } function generateUniqueCode(existing: Set): string { - for (let attempt = 0; attempt < 500; attempt += 1) { + for (let attempt = 0; attempt < PAIRING_CODE_MAX_ATTEMPTS; attempt += 1) { const code = randomCode(); if (!existing.has(code)) { return code; } } - throw new Error("failed to generate unique pairing code"); + throw new Error( + `failed to generate unique pairing code after ${PAIRING_CODE_MAX_ATTEMPTS} attempts; existing code count: ${existing.size}`, + ); } function normalizePairingAccountId(accountId?: string): string {