bluebubbles: close SSRF bypass when user opts out of private network (aisle #68234 HIGH)

omarshahine · omarshahine · commit 3ef012f20c0e · 2026-04-18T03:41:42.000Z
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.
diff --git 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<string, unknown>;
-    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
@@ -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
@@ -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;
   }
 
