diff --git a/CHANGELOG.md b/CHANGELOG.md index e4caf037dd4..686803c49d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,7 +32,7 @@ Docs: https://docs.openclaw.ai - Telegram: when `channels.telegram.commands.native` is `false`, exclude plugin commands from `setMyCommands` menu registration while keeping plugin slash handlers callable. (#15132) Thanks @Glucksberg. - Security/Agents: scope CLI process cleanup to owned child PIDs to avoid killing unrelated processes on shared hosts. Thanks @aether-ai-agent. - Security/Agents (macOS): prevent shell injection when writing Claude CLI keychain credentials. (#15924) Thanks @aether-ai-agent. -- Security: fix Chutes manual OAuth login state validation (thanks @aether-ai-agent). (#16058) +- Security: fix Chutes manual OAuth login state validation by requiring the full redirect URL (reject code-only pastes) (thanks @aether-ai-agent). - Security/Tlon: harden Urbit URL fetching against SSRF by blocking private/internal hosts by default (opt-in: `channels.tlon.allowPrivateNetwork`). Thanks @p80n-sec. - Security/Voice Call (Telnyx): require webhook signature verification when receiving inbound events; configs without `telnyx.publicKey` are now rejected unless `skipSignatureVerification` is enabled. Thanks @p80n-sec. - Security/Discovery: stop treating Bonjour TXT records as authoritative routing (prefer resolved service endpoints) and prevent discovery from overriding stored TLS pins; autoconnect now requires a previously trusted gateway. Thanks @simecek. diff --git a/src/agents/chutes-oauth.test.ts b/src/agents/chutes-oauth.test.ts index 26e99539d41..a9bc417f721 100644 --- a/src/agents/chutes-oauth.test.ts +++ b/src/agents/chutes-oauth.test.ts @@ -2,53 +2,51 @@ import { describe, expect, it } from "vitest"; import { generateChutesPkce, parseOAuthCallbackInput } from "./chutes-oauth.js"; describe("parseOAuthCallbackInput", () => { - const EXPECTED_STATE = "abc123def456"; + it("rejects code-only input (state required)", () => { + const parsed = parseOAuthCallbackInput("abc123", "expected-state"); + expect(parsed).toEqual({ + error: "Paste the full redirect URL (must include code + state).", + }); + }); - it("returns code and state for valid URL with matching state", () => { - const result = parseOAuthCallbackInput( - `http://localhost/cb?code=authcode_xyz&state=${EXPECTED_STATE}`, - EXPECTED_STATE, + it("accepts full redirect URL when state matches", () => { + const parsed = parseOAuthCallbackInput( + "http://127.0.0.1:1456/oauth-callback?code=abc123&state=expected-state", + "expected-state", ); - expect(result).toEqual({ code: "authcode_xyz", state: EXPECTED_STATE }); + expect(parsed).toEqual({ code: "abc123", state: "expected-state" }); }); - it("rejects URL with mismatched state (CSRF protection)", () => { - const result = parseOAuthCallbackInput( - "http://localhost/cb?code=authcode_xyz&state=attacker_state", - EXPECTED_STATE, + it("accepts querystring-only input when state matches", () => { + const parsed = parseOAuthCallbackInput("code=abc123&state=expected-state", "expected-state"); + expect(parsed).toEqual({ code: "abc123", state: "expected-state" }); + }); + + it("rejects missing state", () => { + const parsed = parseOAuthCallbackInput( + "http://127.0.0.1:1456/oauth-callback?code=abc123", + "expected-state", ); - expect(result).toHaveProperty("error"); - expect((result as { error: string }).error).toMatch(/state mismatch/i); + expect(parsed).toEqual({ + error: "Missing 'state' parameter. Paste the full redirect URL.", + }); }); - it("accepts bare code input for manual flow", () => { - const result = parseOAuthCallbackInput("bare_auth_code", EXPECTED_STATE); - expect(result).toEqual({ code: "bare_auth_code", state: EXPECTED_STATE }); - }); - - it("rejects empty input", () => { - const result = parseOAuthCallbackInput("", EXPECTED_STATE); - expect(result).toEqual({ error: "No input provided" }); - }); - - it("rejects URL missing code parameter", () => { - const result = parseOAuthCallbackInput( - `http://localhost/cb?state=${EXPECTED_STATE}`, - EXPECTED_STATE, + it("rejects state mismatch", () => { + const parsed = parseOAuthCallbackInput( + "http://127.0.0.1:1456/oauth-callback?code=abc123&state=evil", + "expected-state", ); - expect(result).toHaveProperty("error"); - }); - - it("rejects URL missing state parameter", () => { - const result = parseOAuthCallbackInput("http://localhost/cb?code=authcode_xyz", EXPECTED_STATE); - expect(result).toHaveProperty("error"); + expect(parsed).toEqual({ + error: "OAuth state mismatch - possible CSRF attack. Please retry login.", + }); }); }); describe("generateChutesPkce", () => { - it("returns verifier and challenge strings", () => { + it("returns verifier and challenge", () => { const pkce = generateChutesPkce(); expect(pkce.verifier).toMatch(/^[0-9a-f]{64}$/); - expect(pkce.challenge).toBeTruthy(); + expect(pkce.challenge).toMatch(/^[A-Za-z0-9_-]+$/); }); }); diff --git a/src/agents/chutes-oauth.ts b/src/agents/chutes-oauth.ts index da36f7506a0..1b730593d22 100644 --- a/src/agents/chutes-oauth.ts +++ b/src/agents/chutes-oauth.ts @@ -42,28 +42,42 @@ export function parseOAuthCallbackInput( return { error: "No input provided" }; } + // Manual flow must validate CSRF state; require URL (or querystring) that includes `state`. + let url: URL; try { - const url = new URL(trimmed); - const code = url.searchParams.get("code"); - const state = url.searchParams.get("state"); - if (!code) { - return { error: "Missing 'code' parameter in URL" }; - } - if (!state) { - return { error: "Missing 'state' parameter. Paste the full URL (or just the code)." }; - } - if (state !== expectedState) { - return { error: "OAuth state mismatch - possible CSRF attack. Please retry login." }; - } - return { code, state }; + url = new URL(trimmed); } catch { - // Manual flow: users often paste only the authorization code. - // In that case we can't validate state, but the user is explicitly opting in by pasting it. - if (!/\s/.test(trimmed) && !trimmed.includes("://") && trimmed.length > 0) { - return { code: trimmed, state: expectedState }; + // Code-only paste (common) is no longer accepted because it defeats state validation. + if ( + !/\s/.test(trimmed) && + !trimmed.includes("://") && + !trimmed.includes("?") && + !trimmed.includes("=") + ) { + return { error: "Paste the full redirect URL (must include code + state)." }; + } + + // Users sometimes paste only the query string: `?code=...&state=...` or `code=...&state=...` + const qs = trimmed.startsWith("?") ? trimmed : `?${trimmed}`; + try { + url = new URL(`http://localhost/${qs}`); + } catch { + return { error: "Paste the full redirect URL (must include code + state)." }; } - return { error: "Paste the redirect URL (or authorization code)." }; } + + const code = url.searchParams.get("code")?.trim(); + const state = url.searchParams.get("state")?.trim(); + if (!code) { + return { error: "Missing 'code' parameter in URL" }; + } + if (!state) { + return { error: "Missing 'state' parameter. Paste the full redirect URL." }; + } + if (state !== expectedState) { + return { error: "OAuth state mismatch - possible CSRF attack. Please retry login." }; + } + return { code, state }; } function coerceExpiresAt(expiresInSeconds: number, now: number): number { diff --git a/src/commands/auth-choice.e2e.test.ts b/src/commands/auth-choice.e2e.test.ts index 9bdf233f526..ede6ce190dc 100644 --- a/src/commands/auth-choice.e2e.test.ts +++ b/src/commands/auth-choice.e2e.test.ts @@ -1127,7 +1127,7 @@ describe("applyAuthChoice", () => { expect(text).toHaveBeenCalledWith( expect.objectContaining({ - message: "Paste the redirect URL (or authorization code)", + message: "Paste the redirect URL", }), ); expect(result.config.auth?.profiles?.["chutes:remote-user"]).toMatchObject({ diff --git a/src/commands/chutes-oauth.ts b/src/commands/chutes-oauth.ts index 1925649bb4a..161ae621db0 100644 --- a/src/commands/chutes-oauth.ts +++ b/src/commands/chutes-oauth.ts @@ -156,7 +156,7 @@ export async function loginChutes(params: { await params.onAuth({ url }); params.onProgress?.("Waiting for redirect URL…"); const input = await params.onPrompt({ - message: "Paste the redirect URL (or authorization code)", + message: "Paste the redirect URL", placeholder: `${params.app.redirectUri}?code=...&state=...`, }); const parsed = parseOAuthCallbackInput(String(input), state); @@ -176,7 +176,7 @@ export async function loginChutes(params: { }).catch(async () => { params.onProgress?.("OAuth callback not detected; paste redirect URL…"); const input = await params.onPrompt({ - message: "Paste the redirect URL (or authorization code)", + message: "Paste the redirect URL", placeholder: `${params.app.redirectUri}?code=...&state=...`, }); const parsed = parseOAuthCallbackInput(String(input), state); diff --git a/src/commands/oauth-flow.ts b/src/commands/oauth-flow.ts index 77ea367c9ab..1b0eba3b4f8 100644 --- a/src/commands/oauth-flow.ts +++ b/src/commands/oauth-flow.ts @@ -17,8 +17,7 @@ export function createVpsAwareOAuthHandlers(params: { onAuth: (event: { url: string }) => Promise; onPrompt: (prompt: OAuthPrompt) => Promise; } { - const manualPromptMessage = - params.manualPromptMessage ?? "Paste the redirect URL (or authorization code)"; + const manualPromptMessage = params.manualPromptMessage ?? "Paste the redirect URL"; let manualCodePromise: Promise | undefined; return {