SunderJS / sunder

Sunder is a minimal server-side framework for building APIs and websites on Cloudflare Workers.
https://sunderjs.com
MIT License
168 stars 5 forks source link

Setting headers on `ctx.request` in middleware (request proxy bug?) #7

Open ptim opened 2 years ago

ptim commented 2 years ago

Hi! Thanks for your work on Sunder - loving it, learning plenty 🙏

I'm not sure if this issue is a bug, or user error…

I'm trying to add headers on the request object so that I can tweak the behaviour of sub-fetches (for caching and image resizing, etc).

I see that HeadersShorthands makes provision for this, but what I'm seeing is:

// in my middleware...
ctx.request['x-foo-header'] = 'bar
console.log(request['x-foo-header']) // 'bar'
console.log(request.headers.get('x-foo-header')) // null

request.headers.set('x-foo-header', 'bar')
// TypeError: immutable
//      at Headers.set ([/project/node_modules/undici/lib/fetch/headers.js:293:13]())

I'm expecting that:

So I guess there are two issues here:

Regards, Tim

gzuidhof commented 2 years ago

Hi Tim,

The shorthands are only for the get, set and delete functions on the Headers object, which is proxied on the request or response object.

So these should be equivalent:

ctx.response.set("my-header", "my-value");
ctx.response.headers.set("my-header", "my-value");

ctx.request.get("some-header");
ctx.request.headers.get("some-header");

I decided against also proxying any ["some-value"] values, as that would stop you from being able to add fields to Request and Response (not sure why you would) and it's a bit too much magic.

gzuidhof commented 2 years ago

As for the request headers being immutable: I haven't run into this before, perhaps incoming request headers are mutable and you have to init a new request using new Request(ctx.request)? That's not necessarily Sunder specific, here's a thread I found about it: https://community.cloudflare.com/t/how-to-modify-immutable-headers-and-add-nonces-to-response-header/146165.

ptim commented 2 years ago

Thanks very much @gzuidhof 👍

I'm reconsidering my approach, as I think I'm building a footgun!

incoming request headers are mutable immutable and you have to init a new request using new Request(ctx.request)

Indeed! (IIUC, headers on a Response received from fetch are also immutable, but everything works fine in Sunder because the response is created, not received? Related: workers/examples/alter-headers)

It's possible to work around this like so:

import { proxyRequest } from 'sunder/util/request'

// in middleware
ctx.request = proxyRequest(new Request(ctx.request.url, ctx.request))
ctx.request.headers.set('x-foo', 'bar')

...but this is probably a bad idea (better to set the specific request headers when constructing my sub-requests!)

I wonder if it would be a good idea to remove Sunder's Request.set shorthand, or add an errorHint? As it is, Sunder's request.set helper throws "TypeError: immutable"...

MikelHearon commented 10 months ago