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

Fetch requests don't expose Set-Cookie response header #183

Closed
ottomated opened this issue Feb 20, 2022 · 7 comments
Closed

Fetch requests don't expose Set-Cookie response header #183

ottomated opened this issue Feb 20, 2022 · 7 comments
Milestone

Comments

@ottomated
Copy link

The following code behaves differently on miniflare VS on a deployed worker:

addEventListener('fetch', event => {
  event.respondWith(handleRequest(event.request))
})
/**
 * Respond with hello worker text
 * @param {Request} request
 */
async function handleRequest(request) {
  const url = new URL(request.url);

  if (url.pathname === '/cookies') {
    return new Response(null, {
      headers: {
        'Set-Cookie': 'foo=bar'
      }
    })
  } else {
    const res = await fetch(`${url.origin}/cookies`);
    return new Response(JSON.stringify(res.headers.get('set-cookie')))
  }
}
  • Visiting the /cookies URL returns a set-cookie header with the value foo=bar. Visiting any other URL does a sub-fetch to the /cookies path and returns the value of the set-cookie header in the response body.

In miniflare, this returns null.. In a worker, this behaves as expected - the set-cookie header is exposed to the user.

The source of this is that undici follows the fetch spec closer than workers. The conflict source is undici/lib/fetch/response.js:370, the filterResponse function, which removes forbidden headers from the response such as set-cookie.

@joona
Copy link

joona commented Feb 22, 2022

I'm hitting the same problem while trying to integrate miniflare to our development process.

In our case, this happens when proxying API requests on path /api to our API Origin behind CF Access (Service Token authentication) without doing any modifications to the response. Request is cloned to be able to attach additional headers for service token authentication. However, the problem also happens when doing no header modifications at all and using IP whitelisting on CF Access side.

The same code works perfectly on Worker runtime.

Invocation on both environments is same-site with valid HTTPS and withCredentials on the client side, etc.

While doing my own digging, I also came to conclusion, that those headers are dropped due undici fetch implementation, but didn't find any way to work around this.

@hansede
Copy link

hansede commented Mar 2, 2022

I also came to the same conclusion today. Unfortunately this is the last thing preventing me from deploying Miniflare as part of our development environment. When the set-cookie header fails to propagate, Django's csrf_token can't be set, so Django auth doesn't work. This is notably different behavior from Cloudflare Workers.

@hansede
Copy link

hansede commented Mar 4, 2022

I did some additional investigation and opened an issue with Undici:
nodejs/undici#1262

@mrbbot
Copy link
Contributor

mrbbot commented Mar 4, 2022

Hey everyone! 👋 Thanks for raising this and apologies for the delayed response. This is definitely a bug, might be possible to extract this header from internal undici properties like we're doing here:

const internalResponse = baseRes[fetchSymbols.kState].internalResponse;

...although probably worth waiting on nodejs/undici#1262 for a proper fix. 👍

@joona
Copy link

joona commented Jun 2, 2022

Fix for the problem has been now released on undici v5.4.0 release.

nodejs/undici@v5.3.0...v5.4.0

@mrbbot
Copy link
Contributor

mrbbot commented Jun 2, 2022

Nice! 🎉 I think this was actually fixed in Miniflare 2.5.0 because of this hack:

const constants: {
readonly forbiddenHeaderNames: string[];
readonly forbiddenResponseHeaderNames: string[];
} = require("undici/lib/fetch/constants.js");
constants.forbiddenHeaderNames.length = 0;
constants.forbiddenResponseHeaderNames.length = 0;

...but I forgot to close this issue. Anyways, upgrading to[email protected] is the correct fix.

@mrbbot mrbbot modified the milestones: 2.6.0, 2.5.0 Jun 2, 2022
@mrbbot mrbbot closed this as completed Jun 2, 2022
@joona
Copy link

joona commented Jun 2, 2022

Thank you @mrbbot! You beat me to it, I was just about to send PR.

I can also confirm that the hack worked and cookies are passed properly on v2.5.0 as well 👍

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

No branches or pull requests

4 participants