-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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] Expand list of allowed mime types for binary bodies #1687
[fix] Expand list of allowed mime types for binary bodies #1687
Conversation
🦋 Changeset detectedLatest commit: e4fac6d The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
I think you need to look at https://github.com/sveltejs/kit/pull/1382/files, which added those checks, and make sure you've thought about all the other changes that PR made and whether they will still work if you remove those checks. For example, #1382 changes |
I can't think of a better solution, what do you think? /** @param {string} type */
function isBinaryContentType(type) {
return (
type.startsWith('image') ||
type.startsWith('audio') ||
type.startsWith('video') ||
type.includes('application/octet-stream')
);
} This would work in most cases, but should this be updated when someone wants to use a different content type header or should they just use |
Is there a way to share core logic with the adapters? |
@@ -63,9 +63,9 @@ export default async function render_route(request, route) { | |||
if ( | |||
typeof body === 'object' && | |||
!(body instanceof Uint8Array) && | |||
(!type || type === 'application/json') | |||
(!type || type === 'application/json' || type === 'application/json; charset=utf-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.
I would use type.startsWith('application/json')
since the presence (or absence) of the encoding does not change the behavior of the code
Here are some examples where we do that:
|
@@ -37,24 +38,27 @@ export default async function render_route(request, route) { | |||
headers = lowercase_keys(headers); | |||
const type = headers['content-type']; | |||
|
|||
const is_type_binary = isContentTypeBinary(type); | |||
/** @type {import('types/hooks').StrictBody} */ | |||
let normalized_body; |
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.
I would move this line back down where it was originally. No reason to declare it higher up since it's not used yet
lgtm once the tests are passing. can you add a changeset as well? |
* @param {string} content_type The `content-type` header of a request/response. | ||
* @returns {boolean} | ||
*/ | ||
export function isContentTypeBinary(content_type) { |
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 logic is possibly still too blunt. There are at least a few content types I'd consider common for which I think this would return the wrong result:
image/svg+xml
application/zip
application/pdf
An escape hatch for the developer here might be useful too given all the potential edge cases that the wonderful world of mime types involves.
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.
I know that's why I made #1829
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.
I sent #1890 to help with this
Fixes #1438 by expanding the list of allowed mime types for binary bodies.
This allows an endpoint to return a response body with the most commonly used binary mime types.
Before submitting the PR, please make sure you do the following
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpx changeset
and following the prompts