Behavior mismatch: POST request with 302 response fails to redirect and doesn't set cookies #133

hansede · 2021-12-23T01:03:28Z

Expected (Cloudflare behavior)

When making a POST request on an endpoint to which the origin server responds with a 302 status code, I expect that my client will receive the 302 status code, proxied through Miniflare, and be redirected accordingly. I also expect the set-cookie header on the 302 response sent from the origin server to be present when the response is received by the client.

Actual (Miniflare behavior)

When making a POST request to an endpoint that responds with a 302 status code, Miniflare internally resolves the 302 and returns a 200 status code instead. My client is not redirected to a new URL, and instead the contents of the page I would have been redirected to are served up at the original URL (where I made the POST request). The set-cookie response header from the 302 response is absent on the 200 response.

Example

Express server (server.js):

const express = require('express');
const app = express();

app.post('/redirect', (req, res) => {
  res.cookie('sessionid', 'abc123');
  res.redirect(302, '/');
});

app.get('/', (req, res) => {
  res.end('<html><body><form action="/redirect/" method="POST"><input type="submit" value="Submit"></form></body></html>');
});

app.listen('3000');

Miniflare worker (worker.js):

addEventListener('fetch', function(event) {
  event.respondWith(handleRequest(event.request))
})

async function handleRequest(request) {
  return fetch(request)
}

To run the Express server:

npm i express
node server.js

To run the Miniflare server:

miniflare worker.js -u http://localhost:3000

Hitting the Express server directly:

Clear your cookies
Navigate to http://localhost:3000/
Click the "submit" button
A POST request will be made to /redirect/
The browser will respond with a 302 status code and you will be redirected back to /
The sessionid cookie will now be set

Hitting the Express server, proxied through Miniflare:

Clear your cookies
Navigate to http://localhost:8787/
Click the "submit" button
A POST request will be made to /redirect/
You will receive a 200 status code, but the browser will not be redirected
You will see the contents of /, but the URL will still be /redirect/
The sessionid cookie will not be set

I would expect the behavior to be the same whether proxied through Miniflare or not.

The text was updated successfully, but these errors were encountered:

vzaramel · 2021-12-28T11:39:14Z

Looking through some code I think it would be possible to change the default behavior from follow to manual doing something like:

async function handleRequest(request) {
  return fetch(request, {redirect: 'manual'})
}

https://github.com/mrbbot/node-fetch/blob/bc466f48166cada665f9a068df933cc8a2a824d7/src/index.js#L81-L86
If manual still has a undesired behavior a dummy value, like none, could be passed

I didn't try this code above but it seems like it should work.

hnrqer · 2021-12-28T22:15:36Z

It seems that httpRedirectFetch is erroring in this part:
https://github.com/nodejs/undici/blob/917fbfaf368f2274f0928f8703b15a85a0c7bd6b/lib/fetch/index.js#L943-L951

hnrqer · 2021-12-31T15:30:16Z

@vzaramel and I worked in a fix for this and we created a commit to fix it: clickfunnels2@4c3d294.

There is a test failing related to subrequests counting which I didn't go deep to investigate, but it's not affecting the purposes of the way we are using Miniflare still:

  packages/core/test/standards/http.spec.ts:859

   858:   await ctx.runWith(() => fetch(url));
   859:   t.is(ctx.subrequests, 4);           
   860: });                                   

  Difference:

  - 1
  + 4

Maybe this change could be considered to be added in current v2 branch.

mrbbot · 2022-01-01T23:33:33Z

Hey everyone! 👋 First off, thank you very much for the detailed repoduction @hansede 🧡. Spent a little time tonight looking into this.

When I tried proxying through Miniflare, I got a fetch failed error which I think @hnrqer mentioned. It looks like undici puts most of the useful debugging information in the cause property of the error which wasn't being logged. Doing that shows a RequestContentLengthMismatchError, that seems to be caused by the Content-Length: 0 header included on the incoming request by Miniflare. You can reproduce this with:

import { fetch } from "undici";
await fetch("http://localhost:3000/", { method: "GET", headers: { "Content-Length": "0" }});

