Skip to content
This repository has been archived by the owner on May 3, 2024. It is now read-only.

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

Open
ptim opened this issue Feb 8, 2022 · 4 comments
Open

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

ptim opened this issue Feb 8, 2022 · 4 comments

Comments

@ptim
Copy link

ptim commented Feb 8, 2022

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:

  • ctx.request['x-foo-header'] will be handled by the get trap on Sunder's proxied request, and return ctx.request.headers.get('x-foo-header')
  • ctx.request['x-foo-header'] = 'bar' would similarly trigger the set trap and mutate request.headers

So I guess there are two issues here:

  • am I using the proxy correctly?
  • do the HeadersShorthands work as expected? I figure if the set trap was getting hit as expected, then I'd see a type error (I didn't find any relevant tests)

Regards, Tim

@gzuidhof
Copy link
Collaborator

gzuidhof commented Feb 8, 2022

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
Copy link
Collaborator

gzuidhof commented Feb 8, 2022

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
Copy link
Author

ptim commented Feb 12, 2022

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
Copy link

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants