-
Notifications
You must be signed in to change notification settings - Fork 7
revert(meditrak): RN-1788: restore image upload payload validation #6571
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
Conversation
Summary of ChangesHello @jaskfla, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request reverts a temporary hotfix related to image upload payload validation that was previously implemented to address issue RN-1788. By removing the permissive validation logic, the system now enforces the intended stricter validation for image data, thereby ensuring data integrity. Additionally, it incorporates minor code quality improvements within the S3 client utility. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
bd820f5 to
ccd549d
Compare
Summary of ChangesHello @jaskfla, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request partially reverts a previous change to restore critical image upload payload validation within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Summary of ChangesHello @jaskfla, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on re-establishing crucial image upload payload validation within the Meditrak application, addressing issue RN-1788. It achieves this by partially reverting a previous change that introduced a temporary permissive validation workaround. The core objective is to restore data integrity for image uploads while intelligently retaining the improved error messaging that was part of the earlier modification, thereby strengthening the robustness of the system's image handling capabilities. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
|
||
| function isSupportedImageMediaTypeString(val: string): val is keyof typeof supportedImageTypes { | ||
| return supportedImageTypes.hasOwnProperty(val); | ||
| return Object.hasOwn(supportedImageTypes, val); |
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.
My bad; I should’ve done this from the start in #6502
See:
- MDN docs for
Object.hasOwn() - Biome’s
noPrototypeBuiltinsrule
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.
Code Review
This pull request reverts a temporary hotfix that bypassed image upload payload validation, which is the correct course of action now that the underlying issue is resolved. The changes in postChanges.js correctly restore the validation. The accompanying changes in S3Client.ts are mostly good housekeeping, such as using Object.hasOwn and improving TypeScript types. However, I've found a potential bug in S3Client.ts related to how base64 data URIs are parsed, which could lead to incorrect behavior with certain inputs. My review includes a suggestion to fix this.
| // remove the base64 prefix from the image. This handles svg and other image types | ||
| const encodedFileString = encodedFile.replace(new RegExp('(data:)(.*)(;base64,)'), ''); | ||
|
|
||
| const encodedFileString = encodedFile.replace(/(data:)(.*)(;base64,)/, ''); |
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.
The regex used here to strip the data URI prefix is greedy (.*). This can lead to incorrect behavior if the base64-encoded data itself contains a data URI string. For example, for a data URI like data:text/plain;base64,data:image/png;base64,abc, the greedy regex will strip everything up to the last ;base64,, resulting in abc as the data. However, getContentTypeFromBase64 finds the first ;base64, and would extract text/plain as the content type. This inconsistency can lead to bugs.
It's safer to use string manipulation methods like indexOf and substring to ensure only the prefix of the top-level data URI is removed, which aligns with how getContentTypeFromBase64 works.
| const encodedFileString = encodedFile.replace(/(data:)(.*)(;base64,)/, ''); | |
| const encodedFileString = encodedFile.substring(encodedFile.indexOf(';base64,') + 8); |
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.
Gonna leave this out-of-scope
- It is true that this regular expression is greedy; but
;and,are illegal characters in Base64, so so unlessconvertEncodedFileToBufferis given bad data this won’t happen
Would be nice to tighten up, but only our own code ever calls convertEncodedFileToBuffer so it’s exceptionally unlikely that we’ll ever get an invalid Base64 string
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.
Code Review
This pull request correctly reverts a temporary hotfix related to RN-1788, restoring the image upload payload validation. The changes are clean and also include several small but valuable code quality improvements, such as using modern JavaScript features and more specific TypeScript types. I have one suggestion to make a regular expression more robust. Overall, this is a solid pull request.
| // remove the base64 prefix from the image. This handles svg and other image types | ||
| const encodedFileString = encodedFile.replace(new RegExp('(data:)(.*)(;base64,)'), ''); | ||
|
|
||
| const encodedFileString = encodedFile.replace(/(data:)(.*)(;base64,)/, ''); |
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.
The regular expression used to strip the data URI prefix is not anchored to the start of the string and is a bit too broad. This could lead to unexpected behavior if a data URI-like pattern appears in the middle of the string, and it's less efficient than it could be. Anchoring the regex and making it more specific would improve robustness and performance.
| const encodedFileString = encodedFile.replace(/(data:)(.*)(;base64,)/, ''); | |
| const encodedFileString = encodedFile.replace(/^data:[^;]+;base64,/, ''); |
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.
Accept the ^ suggestion and removal of redundant parens, but not accepting [^;]
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.
Code Review
This pull request reverts a temporary hotfix that was put in place to handle invalid image upload payloads. The changes restore the validation logic and remove the permissive handling of image data. The code also includes some minor improvements like using Object.hasOwn and adding more specific TypeScript types. Overall, the changes are good and correctly implement the revert. I've added one suggestion to improve the robustness of a public method.
b988c06 to
901b732
Compare
901b732 to
25be3a1
Compare
chris-bes
left a comment
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.
LGTM since it's just reverting
RN-1788
Partially reverts #6564, keeping the improved error messages