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

fix: fixes cookie override during redirection from server action #61633

Merged
merged 6 commits into from
Apr 4, 2024

Conversation

technikhil314
Copy link
Contributor

@technikhil314 technikhil314 commented Feb 4, 2024

What?

Fixes #61611

Why?

Any one having custom server may be having logic to set cookies during GET requests too. Currently nextjs in app directory does not allow to do so but with custom server its very much possible.

How?

By merging cookies of redirect response and server action POSt response

Tests

I have added one more test to existing suite and it passing with fix in place.
image

Notes

This bug is reproducible only if developer has custom server on top of next app but still very probable

Copy link

@gggneo gggneo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted

@technikhil314
Copy link
Contributor Author

@shuding Thanks a lot for approval but I see the merge button is still disabled for me. Let me know if I am missing any steps.

@OlegLustenko
Copy link

OlegLustenko commented Apr 4, 2024

Hi team, we are implementing some auth flow that will be simplified a lot with these changes.

We are currently working around it via returning cookies to a client and then shooting another request and then redirect.

Can attract some attention to this, maybe @ztanner ?

I saw you fixed a bunch of things in recent months, could you take your eyes on it please?

If it doesn't affects your existing priorities or planned scope

🙏🏻🙏🏻

@ijjk ijjk merged commit c502308 into vercel:canary Apr 4, 2024
74 checks passed
epiloguess pushed a commit to epiloguess/next.js that referenced this pull request Apr 5, 2024
)

### What?
Fixes vercel#61611

### Why?
Any one having custom server may be having logic to set cookies during
GET requests too. Currently nextjs in app directory does not allow to do
so but with custom server its very much possible.

### How?
By merging cookies of redirect response and server action POSt response

### Tests
I have added one more test to existing suite and it passing with fix in
place.

![image](https://github.com/vercel/next.js/assets/6815560/858afdbb-c377-49eb-9002-fcbdf06583a4)

### Notes
This bug is reproducible only if developer has custom server on top of
next app but still very probable

---------

Co-authored-by: Shu Ding <g@shud.in>
Co-authored-by: JJ Kasper <jj@jjsweb.site>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cookies in POST response from server action + redirect() are overridden by streamed GET response cookies
5 participants