Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Miniflare is not rewriting headers correctly #64

Closed
Kikobeats opened this issue Oct 13, 2021 · 6 comments
Closed

Miniflare is not rewriting headers correctly #64

Kikobeats opened this issue Oct 13, 2021 · 6 comments
Labels
behaviour mismatch Different behaviour to Workers runtime fixed-in-next Fixed in next release

Comments

@Kikobeats
Copy link
Contributor

Kikobeats commented Oct 13, 2021

Let's say we want to use Cloudflare Workers as just a proxy and rewrite headers passed with the x- prefix.

Current behavior

const { Miniflare } = require('miniflare')

function listener (event) {
  const response = new Response('', { headers: event.request.headers })

  for (const [key, value] of response.headers) {
    response.headers.delete(key)
    response.headers.set(`x-${key}`, value)
  }

  return event.respondWith(response)
}

;(async () => {
  const mf = new Miniflare({
    script: `addEventListener('fetch', ${listener.toString()})`
  })
  const res = await mf.dispatchFetch('http://localhost:8787/', {
    headers: {
      foo: 'bar',
      fooz: 'barz'
    }
  })

  console.log(await Object.fromEntries(res.headers))
  // => { fooz: 'barz', 'x-x-foo': 'bar' }
  // WRONG!
})()

Expected behavior

CleanShot 2021-10-13 at 16 49 13@2x

@mrbbot
Copy link
Contributor

mrbbot commented Oct 13, 2021

Hey! 👋 This is a very interesting issue. Running the following code...

const headers = new Headers({
  foo: "bar",
  fooz: "barz",
});
for (const [key, value] of headers) {
  headers.delete(key);
  headers.set(`x-${key}`, value);
}
console.log(Object.fromEntries(headers));

...gives { fooz: 'barz', 'x-x-foo': 'bar' } with:

  • @mrbbot/node-fetch, the node-fetch fork currently used by Miniflare
  • undici, the fetch implementation Miniflare 2 will use
  • Firefox 93

...but gives { x-foo: 'bar', x-fooz: 'barz' } with:

  • Cloudflare Workers
  • Chrome 94

It's probably worth fixing this issue, but generally mutating the backing source behind an interator whilst iterating is a bad idea. I'd just recommend creating a new Headers object and writing to that:

const headers = new Headers();
for (const [key, value] of event.request.headers) {
  headers.set(`x-${key}`, value);
}
const response = new Response('', { headers });
return event.respondWith(response);

@mrbbot mrbbot added the behaviour mismatch Different behaviour to Workers runtime label Oct 13, 2021
@payellodevsupport
Copy link
Contributor

This is because you are deleting the element from the same set you are iterating.

If you pass new Headers(headers), so it creates a copy, to the for loop it would fix this problem.

const response = new Response('', { headers: {
    foo: "bar",
    fooz: "barz",
} })

for (const [key, value] of new Headers(response.headers)) {
    response.headers.delete(key)
    response.headers.set(`x-${key}`, value)
}

console.log(Object.fromEntries(response.headers));

Output:

{
  'x-foo': 'bar',
  'x-fooz': 'barz'
}

@lukeed
Copy link
Contributor

lukeed commented Oct 31, 2021

This is an upstream issue with undici, whose Header class extends an Array instead of a Map, which I find to be a very strange choice. As a result, it has to deal with splices & indices and that's the culprit here.

@lukeed
Copy link
Contributor

lukeed commented Oct 31, 2021

Fix here: nodejs/undici#1081

@lukeed
Copy link
Contributor

lukeed commented Nov 1, 2021

My fix was merged & released as part of 4.9.3 🎉 Adding a PR to raise the version requirement within miniflare.

lukeed added a commit to lukeed/miniflare that referenced this issue Nov 1, 2021
@mrbbot mrbbot added the fixed-in-next Fixed in next release label Nov 1, 2021
@mrbbot
Copy link
Contributor

mrbbot commented Nov 3, 2021

Hey! 👋 miniflare@2.0.0-next.3 has just been released, including this fix. You can find the full changelog here.

@mrbbot mrbbot closed this as completed Nov 3, 2021
mrbbot pushed a commit that referenced this issue Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behaviour mismatch Different behaviour to Workers runtime fixed-in-next Fixed in next release
Projects
None yet
Development

No branches or pull requests

4 participants