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

set-cookie header is forbidden in local Wrangler #59

Closed
oliverjam opened this issue Nov 29, 2021 · 2 comments · Fixed by #82
Closed

set-cookie header is forbidden in local Wrangler #59

oliverjam opened this issue Nov 29, 2021 · 2 comments · Fixed by #82

Comments

@oliverjam
Copy link

oliverjam commented Nov 29, 2021

So I think this is a weird disparity between the local Miniflare environment and the Workers platform. I can't append a set-cookie header to a response that already exists when running a Worker locally. However this works fine deployed to Workers.

// functions/index.js
export function onRequestGet() {
  let res = new Response("hello");
  res.headers.append("set-cookie", "test=123");
  return res;
}

There is no set-cookie on the response, and no cookie is set in the browser (when run via Wrangler2).

You can work around this in two ways. Either set the header as you create the response

// functions/index.js
export function onRequestGet() {
  let res = new Response("hello", { headers: {  "set-cookie": "test=123" } });
  return res;
}

or (if you need to amend an existing request, i.e. in middleware), you have to awkwardly make copies

// functions/_middleware.js
export async function onRequest({ next }) {
  let res = await next();
  let headers = new Headers(res.headers);
  headers.append("set-cookie", "test=123");
  return new Response(res.body, { status: res.status, headers });
}

The problem is that the set-cookie header is a "forbidden response header name", which means you can't append it to an existing response. This is (correctly according to the fetch spec) implemented here by Undici (which it looks like Miniflare switched to recently).

Does it make sense for Workers to also implement this limitation for Headers? It's obviously very useful for server-side code to be able to easily set cookies, and I assume the security reasons for forbidding it in the browser don't apply. Cloudflare already deviated from the spec for .getAll(), maybe it would make sense here too? 😈 Ideally Wrangler should match the prod environment imo

(apologies if this is the wrong place to open this—the Miniflare repo doesn't look up-to-date with what's actually running within Wrangler2 so didn't seem right)

@threepointone
Copy link
Contributor

Thanks for the detailed report! I'll have a look at this tomorrow.

@mrbbot
Copy link
Contributor

mrbbot commented Dec 2, 2021

Hey! 👋 This is definitely a Miniflare issue. The code being used by Wrangler 2 is in the v2 branch. I'd suggest transfering the issue there.


Assuming Coudflare lets you change all forbidden header names, we could just set kGuard to none in the Response constructor like we do here...
https://github.com/cloudflare/miniflare/blob/7201cb4bda207d1e6261345e4d6c444a13bef2c5/packages/core/src/standards/http.ts#L506
...but worth checking what is and isn't allowed. 👍

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

Successfully merging a pull request may close this issue.

3 participants