fix: buffer as response from API

.changeset/polite-islands-try.md

@@ -0,0 +1,7 @@
+---
+"@rocket.chat/meteor": patch
+---
+
+Fixes buffer as response from API
+
+Currently, the only known case affected is Apps Engine icons. This patch ensures the API returns a buffer as the response, resolving issues with clients expecting a binary payload.

apps/meteor/app/api/server/definition.ts

@@ -217,15 +217,18 @@ type ActionThis<TMethod extends Method, TPathPattern extends PathPattern, TOptio
 				readonly token?: string;
 			});
 
-export type ResultFor<TMethod extends Method, TPathPattern extends PathPattern> =
+export type ResultFor<TMethod extends Method, TPathPattern extends PathPattern> = (
 	| SuccessResult<OperationResult<TMethod, TPathPattern>>
 	| FailureResult<unknown, unknown, unknown, unknown>
 	| UnauthorizedResult<unknown>
 	| NotFoundResult
 	| {
 			statusCode: number;
 			body: unknown;
-	  };
+	  }
+) & {
+	headers?: Record<string, string>;
+};
 
 export type Action<TMethod extends Method, TPathPattern extends PathPattern, TOptions> =
 	| ((this: ActionThis<TMethod, TPathPattern, TOptions>) => Promise<ResultFor<TMethod, TPathPattern>>)

apps/meteor/app/api/server/middlewares/honoAdapter.ts

@@ -25,6 +25,17 @@ export const honoAdapter = (hono: Hono) => async (expressReq: Request, res: Resp
 	);
 	res.status(honoRes.status);
 	honoRes.headers.forEach((value, key) => res.setHeader(key, value));
+
+	if (!honoRes.body) {
+		res.end();
+		return;
+	}
+
+	if (honoRes.body instanceof ReadableStream) {
+		Readable.fromWeb(honoRes.body as any).pipe(res);
+		return;
+	}
+
 	// Converting it to a Buffer because res.send appends always a charset to the Content-Type
 	// https://github.com/expressjs/express/issues/2238
 	res.send(Buffer.from(await honoRes.text()));

apps/meteor/app/api/server/router.spec.ts

@@ -1,9 +1,55 @@
+import { Readable, PassThrough } from 'stream';
+
 import Ajv from 'ajv';
+import type { Request, Response as ExpressResponse } from 'express';
 import express from 'express';
+import type { Hono } from 'hono';
 import request from 'supertest';
 
+import { honoAdapter } from './middlewares/honoAdapter';
 import { Router } from './router';
 
+// Helper to create a ReadableStream from a string
+const stringToReadableStream = (str: string): ReadableStream<Uint8Array> => {
+	const encoder = new TextEncoder();
+	return new ReadableStream({
+		start(controller) {
+			controller.enqueue(encoder.encode(str));
+			controller.close();
+		},
+	});
+};
+
+// Helper to create a ReadableStream from a Buffer
+const bufferToReadableStream = (buffer: Buffer): ReadableStream<Uint8Array> => {
+	return new ReadableStream({
+		start(controller) {
+			controller.enqueue(new Uint8Array(buffer));
+			controller.close();
+		},
+	});
+};
+
+// Helper to create a mock Express Response that captures piped data
+const createMockResponse = () => {
+	const passthrough = new PassThrough();
+	const chunks: Buffer[] = [];
+	passthrough.on('data', (chunk) => chunks.push(Buffer.from(chunk)));
+
+	// Explicitly type the mockRes before casting to Response
+	const mockResBase = passthrough as any;
+	mockResBase.status = jest.fn().mockReturnThis();
+	mockResBase.setHeader = jest.fn().mockReturnThis();
+	mockResBase.send = jest.fn().mockReturnThis();
+	mockResBase.getHeader = jest.fn();
+	mockResBase.getHeaders = jest.fn();
+
+	const getOutputBuffer = () => Buffer.concat(chunks);
+	const getOutputString = (encoding: BufferEncoding = 'utf-8') => getOutputBuffer().toString(encoding);
+
+	return { mockRes: mockResBase as ExpressResponse, getOutputBuffer, getOutputString };
+};
+
 describe('Router use method', () => {
 	it('should fail if the query request is not valid', async () => {
 		const ajv = new Ajv();
@@ -34,7 +80,7 @@ describe('Router use method', () => {
 			async function action() {
 				return {
 					statusCode: 200,
-					body: {},
+					body: { customProperty: 'valid' }, // Ensure valid response for schema
 				};
 			},
 		);
@@ -72,7 +118,7 @@ describe('Router use method', () => {
 			async function action() {
 				return {
 					statusCode: 200,
-					body: {},
+					body: { customProperty: 'valid' }, // Ensure valid response for schema
 				};
 			},
 		);
@@ -97,15 +143,14 @@ describe('Router use method', () => {
 						properties: {
 							customProperty: { type: 'string' },
 						},
-
 						additionalProperties: false,
 					}),
 				},
 			},
 			async () => {
 				return {
 					statusCode: 200,
-					body: { asda: 1 },
+					body: { asda: 1 }, // This body is invalid according to the schema
 				};
 			},
 		);
@@ -191,7 +236,7 @@ describe('Router use method', () => {
 				},
 				query: isTestQueryParams,
 			},
-			async function action() {
+			async function action(this: { queryParams: { outerProperty: { innerProperty: string } } }) {
 				const { outerProperty } = this.queryParams;
 				return {
 					statusCode: 200,
@@ -239,7 +284,7 @@ describe('Router use method', () => {
 			async function action() {
 				return {
 					statusCode: 200,
-					body: {},
+					body: { customProperty: 'test' },
 				};
 			},
 		);
@@ -278,7 +323,7 @@ describe('Router use method', () => {
 			async function action() {
 				return {
 					statusCode: 200,
-					body: {},
+					body: { customProperty: 'test' },
 				};
 			},
 		);
@@ -288,3 +333,174 @@ describe('Router use method', () => {
 		expect(response.body).toHaveProperty('error', "must have required property 'customProperty'");
 	});
 });
+
+describe('honoAdapter', () => {
+	let mockHono: Hono;
+	let mockExpressRequest: Request;
+
+	beforeEach(() => {
+		mockHono = {
+			request: jest.fn(),
+		} as unknown as Hono;
+
+		mockExpressRequest = {
+			originalUrl: '/test',
+			method: 'GET',
+			headers: { 'x-test-header': 'test-value' },
+			body: undefined, // Default, can be overridden
+			on: jest.fn(), // To satisfy Readable.isDisturbed check if it uses .on
+			readable: true, // To satisfy Readable.isDisturbed if it checks this
+			socket: { encrypted: false } as any, // for duplex='half' related logic potentially
+		} as unknown as Request;
+		(mockExpressRequest as any).duplex = 'half'; // Initialize duplex as honoAdapter expects it
+	});
+
+	it('should adapt a Hono response with JSON body', async () => {
+		const jsonData = { message: 'hello' };
+		const jsonString = JSON.stringify(jsonData);
+		const mockHonoResponse = new Response(stringToReadableStream(jsonString), {
+			status: 200,
+			headers: { 'content-type': 'application/json' },
+		});
+		(mockHono.request as jest.Mock).mockResolvedValue(mockHonoResponse);
+
+		const { mockRes, getOutputString } = createMockResponse();
+		const adapter = honoAdapter(mockHono);
+
+		await adapter(mockExpressRequest, mockRes);
+
+		expect(mockHono.request).toHaveBeenCalledWith(
+			'/test',
+			expect.objectContaining({
+				method: 'GET',
+				headers: expect.any(Headers),
+			}),
+			expect.anything(),
+		);
+		expect(mockRes.status).toHaveBeenCalledWith(200);
+		expect(mockRes.setHeader).toHaveBeenCalledWith('content-type', 'application/json');
+
+		// Wait for the stream to finish piping
+		await new Promise((resolve) => (mockRes as any).on('finish', resolve));
+		expect(getOutputString()).toBe(jsonString);
+	});
+
+	it('should adapt a Hono response with text body', async () => {
+		const textData = 'Hello, world!';
+		const mockHonoResponse = new Response(stringToReadableStream(textData), {
+			status: 201,
+			headers: { 'content-type': 'text/plain' },
+		});
+		(mockHono.request as jest.Mock).mockResolvedValue(mockHonoResponse);
+
+		const { mockRes, getOutputString } = createMockResponse();
+		const adapter = honoAdapter(mockHono);
+
+		await adapter(mockExpressRequest, mockRes);
+
+		expect(mockRes.status).toHaveBeenCalledWith(201);
+		expect(mockRes.setHeader).toHaveBeenCalledWith('content-type', 'text/plain');
+		await new Promise((resolve) => (mockRes as any).on('finish', resolve));
+		expect(getOutputString()).toBe(textData);
+	});
+
+	it('should adapt a Hono response with image (Buffer) body', async () => {
+		const imageBuffer = Buffer.from([1, 2, 3, 4, 5]); // Sample image data
+		const mockHonoResponse = new Response(bufferToReadableStream(imageBuffer), {
+			status: 200,
+			headers: { 'content-type': 'image/png' },
+		});
+		(mockHono.request as jest.Mock).mockResolvedValue(mockHonoResponse);
+
+		const { mockRes, getOutputBuffer } = createMockResponse();
+		const adapter = honoAdapter(mockHono);
+
+		await adapter(mockExpressRequest, mockRes);
+
+		expect(mockRes.status).toHaveBeenCalledWith(200);
+		expect(mockRes.setHeader).toHaveBeenCalledWith('content-type', 'image/png');
+		await new Promise((resolve) => (mockRes as any).on('finish', resolve));
+		expect(getOutputBuffer()).toEqual(imageBuffer);
+	});
+
+	it('should adapt a Hono response with no body', async () => {
+		const mockHonoResponse = new Response(null, {
+			status: 204,
+			headers: { 'x-custom-header': 'empty' },
+		});
+		(mockHono.request as jest.Mock).mockResolvedValue(mockHonoResponse);
+
+		const { mockRes, getOutputString } = createMockResponse();
+		const adapter = honoAdapter(mockHono);
+
+		await adapter(mockExpressRequest, mockRes);
+
+		expect(mockRes.status).toHaveBeenCalledWith(204);
+		expect(mockRes.setHeader).toHaveBeenCalledWith('x-custom-header', 'empty');
+		await new Promise((resolve) => (mockRes as any).on('finish', resolve));
+		expect(getOutputString()).toBe('');
+	});
+
+	it('should handle POST request with body', async () => {
+		const postBody = { data: 'some data' };
+		const postBodyString = JSON.stringify(postBody);
+
+		const mockPostRequest = {
+			...mockExpressRequest,
+			method: 'POST',
+			body: Readable.from(postBodyString) as any,
+			headers: { 'content-type': 'application/json', 'content-length': String(postBodyString.length) },
+		} as Request;
+		(mockPostRequest as any).duplex = 'half';
+
+		const mockHonoResponse = new Response(stringToReadableStream('{"status":"ok"}'), {
+			status: 200,
+			headers: { 'content-type': 'application/json' },
+		});
+		(mockHono.request as jest.Mock).mockResolvedValue(mockHonoResponse);
+
+		const { mockRes, getOutputString } = createMockResponse();
+		const adapter = honoAdapter(mockHono);
+
+		await adapter(mockPostRequest, mockRes);
+
+		expect(mockHono.request).toHaveBeenCalledWith(
+			'/test',
+			expect.objectContaining({
+				method: 'POST',
+				body: expect.anything(),
+				headers: expect.any(Headers),
+			}),
+			expect.anything(),
+		);
+		const calledWithHeaders = (mockHono.request as jest.Mock).mock.calls[0][1].headers as Headers;
+		expect(calledWithHeaders.get('content-type')).toBe('application/json');
+
+		expect(mockRes.status).toHaveBeenCalledWith(200);
+		await new Promise((resolve) => (mockRes as any).on('finish', resolve));
+		expect(getOutputString()).toBe('{"status":"ok"}');
+	});
+
+	it('should correctly set duplex and handle isDisturbed for request stream', async () => {
+		const mockHonoResponse = new Response(null, { status: 204 });
+		(mockHono.request as jest.Mock).mockResolvedValue(mockHonoResponse);
+
+		const { mockRes } = createMockResponse();
+		const adapter = honoAdapter(mockHono);
+
+		mockExpressRequest.on = jest.fn();
+		Object.defineProperty(mockExpressRequest, 'readableFlowing', { value: null, writable: true });
+
+		jest.spyOn(Readable, 'isDisturbed').mockReturnValue(false);
+		await adapter(mockExpressRequest, mockRes);
+		expect((mockExpressRequest as any).duplex).toBe('half');
+		expect(mockHono.request).toHaveBeenCalledTimes(1);
+
+		(mockHono.request as jest.Mock).mockClear();
+		jest.spyOn(Readable, 'isDisturbed').mockReturnValue(true);
+		await adapter(mockExpressRequest, mockRes);
+		expect(mockHono.request).not.toHaveBeenCalled();
+
+		jest.restoreAllMocks();
+	});
+});

apps/meteor/ee/server/apps/communication/rest.ts

@@ -1088,7 +1088,7 @@ export class AppsRestApi {
 					return {
 						statusCode: 200,
 						headers: {
-							'Content-Length': buf.length,
+							'Content-Length': String(buf.length),
 							'Content-Type': imageData[0].replace('data:', ''),
 						},
 						body: buf,