Removing the Content-Length header before sending the request, I was able to get the expected incorrect results...

Thanks also @vzaramel and @hnrqer 🧡 for identifying and working on a potential fix for the redirect issue. I've got a few comments/questions:

Should redirect always be set to manual? What about GET requests?
I don't think we should be exposing internalResponse() in the Response class. This would be accessible to users' Worker code.
Is body always going to be null for opaqueredirects? As far as I know, you can still set a body for redirects.
I think all this code should be contained within the fetch function. Doing it just in the HTTP server means you wouldn't be able to see headers for redirects if you wanted to do something with the fetch result in your worker.

Apologies I've only just gotten round to this.

hnrqer · 2022-01-02T23:52:50Z

Should redirect always be set to manual? What about GET requests?

Not entirely completely sure about about this one and the implications on all GET requests. For our tests, normal GET requests weren't impacted when using { redirect: "manual" }. These would only affect real redirect response (301, 302, 303, 304)

I don't think we should be exposing internalResponse() in the Response class. This would be accessible to users' Worker code.

Yes, if we don't wanna expose the internal response we could simply get the internalResponse in the http-server package or somewhere else.

Is body always going to be null for opaqueredirects? As far as I know, you can still set a body for redirects.

This is what undici does for manual redirects (it actually sets the body to null),

we've just set the body to null so that it doesn't run this part:

miniflare/packages/http-server/src/index.ts

Lines 251 to 279 in 6450bd5

    
             if (res) { 
        
               // `initialStream` is the stream we'll write the response to. It 
        
               // should end up as the first encoder, piping to the next encoder, 
        
               // and finally piping to the response: 
        
               // 
        
               // encoders[0] (initialStream) -> encoders[1] -> res 
        
               // 
        
               // Not using `pipeline(passThrough, ...encoders, res)` here as that 
        
               // gives a premature close error with server sent events. This also 
        
               // avoids creating an extra stream even when we're not encoding. 
        
               let initialStream: Writable = res; 
        
               for (let i = encoders.length - 1; i >= 0; i--) { 
        
                 encoders[i].pipe(initialStream); 
        
                 initialStream = encoders[i]; 
        
               } 
        
               if (response.body) { 
        
                 for await (const chunk of response.body) { 
        
                   if (chunk) initialStream.write(chunk); 
        
                 } 
        
                 if (liveReloadEnabled) { 
        
                   initialStream.write(liveReloadScript); 
        
                 } 
        
               } 
        
               initialStream.end(); 
        
             } 
        
           } catch (e: any) {

I think we had an issue in that part because the body was a structure returned by undici where the stream lives in body.stream, but I think that could still be invesitgated.

I think all this code should be contained within the fetch function. Doing it just in the HTTP server means you wouldn't be able to see headers for redirects if you wanted to do something with the fetch result in your worker.

Hmm, do you mean that should live in the undici fetch function? Interestingly, this is how Deno solved this issue (returning the response itself instead of that living in internalResponse when the request redirect is manual):

fix(op_crates/fetch): redirect: "manual" fetch should return type: "default" response denoland/deno#8353

It's also interesting to see the discussion around the issues in that PR.

Apologies I've only just gotten round to this.

NP, really. Thanks for replying this issue, the Miniflare project is really helping us, so actually thank you for all the contributions in this project.

vzaramel · 2022-01-03T17:32:54Z

I think all this code should be contained within the fetch function. Doing it just in the HTTP server means you wouldn't be able to see headers for redirects if you wanted to do something with the fetch result in your worker.

Hmm, do you mean that should live in the undici fetch function? Interestingly, this is how Deno solved this issue (returning the response itself instead of that living in internalResponse when the request redirect is manual):

I think @mrbbot meant to implement it arround here.

miniflare/packages/core/src/standards/http.ts

Lines 637 to 650 in a57b936

    
           const baseRes = await baseFetch(req); 
        
           // Increment the subrequest count by the number of redirects 
        
           // TODO (someday): technically we should check the subrequest count before 
        
           //  each redirect, so requests don't actually get sent to the server if the 
        
           //  subrequest count exceeds the limit 
        
           if (baseRes.redirected && ctx) { 
        
             const urlList = _getURLList(baseRes); 
        
             // Last url is final destination, so subtract 1 for redirect count 
        
             if (urlList) ctx.incrementSubrequests(urlList.length - 1); 
        
           } 
        
           // Convert the response to our hybrid Response 
        
           const res = new Response(baseRes.body, baseRes);

The best case would be to undici to patch their code as deno did as @hnrqer pointed out. But meanwhile we could have this fix in here ^^

Also sets the `redirect` mode of incoming requests to `manual`: https://developers.cloudflare.com/workers/runtime-apis/request#requestinit This ensures redirects are proxied to the end user, meaning cookies set in responses are stored in the browser. Co-Authored-By: Henrique Sarmento <hnrqer@gmail.com> Co-Authored-By: Vinicius Zaramella <vinicius.zaramella@gmail.com>

mrbbot · 2022-01-03T19:12:47Z

Awesome 👍, thanks for all your responses. That Deno PR was very interesting. I noticed Cloudflare actually sets the redirect mode to "manual" for incoming requests so we don't need to do this in fetch. I've implemented your opaqueredirect response unwrapping though, thanks again @vzaramel and @hnrqer for working on this. 😃

Also sets the `redirect` mode of incoming requests to `manual`: https://developers.cloudflare.com/workers/runtime-apis/request#requestinit This ensures redirects are proxied to the end user, meaning cookies set in responses are stored in the browser. Co-Authored-By: Henrique Sarmento <hnrqer@gmail.com> Co-Authored-By: Vinicius Zaramella <vinicius.zaramella@gmail.com>

mrbbot · 2022-01-07T18:51:20Z

Hey! 👋 Miniflare 2.0.0 has just been released, which includes this fix. You can find the changes since 2.0.0-rc.5 at the end of the changelog and install it with npm i miniflare -D. Please let me know if you have any other issues, and feel free to ask questions in the #miniflare channel of the Cloudflare Workers Discord server.

mrbbot added the behaviour mismatch Different behaviour to Workers runtime label Jan 1, 2022

mrbbot mentioned this issue Jan 2, 2022

Bumping lib to 2.0.0-next.3 causes UND_ERR_REQ_CONTENT_LENGTH_MISMATCH in POST request. #92

Closed

mrbbot added the fixed-in-next Fixed in next release label Jan 3, 2022

mrbbot closed this as completed in 1bf7600 Jan 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Behavior mismatch: POST request with 302 response fails to redirect and doesn't set cookies #133

Behavior mismatch: POST request with 302 response fails to redirect and doesn't set cookies #133

hansede commented Dec 23, 2021 •

edited

Loading

vzaramel commented Dec 28, 2021

hnrqer commented Dec 28, 2021 •

edited

Loading

hnrqer commented Dec 31, 2021

mrbbot commented Jan 1, 2022 •

edited

Loading

hnrqer commented Jan 2, 2022 •

edited

Loading

vzaramel commented Jan 3, 2022

mrbbot commented Jan 3, 2022

mrbbot commented Jan 7, 2022

Behavior mismatch: POST request with 302 response fails to redirect and doesn't set cookies #133

Behavior mismatch: POST request with 302 response fails to redirect and doesn't set cookies #133

Comments

hansede commented Dec 23, 2021 • edited Loading

Expected (Cloudflare behavior)

Actual (Miniflare behavior)

Example

vzaramel commented Dec 28, 2021

hnrqer commented Dec 28, 2021 • edited Loading

hnrqer commented Dec 31, 2021

mrbbot commented Jan 1, 2022 • edited Loading

hnrqer commented Jan 2, 2022 • edited Loading

vzaramel commented Jan 3, 2022

mrbbot commented Jan 3, 2022

mrbbot commented Jan 7, 2022

hansede commented Dec 23, 2021 •

edited

Loading

hnrqer commented Dec 28, 2021 •

edited

Loading

mrbbot commented Jan 1, 2022 •

edited

Loading

hnrqer commented Jan 2, 2022 •

edited

Loading