Skip to content

Commit 6dfb043

Browse files
omarshahineomarshahine
committed
bluebubbles: address Greptile review (P1 auth-header forwarding, P2 cache fingerprint)
Greptile findings from the first review cycle on #68234 — both real, both asymptomatic under today's query-string auth default but blocking for the header-auth flip (#66869). P1: Thread auth-decorated headers through multipart + media paths. - `postMultipartFormData` now accepts an optional `extraHeaders` param and merges caller headers with its own Content-Type (Content-Type always wins so the multipart boundary stays authoritative). - `client.requestMultipart` forwards `prepared.init.headers` — attachment uploads and group-icon sets keep working under header auth. - `client.downloadAttachment` fetchImpl merges `prepared.init.headers` with runtime-supplied headers (runtime wins on conflict, e.g. Range for partial reads) — attachment downloads via `fetchRemoteMedia` now carry auth too. P2: Include auth strategy in the client cache fingerprint. - `BlueBubblesAuthStrategy` gets a stable `id` field (`query-string` / `header:<name>`). - Built-in factories set it; the cache key is now `{baseUrl}|{password}|{authStrategyId}` so different strategies for the same account + credential no longer silently share a cached instance. Three new regression tests pin the above behavior (465 total, was 462). Full BB suite green, lint clean.
1 parent 783cad5 commit 6dfb043

3 files changed

Lines changed: 125 additions & 12 deletions

File tree

‎extensions/bluebubbles/src/client.test.ts‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,53 @@ describe("auth strategies", () => {
191191
const headers = new Headers((calledInit as RequestInit | undefined)?.headers);
192192
expect(headers.get("X-BB-Password")).toBe("s3cret");
193193
});
194+
195+
it("header-auth headers flow through requestMultipart (Greptile #68234 P1)", async () => {
196+
// Before this fix, requestMultipart discarded prepared.init entirely
197+
// and postMultipartFormData built its own hardcoded Content-Type header.
198+
// Under header-auth that silently omitted the auth header on every
199+
// attachment upload and group-icon set.
200+
const client = createBlueBubblesClient({
201+
serverUrl: "http://localhost:1234",
202+
password: "s3cret",
203+
authStrategy: blueBubblesHeaderAuth,
204+
});
205+
mockFetch.mockImplementation(() => Promise.resolve(new Response("{}", { status: 200 })));
206+
await client.requestMultipart({
207+
path: "/api/v1/chat/chat-guid/icon",
208+
boundary: "----boundary",
209+
parts: [new Uint8Array([1, 2, 3])],
210+
});
211+
const [, calledInit] = mockFetch.mock.calls[0] ?? [];
212+
const headers = new Headers((calledInit as RequestInit | undefined)?.headers);
213+
expect(headers.get("X-BB-Password")).toBe("s3cret");
214+
// And the multipart Content-Type must still be set correctly.
215+
expect(headers.get("Content-Type")).toContain("multipart/form-data; boundary=----boundary");
216+
});
217+
218+
it("header-auth headers flow through downloadAttachment fetchImpl (Greptile #68234 P1)", async () => {
219+
// Before this fix, downloadAttachment built prepared.init.headers with
220+
// the auth header but never forwarded it to the fetchImpl callback,
221+
// so header-auth would silently 401 on attachment downloads.
222+
const client = createBlueBubblesClient({
223+
serverUrl: "http://localhost:1234",
224+
password: "s3cret",
225+
authStrategy: blueBubblesHeaderAuth,
226+
});
227+
mockFetch.mockImplementation(() =>
228+
Promise.resolve(
229+
new Response(Buffer.from([1, 2, 3]), {
230+
status: 200,
231+
headers: { "content-type": "image/png" },
232+
}),
233+
),
234+
);
235+
await client.downloadAttachment({ attachment: { guid: "att-1", mimeType: "image/png" } });
236+
// fetchRemoteMediaMock delegates to fetchImpl, which calls mockFetch.
237+
const [, calledInit] = mockFetch.mock.calls[0] ?? [];
238+
const headers = new Headers((calledInit as RequestInit | undefined)?.headers);
239+
expect(headers.get("X-BB-Password")).toBe("s3cret");
240+
});
194241
});
195242

