Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions packages/desktop/src/main/net/safe-fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,27 @@ export async function safeFetch(url: string, opts: SafeFetchOptions = {}): Promi
const controller = new AbortController();
const timeout = setTimeout(() => controller.abort(), opts.timeoutMs ?? DEFAULT_TIMEOUT_MS);
try {
const res = await net.fetch(url, { signal: controller.signal });
const res = await net.fetch(url, { signal: controller.signal, redirect: 'error' });
if (!res.ok) throw new Error(`fetch ${url}: ${res.status}`);
const cl = Number(res.headers.get('content-length') ?? '0');
if (cl > maxSize) throw new Error('response too large');
const ab = await res.arrayBuffer();
if (ab.byteLength > maxSize) throw new Error('response too large');
return Buffer.from(ab);

const reader = res.body?.getReader();
if (!reader) throw new Error('no body');
Comment on lines +37 to +38
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current implementation throws an error if res.body is null (e.g., for a 204 No Content response). The previous implementation using res.arrayBuffer() would return an empty buffer in such cases. To maintain compatibility and handle body-less responses gracefully, consider returning an empty buffer instead of throwing.

Suggested change
const reader = res.body?.getReader();
if (!reader) throw new Error('no body');
if (!res.body) return Buffer.alloc(0);
const reader = res.body.getReader();


const chunks: Uint8Array[] = [];
let total = 0;
while (true) {
const { done, value } = await reader.read();
if (done) break;
total += value.byteLength;
if (total > maxSize) {
controller.abort();
throw new Error('response too large');
}
chunks.push(value);
}
return Buffer.concat(chunks.map((c) => Buffer.from(c)));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Buffer.concat accepts an array of Uint8Array directly, so mapping chunks to Buffer instances is unnecessary. Additionally, since the total length is already tracked in the total variable, providing it to Buffer.concat avoids an extra iteration to calculate the length.

Suggested change
return Buffer.concat(chunks.map((c) => Buffer.from(c)));
return Buffer.concat(chunks, total);

} finally {
clearTimeout(timeout);
}
Expand Down
55 changes: 54 additions & 1 deletion packages/desktop/test/safe-fetch.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,31 @@ beforeEach(() => {
netFetchMock.mockReset();
});

function streamReaderFrom(body: ArrayBuffer, chunkSize = 1024): ReadableStreamDefaultReader<Uint8Array> {
const u8 = new Uint8Array(body);
let offset = 0;
return {
read: async () => {
if (offset >= u8.length) return { done: true as const, value: undefined };
const end = Math.min(offset + chunkSize, u8.length);
const value = u8.slice(offset, end);
offset = end;
return { done: false as const, value };
},
cancel: async () => {},
releaseLock: () => {},
closed: Promise.resolve(undefined),
} as ReadableStreamDefaultReader<Uint8Array>;
}

function mockResponse(body: ArrayBuffer, status = 200, headers: Record<string, string> = {}): Response {
return {
ok: status >= 200 && status < 300,
status,
headers: new Headers(headers),
arrayBuffer: () => Promise.resolve(body),
body: {
getReader: () => streamReaderFrom(body),
},
} as unknown as Response;
}

Expand Down Expand Up @@ -99,4 +118,38 @@ describe('safeFetch', () => {

await expect(safeFetch('https://cdn.example.com/missing.png')).rejects.toThrow('404');
});

it('passes redirect:error to net.fetch to prevent SSRF bypass via 3xx', async () => {
assertNotPrivateHostMock.mockResolvedValue(undefined);
netFetchMock.mockResolvedValue(mockResponse(new ArrayBuffer(4)));

await safeFetch('https://cdn.example.com/img.png');
expect(netFetchMock).toHaveBeenCalledWith(expect.any(String), expect.objectContaining({ redirect: 'error' }));
});

it('enforces maxSize incrementally during streaming — no content-length', async () => {
assertNotPrivateHostMock.mockResolvedValue(undefined);
const big = new ArrayBuffer(10 * 1024 * 1024 + 1); // just over default 10MiB
netFetchMock.mockResolvedValue(mockResponse(big, 200, {})); // no content-length header

await expect(safeFetch('https://cdn.example.com/streaming.bin')).rejects.toThrow('too large');
});

it('handles empty response body', async () => {
assertNotPrivateHostMock.mockResolvedValue(undefined);
netFetchMock.mockResolvedValue(mockResponse(new ArrayBuffer(0)));

const buf = await safeFetch('https://cdn.example.com/empty.bin');
expect(Buffer.isBuffer(buf)).toBe(true);
expect(buf.length).toBe(0);
});

it('reads multi-chunk body correctly', async () => {
assertNotPrivateHostMock.mockResolvedValue(undefined);
const data = new TextEncoder().encode('hello world').buffer;
netFetchMock.mockResolvedValue(mockResponse(data, 200, {}));

const buf = await safeFetch('https://cdn.example.com/hello.bin');
expect(buf.toString()).toBe('hello world');
});
});