Fix for incorrect Content-Type headers returned for Admin UI bundles

[molomby]

Fixes #2741.

When the Express res.format() function matches a request (to a non-default function) it replaces the responses Content-Type header with the key it matched on. This causes our */* format handler (which is basically a noop) to return requests with header of Content-Type: */*. This in turn causes problems if a X-Content-Type-Options: nosniff header is also set on the request (ie. somewhere else in the Express middleware, by a reverse proxy, etc.).

We can fix this by resetting the Content-Type header with a mime-type derived from the requested files extension.

Unfortunately, despite being quite redundant, we can't just remove the */* handler. Doing so causes requests with an Accept: */* header or no Accept at all to fall though to the text/html handler, rather than the default.

The default handler itself doesn't require this additional logic as Express has already given the response a correct Content-Type header and doesn't override it when the default format handler is used. If the text/html handler is hit, the overridden Content-Type is correctly set to text/html.

[changeset-bot]

🦋 Changeset is good to go

Latest commit: 2e56648

We got this.

This PR includes changesets to release 1 package

Name	Type
@keystonejs/app-admin-ui	Patch

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

[dominikwilkowski]

LGTM

[jesstelford]

What happens when req.url = /foo/bar?

Wouldn't we want the content type to be text/html then? Or maybe */* to let the client decide?

[dominikwilkowski]

Don't we control 100% of the requests? In the adminUI... which so far I can only see /admin/js/*

[jesstelford]

Maybe something like this?

Suggested change

	res.type(path.extname(req.url));
	const extension = path.extname(req.url);
	res.type(extension ? extension : '*/*');

[jesstelford]

@dominikwilkowski This express app handles everything under /admin, which could include custom pages. Not to mention the pages such as /admin/todos which doesn't have an extension.

[molomby]

Great point. So...

1. If I request /admin/blerg from the browser (address bar), the browser (Chrome in this case) adds a sensible Accept header of..

   Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.9
   

   Which works fine as it matches the text/html resolver (returns with Content-Type: text/html; charset=utf-8).

2. If I request /admin/blerg without an Accept header, it hits the */* format resolver and the Content-Type is returned as application/octet-stream. Presumably this is some kind of default that's applied since we're passing res.type() and empty string.

3. If I request with an unrecognised Accept header (food/pasta) it hits the default format resolver, returning with Content-Type: text/html; charset=UTF-8 which is correct for the content -- the main html page for the admin UI.

This all seems to be true regardless of the depth of the path, trailing slashes, etc.

Point 2 is probably wrong. The content being returned is actual html; the incorrect mime type here is our fault -- I've patched to only override if a file extension is present, meaning it will return */* as before.

Note this still isn't totally correct. If we removed the res.format() block entirely, these requests would return text/html. But as you say, at least the browser can decide (and it's still correct more often than what we have now).