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 multi-level redirect in server actions #72770

Merged
merged 2 commits into from
Nov 13, 2024

Conversation

ztanner
Copy link
Member

@ztanner ztanner commented Nov 13, 2024

redirect in server actions has handling to support relative redirects (eg redirect('./foo')) but it doesn't handle a multi-level relative path, eg redirect('../foo').

This used to work by accident because the invalid URL would be handled as an error and then would trigger an MPA navigation. In Next 15, it surfaces an actual error because we are less tolerant to them.

Fixes #72765
Closes NDX-479

Copy link
Member Author

ztanner commented Nov 13, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @ztanner and the rest of your teammates on Graphite Graphite

@ztanner ztanner force-pushed the 11-13-fix_multi-level_redirect_in_server_actions branch from a04970a to f1c0ba9 Compare November 13, 2024 16:39
@ztanner ztanner marked this pull request as ready for review November 13, 2024 16:58
@devjiwonchoi
Copy link
Member

In Next 15, it surfaces an actual error because we are less tolerant to them.

Q: So would the added test be passing on v14?

Would be great if you explain/ref about how we are less tolerant.

@ztanner ztanner force-pushed the 11-13-fix_multi-level_redirect_in_server_actions branch from f1c0ba9 to 9e137bd Compare November 13, 2024 17:12
@ztanner
Copy link
Member Author

ztanner commented Nov 13, 2024

Q: So would the added test be passing on v14?

I added an MPA navigation check in 9e137bd

Would be great if you explain/ref about how we are less tolerant.

Previously the relative redirect check only looked for the redirectUrl starting with / to determine if it was relative. (ref). If it wasn't a relative redirect, we returned a static response, which signals to the client that it should perform an MPA navigation. This is the code path that was being hit in Next 14.

In #64604, this was updated to parse the URL via new URL() to determine if it was a relative redirect. This would actually throw an error because it doesn't correctly append an origin, as it assumed it was a fully qualified path.

@ztanner ztanner enabled auto-merge (squash) November 13, 2024 17:33
@ztanner ztanner merged commit 6d8095d into canary Nov 13, 2024
104 of 110 checks passed
@ztanner ztanner deleted the 11-13-fix_multi-level_redirect_in_server_actions branch November 13, 2024 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

next/navigation redirect error with relative URL
4 participants