From 79c4197f07d3e55e5cd74973c43e29fbcae99779 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Wed, 16 Sep 2026 06:10:58 -0300 Subject: [PATCH] fix(security): reject unverifiable X-Cloud-Sig in cloud-sync HMAC check (#13679 PR A) (#13804) Merged in the 2026-09-16 sweep of the maintainer's own open PRs, at the owner's explicit instruction. No push was made to the PR branch: the merge took the head as the owning session left it (verified OPEN, non-draft and MERGEABLE against the release tip immediately before merging). --- .../fixes/13679-cloudsync-hmac-fail-open.md | 1 + src/lib/cloudSync.ts | 47 ++++++++-- ...loudsync-signature-fail-open-13679.test.ts | 91 +++++++++++++++++++ 3 files changed, 132 insertions(+), 7 deletions(-) create mode 100644 changelog.d/fixes/13679-cloudsync-hmac-fail-open.md create mode 100644 tests/unit/security/cloudsync-signature-fail-open-13679.test.ts diff --git a/changelog.d/fixes/13679-cloudsync-hmac-fail-open.md b/changelog.d/fixes/13679-cloudsync-hmac-fail-open.md new file mode 100644 index 0000000000..c4b72aae69 --- /dev/null +++ b/changelog.d/fixes/13679-cloudsync-hmac-fail-open.md @@ -0,0 +1 @@ +- **fix(auth):** `verifyCloudSignature()` no longer accepts an unverifiable `X-Cloud-Sig` when `OMNIROUTE_CLOUD_SYNC_SECRET` is unset — a forged/garbage signature is rejected outright, and the new opt-in `OMNIROUTE_CLOUD_SYNC_ENFORCE_SIGNATURE=true` flag rejects unsigned Cloud-sync payloads too (default stays legacy pass-through for v3.8.x; the default flips in v3.9) ([#13679](https://github.com/diegosouzapw/OmniRoute/issues/13679)) diff --git a/src/lib/cloudSync.ts b/src/lib/cloudSync.ts index 48084d0025..8f0867bd93 100644 --- a/src/lib/cloudSync.ts +++ b/src/lib/cloudSync.ts @@ -13,6 +13,14 @@ const CLOUD_SYNC_SECRET = process.env.OMNIROUTE_CLOUD_SYNC_SECRET || ""; // hostile CLOUD_URL cannot silently swap user OAuth tokens. const CLOUD_SYNC_SECRETS_ENABLED = process.env.OMNIROUTE_CLOUD_SYNC_SECRETS === "true"; +// #13679 PR A — opt-in early enforcement of the "no secret configured" branch +// below. Bringing the v3.9 enforce-by-default switch forward as an explicit +// opt-out-safe flag: default OFF preserves v3.8.x back-compat for peers that +// haven't rotated in a shared secret yet (an unsigned payload still passes). +// Set to "true" to reject even an unsigned payload when no local secret is +// configured — the default flips to enforced in v3.9. +const CLOUD_SYNC_ENFORCE_SIGNATURE = process.env.OMNIROUTE_CLOUD_SYNC_ENFORCE_SIGNATURE === "true"; + type JsonRecord = Record; function asRecord(value: unknown): JsonRecord { @@ -40,18 +48,38 @@ function toDateMs(value: unknown): number { // 2. We verify the signature with `crypto.timingSafeEqual` before parsing the // JSON, so a MITM on the CLOUD_URL channel — or a misconfigured CLOUD_URL // pointing at an attacker — cannot inject providers/tokens. -// If `OMNIROUTE_CLOUD_SYNC_SECRET` is unset, signature validation is logged but -// not enforced (back-compat for users on v3.8.x who haven't issued a shared -// secret yet). The enforce-by-default switch will flip in v3.9. +// If `OMNIROUTE_CLOUD_SYNC_SECRET` is unset, a PRESENT signature is always +// rejected (#13679 PR A — we have no key to check it against, so a signature +// we cannot verify is treated as invalid rather than blindly trusted) and an +// ABSENT signature falls through in legacy unverified mode by default +// (back-compat for users on v3.8.x who haven't issued a shared secret yet; +// opt in early via `OMNIROUTE_CLOUD_SYNC_ENFORCE_SIGNATURE=true`). The +// enforce-by-default switch for the absent-signature case will flip in v3.9. export function verifyCloudSignature(rawBody: string, sigHeader: string | null): boolean { if (!CLOUD_SYNC_SECRET) { if (sigHeader) { - // We can't verify, but the server is at least trying. Pass through. - return true; + // We have no secret to verify against, so a signature we can't check is + // treated as invalid rather than passed through (#13679 PR A item (b) — + // closes the "forge any X-Cloud-Sig and it's accepted" fail-open case). + console.warn( + "[cloudSync] OMNIROUTE_CLOUD_SYNC_SECRET is not set but the Cloud response carries an " + + "X-Cloud-Sig header — rejecting an unverifiable signature. Set the secret to enable " + + "verification." + ); + return false; + } + if (CLOUD_SYNC_ENFORCE_SIGNATURE) { + console.warn( + "[cloudSync] OMNIROUTE_CLOUD_SYNC_SECRET is not set and the Cloud response carries no " + + "X-Cloud-Sig, and OMNIROUTE_CLOUD_SYNC_ENFORCE_SIGNATURE=true — rejecting unsigned payload." + ); + return false; } console.warn( "[cloudSync] OMNIROUTE_CLOUD_SYNC_SECRET is not set and the Cloud response carries no X-Cloud-Sig. " + - "Token sync runs in legacy unverified mode — set the secret to enforce HMAC verification." + "Token sync runs in legacy unverified mode — set the secret (or " + + "OMNIROUTE_CLOUD_SYNC_ENFORCE_SIGNATURE=true) to enforce HMAC verification. This legacy " + + "pass-through default will flip to enforced in v3.9." ); return true; } @@ -207,4 +235,9 @@ async function updateLocalTokens(cloudProviders: unknown) { } } -export { CLOUD_URL, CLOUD_SYNC_TIMEOUT_MS, CLOUD_SYNC_SECRETS_ENABLED }; +export { + CLOUD_URL, + CLOUD_SYNC_TIMEOUT_MS, + CLOUD_SYNC_SECRETS_ENABLED, + CLOUD_SYNC_ENFORCE_SIGNATURE, +}; diff --git a/tests/unit/security/cloudsync-signature-fail-open-13679.test.ts b/tests/unit/security/cloudsync-signature-fail-open-13679.test.ts new file mode 100644 index 0000000000..c8c960b35c --- /dev/null +++ b/tests/unit/security/cloudsync-signature-fail-open-13679.test.ts @@ -0,0 +1,91 @@ +/** + * Regression for #13679 PR A: verifyCloudSignature() must not fail open when a + * present X-Cloud-Sig cannot be verified (no local OMNIROUTE_CLOUD_SYNC_SECRET). + * + * Before this fix: a garbage/forged X-Cloud-Sig header was ALWAYS accepted when + * the local secret was unset ("we can't verify, but the server is at least + * trying — pass through"). That let a MITM on the CLOUD_URL channel, or a + * misconfigured/compromised CLOUD_URL, forge any signature value and have it + * accepted — defeating the point of the signature check for any install that + * hasn't issued a shared secret yet. + * + * Fix (owner decision 2026-09-15, PR A): + * (b) unconditional: a PRESENT-but-unverifiable signature is now rejected, + * regardless of the opt-in enforce flag below. + * (a) opt-in only (OMNIROUTE_CLOUD_SYNC_ENFORCE_SIGNATURE=true, default OFF): + * also rejects a payload carrying NO signature at all. Default stays + * legacy pass-through for v3.8.x peers that haven't rotated in a shared + * secret yet — the default flips to enforced in v3.9. + */ +import { test } from "node:test"; +import assert from "node:assert/strict"; + +const ORIGINAL_SECRET = process.env.OMNIROUTE_CLOUD_SYNC_SECRET; +const ORIGINAL_ENFORCE = process.env.OMNIROUTE_CLOUD_SYNC_ENFORCE_SIGNATURE; + +function restoreEnv() { + if (ORIGINAL_SECRET === undefined) delete process.env.OMNIROUTE_CLOUD_SYNC_SECRET; + else process.env.OMNIROUTE_CLOUD_SYNC_SECRET = ORIGINAL_SECRET; + if (ORIGINAL_ENFORCE === undefined) delete process.env.OMNIROUTE_CLOUD_SYNC_ENFORCE_SIGNATURE; + else process.env.OMNIROUTE_CLOUD_SYNC_ENFORCE_SIGNATURE = ORIGINAL_ENFORCE; +} + +test.after(restoreEnv); + +test("issue #13679: a present-but-unverifiable X-Cloud-Sig is rejected even without a local secret", async () => { + delete process.env.OMNIROUTE_CLOUD_SYNC_SECRET; + delete process.env.OMNIROUTE_CLOUD_SYNC_ENFORCE_SIGNATURE; + try { + const { verifyCloudSignature } = await import( + `../../../src/lib/cloudSync.ts?case=13679-forged-${Date.now()}-${Math.random()}` + ); + const rawBody = JSON.stringify({ providers: [{ id: "evil", accessToken: "stolen" }] }); + const forgedSig = "0".repeat(64); + + assert.equal( + verifyCloudSignature(rawBody, forgedSig), + false, + "a garbage X-Cloud-Sig must be REJECTED even when the local secret is unset (fail-open closed)" + ); + } finally { + restoreEnv(); + } +}); + +test("issue #13679: legacy peers with NO X-Cloud-Sig header still pass by default (v3.8.x back-compat)", async () => { + delete process.env.OMNIROUTE_CLOUD_SYNC_SECRET; + delete process.env.OMNIROUTE_CLOUD_SYNC_ENFORCE_SIGNATURE; + try { + const { verifyCloudSignature } = await import( + `../../../src/lib/cloudSync.ts?case=13679-legacy-${Date.now()}-${Math.random()}` + ); + const rawBody = JSON.stringify({ providers: [] }); + + assert.equal( + verifyCloudSignature(rawBody, null), + true, + "an unsigned payload from a legacy peer must still pass through by default in v3.8.x" + ); + } finally { + restoreEnv(); + } +}); + +test("issue #13679: OMNIROUTE_CLOUD_SYNC_ENFORCE_SIGNATURE=true rejects an unsigned payload too", async () => { + delete process.env.OMNIROUTE_CLOUD_SYNC_SECRET; + process.env.OMNIROUTE_CLOUD_SYNC_ENFORCE_SIGNATURE = "true"; + try { + const { verifyCloudSignature } = await import( + `../../../src/lib/cloudSync.ts?case=13679-enforced-${Date.now()}-${Math.random()}` + ); + const rawBody = JSON.stringify({ providers: [] }); + + assert.equal( + verifyCloudSignature(rawBody, null), + false, + "with the opt-in enforce flag set, an unsigned payload must be rejected" + ); + } finally { + restoreEnv(); + } +});