Skip to content

fix(security): reject unverifiable X-Cloud-Sig in cloud-sync HMAC check (#13679 PR A) - #13804

Merged
diegosouzapw merged 2 commits into
release/v3.8.51from
fix/13679a-cloudsync-hmac-fail-open
Sep 16, 2026
Merged

diegosouzapw merged 2 commits into
release/v3.8.51from
fix/13679a-cloudsync-hmac-fail-open

Conversation

@diegosouzapw

Copy link
Copy Markdown
Owner

Refs #13679

This is PR A of the #13679 umbrella (16 verified insecure defaults) — only item #1 (cloud-sync
HMAC fail-open). The umbrella stays open until all 6 planned PRs (A→F) land.

Not covered here (tracked in the same umbrella, separate PRs):

Root cause (short)

verifyCloudSignature() in src/lib/cloudSync.ts failed open whenever
OMNIROUTE_CLOUD_SYNC_SECRET was unset: any X-Cloud-Sig header — including a completely
forged/garbage one — was accepted ("we can't verify, but the server is at least trying — pass
through"). A MITM on the CLOUD_URL channel, or a misconfigured/compromised CLOUD_URL, could
send an arbitrary signature value and have it accepted unconditionally, since the code never
actually checked it against anything in that branch.

Fix

Per the owner's decision on the #13679 umbrella (chat, 2026-09-15), PR A ships both sub-fixes for
v3.8.x, with the full default flip deferred to v3.9:

  • (b) unconditional: a present X-Cloud-Sig is now rejected whenever no local secret is
    configured — we have no key to verify it against, so an unverifiable signature is treated as
    invalid instead of blindly trusted. This closes the "forge any signature and it's accepted"
    fail-open case regardless of the flag below.
  • (a) opt-in, default OFF: a new OMNIROUTE_CLOUD_SYNC_ENFORCE_SIGNATURE=true flag brings the
    v3.9 enforce-by-default behavior forward early — when set, an absent X-Cloud-Sig is also
    rejected. Left OFF by default so v3.8.x peers that haven't rotated in a shared secret yet keep
    working unsigned (documented in the updated code comment); the default flips to enforced in
    v3.9.

Only the "no local secret configured" branch of verifyCloudSignature() changed. The
"secret configured" branch (proper HMAC-SHA256 + timingSafeEqual comparison) was already
correct and is unchanged.

Regression test

tests/unit/security/cloudsync-signature-fail-open-13679.test.ts (new, permanent suite)

RED (on unfixed origin/release/v3.8.51 code):

✖ issue #13679: a present-but-unverifiable X-Cloud-Sig is rejected even without a local secret
  AssertionError [ERR_ASSERTION]: a garbage X-Cloud-Sig must be REJECTED even when the local secret is unset (fail-open closed)
  true !== false
✔ issue #13679: legacy peers with NO X-Cloud-Sig header still pass by default (v3.8.x back-compat)
✖ issue #13679: OMNIROUTE_CLOUD_SYNC_ENFORCE_SIGNATURE=true rejects an unsigned payload too
  AssertionError [ERR_ASSERTION]: with the opt-in enforce flag set, an unsigned payload must be rejected
  true !== false
tests 3 / pass 1 / fail 2

GREEN (after the fix):

✔ issue #13679: a present-but-unverifiable X-Cloud-Sig is rejected even without a local secret
✔ issue #13679: legacy peers with NO X-Cloud-Sig header still pass by default (v3.8.x back-compat)
✔ issue #13679: OMNIROUTE_CLOUD_SYNC_ENFORCE_SIGNATURE=true rejects an unsigned payload too
tests 3 / pass 3 / fail 0

Command: DATA_DIR=$(mktemp -d) timeout 300 node --import tsx/esm --test --test-force-exit tests/unit/security/cloudsync-signature-fail-open-13679.test.ts

Gates run

  • npx eslint --suppressions-location config/quality/eslint-suppressions.json src/lib/cloudSync.ts tests/unit/security/cloudsync-signature-fail-open-13679.test.ts → clean, exit 0
  • npm run typecheck:core → clean, exit 0
  • node scripts/check/check-file-size.mjs → no violation on src/lib/cloudSync.ts
  • node scripts/check/check-complexity.mjs → OK
  • node scripts/check/check-cognitive-complexity.mjs → OK
  • node scripts/check/check-test-discovery.mjs → new test file discovered
  • Existing cloud-sync suites re-run for regression: tests/unit/security/cloud-sync-hmac.test.ts +
    tests/unit/cloud-sync.test.ts → 9/9 pass, no assertion needed changing (the pre-existing
    "falls through (legacy mode) when secret is unset" test only asserts the result is a boolean,
    compatible with the unchanged default-off legacy pass-through path)

Existing tests aligned

None needed changing — all pre-existing cloudSync/verifyCloudSignature assertions already
matched the corrected contract (they exercise the "secret configured" branch, or only assert the
result type for the "no header, no secret" legacy case).

Deviation from the plan-file

The plan-file's own repro test asserted that BOTH a forged signature and a fully absent
X-Cloud-Sig must be rejected under default settings. Per the owner's decision block (which
supersedes the plan-file body), the shipped v3.8.x behavior instead keeps the absent-signature
("legacy peer") case passing by default and gates full enforcement behind
OMNIROUTE_CLOUD_SYNC_ENFORCE_SIGNATURE — the default flips in v3.9. The regression test was
written to match this reconciled decision rather than the plan-file's original blanket-rejection
repro.

diegosouzapw and others added 2 commits September 15, 2026 19:01
…ck (#13679 PR A)

verifyCloudSignature() previously fell open whenever OMNIROUTE_CLOUD_SYNC_SECRET
was unset: any X-Cloud-Sig header, including a forged/garbage one, was accepted
unconditionally. A MITM on the CLOUD_URL channel (or a compromised/misconfigured
CLOUD_URL) could send an arbitrary signature and have it accepted.

Per the owner's decision on the #13679 umbrella (PR A): a PRESENT-but-unverifiable
signature is now rejected outright, regardless of any flag. A new opt-in
OMNIROUTE_CLOUD_SYNC_ENFORCE_SIGNATURE=true flag (default OFF) also rejects an
ABSENT signature, bringing the v3.9 enforce-by-default plan forward early without
breaking v3.8.x peers that haven't rotated in a shared secret yet.

Regression test: tests/unit/security/cloudsync-signature-fail-open-13679.test.ts
@diegosouzapw
diegosouzapw merged commit 79c4197 into release/v3.8.51 Sep 16, 2026
16 of 21 checks passed
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…ck (diegosouzapw#13679 PR A) (diegosouzapw#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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant