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

307 redirects within actions do not result in a new POST request being made #4356

Closed
edwardbrowncross opened this issue Oct 11, 2022 · 7 comments
Labels
bug Something isn't working

Comments

@edwardbrowncross
Copy link

What version of Remix are you using?

1.7.0

Steps to Reproduce

Inside an action, redirect with a 307 status code. For example:

throw redirect(request.url, {
    status: 307,
    headers: {
        'Set-Cookie': refreshedAuthToken,
    },
});

Expected Behavior

The semantic meaning of 307 is

make another request with the same method at the new location

Therefore I expect the code above to be interpreted as

Call this action again (making a POST request to this endpoint)

Actual Behavior

It makes a GET request to the URL associated to the action (the normal behaviour of a 302 redirect on the web)

@edwardbrowncross
Copy link
Author

My current hack to make this work as I expected is to instead construct a response like this:

throw new Response(null, {
    status: 204,
    headers: {
        'Set-Cookie': cookie,
        'x-three-zero-seven': 'true',
        'Location': request.url,
    },
});

And then have a handleDataRequest function exported from entry.server.tsx like this:

export const handleDataRequest: HandleDataRequestFunction = (response: Response) => {
    if (response.headers.has('x-three-zero-seven')) {
        return new Response(response.body, {
            status: 307,
            headers: response.headers,
        });
    }
    return response;
};

This works since the handleDataRequest is called after the server code that turns redirects into x-remix-redirect responses has already happened.

@kiliman
Copy link
Collaborator

kiliman commented Oct 12, 2022

That's an odd status. It assumes the user-agent still has access to the originally posted data.

@machour
Copy link
Collaborator

machour commented Oct 12, 2022

Really interesting issue, thanks!
MDN docs for reference: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/307

@brophdawg11
Copy link
Contributor

brophdawg11 commented Oct 31, 2022

Very interesting indeed!

It assumes the user-agent still has access to the originally posted data.

I don't think this is a problem on our end, as we create the Request from source "submission" data, so we should still have the source data available to create another Request in response.

Based on this wording, it sounds like preserving the method is the right behavior in most but not all 3xx cases:

With 302, some old clients were incorrectly changing the method to GET

I'll do some deeper reading on that to be sure, but at a very quick glance it sounds like maybe 301/303 changing to a GET is ok, but 302/307/308 should preserve the method.

Update: @machour is probably correct below that we shouldn't change 302

@machour
Copy link
Collaborator

machour commented Nov 1, 2022

@brophdawg11 to continue the quote:

the behavior with non-GET methods and 302 is then unpredictable on the Web

As far as I remember, I've never seen a 302 doing something other than GET. I'm not sure we should start preserving the method for 302, as it would break a lot of apps..

@brophdawg11
Copy link
Contributor

remix-run/react-router#9597 adds this 307/308 handling to react-router, and Remix should pick that up once we finish moving Remix onto React Router 6.4

@machour machour added the awaiting release This issue has been fixed and will be released soon label Nov 24, 2022
@brophdawg11
Copy link
Contributor

We finished the above work in Remix 1.10.0 so this should be handled correctly now. I'm going to close this out but please re-open if you are still running into issues!

@brophdawg11 brophdawg11 added bug Something isn't working and removed bug:unverified awaiting release This issue has been fixed and will be released soon labels Jan 18, 2023
@brophdawg11 brophdawg11 removed their assignment Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants