diff --git a/.changeset/polite-islands-try.md b/.changeset/polite-islands-try.md new file mode 100644 index 0000000000000..3d25bf4a57854 --- /dev/null +++ b/.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. diff --git a/apps/meteor/app/api/server/definition.ts b/apps/meteor/app/api/server/definition.ts index 69e2736e5abd0..44be5a934926a 100644 --- a/apps/meteor/app/api/server/definition.ts +++ b/apps/meteor/app/api/server/definition.ts @@ -217,7 +217,7 @@ type ActionThis = +export type ResultFor = ( | SuccessResult> | FailureResult | UnauthorizedResult @@ -225,7 +225,10 @@ export type ResultFor | { statusCode: number; body: unknown; - }; + } +) & { + headers?: Record; +}; export type Action = | ((this: ActionThis) => Promise>) diff --git a/apps/meteor/app/api/server/middlewares/honoAdapter.ts b/apps/meteor/app/api/server/middlewares/honoAdapter.ts index 86d9e83cb3a1b..36ee9ac1ca221 100644 --- a/apps/meteor/app/api/server/middlewares/honoAdapter.ts +++ b/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())); diff --git a/apps/meteor/app/api/server/router.spec.ts b/apps/meteor/app/api/server/router.spec.ts index deaa3a6335be3..fb902037912a0 100644 --- a/apps/meteor/app/api/server/router.spec.ts +++ b/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 => { + 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 => { + 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,7 +143,6 @@ describe('Router use method', () => { properties: { customProperty: { type: 'string' }, }, - additionalProperties: false, }), }, @@ -105,7 +150,7 @@ describe('Router use method', () => { 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(); + }); +}); diff --git a/apps/meteor/ee/server/apps/communication/rest.ts b/apps/meteor/ee/server/apps/communication/rest.ts index 664e7da39308a..9df3555448d6a 100644 --- a/apps/meteor/ee/server/apps/communication/rest.ts +++ b/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,