chore: add hono as router #35078
chore: add hono as router #35078
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
|
ggazzo
force-pushed
the
chore/remove-restivus
branch
from
78b097a to
bc242f5
Compare
ggazzo
force-pushed
the
chore/hono
branch
2 times, most recently
from
c7f983c to
f5a3898
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #35078 +/- ##
===========================================
+ Coverage 59.63% 59.64% +0.01%
===========================================
Files 2832 2832
Lines 68301 68322 +21
Branches 15131 15133 +2
===========================================
+ Hits 40730 40750 +20
- Misses 24962 24963 +1
Partials 2609 2609
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ggazzo
force-pushed
the
chore/hono
branch
5 times, most recently
from
11c5ea1 to
79ba5e9
Compare
MarcosSpessatto
force-pushed
the
chore/hono
branch
from
b19b796 to
689af97
Compare
ggazzo
force-pushed
the
chore/hono
branch
4 times, most recently
from
a0962f3 to
301b101
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ggazzo
force-pushed
the
chore/hono
branch
5 times, most recently
from
0c6dd15 to
9d5cc6e
Compare
MarcosSpessatto
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
|
@kody start-review |
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
![kody-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
| export async function getLoggedInUser(request: Request): Promise<Pick<IUser, '_id' | 'username'> | null> { | ||
| const token = request.headers['x-auth-token']; | ||
| const userId = request.headers['x-user-id']; | ||
| const token = request.headers.get('x-auth-token'); |
There was a problem hiding this comment.
const token = request.headers.get('x-auth-token')?.trim();
const userId = request.headers.get('x-user-id')?.trim();Multiple instances of missing input validation for critical parameters, potentially leading to security vulnerabilities or runtime errors.
This issue appears in multiple locations:
- apps/meteor/app/api/server/helpers/getLoggedInUser.ts: Lines 6-7
- apps/meteor/app/integrations/server/api/api.js: Lines 243-243
- apps/meteor/server/services/omnichannel-integrations/providers/twilio.ts: Lines 248-250
- apps/meteor/server/services/omnichannel-integrations/providers/voxtelesys.ts: Lines 165-167
Please add validation checks for critical parameters to ensure they are not null, undefined, or malformed before processing.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| import { ajv } from '@rocket.chat/rest-typings/src/v1/Ajv'; | ||
| import { wrapExceptions } from '@rocket.chat/tools'; | ||
| import type { ValidateFunction } from 'ajv'; | ||
| import express from 'express'; | ||
| import type { Request, Response } from 'express'; | ||
| import type express from 'express'; | ||
| import { Accounts } from 'meteor/accounts-base'; | ||
| import { DDP } from 'meteor/ddp'; | ||
| import { DDPCommon } from 'meteor/ddp-common'; |
There was a problem hiding this comment.
// Import the necessary types from the '@rocket.chat/rest-typings' package or define custom types if needed.
import type { Request, Response } from '@rocket.chat/rest-typings';
// Or, if using custom types:
// interface Request { /* ... */ }
// interface Response { /* ... */ }The removal of the explicit 'express' import and the associated types 'Request' and 'Response' might lead to issues with type checking and code completion. While the 'WebApp' object might provide similar functionality, it's crucial to ensure type safety. Consider adding explicit types for request and response objects within the affected methods to maintain type correctness and avoid potential runtime errors.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| export type AuthorizationStatusCodes = Exclude<Range<451>, Range<400>>; | ||
|
|
||
| export type ErrorStatusCodes = Exclude<Range<511>, Range<500>>; | ||
| export type ErrorStatusCodes = Exclude<Exclude<Range<511>, Range<500>>, 509>; |
There was a problem hiding this comment.
export type ErrorStatusCodes = 500 | 510;The ErrorStatusCodes type now excludes 509 from the range. This could potentially cause issues if 509 is a valid error code in the system. Consider documenting this exclusion or ensuring it's intentional.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| const nodeReadableStream = new Readable({ | ||
| async read() { | ||
| const reader = webReadableStream.getReader(); |
There was a problem hiding this comment.
let reader;
try {
const error = unknown;
reader = webReadableStream.getReader();
} catch (error: unknown) {
this.destroy(error);
return;
}Add error handling for the webReadableStream.getReader() operation to catch potential stream reading errors.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| }); | ||
|
|
||
| request.pipe(bb); | ||
| const webReadableStream = await request.blob().then((blob) => blob.stream()); |
There was a problem hiding this comment.
+ nodeReadableStream.pipe(bb);
+
+ try {
+ return new Promise<UploadResultWithOptionalFile<K>>((resolve, reject) => {
+ returnResult = resolve;
+ } finally {
+ nodeReadableStream.destroy();
+ bb.removeAllListeners();
+ }The code pipes a Node.js Readable stream to the 'busboy' instance but doesn't remove the listener or close the stream when finished or in case of errors. This can lead to memory leaks. Add a 'close' event listener to the Readable stream and/or 'busboy' to ensure proper cleanup. Consider using a try/finally block to guarantee execution of the cleanup logic.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return c.body('CORS not enabled. Go to "Admin > General > REST Api" to enable it.', 405); | ||
| } | ||
|
|
||
| const CORSOriginSetting = String(settings.get('API_CORS_Origin')); |
There was a problem hiding this comment.
const CORSOriginSetting = String(settings.get('API_CORS_Origin') ?? '*');Add validation for CORSOriginSetting to ensure it's not null or undefined before processing.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const honoRes = await hono.request( | ||
| expressReq.originalUrl, | ||
| { | ||
| ...req, | ||
| ...(['POST', 'PUT', 'DELETE'].includes(expressReq.method) && { body: expressReq as unknown as ReadableStream }), | ||
| headers: new Headers(Object.fromEntries(Object.entries(expressReq.headers)) as Record<string, string>), | ||
| }, | ||
| { | ||
| incoming: expressReq, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
try {
const honoRes = await hono.request(
expressReq.originalUrl,
{
...req,
...(['POST', 'PUT', 'DELETE'].includes(expressReq.method) && { body: expressReq as unknown as ReadableStream }),
headers: new Headers(Object.fromEntries(Object.entries(expressReq.headers)) as Record<string, string>),
},
{
incoming: expressReq,
},
);
const responseText = await honoRes.text().catch((error) => {
console.error('Error reading response:', error);
throw error;
});
res.status(honoRes.status);
honoRes.headers.forEach((value, key) => res.setHeader(key, value));
res.send(Buffer.from(responseText));
} catch (error) {
console.error('Hono adapter error:', error);
res.status(500).send('Internal Server Error');
}Add error handling for the Hono request and response processing to prevent unhandled promise rejections.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| _id: this.user._id, | ||
| ip: this.requestIp, | ||
| useragent: this.request.headers['user-agent'] || '', | ||
| useragent: this.request.headers.get('user-agent') || '', |
There was a problem hiding this comment.
const userAgent = this.request.headers.get('user-agent') || '';
if (!/^[\w\d\s\-\.\/\:;()\[\]{}@!?=+]*$/.test(userAgent)) {
throw new Error('Invalid user-agent header');
}
useragent: userAgent,Add validation for user-agent header to ensure it matches expected format and prevent potential injection attacks.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| '/oauth/*', | ||
| express.json({ | ||
| limit: '50mb', | ||
| }), | ||
| express.urlencoded({ | ||
| extended: true, | ||
| limit: '50mb', |
There was a problem hiding this comment.
express.json({
limit: '1mb',
}),
express.urlencoded({
extended: true,
limit: '1mb',
}),The 50MB limit for JSON and URL-encoded data is quite high and could lead to potential memory exhaustion attacks. Consider reducing this limit to a more reasonable size.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
https://rocketchat.atlassian.net/browse/ARCH-1485
hono is a new router designed to replace express.
it is based on new language features and made 100% to be used with typescript
Better typings
Improved way to implement middlewares
Multiple architectures (node/bun/deno)
Faster (!/?)
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
This pull request introduces significant changes to the Rocket.Chat codebase by integrating the Hono framework as the primary router, replacing the existing Express setup. The changes span multiple files and focus on improving type safety, request handling, and middleware functionality. Key updates include:
get()method, enhancing security and consistency with modern web standards.Overall, this pull request aims to modernize the Rocket.Chat codebase by adopting the Hono framework, improving maintainability, and ensuring better type safety and security across the application.