Skip to content
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

ETag generation throws on binary responses since 1.0.0-next.82 #1136

Closed
intrikate opened this issue Apr 19, 2021 · 3 comments · Fixed by #1382
Closed

ETag generation throws on binary responses since 1.0.0-next.82 #1136

intrikate opened this issue Apr 19, 2021 · 3 comments · Fixed by #1382
Labels
bug Something isn't working
Milestone

Comments

@intrikate
Copy link

intrikate commented Apr 19, 2021

Describe the bug
Since 1.0.0-next.82, attempting to set a Content-Type header for certain binary responses in endpoint handlers will throw an exception: str.charCodeAt is not a function. Responding with no type or with explicit application/json won’t throw. Downgrading to next.81 fixes the issue. Upgrading to next.83 does not.

Update: This appears to be a regression introduced with the new ETag generation in #1117.

To Reproduce
Make an endpoint that responds with e.g. a JPEG with the Content-Type (or content-type) header set to image/jpeg.

See repo: https://github.com/intrikate/sveltekit-jpeg-endpoint-issue

Expected behavior
Content-Type set, the body sent, no exceptions thrown. Essentially what 1.0.0-next.81 does.

Stacktraces

Stack trace
str.charCodeAt is not a function
TypeError: str.charCodeAt is not a function
    at hash (file:///Users/leek/Code/Playground/sveltekit-jpeg-issue/node_modules/@sveltejs/kit/dist/ssr.js:1509:37)
    at render (file:///Users/leek/Code/Playground/sveltekit-jpeg-issue/node_modules/@sveltejs/kit/dist/ssr.js:1473:26)
    at async ssr (file:///Users/leek/Code/Playground/sveltekit-jpeg-issue/node_modules/@sveltejs/kit/dist/ssr.js:1454:10)
    at async Immediate.<anonymous> (file:///Users/leek/Code/Playground/sveltekit-jpeg-issue/node_modules/@sveltejs/kit/dist/chunks/index.js:3297:23)

Information about your SvelteKit Installation:

Diagnostics
System:
    OS: macOS 11.2.3
    CPU: (4) x64 Intel(R) Core(TM) i5-4288U CPU @ 2.60GHz
    Memory: 46.50 MB / 8.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 14.16.0 - /usr/local/bin/node
    Yarn: 1.22.10 - /usr/local/bin/yarn
    npm: 6.14.11 - /usr/local/bin/npm
  Browsers:
    Chrome: 89.0.4389.128
    Firefox Developer Edition: 88.0
    Safari: 14.0.3
  npmPackages:
    @sveltejs/kit: next => 1.0.0-next.83 
    svelte: ^3.29.0 => 3.37.0 
    vite: ^2.1.0 => 2.2.1

Severity
Pretty high, especially if this is indeed a regression.

Because nobody else seems to be affected, and the entire hashing business may currently be circumvented with cache-control, I’m inclined to downgrade severity to low.

@intrikate intrikate changed the title Unable to set content-type for binary response in endpoint ETag generation throws on binary responses since 1.0.0-next.82 Apr 20, 2021
@intrikate
Copy link
Author

More context:

export function hash(str) {
let hash = 5381,
i = str.length;
while (i) hash = (hash * 33) ^ str.charCodeAt(--i);
return (hash >>> 0).toString(36);
}

@benmccann benmccann added this to the 1.0 milestone Apr 26, 2021
@benmccann benmccann added the bug Something isn't working label Apr 26, 2021
Rich-Harris pushed a commit that referenced this issue May 9, 2021
* incoming rawBody is string or Uint8Array

* make body types stricter

* add etag tests

* generate etags for binary bodies - fixes #1136

* oops

* rawBody is a Uint8Array

* changesets

* use shared hash helper
@drzax
Copy link

drzax commented May 11, 2021

Did #1382 actually solve this? Unless I've missed something the changes actually prevent serving binary images in the way the example repo does.

@intrikate
Copy link
Author

You’re right, it effectively kills binary endpoints, with one small exception. Is this really necessary?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants