Skip to content

fix: improve getRawBody parsing & handle error(s)#1528

Merged
benmccann merged 1 commit intomasterfrom
fix/getRawBody
May 28, 2021
Merged

fix: improve getRawBody parsing & handle error(s)#1528
benmccann merged 1 commit intomasterfrom
fix/getRawBody

Conversation

@lukeed
Copy link
Copy Markdown
Member

@lukeed lukeed commented May 23, 2021

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts

Summary

  • The getRawBody utility did not handle the case where the supplied body actually exceeds the Content-Length header. Added that logic. (This is impossible to spoof on Playwright/browser clients. Can add a test once a unit testing system is in place.)
  • fixed case where octect-stream request bodies were double-fulfilled
  • fixed rawBody type interfaces (missing null possibility)
  • Handle getRawBody possible error(s) in consumers/adapters

Related to (but does not close) #1523


Comment thread packages/adapter-node/src/server.js
Comment thread packages/kit/src/core/dev/index.js Outdated
fulfil(null);
return;
}
const new_len = offset + Buffer.byteLength(chunk);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the chunk.length was a bug. Incoming chunks can be strings, but even if they're still Buffer instances, length is not always the same as byteLength, which is what content-length tracks


if (type === 'application/octet-stream') {
fulfil(data);
return fulfil(data);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this return, then decoding continued (below)

Comment thread packages/kit/types/endpoint.d.ts Outdated
Comment thread packages/adapter-node/src/server.js Outdated
@lukeed lukeed mentioned this pull request May 24, 2021
2 tasks
Comment thread packages/kit/src/core/node/index.js Outdated
@benmccann
Copy link
Copy Markdown
Member

lgtm. I wonder if getRawBody should have a couple unit tests

Comment thread packages/kit/src/core/node/index.js Outdated
@benmccann
Copy link
Copy Markdown
Member

I rebased this and am going to go ahead and merge it. We can always add more tests later

@benmccann benmccann merged commit c3d36a3 into master May 28, 2021
@benmccann benmccann deleted the fix/getRawBody branch May 28, 2021 16:35
sidharthv96 added a commit to sidharthv96/kit that referenced this pull request May 29, 2021
* 'master' of https://github.com/sveltejs/kit:
  Version Packages (next) (sveltejs#1543)
  type fixes for adapter-node and adapter-static (sveltejs#1578)
  Upgrade to Vite 2.3.3 (sveltejs#1580)
  fix: improve getRawBody parsing & handle error(s) (sveltejs#1528)
  create-svelte: add svelte-check for TS (sveltejs#1556)
  pass validated svelte config to adapters (sveltejs#1559)
  types: group related and reduce potential inconsistencies (sveltejs#1539)
  Use sveltekit tag on StackOverflow (sveltejs#1558)
  Fix create-svelte build-template script (sveltejs#1555)
  Remove err param from Polka .listen() callback (sveltejs#1550)
  bump: polka and sirv versions (sveltejs#1548)
  svelte-kit package (sveltejs#1499)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants