From 687f256e4c0afdbab59150875d6dfac003401598 Mon Sep 17 00:00:00 2001 From: Omar Shahine <10343873+omarshahine@users.noreply.github.com> Date: Sat, 18 Apr 2026 03:41:42 +0000 Subject: [PATCH] bluebubbles: close SSRF bypass when user opts out of private network (aisle #68234 HIGH) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Aisle security analysis flagged `resolveBlueBubblesClientSsrfPolicy` mode 3 as an SSRF bypass: it returned `ssrfPolicy: undefined`, which `blueBubblesFetchWithTimeout` treats as a signal to skip `fetchWithSsrFGuard` and call `fetch()` directly. Mode 3 fires in two cases: A. Private hostname + user explicitly opted out (`network.dangerouslyAllowPrivateNetwork: false`). The user asked us to block private networks and we instead sent the request via the unguarded fallback path — a real bypass. B. Unparseable baseUrl. Would route through the unguarded path (cosmetic in practice since the fetch would fail elsewhere, but still wrong). Fix: return `ssrfPolicy: {}` (default-deny guarded policy) instead of `undefined`. All three modes now go through `fetchWithSsrFGuard`: 1. `{ allowPrivateNetwork: true }` — user opted in 2. `{ allowedHostnames: [host] }` — narrow allowlist for trusted host 3. `{}` — default-deny, honors opt-out Tightened `ssrfPolicy` field/param types from `SsrFPolicy | undefined` to `SsrFPolicy` so the bypass cannot be reintroduced by a future caller handing back `undefined`. Added a mode-3 invariant regression test that walks every resolution case and asserts `ssrfPolicy` is always defined (caught any future `undefined` reintroduction). Updated `attachments.test.ts` case that was asserting the old (buggy) `undefined` behavior. Existing-user impact: only users with `dangerouslyAllowPrivateNetwork: false` set on a private-address BB server see a behavior change (they now get a guarded refusal instead of an accidental success). That's the correct posture per their config. --- .../bluebubbles/src/attachments.test.ts | 6 +++- extensions/bluebubbles/src/client.test.ts | 31 ++++++++++++++++--- extensions/bluebubbles/src/client.ts | 23 +++++++++----- 3 files changed, 47 insertions(+), 13 deletions(-) diff --git a/extensions/bluebubbles/src/attachments.test.ts b/extensions/bluebubbles/src/attachments.test.ts index 48330ee9286..acf8e9be437 100644 --- a/extensions/bluebubbles/src/attachments.test.ts +++ b/extensions/bluebubbles/src/attachments.test.ts @@ -341,8 +341,12 @@ describe("downloadBlueBubblesAttachment", () => { }, }); + // Default-deny policy via the guard, NOT unguarded fetch. Aisle #68234 + // flagged the previous `undefined` fallback as a real SSRF bypass because + // `blueBubblesFetchWithTimeout` treats `undefined` as "skip the SSRF + // guard entirely", exactly when the user asked us to block private nets. const fetchMediaArgs = fetchRemoteMediaMock.mock.calls[0][0] as Record; - expect(fetchMediaArgs.ssrfPolicy).toBeUndefined(); + expect(fetchMediaArgs.ssrfPolicy).toEqual({}); }); it("allowlists public serverUrl hostname when allowPrivateNetwork is not set", async () => { diff --git a/extensions/bluebubbles/src/client.test.ts b/extensions/bluebubbles/src/client.test.ts index 2610f56eb2d..441fa648baa 100644 --- a/extensions/bluebubbles/src/client.test.ts +++ b/extensions/bluebubbles/src/client.test.ts @@ -115,24 +115,47 @@ describe("resolveBlueBubblesClientSsrfPolicy (3-mode policy)", () => { expect(result.trustedHostnameIsPrivate).toBe(false); }); - it("mode 3: private hostname + explicit opt-out → undefined (falls back to non-SSRF path)", () => { + it("mode 3: private hostname + explicit opt-out → {} (guarded default-deny, honors the opt-out) (aisle #68234)", () => { + // Previously returned `undefined`, which routed through the unguarded + // fetch fallback and effectively bypassed SSRF protection exactly when + // the user had explicitly asked to disable private-network access. const result = resolveBlueBubblesClientSsrfPolicy({ baseUrl: "http://192.168.1.50:1234", allowPrivateNetwork: false, allowPrivateNetworkConfig: false, }); - expect(result.ssrfPolicy).toBeUndefined(); + expect(result.ssrfPolicy).toEqual({}); expect(result.trustedHostnameIsPrivate).toBe(true); }); - it("mode 3: unparseable baseUrl → undefined policy", () => { + it("mode 3: unparseable baseUrl → {} (fail-safe guarded, never bypass)", () => { const result = resolveBlueBubblesClientSsrfPolicy({ baseUrl: "not a url", allowPrivateNetwork: false, }); - expect(result.ssrfPolicy).toBeUndefined(); + expect(result.ssrfPolicy).toEqual({}); expect(result.trustedHostname).toBeUndefined(); }); + + it("never returns undefined ssrfPolicy — every mode is guarded (aisle #68234 invariant)", () => { + // This invariant is what closes the SSRF bypass aisle flagged. Any + // refactor that reintroduces `ssrfPolicy: undefined` should break here. + const cases = [ + { baseUrl: "http://localhost:1234", allowPrivateNetwork: true }, + { baseUrl: "http://localhost:1234", allowPrivateNetwork: false }, + { + baseUrl: "http://192.168.1.50:1234", + allowPrivateNetwork: false, + allowPrivateNetworkConfig: false, + }, + { baseUrl: "https://bb.example.com", allowPrivateNetwork: false }, + { baseUrl: "not a url", allowPrivateNetwork: false }, + ]; + for (const c of cases) { + const result = resolveBlueBubblesClientSsrfPolicy(c); + expect(result.ssrfPolicy).toBeDefined(); + } + }); }); // --- Auth strategies ------------------------------------------------------- diff --git a/extensions/bluebubbles/src/client.ts b/extensions/bluebubbles/src/client.ts index 6f998f88fbb..2454f03f05d 100644 --- a/extensions/bluebubbles/src/client.ts +++ b/extensions/bluebubbles/src/client.ts @@ -87,7 +87,9 @@ function safeExtractHostname(baseUrl: string): string | undefined { } /** - * Resolve the BB client's SSRF policy at construction time. Three modes: + * Resolve the BB client's SSRF policy at construction time. Three modes — + * all of which go through `fetchWithSsrFGuard`; we never hand back a policy + * that skips the guard: * * 1. `{ allowPrivateNetwork: true }` — user explicitly opted in * (`network.dangerouslyAllowPrivateNetwork: true`). Private/loopback @@ -99,8 +101,11 @@ function safeExtractHostname(baseUrl: string): string | undefined { * that closes #34749, #57181, #59722, #60715 for self-hosted BB on * private/localhost addresses without requiring a full opt-in. * - * 3. `undefined` — no policy; use the non-SSRF fallback path. Applied only - * when we can't identify a trusted hostname. (#64105) + * 3. `{}` — guarded with the default-deny policy. Applied when we can't + * produce a valid allowlist (opt-out on a private hostname, or an + * unparseable baseUrl). Previously returned `undefined` and skipped + * the guard entirely, which was an SSRF bypass when a user explicitly + * opted out of private-network access. Aisle #68234 found this. * * Prior to this helper, the logic lived inline in `attachments.ts` and was * inconsistently replicated across 15+ callsites. Resolving once ensures @@ -111,7 +116,7 @@ export function resolveBlueBubblesClientSsrfPolicy(params: { allowPrivateNetwork: boolean; allowPrivateNetworkConfig?: boolean; }): { - ssrfPolicy: SsrFPolicy | undefined; + ssrfPolicy: SsrFPolicy; trustedHostname?: string; trustedHostnameIsPrivate: boolean; } { @@ -137,7 +142,9 @@ export function resolveBlueBubblesClientSsrfPolicy(params: { }; } - return { ssrfPolicy: undefined, trustedHostname, trustedHostnameIsPrivate }; + // Mode 3: default-deny guard. Honors an explicit opt-out on a private + // hostname and fails-safe on unparseable URLs. Never undefined. (aisle #68234) + return { ssrfPolicy: {}, trustedHostname, trustedHostnameIsPrivate }; } // --- Client ---------------------------------------------------------------- @@ -155,7 +162,7 @@ type ClientConstructorParams = { accountId: string; baseUrl: string; password: string; - ssrfPolicy: SsrFPolicy | undefined; + ssrfPolicy: SsrFPolicy; trustedHostname: string | undefined; trustedHostnameIsPrivate: boolean; defaultTimeoutMs: number; @@ -181,7 +188,7 @@ export class BlueBubblesClient { readonly trustedHostnameIsPrivate: boolean; private readonly password: string; - private readonly ssrfPolicy: SsrFPolicy | undefined; + private readonly ssrfPolicy: SsrFPolicy; private readonly defaultTimeoutMs: number; private readonly authStrategy: BlueBubblesAuthStrategy; @@ -200,7 +207,7 @@ export class BlueBubblesClient { * Read the resolved SSRF policy for this client. Exposed primarily for tests * and diagnostics; production code should never need to inspect it. */ - getSsrfPolicy(): SsrFPolicy | undefined { + getSsrfPolicy(): SsrFPolicy { return this.ssrfPolicy; }