diff --git a/CHANGELOG.md b/CHANGELOG.md index 6fe029faa0..bceef06d24 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,14 @@ ## [Unreleased] +### Security + +- **fix(oauth/windsurf):** Windsurf Firebase token refresh now reads `WINDSURF_CONFIG.firebaseApiKey` instead of `process.env.WINDSURF_FIREBASE_API_KEY` directly. The literal was removed from `.env.example` in this release, so the previous direct read would have silently skipped refresh for browser-flow Windsurf/Devin sessions (forcing re-auth instead of renewing). Operators with a legacy `WINDSURF_FIREBASE_API_KEY` value in their `.env` keep working — the env override path is preserved by `resolvePublicCred()`. See [`docs/security/PUBLIC_CREDS.md`](docs/security/PUBLIC_CREDS.md). +- **fix(kiro/translator):** assistant-first conversations no longer collide on a single `conversationId`. The synthetic "(empty)" user turn injected to satisfy Kiro's "first message must be user" rule is now marked non-enumerable `__synthetic` and excluded from the `uuidv5` conversationId derivation, so unrelated chats no longer share the same upstream AWS Builder ID context. Prevents leaking prior session state across unrelated chats. +- **fix(utils/publicCreds):** `decodePublicCred()` no longer silently mangles raw credential overrides that don't match `RAW_VALUE_PATTERN`. The previous path always base64-decoded + XOR-unmasked (and `Buffer.from(v, "base64")` is lenient, accepting many non-base64 inputs without throwing). Now: strict-base64 alphabet check + printable-plain check on the decoded result; failing either, the original value is returned untouched. +- **fix(auth/extractApiKey):** `x-api-key` fallback now only triggers when the request also carries an `anthropic-version` header. Without this scoping, non-Anthropic clients in local mode (placeholder `x-api-key`) would get `401 Invalid API key` from per-route gates even with `REQUIRE_API_KEY` off. +- **fix(providers/qoder):** the OAuth+PAT disambiguation message now actually surfaces. The `getProviderRuntimeStatus()` early-return on `qoder + !apikey` was masking the new branch added in #2247. + ### Fixed - **fix(executor/claude-code):** store tool-name round-trip metadata in non-enumerable `_toolNameMap` so it survives in-memory but is stripped by `JSON.stringify()` — prevents internal OmniRoute metadata from leaking to upstream providers. ([#2254](https://github.com/diegosouzapw/OmniRoute/pull/2254) — thanks @Rikonorus) diff --git a/open-sse/services/tokenRefresh.ts b/open-sse/services/tokenRefresh.ts index 33f0848756..974f53497d 100755 --- a/open-sse/services/tokenRefresh.ts +++ b/open-sse/services/tokenRefresh.ts @@ -3,6 +3,7 @@ import { PROVIDERS, OAUTH_ENDPOINTS } from "../config/constants.ts"; import { getGitHubCopilotRefreshHeaders } from "../config/providerHeaderProfiles.ts"; import { pbkdf2Sync } from "node:crypto"; import { runWithProxyContext } from "../utils/proxyFetch.ts"; +import { WINDSURF_CONFIG } from "@/lib/oauth/constants/oauth"; // Token expiry buffer (refresh if expires within 5 minutes) export const TOKEN_EXPIRY_BUFFER_MS = 5 * 60 * 1000; @@ -148,12 +149,14 @@ export async function refreshWindsurfToken( } // Firebase STS refresh for browser-flow tokens. - // Key is read from WINDSURF_FIREBASE_API_KEY env var (set in .env.example). - const firebaseApiKey = process.env.WINDSURF_FIREBASE_API_KEY || ""; + // Resolves via WINDSURF_CONFIG.firebaseApiKey, which honors the + // WINDSURF_FIREBASE_API_KEY env override and falls back to the embedded + // public default in publicCreds.ts. See docs/security/PUBLIC_CREDS.md. + const firebaseApiKey = WINDSURF_CONFIG.firebaseApiKey || ""; if (!firebaseApiKey) { log?.warn?.( "TOKEN_REFRESH", - "WINDSURF_FIREBASE_API_KEY not set — skipping Windsurf Firebase token refresh" + "Windsurf Firebase API key unavailable — skipping Firebase token refresh" ); return null; } diff --git a/open-sse/translator/request/openai-to-kiro.ts b/open-sse/translator/request/openai-to-kiro.ts index 50fc19a9fe..3404cc4507 100644 --- a/open-sse/translator/request/openai-to-kiro.ts +++ b/open-sse/translator/request/openai-to-kiro.ts @@ -473,13 +473,23 @@ function convertMessages(messages, tools, model) { // Ensure first message is user. Kiro API requires conversations to start // with a user message (fixes "Improperly formed request" for assistant-first). if (mergedHistory.length > 0 && mergedHistory[0].assistantResponseMessage) { - mergedHistory.unshift({ + const syntheticUserTurn = { userInputMessage: { content: "(empty)", modelId: model, origin: "AI_EDITOR", }, + }; + // Mark as synthetic (non-enumerable so it doesn't leak to upstream JSON) + // so conversationId derivation can skip it — otherwise every + // assistant-first conversation collapses onto the same uuidv5(empty) + // namespace and leaks AWS Builder ID context across unrelated sessions. + Object.defineProperty(syntheticUserTurn, "__synthetic", { + value: true, + enumerable: false, + configurable: true, }); + mergedHistory.unshift(syntheticUserTurn); } // Ensure assistant exists before toolResults. Kiro API validates that every @@ -555,9 +565,15 @@ function convertMessages(messages, tools, model) { for (const item of mergedHistory) { const last = alternatingHistory[alternatingHistory.length - 1]; if (item.userInputMessage && last?.userInputMessage) { - alternatingHistory.push({ + const syntheticAssistantTurn = { assistantResponseMessage: { content: "(empty)" }, + }; + Object.defineProperty(syntheticAssistantTurn, "__synthetic", { + value: true, + enumerable: false, + configurable: true, }); + alternatingHistory.push(syntheticAssistantTurn); } alternatingHistory.push(item); } @@ -685,12 +701,15 @@ export function buildKiroPayload(model, body, stream, credentials) { }, }; - // Determistic session caching for Kiro + // Deterministic session caching for Kiro. + // Skip synthetic placeholder turns ("(empty)" injected for assistant-first + // conversations or alternating-role gaps) — otherwise unrelated assistant- + // first chats would all hash to the same uuidv5(empty) and reuse the same + // upstream Kiro/AWS conversation context, leaking prior state across + // sessions. See conversionMessages() above for the `__synthetic` marker. const NAMESPACE_KIRO = "34f7193f-561d-4050-bc84-9547d953d6bf"; - const firstContent = - history.length > 0 && history[0].userInputMessage?.content - ? history[0].userInputMessage.content - : finalContent; + const firstRealUserTurn = history.find((h) => h?.userInputMessage?.content && !h.__synthetic); + const firstContent = firstRealUserTurn?.userInputMessage?.content || finalContent; // Use uuidv5 with the hash of the system prompt / first message to maintain AWS Builder ID context cache payload.conversationState.conversationId = uuidv5( diff --git a/open-sse/utils/publicCreds.ts b/open-sse/utils/publicCreds.ts index ca67ac3ee9..04cac84fb7 100644 --- a/open-sse/utils/publicCreds.ts +++ b/open-sse/utils/publicCreds.ts @@ -50,22 +50,53 @@ function maskBytes(plain: string): number[] { return arr; } +// A valid base64-encoded masked value uses only the base64 alphabet plus +// optional padding. Anything outside that alphabet is definitely a raw +// credential the user supplied (a token format we don't yet recognize in +// RAW_VALUE_PATTERN) — never try to base64-decode it. +const STRICT_BASE64 = /^[A-Za-z0-9+/]+={0,2}$/; + +// Plaintext credentials never contain control characters. If unmasking +// produces non-printable bytes, the input wasn't actually masked and we +// must return it untouched to avoid silently mangling raw overrides. +function looksLikePrintablePlain(s: string): boolean { + if (!s) return false; + for (let i = 0; i < s.length; i++) { + const code = s.charCodeAt(i); + // Allow printable ASCII (0x20–0x7E). Everything outside that is suspect. + if (code < 0x20 || code > 0x7e) return false; + } + return true; +} + /** * Decode a public credential. Accepts either a raw literal (well-known prefix) * or a base64 string produced by `encodePublicCred()`. Returns the plaintext. * Empty / nullish input returns "". + * + * When the input doesn't match a known raw-credential prefix, we tentatively + * base64-decode + XOR-unmask, but only adopt the result if it looks like a + * printable plaintext. Otherwise we return the original value unchanged — + * `Buffer.from(value, "base64")` is lenient (it silently drops invalid chars + * instead of throwing) so a raw secret with a unknown format would otherwise + * be silently mangled. See docs/security/PUBLIC_CREDS.md. */ export function decodePublicCred(value: string | null | undefined): string { if (!value || typeof value !== "string") return ""; if (RAW_VALUE_PATTERN.test(value)) return value; + // Reject anything that isn't strict base64 — saves us from feeding raw + // ASCII overrides into the lenient Buffer.from(...,"base64") path. + if (!STRICT_BASE64.test(value)) return value; + try { const buf = Buffer.from(value, "base64"); if (buf.length === 0) return value; const arr: number[] = []; for (let i = 0; i < buf.length; i++) arr.push(buf[i]); - return unmaskBytes(arr); + const decoded = unmaskBytes(arr); + return looksLikePrintablePlain(decoded) ? decoded : value; } catch { return value; } diff --git a/src/app/api/providers/[id]/test/route.ts b/src/app/api/providers/[id]/test/route.ts index 513a589a78..2bbf28b039 100644 --- a/src/app/api/providers/[id]/test/route.ts +++ b/src/app/api/providers/[id]/test/route.ts @@ -233,6 +233,26 @@ function hasQoderToken(connection: any): boolean { async function getProviderRuntimeStatus(connection: any) { const provider = typeof connection?.provider === "string" ? connection.provider : ""; let toolId = CLI_RUNTIME_PROVIDER_MAP[provider]; + + // Issue #2247: detect Qoder in OAuth/CLI-flavored mode with a PAT pasted + // BEFORE the CLI-runtime early-return below, otherwise the disambiguation + // message never reaches the user (they keep seeing the generic "CLI not + // installed" + 401 cascade). For Qoder, this short-circuits the runtime + // check entirely with an actionable diagnosis. + const isQoderOauthWithToken = + provider === "qoder" && connection?.authType !== "apikey" && hasQoderToken(connection); + if (isQoderOauthWithToken) { + const message = + "Qoder OAuth/Local CLI mode is selected but a Personal Access Token is stored on this connection. Switch this connection to API Key auth to use the PAT directly."; + return { + installed: false, + runnable: false, + reason: "qoder_oauth_with_token", + diagnosis: makeDiagnosis("runtime_error", "local", message, "qoder_oauth_with_token"), + error: message, + }; + } + if (provider === "qoder" && connection?.authType !== "apikey") { toolId = null; } @@ -244,17 +264,9 @@ async function getProviderRuntimeStatus(connection: any) { return runtime; } - // Issue #2247: when Qoder is in OAuth/CLI-flavored mode but the user has - // pasted a Personal Access Token, the bare "CLI not installed" message - // hides the real fix — switch the connection to API Key auth. - const isQoderOauthWithToken = - provider === "qoder" && connection?.authType !== "apikey" && hasQoderToken(connection); - const runtimeMessage = runtime.installed ? `Local CLI runtime is installed but not runnable (${runtime.reason || "healthcheck_failed"})` - : isQoderOauthWithToken - ? "Qoder OAuth/Local CLI mode is selected but the Qoder CLI is not detected. If you have a Personal Access Token, switch this connection to API Key auth instead." - : "Local CLI runtime is not installed"; + : "Local CLI runtime is not installed"; return { ...runtime, diff --git a/src/sse/services/auth.ts b/src/sse/services/auth.ts index 10e7e3db10..6ef32dd173 100644 --- a/src/sse/services/auth.ts +++ b/src/sse/services/auth.ts @@ -1691,6 +1691,13 @@ export async function clearRecoveredProviderState( * * When both are present, `Authorization: Bearer` wins for back-compat * (issue #2225). + * + * The `x-api-key` fallback only triggers when the request also carries an + * `anthropic-version` header — the documented signal that the caller is + * speaking the Anthropic Messages API contract. Without this scoping, + * non-Anthropic SDKs that happen to set `x-api-key` (or local-mode tools + * with placeholder keys) would be treated as authenticated attempts and + * rejected by per-route gates that compare against OmniRoute keys. */ export function extractApiKey(request: Request) { const authHeader = request.headers.get("Authorization") || request.headers.get("authorization"); @@ -1701,12 +1708,17 @@ export function extractApiKey(request: Request) { } } // Issue #2225: Anthropic Messages API clients authenticate via x-api-key. - // Without this fallback, per-key policies are bypassed and traffic is - // recorded with null api_key_id (invisible in Costs / Analytics). - const xApiKey = request.headers.get("x-api-key") || request.headers.get("X-Api-Key"); - if (typeof xApiKey === "string") { - const trimmed = xApiKey.trim(); - if (trimmed.length > 0) return trimmed; + // Gate the fallback on the anthropic-version header so we don't trip up + // local-mode requests from non-Anthropic clients that send placeholder + // x-api-key values (which would otherwise be rejected as Invalid API key). + const anthropicVersion = + request.headers.get("anthropic-version") || request.headers.get("Anthropic-Version"); + if (anthropicVersion) { + const xApiKey = request.headers.get("x-api-key") || request.headers.get("X-Api-Key"); + if (typeof xApiKey === "string") { + const trimmed = xApiKey.trim(); + if (trimmed.length > 0) return trimmed; + } } return null; } diff --git a/tests/unit/auth-extract-api-key.test.ts b/tests/unit/auth-extract-api-key.test.ts index 48dee69e0b..8ed964e4c6 100644 --- a/tests/unit/auth-extract-api-key.test.ts +++ b/tests/unit/auth-extract-api-key.test.ts @@ -7,6 +7,8 @@ function makeRequest(headers: Record): Request { return new Request("https://omniroute.test/v1/messages", { headers }); } +const ANTHROPIC = { "anthropic-version": "2023-06-01" } as const; + test("extractApiKey returns Bearer key when Authorization header is set", () => { const req = makeRequest({ Authorization: "Bearer sk-test-bearer" }); assert.equal(extractApiKey(req), "sk-test-bearer"); @@ -27,18 +29,18 @@ test("extractApiKey is case-insensitive on the 'bearer' prefix", () => { assert.equal(extractApiKey(req), "sk-lowercase-prefix"); }); -test("extractApiKey falls back to x-api-key when Authorization is absent (#2225)", () => { - const req = makeRequest({ "x-api-key": "sk-anthropic-native" }); +test("extractApiKey falls back to x-api-key when Authorization is absent and anthropic-version is set (#2225)", () => { + const req = makeRequest({ "x-api-key": "sk-anthropic-native", ...ANTHROPIC }); assert.equal(extractApiKey(req), "sk-anthropic-native"); }); -test("extractApiKey accepts uppercase X-Api-Key header (#2225)", () => { - const req = makeRequest({ "X-Api-Key": "sk-uppercase-xapikey" }); +test("extractApiKey accepts uppercase X-Api-Key header alongside anthropic-version (#2225)", () => { + const req = makeRequest({ "X-Api-Key": "sk-uppercase-xapikey", ...ANTHROPIC }); assert.equal(extractApiKey(req), "sk-uppercase-xapikey"); }); test("extractApiKey trims surrounding whitespace from x-api-key value", () => { - const req = makeRequest({ "x-api-key": " sk-padded-xapikey " }); + const req = makeRequest({ "x-api-key": " sk-padded-xapikey ", ...ANTHROPIC }); assert.equal(extractApiKey(req), "sk-padded-xapikey"); }); @@ -46,6 +48,7 @@ test("extractApiKey prefers Bearer over x-api-key when both are present (back-co const req = makeRequest({ Authorization: "Bearer sk-bearer-wins", "x-api-key": "sk-loser", + ...ANTHROPIC, }); assert.equal(extractApiKey(req), "sk-bearer-wins"); }); @@ -56,7 +59,7 @@ test("extractApiKey returns null when neither header is present", () => { }); test("extractApiKey returns null when x-api-key contains only whitespace", () => { - const req = makeRequest({ "x-api-key": " " }); + const req = makeRequest({ "x-api-key": " ", ...ANTHROPIC }); assert.equal(extractApiKey(req), null); }); @@ -65,10 +68,24 @@ test("extractApiKey returns null when Authorization is not a Bearer scheme and x assert.equal(extractApiKey(req), null); }); -test("extractApiKey falls back to x-api-key when Authorization is a non-Bearer scheme", () => { +test("extractApiKey falls back to x-api-key when Authorization is a non-Bearer scheme (anthropic-version present)", () => { const req = makeRequest({ Authorization: "Basic ", "x-api-key": "stub-fallback-after-basic", + ...ANTHROPIC, }); assert.equal(extractApiKey(req), "stub-fallback-after-basic"); }); + +test("extractApiKey ignores x-api-key when anthropic-version is missing — protects local-mode non-Anthropic clients", () => { + const req = makeRequest({ "x-api-key": "placeholder-key" }); + assert.equal(extractApiKey(req), null); +}); + +test("extractApiKey accepts Anthropic-Version (TitleCase) header", () => { + const req = makeRequest({ + "x-api-key": "sk-titlecase-version", + "Anthropic-Version": "2024-10-22", + }); + assert.equal(extractApiKey(req), "sk-titlecase-version"); +});