From 8ab6966038ff9ab7acf04f0d42057cc0edafd02f Mon Sep 17 00:00:00 2001 From: jay79-boop Date: Mon, 3 Aug 2026 12:51:44 -0500 Subject: [PATCH] Fix WhatsApp accountId path traversal in login-status/login resolveWhatsAppCredentialCandidatePaths() joins accountId straight into a credentials file path (OPENCLAW_DIR/credentials/whatsapp// creds.json). normalizeChannelAccountId() -- the only normalization applied to it -- just trims and defaults to "default"; it never rejects `../`. Three call sites feed it accountId values with no validation: - GET /api/channels/accounts/login-status (routes/agents.js): accountId comes straight from req.query.accountId - POST /api/channels/accounts/login: same, from req.body.accountId - readPairedCountsByAccount (gateway.js, channel status reporting): accountId comes from the *keys* of the live openclaw.json config's channels.whatsapp.accounts object -- which, for an imported workspace, is promoted directly from whatever the imported repo's config contained hasSavedWhatsAppCredentials() reads whatever file resolves and returns whether it exists and is non-empty -- so accountId=../../../../etc/shadow turns the (authenticated) login-status endpoint into a boolean exists-and-non-empty oracle for arbitrary host files, reaching well outside the OPENCLAW_DIR sandbox the Browse routes otherwise enforce carefully for this same admin. This codebase already has the right validator for exactly this field -- isValidChannelAccountId() (kChannelAccountIdPattern, lowercase/digits/ hyphens only) -- and createChannelAccount() already uses it. The two sibling functions, runChannelAccountLogin() and getChannelAccountLoginStatus(), just never got the same check. Fixed at both layers: - agents/channels.js: runChannelAccountLogin() and getChannelAccountLoginStatus() now validate accountId with isValidChannelAccountId() before doing anything else, matching createChannelAccount()'s existing behavior. - agents/shared.js: resolveWhatsAppCredentialCandidatePaths() also filters its own candidate paths to stay within the credentials directory, closing the config-derived path (readPairedCountsByAccount) that doesn't go through the two HTTP-facing validators above. Added tests: agents-service.test.js covers both HTTP-facing rejections (with an assertion that readFileSync is never reached), and a new agents-shared.test.js exercises the containment check directly with a real external file, verified locally to fail without the fix (the external file was reported as "linked") and pass with it. --- lib/server/agents/channels.js | 14 +++++ lib/server/agents/shared.js | 16 +++++- tests/server/agents-service.test.js | 37 ++++++++++++++ tests/server/agents-shared.test.js | 79 +++++++++++++++++++++++++++++ 4 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 tests/server/agents-shared.test.js diff --git a/lib/server/agents/channels.js b/lib/server/agents/channels.js index 6ca8f198..5cca0169 100644 --- a/lib/server/agents/channels.js +++ b/lib/server/agents/channels.js @@ -1026,6 +1026,11 @@ const createChannelsDomain = ({ throw new Error("Channel login is currently only supported for WhatsApp"); } const accountId = String(rawAccountId || "").trim() || "default"; + if (!isValidChannelAccountId(accountId)) { + throw new Error( + "Channel account id must be lowercase letters, numbers, and hyphens only", + ); + } const loginArgs = [ "channels login", `--channel ${shellEscapeArg(provider)}`, @@ -1060,6 +1065,15 @@ const createChannelsDomain = ({ throw new Error("Channel login status is currently only supported for WhatsApp"); } const accountId = String(rawAccountId || "").trim() || "default"; + // accountId reaches hasSavedWhatsAppCredentials() below, which joins it + // straight into a credentials file path -- reject anything shaped like a + // path (e.g. `../../../../etc/shadow`) before it gets there, same as + // createChannelAccount already does for account creation. + if (!isValidChannelAccountId(accountId)) { + throw new Error( + "Channel account id must be lowercase letters, numbers, and hyphens only", + ); + } return { provider, accountId, diff --git a/lib/server/agents/shared.js b/lib/server/agents/shared.js index 86b4a6a9..b61a4993 100644 --- a/lib/server/agents/shared.js +++ b/lib/server/agents/shared.js @@ -49,18 +49,32 @@ const shellEscapeArg = (value) => const resolveCredentialsDirPath = ({ OPENCLAW_DIR }) => path.join(OPENCLAW_DIR, "credentials"); +const isWithinDir = (rootDir, candidatePath) => { + const resolvedRoot = path.resolve(rootDir); + const resolvedCandidate = path.resolve(candidatePath); + return ( + resolvedCandidate === resolvedRoot || + resolvedCandidate.startsWith(`${resolvedRoot}${path.sep}`) + ); +}; + const resolveWhatsAppCredentialCandidatePaths = ({ OPENCLAW_DIR, accountId, }) => { const credentialsDir = resolveCredentialsDirPath({ OPENCLAW_DIR }); + // normalizeChannelAccountId only trims/defaults -- it doesn't reject `../`, + // and accountId can come straight from a query param (login-status route) + // or a config-supplied account key (e.g. an imported openclaw.json), so + // guard the actual filesystem paths here regardless of what already + // validated (or didn't) upstream. const normalizedAccountId = normalizeChannelAccountId(accountId); return [ path.join(credentialsDir, "whatsapp", normalizedAccountId, "creds.json"), ...(normalizedAccountId === "default" ? [path.join(credentialsDir, "creds.json")] : []), - ]; + ].filter((candidatePath) => isWithinDir(credentialsDir, candidatePath)); }; const hasSavedWhatsAppCredentials = ({ diff --git a/tests/server/agents-service.test.js b/tests/server/agents-service.test.js index 5d699862..eecb0caf 100644 --- a/tests/server/agents-service.test.js +++ b/tests/server/agents-service.test.js @@ -1954,6 +1954,43 @@ describe("server/agents/service", () => { }); }); + it("rejects a path-traversal-shaped accountId for whatsapp login status", () => { + const fsMock = buildFsMock({ initialConfig: {} }); + const service = createAgentsService({ + fs: fsMock, + OPENCLAW_DIR: "/test/.openclaw", + }); + + expect(() => + service.getChannelAccountLoginStatus({ + provider: "whatsapp", + accountId: "../../../../etc/shadow", + }), + ).toThrow(/lowercase letters, numbers, and hyphens/); + // Must reject before ever touching the filesystem. + expect(fsMock.readFileSync).not.toHaveBeenCalled(); + }); + + it("rejects a path-traversal-shaped accountId for whatsapp login", async () => { + const fsMock = buildFsMock({ initialConfig: {} }); + const service = createAgentsService({ + fs: fsMock, + OPENCLAW_DIR: "/test/.openclaw", + readEnvFile: vi.fn(() => []), + writeEnvFile: vi.fn(), + reloadEnv: vi.fn(), + restartGateway: vi.fn(async () => {}), + clawCmd: vi.fn(async () => ({ ok: true })), + }); + + await expect( + service.runChannelAccountLogin({ + provider: "whatsapp", + accountId: "../../../../etc/shadow", + }), + ).rejects.toThrow(/lowercase letters, numbers, and hyphens/); + }); + it("rejects channel login for non-whatsapp providers", async () => { const fsMock = buildFsMock({ initialConfig: {} }); const service = createAgentsService({ diff --git a/tests/server/agents-shared.test.js b/tests/server/agents-shared.test.js new file mode 100644 index 00000000..2232f0ab --- /dev/null +++ b/tests/server/agents-shared.test.js @@ -0,0 +1,79 @@ +const fs = require("fs"); +const os = require("os"); +const path = require("path"); + +const { + resolveWhatsAppCredentialCandidatePaths, + hasSavedWhatsAppCredentials, +} = require("../../lib/server/agents/shared"); + +const kTempDirs = []; +const createTempDir = () => { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "alphaclaw-agents-shared-")); + kTempDirs.push(tempDir); + return tempDir; +}; +afterEach(() => { + while (kTempDirs.length > 0) { + fs.rmSync(kTempDirs.pop(), { recursive: true, force: true }); + } +}); + +describe("agents/shared WhatsApp credential path containment", () => { + it("never returns a candidate path outside the credentials directory for a traversal-shaped accountId", () => { + const OPENCLAW_DIR = createTempDir(); + + const candidates = resolveWhatsAppCredentialCandidatePaths({ + OPENCLAW_DIR, + accountId: "../../../../etc/shadow", + }); + + const credentialsDir = path.join(OPENCLAW_DIR, "credentials"); + for (const candidate of candidates) { + expect( + candidate === credentialsDir || + candidate.startsWith(`${credentialsDir}${path.sep}`), + ).toBe(true); + } + }); + + it("does not report a real file outside OPENCLAW_DIR as linked WhatsApp credentials", () => { + const OPENCLAW_DIR = createTempDir(); + const outsideDir = createTempDir(); + + // A real, non-empty file named exactly like what the function looks for + // (creds.json), sitting outside OPENCLAW_DIR entirely -- stands in for + // some unrelated real file the accountId traversal happens to reach. + fs.writeFileSync(path.join(outsideDir, "creds.json"), "root:x:0:0\n", "utf8"); + + // accountId such that OPENCLAW_DIR/credentials/whatsapp//creds.json + // resolves to outsideDir/creds.json. + const accountId = path + .relative(path.join(OPENCLAW_DIR, "credentials", "whatsapp"), outsideDir) + .split(path.sep) + .join("/"); + + const linked = hasSavedWhatsAppCredentials({ + fsImpl: fs, + OPENCLAW_DIR, + accountId, + }); + + expect(linked).toBe(false); + }); + + it("still reports linked when real, in-directory WhatsApp credentials exist", () => { + const OPENCLAW_DIR = createTempDir(); + const credsDir = path.join(OPENCLAW_DIR, "credentials", "whatsapp", "default"); + fs.mkdirSync(credsDir, { recursive: true }); + fs.writeFileSync(path.join(credsDir, "creds.json"), "{}", "utf8"); + + const linked = hasSavedWhatsAppCredentials({ + fsImpl: fs, + OPENCLAW_DIR, + accountId: "default", + }); + + expect(linked).toBe(true); + }); +});