close
Skip to content

[Website] Fix redirect loop on Safari by awaiting cookie store population and re-scoping redirect URLs#3365

Merged
adamziel merged 4 commits intotrunkfrom
fix-safari-absolute-path-to-relative-path-conversion
Mar 10, 2026
Merged

[Website] Fix redirect loop on Safari by awaiting cookie store population and re-scoping redirect URLs#3365
adamziel merged 4 commits intotrunkfrom
fix-safari-absolute-path-to-relative-path-conversion

Conversation

@mho22
Copy link
Collaborator

@mho22 mho22 commented Mar 10, 2026

Motivation for the change, related issues

#3361 introduced streamed PHP responses in the service worker. This changed the cookie store population from synchronous (awaited via PHPResponse.fromStreamedResponse()) to a floating .then() that could resolve after the next request arrived. This caused playground_auto_login to redirect infinitely on Safari because the playground_auto_login_already_happened cookie was never stored in time. Response.redirect() drops Set-Cookie headers, and the internal cookie store was the only mechanism preserving them across requests.

A secondary issue was that Response.redirect(new URL('/wp-admin/', scopedUrl)) resolves absolute paths against the origin, stripping the /scope:…/ prefix.

cc @bgrgicak

Implementation details

  • php-request-handler.ts: Replace the floating .then() with await response.headers before returning from requestStreamed(), ensuring the cookie store is populated before the next request can arrive. Headers are available as soon as PHP flushes them, well before the body finishes streaming, so this does not hurt throughput.
  • utils.ts: Re-scope the Location URL before calling Response.redirect(). If the resolved URL lacks the /scope:…/ prefix, add it back using setURLScope.

Testing Instructions (or ideally a Blueprint)

run npm run dev and go to http://127.0.0.1:5400/website-server/ on safari

It will crash on trunk and run correctly from this branch.

Copilot AI review requested due to automatic review settings March 10, 2026 14:40
@mho22 mho22 added [Type] Bug An existing feature does not function as intended [Aspect] Service Worker labels Mar 10, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adjusts landing-page redirection to use relative paths so Safari’s service-worker Response.redirect() workaround preserves the scoped URL prefix during resolution.

Changes:

  • Adds detailed rationale/docs for why relative landing-page paths are required for scoped service-worker routing.
  • Replaces pathToInternalUrl() usage with a simple absolute-to-relative path rewrite (/foo./foo) before redirecting via the intermediate handler.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mho22 mho22 force-pushed the fix-safari-absolute-path-to-relative-path-conversion branch from 93652ca to 79bc32a Compare March 10, 2026 15:39
@mho22 mho22 requested review from a team and adamziel and removed request for a team March 10, 2026 16:26
@mho22 mho22 changed the title Convert absolute paths to relative paths to preserve scope after URL resolution [Website] ⏺ Fix redirect loop on Safari by awaiting cookie store population and re-scoping redirect URLs Mar 10, 2026
@mho22 mho22 changed the title [Website] ⏺ Fix redirect loop on Safari by awaiting cookie store population and re-scoping redirect URLs [Website] Fix redirect loop on Safari by awaiting cookie store population and re-scoping redirect URLs Mar 10, 2026
Copy link
Member

@brandonpayton brandonpayton left a comment

Choose a reason for hiding this comment

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

This looks great. Thank you, @mho22!

Updated comments to clarify the purpose of awaiting response headers for cookie storage.
@adamziel adamziel merged commit 6e19bae into trunk Mar 10, 2026
42 checks passed
@adamziel adamziel deleted the fix-safari-absolute-path-to-relative-path-conversion branch March 10, 2026 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants