[Service Worker] Support redirects to relative URLs in Safari#1978
Conversation
bgrgicak
added
[Type] Bug
bgrgicak
changed the title
brandonpayton
left a comment
brandonpayton
left a comment
There was a problem hiding this comment.
Since this is fixing a fairly bothersome problem for Safari users, I'm going to make a couple of the changes I suggested in this review and merge. And we can follow up later if needed.
brandonpayton
left a comment
brandonpayton
left a comment
There was a problem hiding this comment.
Since this is fixing a fairly bothersome problem for Safari users, I'm going to make a couple of the changes I suggested in this review and merge. And we can follow up later if needed.
Scratch that. Let's wait to merge this. As I started implementing the aforementioned suggestions, it seemed more and more like maybe we needed to adjust our approach. So I left a couple of comments and will get @bgrgicak's opinion on those.
Co-authored-by: Brandon Payton <brandon@happycode.net>
|
There is a reply on the WebKit bug about using Response.redirect to work around the issue. I was able to make it work by adding a Redirect in RemoteI took a look at it in this branch, but it didn't work, the 302 response locations are messed up (see screenshot). This is because it's to late to do the redirect there. Redirects
|
| /** | ||
| * Safari has a bug that prevents Service Workers from redirecting relative URLs. | ||
| * When attempting to redirect to a relative URL, the network request will return an error. | ||
| * See the Webkit bug for details: https://bugs.webkit.org/show_bug.cgi?id=282427. | ||
| * | ||
| * Because PHP and WordPress can redirect to both relative and absolute URLs | ||
| * using the `location` header we need to ensure redirects are processed | ||
| * correctly by the Service Worker. | ||
| * | ||
| * As a workaround for Safari Service Workers, we convert the `location` header | ||
| * to an absolute URL for all redirect responses (300-399 status codes). | ||
| * This resolves the issue because Safari correctly handles redirects | ||
| * to absolute URLs provided by the `location` header. | ||
| */ | ||
| if ( | ||
| phpResponse.httpStatusCode >= 300 && | ||
| phpResponse.httpStatusCode <= 399 && | ||
| phpResponse.headers['location'] | ||
| ) { | ||
| return Response.redirect( | ||
| phpResponse.headers['location'][0], | ||
| phpResponse.httpStatusCode | ||
| ); | ||
| } |
There was a problem hiding this comment.
@brandonpayton @adamziel This is now the new fix for the Safari issue. I closed all the outdated threads that aren't relevant anymore.
This code is ready for another review.
|
This looks good and I think we can merge. I'm just confused about this part of the PR description:
The redirection URL still can be relative even with |
@adamziel maybe something like this? |
3 participants
Before this PR when PHP tried to make a redirect (300 response) to a relative URL, Safari would return a network error and the new page wouldn't load in Playground, we would see a white screen.
This is caused by a Safari bug which prevents Safari from redirecting responses to relative URLs if the response comes from a Service Worker.
Until the bugfix is implemented in Safari, relative URLs can only be correctly redirected using
Response.redirect.This workaround was suggested in the bug report.
Fixes #645
Testing Instructions (or ideally a Blueprint)
Test this in Safari