validation: throw exception if content-type is missing

[flayks]

Describe the bug
I get this error since upgrading to SK next-81 (also happens on 83). This possibly happens because of an API route with POST requests like so:

import fetch from 'node-fetch'


/**
 * Load data from API
 */
// Get results from a POST request
export async function post ({ headers, query, body, params, ...rest }) {
    try {
        const res = await fetch(`https://${import.meta.env.VITE_SANITY_ID}.apicdn.sanity.io/v1/data/query/production?query=${encodeURIComponent(body)}`, {
            method: 'GET',
            headers: {
                Authorization: `Bearer ${import.meta.env.VITE_SANITY_TOKEN}`,
            }
        })
        const data = await res.json()

        return {
            status: 200,
            body: data
        }
    }
     catch (error) {
        return {
            status: error.status || 500,
            body: error.message || error.text || `Can't fetch query`
        }
    }
}

Logs

Cannot read property 'split' of undefined
TypeError: Cannot read property 'split' of undefined
    at parse_body (file:///site/node_modules/.pnpm/@sveltejs+kit@1.0.0-next.83_svelte@3.37.0+vite@2.1.5/node_modules/@sveltejs/kit/dist/ssr.js:1330:60)
    at ssr (file:///site/node_modules/.pnpm/@sveltejs+kit@1.0.0-next.83_svelte@3.37.0+vite@2.1.5/node_modules/@sveltejs/kit/dist/ssr.js:1448:9)
    at fetch (file:///site/node_modules/.pnpm/@sveltejs+kit@1.0.0-next.83_svelte@3.37.0+vite@2.1.5/node_modules/@sveltejs/kit/dist/ssr.js:697:30)

Information about your SvelteKit Installation:

Diagnostics

  System:
    OS: macOS 11.2.3
    CPU: (12) x64 Intel(R) Core(TM) i7-8850H CPU @ 2.60GHz
    Memory: 480.96 MB / 32.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 15.14.0 - /usr/local/bin/node
    Yarn: 1.22.10 - /usr/local/bin/yarn
    npm: 7.7.6 - /usr/local/bin/npm
  Browsers:
    Brave Browser: 90.1.23.71
    Firefox: 84.0.1
    Safari: 14.0.3
  npmPackages:
    @sveltejs/kit: next => 1.0.0-next.83
    svelte: ^3.37.0 => 3.37.0
    vite: ^2.1.5 => 2.1.5

Severity
Well, it kind of blocks the whole app 🙈

[GrygrFlzr]

This is failing inside Vite and cannot be reproduced without seeing what you are sending with your POST request.

Other notes:

• You don't need to import node-fetch inside endpoints
• The type of body should not be assumed to be a string

[Conduitry]

The line where the error is being thrown is

kit/packages/kit/src/runtime/server/parse_body/index.js

Line 8 in 1237eb3

const [type, ...directives] = req.headers['content-type'].split(/;\s*/);

Presumably this is crashing when you don't have a Content-Type header, and this is something that probably ought to be supported. It also looks like the server would refuse to handle requests that had a present but unknown Content-Type, and I'm not sure that's wise.

[Conduitry]

Hm. Although I'm also not clear on why this code is getting run at all. I don't know why rawBody would be truthy for a GET request. A repro (and a cURL to hit the server with) would certainly help.

[Rich-Harris]

I think we should warn if content-type is unspecified (which is technically allowed, but not recommended), and default to treating it as an octet-stream, which is sufficiently annoying to deal with that people will be motivated to make content-type mandatory in their apps

[Conduitry]

If I'm interpreting parse_body correctly, that would currently throw with application/octet-stream. I guess we'd be adding support for that too. If we don't know what to do with the rawBody, what do you think we should make the body? And do you still think it ought to be an error to get an unfamiliar content type? or just that this should be handled the same way as a missing one or as an application/octet-stream one?

[Rich-Harris]

that would currently throw with application/octet-stream

hmm. if typeof raw === 'string' then yes. we would be relying on adapters not to send a string body for a content-type-less request, which seems brittle

[Rich-Harris]

thinking about it further i think we should just throw if the content type is missing, just like we throw if it's an unknown type (and raw is a string). there's really no benefit to being all loosey-goosey about this

[dummdidumm]

What's the drawback of allowing unknown or missing types though? Could there be ContentTypes that are unknown to us but need to be set like this by the user because of some backend quirks?

[Conduitry]

I think I'm confused about in what sorts of situations rawBody would be a string. When is it not going to be a Buffer/ArrayBuffer/whatever? Would that be when you're using certain adapters?

[Rich-Harris]

For adapters using getRawBody from @sveltejs/kit/http, it follows this logic:

kit/packages/kit/src/core/http/index.js

Lines 53 to 62 in 1237eb3

	req.on('end', () => {
	const [type] = h['content-type'].split(/;\s*/);
	if (type === 'application/octet-stream') {
	fulfil(data.buffer);
	}
	const decoder = new TextDecoder(h['content-encoding'] || 'utf-8');
	fulfil(decoder.decode(data));
	});

For e.g. Netlify, it's

kit/packages/adapter-netlify/files/entry.js

Lines 17 to 22 in 5c2665f

	const rawBody =
	headers['content-type'] === 'application/octet-stream'
	? new TextEncoder('base64').encode(body).buffer
	: isBase64Encoded
	? Buffer.from(body, 'base64').toString()
	: body;

For CFW, it's

kit/packages/adapter-cloudflare-workers/files/entry.js

Lines 55 to 62 in 5c2665f

	function read(request) {
	const type = request.headers.get('content-type') || '';
	if (type.includes('application/octet-stream')) {
	return request.arrayBuffer();
	}
	return request.text();
	}

Perhaps it would be easier if rawBody was always a buffer

[Conduitry]

I think that might make more sense. That's what it really is, right? Any decoding of that to a string involves some awareness of what its encoding was, and it doesn't seem like a 'raw' body anymore.

The base64 stuff in Netlify seems like somewhat of an anomaly, and if that's what their API is, I think we should be transparently handling that, but probably only decoding it back to a buffer, not to a string.

[Rich-Harris]

What do we do in the Netlify adapter in the case where it's not base64 encoded — turn it into a buffer...

const encoding = get_charset(headers['content-type']) || 'utf-8';
rawBody = new TextEncoder(encoding).encode(str).buffer;

...so that SvelteKit can turn it back into a string...

const text = new TextDecoder(encoding).decode(new UInt8Array(rawBody));

...or just leave it as a string?

[Rich-Harris]

also, it seems the use of content-encoding in http/index.js is incorrect —

 	const decoder = new TextDecoder(h['content-encoding'] || 'utf-8'); 
 	fulfil(decoder.decode(data)); 

i got confused, content-encoding is one of gzip, compress, deflate, br, it's not a character encoding. how silly of my. presumably we need to handle that somewhere too?

[Conduitry]

I just wrote a big paragraph here about what I would consider the ideal API which I then erased, because I realized it was more complicated than I was thinking it was. I think it would be nice if endpoints had access to the request body before it's even been uncompressed, because this might be a proxy to some other endpoint elsewhere, and we don't want to decompress that data, just send it along as-is.

Do we need three types of bodies? Entirely-raw, content-encoding-decoded, and content-type-decoded?

[Rich-Harris]

man, at this point i don't have a clue

[Conduitry]

veryRawBody

[vinkodlak]

The line where the error is being thrown is

kit/packages/kit/src/runtime/server/parse_body/index.js

Line 8 in 1237eb3

const [type, ...directives] = req.headers['content-type'].split(/;\s*/);

Presumably this is crashing when you don't have a Content-Type header, and this is something that probably ought to be supported. It also looks like the server would refuse to handle requests that had a present but unknown Content-Type, and I'm not sure that's wise.

@Conduitry Thnx for pointing out this for me, found another 'thing':

const [type, ...directives] = req.headers['content-type'].split(/;\s*/);

content-type should be lowercase letters in your header: { 'content-type': ...}
otherwise doesn't work too

[Conduitry]

Hm. By the time it gets to the req.headers object it should already be getting normalized to lowercase. The case of the header name shouldn't matter, but maybe some adapter is neglecting to lowercase it.

What are you doing to call the endpoint with Content-Header where it's not being recognized here?

[vinkodlak]

Really nothing, I created fresh project to see what was the problem.
This is in index.svelte:

<script context="module">
  export async function load({fetch}) {
    const res = await fetch('/server', {
      method: 'POST',
      headers: {
        'Content-Type': 'application/json' // on purpose like this
      },
      body: {
        name: 'Hello world'
      }
    })

    const { name } = await res.json()

    return {
      props: {
        name
      }
    }
  }
</script>
<script>
  export let name
</script>

<h1>{name}, Welcome to SvelteKit</h1>
<p>Visit <a href="https://kit.svelte.dev">kit.svelte.dev</a> to read the documentation</p>

and server.js:

export async function post({body}) {
  return {
    body: {
      name: body.name
    }
  }
}

i put this in @sveltejs/kit/dist/ssr.js to see what's going on:

console.log('headers', req.headers)
const [type, ...directives] = req.headers['content-type'].split(/;\s*/);

and i get this fromin the console:

headers {
  'Content-Type': 'application/json',
  cookie: undefined,
  authorization: undefined
}
Cannot read property 'split' of undefined
TypeError: Cannot read property 'split' of undefined
    at parse_body (file:///Users/vinkov/svelte/novo/node_modules/@sveltejs/kit/dist/ssr.js:1311:60)

so I guess it's not normalized. when i change to lowercase 'content-type' it works fine

[flayks]

Alright, since I didn't specified a content-type to my requests, adding it now fixes the problem apparently. Also, as I use Sanity without the client, in my case the content-type is not application/json but text/plain (for their funky GROQ syntax), and it still works with the newest version next-84. Removing node-fetch also doesn't kill everything, which is cool 🤡 (thanks @GrygrFlzr!)

[phaleth]

@Rich-Harris can you please reopen this issue and rollback the commit made yesterday? I've just tested my code with the new SvelteKit version (next.95) and the problem still persists. Seems like the fix which has been done yesterday has no effect and has been done in a wrong place of the code. Please, see the picture bellow. It all doesn't make much sense to me right now, but that's just how things are.

In case you reopen the issue I'm willing to try and put together steps to reproduce the problem. It seems to be a rather tricky one. It's prolly gonna take me a while to put all the steps together with enough details. So, please, bear with me.

[bwklein]

@phaleth did you find a solution to this? I am stuck with a broken deployment to Netlify, while local dev server and adapter-node works.

[bwklein]

Alright, since I didn't specified a content-type to my requests, adding it now fixes the problem apparently. Also, as I use Sanity without the client, in my case the content-type is not application/json but text/plain (for their funky GROQ syntax), and it still works with the newest version next-84. Removing node-fetch also doesn't kill everything, which is cool

@flayks Where did you add 'content-type' into your code to make it work and what type did you use?

[flayks]

Alright, since I didn't specified a content-type to my requests, adding it now fixes the problem apparently. Also, as I use Sanity without the client, in my case the content-type is not application/json but text/plain (for their funky GROQ syntax), and it still works with the newest version next-84. Removing node-fetch also doesn't kill everything, which is cool

@flayks Where did you add 'content-type' into your code to make it work and what type did you use?

I don't anymore as the content-type is by default AFAIK on the API side, but I use this for my routes requests:

<script context="module">
    /** @type {import('@sveltejs/kit').Load} */
	export async function load ({ page, fetch, session, context }) {
        const res = await fetch(`/api/sanity`, {
            method: 'POST',
            headers: {
                'content-type': 'text/plain',
            },
            body: `…`
        })
    }
...

[phaleth]

@phaleth did you find a solution to this? I am stuck with a broken deployment to Netlify, while local dev server and adapter-node works.

@bwklein This very same annoying issue was brought up with a reproduction here #1463 and closed by Rich's successful commit couple of weeks afterwards. You can go through the steps described in the issue #1463 and confirm to yourself that everything still works. In case the issue reapeared with that repro please open a new issue.

Edit: Actually, someone else fixed the problem in the middle of May, but Rich should still get the credit.