diff --git a/docs/reference/ENVIRONMENT.md b/docs/reference/ENVIRONMENT.md index b04c03601a..5c9096569b 100644 --- a/docs/reference/ENVIRONMENT.md +++ b/docs/reference/ENVIRONMENT.md @@ -702,7 +702,7 @@ REQUEST_TIMEOUT_MS (global override) | `REQUEST_TIMEOUT_MS` | _(unset)_ | Global shortcut — overrides both `FETCH_TIMEOUT_MS` and `STREAM_IDLE_TIMEOUT_MS` defaults. | | `FETCH_TIMEOUT_MS` | `600000` | Total HTTP request timeout for upstream provider calls. | | `STREAM_IDLE_TIMEOUT_MS` | `600000` | Max silence between SSE chunks before aborting. Extended-thinking models rarely pause >90s. | -| `OMNIROUTE_SSE_COMMENTS` | _(enabled)_ | Whether OmniRoute may emit SSE `:` comment lines (e.g. the `: keepalive` heartbeat). Set `off` to suppress comment-shaped heartbeats (no-op) for strict OpenAI-compatible clients that JSON.parse every SSE line; `data:` heartbeats are unaffected. Used by `open-sse/utils/sseHeartbeat.ts`. | +| `OMNIROUTE_SSE_COMMENTS` | _(disabled)_ | Whether OmniRoute may emit SSE `:` comment lines (e.g. the `: keepalive` heartbeat and `x-omniroute-*` metadata trailers). Disabled by default (#10524) since strict OpenAI-compatible clients JSON.parse every SSE line and crash on `:` comments; `data:` heartbeats are unaffected. Set `on`/`true`/`1`/`yes` to opt back in. Used by `open-sse/utils/sseHeartbeat.ts`. | | `STREAM_READINESS_TIMEOUT_MS` | `80000` | Time to receive the first non-ping SSE event. Inherits `REQUEST_TIMEOUT_MS` when set. | | `STREAM_READINESS_MAX_TIMEOUT_MS` | `180000` | Maximum adaptive first-event readiness window for large, tool-heavy, or high-reasoning streaming requests. | | `OMNIROUTE_AGENT_GOAL_POLICY_ENABLED` | `true` | Kill-switch for the `/goal` heuristic. Set `false`/`0`/`off` to fully disable detection — readiness timeouts and stream recovery are never elevated by request body/headers, mitigating client-controlled timeout amplification. | diff --git a/open-sse/utils/sseHeartbeat.ts b/open-sse/utils/sseHeartbeat.ts index 5fb415ea8b..bdb355b96e 100644 --- a/open-sse/utils/sseHeartbeat.ts +++ b/open-sse/utils/sseHeartbeat.ts @@ -71,16 +71,17 @@ const HEARTBEAT_ENCODER = new TextEncoder(); /** * Whether OmniRoute may emit SSE `:` comment lines (e.g. the `: keepalive` heartbeat). * Some strict OpenAI-compatible clients parse every SSE line as JSON and crash on `:` comments. - * Set OMNIROUTE_SSE_COMMENTS=off to suppress comment-shaped heartbeats (they become a no-op). - * Defaults to enabled for backward compatibility. + * Set OMNIROUTE_SSE_COMMENTS=on to enable comment-shaped heartbeats and telemetry trailers. + * #10524: defaults to disabled — strict SSE clients (WorkBuddy, etc.) break on `: x-omniroute-*` + * comment lines. Operators who want the telemetry can opt in with OMNIROUTE_SSE_COMMENTS=on. */ export function sseCommentsEnabled(): boolean { // SSR/edge safety: `process` is not defined in Workers/Deno/edge runtimes. - if (typeof process === "undefined") return true; + if (typeof process === "undefined") return false; const v = process.env.OMNIROUTE_SSE_COMMENTS; - if (v === undefined || v === "") return true; + if (v === undefined || v === "") return false; const normalized = v.trim().toLowerCase(); - return normalized !== "off" && normalized !== "false" && normalized !== "0" && normalized !== "no"; + return normalized === "on" || normalized === "true" || normalized === "1" || normalized === "yes"; } export function createSseHeartbeatTransform({ diff --git a/src/shared/constants/featureFlagDefinitions.ts b/src/shared/constants/featureFlagDefinitions.ts index ed7e74c652..d1972eb1e1 100644 --- a/src/shared/constants/featureFlagDefinitions.ts +++ b/src/shared/constants/featureFlagDefinitions.ts @@ -12,7 +12,7 @@ export interface FeatureFlagDefinition { } export const FEATURE_FLAG_DEFINITIONS: FeatureFlagDefinition[] = [ - // ──────────────── Security (9) ──────────────── + // ──────────────── Security (10) ──────────────── { key: "REQUIRE_API_KEY", label: "Require API Key", @@ -105,6 +105,20 @@ export const FEATURE_FLAG_DEFINITIONS: FeatureFlagDefinition[] = [ requiresRestart: false, warningLevel: "danger", }, + { + key: "AUTH_LOG_INCLUDE_ACCOUNT_ID", + label: "Log Account IDs", + description: + "Include the account ID prefix in AUTH log lines (e.g. \"Using account: abc12345...\"). " + + "Disabled by default so the account identifier is redacted in shared/multi-tenant process logs. " + + "Independent of Debug Mode — flipping Debug Mode on does not reveal this.", + descriptionI18nKey: "featureFlagAuthLogIncludeAccountIdDescription", + category: "security", + defaultValue: "false", + type: "boolean", + requiresRestart: false, + warningLevel: "info", + }, // ──────────────── Network (7) ──────────────── { key: "ENABLE_TLS_FINGERPRINT", diff --git a/src/sse/handlers/chat.ts b/src/sse/handlers/chat.ts index 0fdfbabafa..3845c1970f 100644 --- a/src/sse/handlers/chat.ts +++ b/src/sse/handlers/chat.ts @@ -119,6 +119,7 @@ import { getComboFailureLogError } from "./comboFailureLogging"; import { classify429FromError, type FailureKind } from "@/shared/utils/classify429"; import { isSubscriptionQuotaText } from "@omniroute/open-sse/services/quotaTextCooldowns.ts"; import { resolveUseUpstream429BreakerHints } from "@/shared/utils/providerHints"; +import { isFeatureFlagEnabled } from "@/shared/utils/featureFlags"; import { getCircuitBreaker, isLocalStreamLifecycleError } from "../../shared/utils/circuitBreaker"; import { markAccountExhaustedFrom429 } from "../../domain/quotaCache"; import { resolveForcedConnectionForCredentialPool } from "../services/sessionAffinityPin.ts"; @@ -1551,7 +1552,22 @@ async function handleSingleModelChat( const accountId = credentials.connectionId.slice(0, 8); const releaseOAuthSession = credentials.releaseOAuthSession ?? (() => {}); - log.info("AUTH", `Using ${provider} account: ${accountId}...`); + // #10348: redact the account prefix by default. Gated on the narrow + // AUTH_LOG_INCLUDE_ACCOUNT_ID flag (default off) rather than the broad + // `debugMode` setting — `debugMode` is a general dashboard-visibility + // toggle unrelated to log privacy (its own default has changed + // independently for unrelated reasons, see #10312/#10372), so deriving + // redaction from it would make log leakage depend on an unrelated + // setting. resolveFeatureFlag() reads straight from SQLite on every + // call (no stale cache to invalidate) and fails safe (redacted) if the + // lookup throws. + let includeAccountId = false; + try { + includeAccountId = isFeatureFlagEnabled("AUTH_LOG_INCLUDE_ACCOUNT_ID"); + } catch { + includeAccountId = false; + } + log.info("AUTH", `Using ${provider} account: ${includeAccountId ? accountId : "***"}...`); // #474: when the request used a bare model name (no "/" — e.g. an alias // that resolved to "auto") and the selected connection declares a // defaultModel, resolve the bare name to that real model ID before the diff --git a/tests/unit/auth-log-account-id-redaction-10539.test.ts b/tests/unit/auth-log-account-id-redaction-10539.test.ts new file mode 100644 index 0000000000..f31508039b --- /dev/null +++ b/tests/unit/auth-log-account-id-redaction-10539.test.ts @@ -0,0 +1,98 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { dirname, resolve } from "node:path"; + +// Regression guard for #10348/#10539: the AUTH log line in the SSE chat +// handler ("Using account: ...") must redact the account +// prefix by default. Redaction MUST be governed by a narrow, dedicated +// feature flag (AUTH_LOG_INCLUDE_ACCOUNT_ID) — NOT by the broad `debugMode` +// setting. `debugMode` is a general dashboard-visibility toggle unrelated to +// log privacy (its own default has flipped independently more than once, +// see #10372/#10312); deriving log redaction from it means any future, +// unrelated change to debugMode's default silently changes whether account +// prefixes leak into logs. The narrow flag keeps the two concerns separate. + +// Isolate DB state so the resolution chain (DB override > env > default) +// reads a clean store and we exercise the definition default, not a leaked +// override from another test file. +const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-test-auth-log-account-id-")); +process.env.DATA_DIR = tmpDir; + +const { FEATURE_FLAG_DEFINITIONS } = await import( + "../../src/shared/constants/featureFlagDefinitions.ts" +); + +test("AUTH_LOG_INCLUDE_ACCOUNT_ID feature flag defaults to OFF, independent of debugMode", async (t) => { + const def = (key: string) => FEATURE_FLAG_DEFINITIONS.find((d) => d.key === key); + + await t.test("AUTH_LOG_INCLUDE_ACCOUNT_ID definition exists", () => { + assert.ok( + def("AUTH_LOG_INCLUDE_ACCOUNT_ID"), + "AUTH_LOG_INCLUDE_ACCOUNT_ID definition must exist in FEATURE_FLAG_DEFINITIONS" + ); + }); + + await t.test("AUTH_LOG_INCLUDE_ACCOUNT_ID default value is 'false'", () => { + assert.strictEqual( + def("AUTH_LOG_INCLUDE_ACCOUNT_ID")!.defaultValue, + "false", + "AUTH_LOG_INCLUDE_ACCOUNT_ID must default OFF — account prefixes are redacted by default" + ); + }); + + await t.test("AUTH_LOG_INCLUDE_ACCOUNT_ID category is 'security'", () => { + assert.strictEqual(def("AUTH_LOG_INCLUDE_ACCOUNT_ID")!.category, "security"); + }); + + await t.test("AUTH_LOG_INCLUDE_ACCOUNT_ID type is 'boolean'", () => { + assert.strictEqual(def("AUTH_LOG_INCLUDE_ACCOUNT_ID")!.type, "boolean"); + }); + + await t.test("effective runtime resolution is OFF with no override", async () => { + delete process.env.AUTH_LOG_INCLUDE_ACCOUNT_ID; + const { clearAllFeatureFlagOverrides } = await import("@/lib/db/featureFlags"); + clearAllFeatureFlagOverrides(); + + const { isFeatureFlagEnabled } = await import("@/shared/utils/featureFlags"); + assert.strictEqual(isFeatureFlagEnabled("AUTH_LOG_INCLUDE_ACCOUNT_ID"), false); + }); + + await t.test("explicit env override enables it", async () => { + process.env.AUTH_LOG_INCLUDE_ACCOUNT_ID = "true"; + try { + const { isFeatureFlagEnabled } = await import("@/shared/utils/featureFlags"); + assert.strictEqual(isFeatureFlagEnabled("AUTH_LOG_INCLUDE_ACCOUNT_ID"), true); + } finally { + delete process.env.AUTH_LOG_INCLUDE_ACCOUNT_ID; + } + }); +}); + +test("chat.ts AUTH account log line is gated on the narrow flag, not on debugMode", () => { + const here = dirname(fileURLToPath(import.meta.url)); + const chatHandlerPath = resolve(here, "../../src/sse/handlers/chat.ts"); + const src = fs.readFileSync(chatHandlerPath, "utf8"); + + assert.match( + src, + /Using \$\{provider\} account: \$\{includeAccountId \? accountId : "\*\*\*"\}/, + "chat.ts must redact the account prefix behind a boolean gate variable" + ); + + assert.match( + src, + /isFeatureFlagEnabled\("AUTH_LOG_INCLUDE_ACCOUNT_ID"\)/, + "chat.ts must resolve the redaction gate via the narrow AUTH_LOG_INCLUDE_ACCOUNT_ID flag" + ); + + // The old, broad debugMode-derived gate must be gone from this call site. + assert.doesNotMatch( + src, + /debugMode === true[\s\S]{0,80}Using \$\{provider\} account/, + "the AUTH account log line must not be gated on the broad debugMode setting" + ); +}); diff --git a/tests/unit/feature-flags-settings.test.ts b/tests/unit/feature-flags-settings.test.ts index 8458edf857..e12c67213d 100644 --- a/tests/unit/feature-flags-settings.test.ts +++ b/tests/unit/feature-flags-settings.test.ts @@ -30,13 +30,13 @@ const { isControlPlaneProxyDirectFallbackEnabled, } = await import("../../src/shared/utils/featureFlags.ts"); -const EXPECTED_FEATURE_FLAG_COUNT = 48; +const EXPECTED_FEATURE_FLAG_COUNT = 49; // ────────────────────────────────────────────────────── // Test group 1 — Flag definitions registry // ────────────────────────────────────────────────────── describe("featureFlagDefinitions", () => { - it("has exactly 47 flag definitions", () => { + it(`has exactly ${EXPECTED_FEATURE_FLAG_COUNT} flag definitions`, () => { assert.strictEqual(FEATURE_FLAG_DEFINITIONS.length, EXPECTED_FEATURE_FLAG_COUNT); }); @@ -344,7 +344,7 @@ describe("resolveFeatureFlag", () => { }); describe("resolveAllFeatureFlags", () => { - it("returns all 47 flags", () => { + it(`returns all ${EXPECTED_FEATURE_FLAG_COUNT} flags`, () => { const all = resolveAllFeatureFlags(); assert.strictEqual(all.length, EXPECTED_FEATURE_FLAG_COUNT); }); diff --git a/tests/unit/sse-comments-default-10524.test.ts b/tests/unit/sse-comments-default-10524.test.ts new file mode 100644 index 0000000000..2eaeedcc10 --- /dev/null +++ b/tests/unit/sse-comments-default-10524.test.ts @@ -0,0 +1,39 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { sseCommentsEnabled } from "../../open-sse/utils/sseHeartbeat.ts"; + +// #10524: SSE comment lines (`: x-omniroute-*`) break strict SSE clients. +// Default should be disabled (opt-in via OMNIROUTE_SSE_COMMENTS=on). + +test("#10524: sseCommentsEnabled defaults to false when env var is unset", () => { + const original = process.env.OMNIROUTE_SSE_COMMENTS; + delete process.env.OMNIROUTE_SSE_COMMENTS; + + assert.strictEqual(sseCommentsEnabled(), false, "SSE comments must be disabled by default"); + + if (original !== undefined) process.env.OMNIROUTE_SSE_COMMENTS = original; +}); + +test("#10524: sseCommentsEnabled returns true when explicitly enabled", () => { + const original = process.env.OMNIROUTE_SSE_COMMENTS; + + for (const value of ["on", "true", "1", "yes", "ON", "TRUE"]) { + process.env.OMNIROUTE_SSE_COMMENTS = value; + assert.strictEqual(sseCommentsEnabled(), true, `SSE comments must be enabled for "${value}"`); + } + + if (original !== undefined) process.env.OMNIROUTE_SSE_COMMENTS = original; + else delete process.env.OMNIROUTE_SSE_COMMENTS; +}); + +test("#10524: sseCommentsEnabled returns false when explicitly disabled", () => { + const original = process.env.OMNIROUTE_SSE_COMMENTS; + + for (const value of ["off", "false", "0", "no", "OFF", "FALSE"]) { + process.env.OMNIROUTE_SSE_COMMENTS = value; + assert.strictEqual(sseCommentsEnabled(), false, `SSE comments must be disabled for "${value}"`); + } + + if (original !== undefined) process.env.OMNIROUTE_SSE_COMMENTS = original; + else delete process.env.OMNIROUTE_SSE_COMMENTS; +}); diff --git a/tests/unit/sse-comments-optout-9305.test.ts b/tests/unit/sse-comments-optout-9305.test.ts index e8479130f1..8928738bbe 100644 --- a/tests/unit/sse-comments-optout-9305.test.ts +++ b/tests/unit/sse-comments-optout-9305.test.ts @@ -127,7 +127,9 @@ for (const upstreamDone of [true, false]) { const finalization = upstreamDone ? "upstream [DONE]" : "natural EOF"; for (const [label, envValue, commentsExpected] of [ - ["default", undefined, true], + // #10524: OMNIROUTE_SSE_COMMENTS now defaults to disabled — strict SSE + // clients (WorkBuddy, etc.) crash on `: x-omniroute-*` comment lines. + ["default", undefined, false], ["explicitly enabled", "yes", true], ["disabled", "off", false], ] as const) { diff --git a/tests/unit/sseHeartbeat.test.ts b/tests/unit/sseHeartbeat.test.ts index e83263a933..d7dc19f3d9 100644 --- a/tests/unit/sseHeartbeat.test.ts +++ b/tests/unit/sseHeartbeat.test.ts @@ -51,8 +51,8 @@ async function collectHeartbeatOutput( ).text(); } -test("sseCommentsEnabled defaults to true when the env var is unset", () => { - withEnv(undefined, () => assert.equal(sseCommentsEnabled(), true)); +test("sseCommentsEnabled defaults to false when the env var is unset (#10524)", () => { + withEnv(undefined, () => assert.equal(sseCommentsEnabled(), false)); }); test("sseCommentsEnabled is false for 'off', 'false', '0', 'no' (case-insensitive)", () => {