-
Notifications
You must be signed in to change notification settings - Fork 511
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
fix(aws-lambda,netlify): base64 encode binary responses #1274
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1274 +/- ##
=======================================
Coverage 76.21% 76.22%
=======================================
Files 73 73
Lines 7433 7435 +2
Branches 728 728
=======================================
+ Hits 5665 5667 +2
Misses 1767 1767
Partials 1 1 |
@@ -30,9 +31,20 @@ export async function lambda( | |||
body: event.body, // TODO: handle event.isBase64Encoded | |||
}); | |||
|
|||
const headers = normalizeOutgoingHeaders(r.headers); | |||
// image buffers must be base64 encoded | |||
if (Buffer.isBuffer(r.body) && headers["content-type"].startsWith("image/")) { |
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.
this header check is hacky, looking for a better alternative
maybe it could just be omitted?
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.
If we trust mime.getType to return the correct type, this should be fine for images, as all images types should start with image
However we should do this for all Binary media for lambda based presets.
As always with AWS, the list of supported media types isn't documented anywhere, altough, chatGPT gives me this:
image/png
image/jpeg
image/gif
application/octet-stream
application/pdf
application/x-zip-compressed
audio/mpeg
audio/mp4
video/mp4
application/zip
audio/webm
video/webm
Which sorts of make sense... But I think a cleaner way to handle all possible use cases without relying on the header would be to use a package such as https://www.npmjs.com/package/isbinaryfile, because I'm not sure Buffer works in all scenarios.
We might also need to rely on the header anyways, as what AWS does with Binary files which aren't media (whatever they mean) is currently a mistery.
This reverts commit 1db2102.
Indeed it should be also generic to support any blob/binrary response regardless of mime type. |
…lify-images # Conflicts: # src/runtime/utils.lambda.ts
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
…lify-images # Conflicts: # src/runtime/utils.lambda.ts
🔗 Linked issue
See https://nuxt-og-image-playground-netlify.netlify.app/api/resvg.png
Code
❓ Type of change
📚 Description
Netlify uses AWS Lambda. AWS Lambda requires sending buffers as strings (they are re-encoded via a proxy afaik).
Nitro currently converts all buffers to default
toString()
. This breaks images, as they need to be sent with base64 encoding (see https://aws.amazon.com/blogs/compute/handling-binary-data-using-amazon-api-gateway-http-apis/), with a special flagisBase64Encoded
.The fix is to check if we're working with a buffer and the response is an image, then use base 64 encoding. This may affect other response types though.
📝 Checklist