diff --git a/src/client/hub-client.ts b/src/client/hub-client.ts index 47604e33982..8af0426082c 100644 --- a/src/client/hub-client.ts +++ b/src/client/hub-client.ts @@ -6,14 +6,8 @@ import { clearableDeadline } from "../lib/abort"; import type { Desktop3pModelEntry } from "../claude/desktop-3p"; import { assertDesktop3pModelsValid } from "../claude/desktop-3p-guard"; -/** - * A pairing grant may cross loopback or authenticated HTTPS, and nothing else. - * - * Mirrors the hub-side rule in src/server/gui-session.ts. Checking here too is not - * redundant: it keeps the client from spending a single-use code on a request the hub is - * certain to refuse. - */ -function isPairingTransportPermitted(origin: string): boolean { +/** Hub traffic may cross loopback or authenticated HTTPS, and nothing else. */ +function isHubTransportPermitted(origin: string): boolean { let url: URL; try { url = new URL(origin); @@ -193,6 +187,12 @@ export function normalizeHubOrigin(input: string): string { "Hub URL must be an HTTP(S) origin without credentials, query, fragment, or non-/v1 path", ); } + if (!isHubTransportPermitted(parsed.origin)) { + throw new HubClientError( + "insecure_http_refused", + "Hub URLs require loopback or HTTPS; plaintext remote HTTP is not permitted", + ); + } return parsed.origin; } @@ -252,7 +252,7 @@ export async function exchangeConnectPairingGrant( // Deliberateness is not the control that matters: the grant is readable by anything on the // path and the session it mints is reusable. The hub refuses this exchange outright now, so // sending it would only burn a single-use code against a certain rejection. - if (!isPairingTransportPermitted(origin)) { + if (!isHubTransportPermitted(origin)) { throw new HubClientError("insecure_http_refused", "Pairing requires loopback or HTTPS; plaintext HTTP cannot carry a grant"); } const response = await fetchBounded(options.fetchImpl ?? fetch, `${origin}/opencodex-session`, { @@ -481,7 +481,7 @@ export async function fetchHubUsage( options: { timeoutMs?: number; fetchImpl?: typeof fetch } = {}, ): Promise { const origin = normalizeHubOrigin(serverUrl); - if (!isPairingTransportPermitted(origin)) { + if (!isHubTransportPermitted(origin)) { throw new HubClientError("insecure_http_refused", "Client usage requires HTTPS or loopback HTTP"); } const response = await fetchBounded(options.fetchImpl ?? fetch, `${origin}/v1/usage?${query}`, { @@ -593,7 +593,7 @@ export async function downloadDesktop3pModels( options: { timeoutMs?: number; fetchImpl?: typeof fetch } = {}, ): Promise<{ version: 1; models: Desktop3pModelEntry[] }> { const origin = normalizeHubOrigin(serverUrl); - if (!isPairingTransportPermitted(origin)) { + if (!isHubTransportPermitted(origin)) { throw new HubClientError("insecure_http_refused", "Desktop model snapshots require HTTPS or loopback HTTP"); } try { diff --git a/src/providers/alibaba-region-backup.ts b/src/providers/alibaba-region-backup.ts index d200e3df1bf..933c3037a25 100644 --- a/src/providers/alibaba-region-backup.ts +++ b/src/providers/alibaba-region-backup.ts @@ -1,10 +1,12 @@ -import { copyFileSync, existsSync, linkSync, readFileSync, rmSync } from "node:fs"; +import { chmodSync, copyFileSync, existsSync, linkSync, readFileSync, rmSync } from "node:fs"; import { getConfigPath } from "../config"; +import { hardenSecretPath } from "../lib/windows-secret-acl"; export interface AlibabaBackupIO { exists: (path: string) => boolean; read: (path: string) => Buffer; copy: (source: string, destination: string) => void; + harden: (path: string) => void; /** Publish with no-replace semantics: fails with EEXIST if the destination exists. */ publishNoReplace: (temp: string, destination: string) => void; remove: (path: string) => void; @@ -14,6 +16,16 @@ const DEFAULT_IO: AlibabaBackupIO = { exists: existsSync, read: path => readFileSync(path), copy: (source, destination) => copyFileSync(source, destination), + harden: path => { + // POSIX: a failed chmod must not publish a credential-bearing backup with weak permissions. + // Windows keeps its own control via hardenSecretPath below, which is the required check. + if (process.platform === "win32") { + try { chmodSync(path, 0o600); } catch { /* Windows may not support POSIX chmod */ } + } else { + chmodSync(path, 0o600); + } + if (process.platform === "win32") hardenSecretPath(path, { required: true }); + }, publishNoReplace: linkSync, remove: path => rmSync(path, { force: true }), }; @@ -57,6 +69,9 @@ export function backupConfigBeforeAlibabaRegionMigration( const temp = `${backup}.${process.pid}.tmp`; try { io.copy(configPath, temp); + // The snapshot contains credentials. Harden it before publication so the + // stable backup path is never exposed with inherited permissions or ACLs. + io.harden(temp); // Verify before publishing: a short copy must never become the snapshot. if (!io.read(temp).equals(source)) { throw new AlibabaBackupIntegrityError(`failed to write a complete backup to ${temp}`); diff --git a/src/server/live.ts b/src/server/live.ts index 673e93ea8f2..1f5a47f4d6e 100644 --- a/src/server/live.ts +++ b/src/server/live.ts @@ -27,7 +27,7 @@ import { codexCompatibleUrl } from "../codex/context-compat"; * - `GET /v1/realtime?model=` — RealtimeV2 standalone (no intent) * - `GET /v1/live?model=` — Frameless standalone */ -import { appendFileSync } from "node:fs"; +import { closeSync, fchmodSync, openSync, writeSync } from "node:fs"; import { formatErrorResponse } from "../bridge"; import { CodexAccountCooldownError, @@ -103,9 +103,25 @@ export const LIVE_CLIENT_PROTOCOL_HEADERS = [ * JSONL record: direction, frame kind, byte length, and whether the payload contains U+FFFD. * Privacy: no frame content is written, including excerpts around replacement characters. * For binary frames, U+FFFD may also be introduced by UTF-8 decoding; the flag alone does not - * identify the source of corruption. Disabled entirely when the env var is unset. + * identify the source of corruption. The log is created with owner-only permissions and is + * disabled entirely when the env var is unset. */ export const LIVE_FRAME_LOG_ENV = "OCX_LIVE_FRAME_LOG"; +/** + * Append one JSONL record with owner-only permissions. `appendFileSync`'s `mode` only applies + * when it creates the file, so an existing permissive log would stay readable by other local + * users. Open for append, harden the opened descriptor, then write. + */ +function appendOwnerOnly(path: string, line: string): void { + const fd = openSync(path, "a", 0o600); + try { + try { fchmodSync(fd, 0o600); } catch { /* platforms without fchmod keep the create mode */ } + writeSync(fd, line); + } finally { + closeSync(fd); + } +} + export function logLiveSidebandFrame(dir: "c2u" | "u2c", data: unknown): void { const logPath = process.env[LIVE_FRAME_LOG_ENV]; if (!logPath) return; @@ -134,7 +150,7 @@ export function logLiveSidebandFrame(dir: "c2u" | "u2c", data: unknown): void { bytes, fffd, }; - appendFileSync(logPath, `${JSON.stringify(record)}\n`); + appendOwnerOnly(logPath, `${JSON.stringify(record)}\n`); } catch { // Frame forensics must never break the relay. } @@ -168,7 +184,7 @@ export function logLiveSidebandStage( const record: Record = { ts: new Date().toISOString(), stage }; if (detail?.status !== undefined) record.status = detail.status; if (detail?.code !== undefined) record.code = detail.code; - appendFileSync(logPath, JSON.stringify(record) + "\n"); + appendOwnerOnly(logPath, JSON.stringify(record) + "\n"); } catch { // Diagnostics must never break the relay. } diff --git a/tests/clients/client-connect.test.ts b/tests/clients/client-connect.test.ts index 1fc0c72df30..75267847466 100644 --- a/tests/clients/client-connect.test.ts +++ b/tests/clients/client-connect.test.ts @@ -237,8 +237,12 @@ describe("remote hub client boundary", () => { test("canonicalizes origin and terminal /v1 only", () => { expect(normalizeHubOrigin("https://hub.example.test/v1")).toBe("https://hub.example.test"); expect(normalizeHubOrigin("https://hub.example.test/v1/")).toBe("https://hub.example.test"); + expect(normalizeHubOrigin("http://localhost:10100/v1")).toBe("http://localhost:10100"); + expect(normalizeHubOrigin("http://127.0.0.1:10100")).toBe("http://127.0.0.1:10100"); + expect(normalizeHubOrigin("http://[::1]:10100")).toBe("http://[::1]:10100"); for (const value of [ "ftp://hub.example.test", + "http://hub.example.test", "https://user@hub.example.test", "https://hub.example.test/private", "https://hub.example.test/?secret=1", @@ -263,9 +267,17 @@ describe("remote hub client boundary", () => { } }); + test("rejects plaintext remote discovery before sending a request", async () => { + let calls = 0; + await expect(fetchHubReady("http://hub.example.test", { + fetchImpl: async () => { calls += 1; return Response.json(readyBody()); }, + })).rejects.toThrow("plaintext remote HTTP is not permitted"); + expect(calls).toBe(0); + }); + test("admin key issuance is HTTPS-only and pairing exchanges into a full GUI session", async () => { let calls = 0; - await expect(issueClientKey("http://hub.example.test", { + await expect(issueClientKey("http://localhost:10100", { kind: "admin", value: new TextEncoder().encode("ocx_admin_secret"), }, "client", { diff --git a/tests/providers/alibaba-region-backup.test.ts b/tests/providers/alibaba-region-backup.test.ts index 6084c5c7b64..897a17e7aea 100644 --- a/tests/providers/alibaba-region-backup.test.ts +++ b/tests/providers/alibaba-region-backup.test.ts @@ -1,5 +1,5 @@ import { expect, test } from "bun:test"; -import { existsSync, linkSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { existsSync, linkSync, mkdtempSync, readFileSync, rmSync, statSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { @@ -23,6 +23,7 @@ test("creates a snapshot, then never replaces it", () => { writeFileSync(configPath, '{"before":true}', "utf8"); expect(backupConfigBeforeAlibabaRegionMigration(configPath)).toBe("created"); expect(readFileSync(backupPath, "utf8")).toBe('{"before":true}'); + if (process.platform !== "win32") expect(statSync(backupPath).mode & 0o777).toBe(0o600); expect(backupConfigBeforeAlibabaRegionMigration(configPath)).toBe("reused"); expect(readFileSync(backupPath, "utf8")).toBe('{"before":true}'); } finally { removeTreeWithRetry(dir); } @@ -53,6 +54,7 @@ test("a short copy is never published", () => { exists: existsSync, read: path => readFileSync(path), copy: (_source, destination) => { writeFileSync(destination, '{"bef', "utf8"); }, + harden: () => {}, publishNoReplace: linkSync, remove: path => rmSync(path, { force: true }), })).toThrow(AlibabaBackupIntegrityError); @@ -70,6 +72,7 @@ test("a failed copy leaves no snapshot and no temp file", () => { exists: existsSync, read: path => readFileSync(path), copy: () => { throw new Error("disk full"); }, + harden: () => {}, publishNoReplace: linkSync, remove: path => { removed.push(path); rmSync(path, { force: true }); }, })).toThrow("disk full"); @@ -77,3 +80,22 @@ test("a failed copy leaves no snapshot and no temp file", () => { expect(removed).toHaveLength(1); } finally { removeTreeWithRetry(dir); } }); + +test("a failed harden never publishes the secret-bearing snapshot", () => { + const dir = mkdtempSync(join(tmpdir(), "ocx-bak-")); + const configPath = join(dir, "config.json"); + const backupPath = `${configPath}.pre-alibaba-region-v1.bak`; + try { + writeFileSync(configPath, '{"before":true}', "utf8"); + expect(() => backupConfigBeforeAlibabaRegionMigration(configPath, { + exists: existsSync, + read: path => readFileSync(path), + copy: (source, destination) => writeFileSync(destination, readFileSync(source)), + harden: () => { throw new Error("ACL hardening failed"); }, + publishNoReplace: linkSync, + remove: path => rmSync(path, { force: true }), + })).toThrow("ACL hardening failed"); + expect(existsSync(backupPath)).toBe(false); + expect(existsSync(`${backupPath}.${process.pid}.tmp`)).toBe(false); + } finally { removeTreeWithRetry(dir); } +}); diff --git a/tests/server/server-live.test.ts b/tests/server/server-live.test.ts index 7834021604d..61082b2f125 100644 --- a/tests/server/server-live.test.ts +++ b/tests/server/server-live.test.ts @@ -3,7 +3,7 @@ * so the proxy must relay it to an OpenAI upstream instead of the /v1/* JSON-404 guard. */ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { existsSync, mkdirSync, readFileSync } from "node:fs"; +import { chmodSync, existsSync, mkdirSync, readFileSync, statSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { saveCodexAccountCredential } from "../../src/codex/account-store"; import { clearAccountNeedsReauth, clearAccountQuota } from "../../src/codex/auth-api"; @@ -1334,6 +1334,7 @@ test("sideband frame log preserves delivery without recording damaged or clean t expect(JSON.stringify(line)).not.toContain("clean-frame"); expect(JSON.stringify(line)).not.toContain(FFFD_TEXT); } + if (process.platform !== "win32") expect(statSync(frameLogPath).mode & 0o777).toBe(0o600); client.close(); } finally { @@ -1345,6 +1346,28 @@ test("sideband frame log preserves delivery without recording damaged or clean t } }); +// appendFileSync's mode option only applies when it creates the file, so an +// existing permissive log would have stayed readable by other local users. +// appendOwnerOnly hardens the opened descriptor instead. +test("frame log hardens a pre-existing permissive file", async () => { + if (process.platform === "win32") return; + const { logLiveSidebandStage } = await import("../../src/server/live"); + const frameLogPath = join(TEST_DIR, "frames-permissive.jsonl"); + const previousFrameLog = process.env.OCX_LIVE_FRAME_LOG; + try { + writeFileSync(frameLogPath, "", { mode: 0o644 }); + chmodSync(frameLogPath, 0o644); + process.env.OCX_LIVE_FRAME_LOG = frameLogPath; + logLiveSidebandStage("relay-attached"); + expect(statSync(frameLogPath).mode & 0o777).toBe(0o600); + const line = readFileSync(frameLogPath, "utf8").trim(); + expect(JSON.parse(line)).toMatchObject({ stage: "relay-attached" }); + } finally { + if (previousFrameLog === undefined) delete process.env.OCX_LIVE_FRAME_LOG; + else process.env.OCX_LIVE_FRAME_LOG = previousFrameLog; + } +}); + test("frame diagnostics retain only metadata for text, binary, and bounded views", async () => { const { logLiveSidebandFrame } = await import("../../src/server/live"); const previousFrameLog = process.env.OCX_LIVE_FRAME_LOG;