196243
// --- Core request path -----------------------------------------------------
@@ -494,6 +541,23 @@ describe("client cache", () => {
494541
});
495542
expect(b.baseUrl).toBe("http://host-b:1234");
496543
});
544+
545+
it("different authStrategy for the same account + credential rebuilds the client (Greptile #68234 P2)", () => {
546+
// Before this fix the fingerprint keyed only on {baseUrl, password}.
547+
// A second call with a different authStrategy would silently return
548+
// the cached first strategy's client.
549+
const a = createBlueBubblesClient({
550+
serverUrl: "http://localhost:1234",
551+
password: "s3cret",
552+
// default: blueBubblesQueryStringAuth
553+
});
554+
const b = createBlueBubblesClient({
555+
serverUrl: "http://localhost:1234",
556+
password: "s3cret",
557+
authStrategy: blueBubblesHeaderAuth,
558+
});
559+
expect(a).not.toBe(b);
560+
});
497561
});
498562

499563
describe("client construction", () => {

‎extensions/bluebubbles/src/client.ts‎

Lines changed: 44 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -42,11 +42,19 @@ const DEFAULT_MULTIPART_TIMEOUT_MS = 60_000;
4242
* BB Server ships the header-auth change for #66869.
4343
*/
4444
export interface BlueBubblesAuthStrategy {
45+
/**
46+
* Stable identifier for this strategy. Used by the client cache fingerprint
47+
* so two clients for the same account + credential that differ only in auth
48+
* strategy don't silently collapse onto the same cached instance.
49+
* (Greptile #68234 P2)
50+
*/
51+
readonly id: string;
4552
decorate(req: { url: URL; init: RequestInit }): void;
4653
}
4754

4855
export function blueBubblesQueryStringAuth(password: string): BlueBubblesAuthStrategy {
4956
return {
57+
id: "query-string",
5058
decorate({ url }) {
5159
url.searchParams.set("password", password);
5260
},
@@ -58,6 +66,7 @@ export function blueBubblesHeaderAuth(
5866
headerName = "X-BB-Password",
5967
): BlueBubblesAuthStrategy {
6068
return {
69+
id: `header:${headerName}`,
6170
decorate({ init }) {
6271
const headers = new Headers(init.headers ?? undefined);
6372
headers.set(headerName, password);
@@ -265,6 +274,9 @@ export class BlueBubblesClient {
265274
* Multipart POST (attachment send, group icon set). The caller supplies the
266275
* boundary and body parts; the client handles URL construction, auth, and
267276
* SSRF policy. Timeout defaults to 60s because uploads can be large.
277+
*
278+
* Auth-decorated headers from `prepared.init` are forwarded via `extraHeaders`
279+
* so header-auth strategies keep working on multipart paths. (Greptile #68234 P1)
268280
*/
269281
async requestMultipart(params: {
270282
path: string;
@@ -283,6 +295,7 @@ export class BlueBubblesClient {
283295
parts: params.parts,
284296
timeoutMs: params.timeoutMs ?? DEFAULT_MULTIPART_TIMEOUT_MS,
285297
ssrfPolicy: this.ssrfPolicy,
298+
extraHeaders: prepared.init.headers,
286299
});
287300
}
288301

@@ -387,20 +400,33 @@ export class BlueBubblesClient {
387400
});
388401
const clientSsrfPolicy = this.ssrfPolicy;
389402
const effectiveTimeoutMs = params.timeoutMs ?? this.defaultTimeoutMs;
403+
// Auth-decorated headers from buildAuthorizedRequest (for header-auth
404+
// strategies) must flow through the fetchImpl callback too, otherwise
405+
// the runtime might dispatch with only its own default headers. Merge
406+
// prepared.init.headers with any headers the runtime supplies; runtime
407+
// headers (typically Range for partial reads) win on conflict.
408+
// (Greptile #68234 P1)
409+
const preparedHeaders = prepared.init.headers;
390410

391411
try {
392412
const fetched = await getBlueBubblesRuntime().channel.media.fetchRemoteMedia({
393413
url: prepared.url,
394414
filePathHint: params.attachment.transferName ?? params.attachment.guid ?? "attachment",
395415
maxBytes,
396416
ssrfPolicy: clientSsrfPolicy,
397-
fetchImpl: async (input, init) =>
398-
await blueBubblesFetchWithTimeout(
417+
fetchImpl: async (input, init) => {
418+
const mergedHeaders = new Headers(preparedHeaders);
419+
if (init?.headers) {
420+
const runtimeHeaders = new Headers(init.headers);
421+
runtimeHeaders.forEach((value, key) => mergedHeaders.set(key, value));
422+
}
423+
return await blueBubblesFetchWithTimeout(
399424
resolveRequestUrl(input),
400-
{ ...init, method: init?.method ?? "GET" },
425+
{ ...init, method: init?.method ?? "GET", headers: mergedHeaders },
401426
effectiveTimeoutMs,
402427
clientSsrfPolicy,
403-
),
428+
);
429+
},
404430
});
405431
return {
406432
buffer: new Uint8Array(fetched.buffer),
@@ -423,13 +449,20 @@ export class BlueBubblesClient {
423449

424450
type CachedClientEntry = {
425451
client: BlueBubblesClient;
426-
/** Fingerprint of {baseUrl, password} — cache hit requires full match. */
452+
/** Fingerprint of {baseUrl, password, authStrategy.id} — cache hit requires full match. */
427453
fingerprint: string;
428454
};
429455
const clientFingerprints = new Map<string, CachedClientEntry>();
430456

431-
function buildClientFingerprint(params: { baseUrl: string; password: string }): string {
432-
return `${params.baseUrl}|${params.password}`;
457+
function buildClientFingerprint(params: {
458+
baseUrl: string;
459+
password: string;
460+
authStrategyId: string;
461+
}): string {
462+
// authStrategyId is included so two clients for the same account + credential
463+
// that differ only in auth strategy do not silently share a cached instance.
464+
// (Greptile #68234 P2)
465+
return `${params.baseUrl}|${params.password}|${params.authStrategyId}`;
433466
}
434467

435468
/**
@@ -447,9 +480,12 @@ export function createBlueBubblesClient(opts: BlueBubblesClientOptions = {}): Bl
447480
password: opts.password,
448481
});
449482
const cacheKey = resolved.accountId || DEFAULT_ACCOUNT_ID;
483+
const authFactory = opts.authStrategy ?? blueBubblesQueryStringAuth;
484+
const authStrategy = authFactory(resolved.password);
450485
const fingerprint = buildClientFingerprint({
451486
baseUrl: resolved.baseUrl,
452487
password: resolved.password,
488+
authStrategyId: authStrategy.id,
453489
});
454490
const cached = clientFingerprints.get(cacheKey);
455491
if (cached && cached.fingerprint === fingerprint) {
@@ -461,7 +497,6 @@ export function createBlueBubblesClient(opts: BlueBubblesClientOptions = {}): Bl
461497
allowPrivateNetwork: resolved.allowPrivateNetwork,
462498
allowPrivateNetworkConfig: resolved.allowPrivateNetworkConfig,
463499
});
464-
const authFactory = opts.authStrategy ?? blueBubblesQueryStringAuth;
465500

466501
const client = new BlueBubblesClient({
467502
accountId: cacheKey,
@@ -471,7 +506,7 @@ export function createBlueBubblesClient(opts: BlueBubblesClientOptions = {}): Bl
471506
trustedHostname: policyResult.trustedHostname,
472507
trustedHostnameIsPrivate: policyResult.trustedHostnameIsPrivate,
473508
defaultTimeoutMs: opts.timeoutMs ?? DEFAULT_TIMEOUT_MS,
474-
authStrategy: authFactory(resolved.password),
509+
authStrategy,
475510
});
476511
clientFingerprints.set(cacheKey, { client, fingerprint });
477512
return client;

‎extensions/bluebubbles/src/multipart.ts‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,15 +18,29 @@ export async function postMultipartFormData(params: {
1818
parts: Uint8Array[];
1919
timeoutMs: number;
2020
ssrfPolicy?: SsrFPolicy;
21+
/**
22+
* Extra headers to merge with the multipart Content-Type. Used to forward
23+
* auth-decorated headers from `BlueBubblesClient` (e.g. `X-BB-Password`
24+
* under header-auth mode). Per-request Content-Type wins over callers so
25+
* the multipart boundary is always authoritative. (Greptile #68234 P1)
26+
*/
27+
extraHeaders?: HeadersInit;
2128
}): Promise<Response> {
2229
const body = Buffer.from(concatUint8Arrays(params.parts));
30+
const headers: Record<string, string> = {};
31+
if (params.extraHeaders) {
32+
new Headers(params.extraHeaders).forEach((value, key) => {
33+
headers[key] = value;
34+
});
35+
}
36+
// Per-request Content-Type wins over callers so the multipart boundary is
37+
// always authoritative.
38+
headers["Content-Type"] = `multipart/form-data; boundary=${params.boundary}`;
2339
return await blueBubblesFetchWithTimeout(
2440
params.url,
2541
{
2642
method: "POST",
27-
headers: {
28-
"Content-Type": `multipart/form-data; boundary=${params.boundary}`,
29-
},
43+
headers,
3044
body,
3145
},
3246
params.timeoutMs,

0 commit comments

Comments
 (0)