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 25ae81f08edb7..960bdac971151 100644 --- a/apps/meteor/app/api/server/definition.ts +++ b/apps/meteor/app/api/server/definition.ts @@ -206,7 +206,7 @@ type ActionThis = +export type ResultFor = ( | SuccessResult> | FailureResult | UnauthorizedResult @@ -214,7 +214,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 5c11ef6e0456f..02b2d37cc6984 100644 --- a/apps/meteor/app/api/server/router.spec.ts +++ b/apps/meteor/app/api/server/router.spec.ts @@ -1,10 +1,133 @@ +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(); + const app = express(); + const api = new Router('/api'); + const test = new Router('/test').get( + '/', + { + typed: true, + query: ajv.compile({ + type: 'object', + properties: { + customProperty: { type: 'string' }, + }, + additionalProperties: false, + required: ['customProperty'], + }), + response: { + 200: ajv.compile({ + type: 'object', + properties: { + customProperty: { type: 'string' }, + }, + additionalProperties: false, + }), + }, + }, + async function action() { + return { + statusCode: 200, + body: { customProperty: 'valid' }, // Ensure valid response for schema + }; + }, + ); + app.use(api.use(test).router); + const response = await request(app).get('/api/test'); + expect(response.statusCode).toBe(400); + expect(response.body).toHaveProperty('error', "must have required property 'customProperty'"); + }); + it('should fail if the body request is not valid', async () => { + const ajv = new Ajv(); + const app = express(); + const api = new Router('/api'); + const test = new Router('/test').post( + '/', + { + typed: true, + body: ajv.compile({ + type: 'object', + properties: { + customProperty: { type: 'string' }, + }, + additionalProperties: false, + required: ['customProperty'], + }), + response: { + 200: ajv.compile({ + type: 'object', + properties: { + customProperty: { type: 'string' }, + }, + additionalProperties: false, + }), + }, + }, + async function action() { + return { + statusCode: 200, + body: { customProperty: 'valid' }, // Ensure valid response for schema + }; + }, + ); + app.use(api.use(test).router); + const response = await request(app).post('/api/test').send({}); + expect(response.statusCode).toBe(400); + expect(response.body).toHaveProperty('error', "must have required property 'customProperty'"); + }); + it('middleware should be applied only on the right path', async () => { const ajv = new Ajv(); const app = express(); @@ -88,4 +211,252 @@ describe('Router use method', () => { expect(response1.body).toHaveProperty('outerProperty'); expect(response1.body.outerProperty).toHaveProperty('innerProperty', 'test'); }); + it('should fail if the delete request body is not valid', async () => { + const ajv = new Ajv(); + const app = express(); + const api = new Router('/api'); + const test = new Router('/test').delete( + '/', + { + typed: true, + body: ajv.compile({ + type: 'object', + properties: { + customProperty: { type: 'string' }, + }, + additionalProperties: false, + required: ['customProperty'], + }), + response: { + 200: ajv.compile({ + type: 'object', + properties: { + customProperty: { type: 'string' }, + }, + additionalProperties: false, + }), + }, + }, + async function action() { + return { + statusCode: 200, + body: { customProperty: 'test' }, + }; + }, + ); + app.use(api.use(test).router); + const response = await request(app).delete('/api/test').send({}); + expect(response.statusCode).toBe(400); + expect(response.body).toHaveProperty('error', "must have required property 'customProperty'"); + }); + + it('should fail if the put request body is not valid', async () => { + const ajv = new Ajv(); + const app = express(); + const api = new Router('/api'); + const test = new Router('/test').put( + '/', + { + typed: true, + body: ajv.compile({ + type: 'object', + properties: { + customProperty: { type: 'string' }, + }, + additionalProperties: false, + required: ['customProperty'], + }), + response: { + 200: ajv.compile({ + type: 'object', + properties: { + customProperty: { type: 'string' }, + }, + additionalProperties: false, + }), + }, + }, + async function action() { + return { + statusCode: 200, + body: { customProperty: 'test' }, + }; + }, + ); + app.use(api.use(test).router); + const response = await request(app).put('/api/test').send({}); + expect(response.statusCode).toBe(400); + 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 8af2aefe6be6d..4cf55bbc4bb78 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,