-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Hotfix: REST API multiform data missing boundary on headers #10505
Conversation
Deployment: https://tooljet-pr-10505.onrender.com |
Code Review Agent Run #1b4eb0
High-level FeedbackEnsure that headers are in the expected key-value pair format before processing them to avoid runtime errors. Add validation to check for key conflicts between 'queryHeaders' and 'sourceHeaders' before merging to prevent unintended overwrites or data loss. Check that the content type string is defined before calling 'toLowerCase()' to avoid runtime errors. Consider using 'Object.fromEntries' for converting header arrays to objects for better performance.Actionable Issues
📄 plugins/packages/common/lib/utils.helper.ts
Issues: Total - 2, High importance - 2
📄 plugins/packages/common/lib/oauth.ts
Issues: Total - 2, High importance - 2
|
|
hasDataSource = true | ||
): { [k: string]: string } => { | ||
const cleanHeaders = (headers) => | ||
headers.filter(([_, v]) => !isEmpty(v)).map(([k, v]) => [k.trim().toLowerCase(), v]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bito Code Review Agent Run #1b4eb0 - 07/29/2024, 11:46 am
Code suggestion
@@ -85,7 85,11 @@
const cleanHeaders = (headers) =>
- headers.filter(([_, v]) => !isEmpty(v)).map(([k, v]) => [k.trim().toLowerCase(), v]);
headers
.filter(header => Array.isArray(header) && header.length === 2 && typeof header[0] === 'string')
.filter(([_, v]) => !isEmpty(v))
.map(([k, v]) => [k.trim().toLowerCase(), v]);
const _queryHeaders = cleanHeaders(queryOptions.headers || []);
const queryHeaders = Object.fromEntries(_queryHeaders);
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
|
||
return headers; | ||
return { ...queryHeaders, ...sourceHeaders }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bito Code Review Agent Run #1b4eb0 - 07/29/2024, 11:46 am
Code suggestion
@@ -92,7 96,11 @@
- return headers;
const mergedHeaders = { ...queryHeaders, ...sourceHeaders };
const conflictingKeys = Object.keys(queryHeaders).filter(key => key in sourceHeaders);
if (conflictingKeys.length > 0) {
console.warn('Conflicting headers found: ${conflictingKeys.join(', ')}');
}
return mergedHeaders;
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
return contentType === 'application/x-www-form-urlencoded'; | ||
export function checkIfContentTypeIsURLenc(headers: [string, string][] = []): boolean { | ||
const contentType = headers.find(([key, _]) => key.toLowerCase() === 'content-type')?.[1]; | ||
return contentType?.toLowerCase() === 'application/x-www-form-urlencoded'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bito Code Review Agent Run #1b4eb0 - 07/29/2024, 11:46 am
Code suggestion
@@ -11,3 11,4 @@
const contentType = headers.find(([key, _]) => key.toLowerCase() === 'content-type')?.[1];
return contentType ? contentType.toLowerCase() === 'application/x-www-form-urlencoded' : false;
- return contentType?.toLowerCase() === 'application/x-www-form-urlencoded';
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
const contentType = objectHeaders['content-type'] ?? objectHeaders['Content-Type']; | ||
return contentType === 'multipart/form-data'; | ||
export function checkIfContentTypeIsMultipartFormData(headers: [string, string][] = []): boolean { | ||
const contentType = headers.find(([key, _]) => key.toLowerCase() === 'content-type')?.[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bito Code Review Agent Run #1b4eb0 - 07/29/2024, 11:46 am
Code suggestion
@@ -15,3 15,4 @@
const objectHeaders = Object.fromEntries(headers);
const contentType = objectHeaders['content-type'] ?? objectHeaders['Content-Type'];
return contentType?.toLowerCase().startsWith('multipart/form-data') ?? false;
- const contentType = headers.find(([key, _]) => key.toLowerCase() === 'content-type')?.[1];
- return contentType?.toLowerCase().startsWith('multipart/form-data') ?? false;
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
QA Checklist
Subpath Testing. (Automation/Smoke)Reverse Proxy/Reserve Proxy with Subpath. (Smoke)