[Query API] Use the exact redirect URL provided in the ?url= query param by adamziel · Pull Request #1945 · WordPress/wordpress-playground

adamziel · 2024-10-25T17:19:23Z

This PR:

Replaces the wp_redirect() call with header("Location: ") to ensure support for all valid redirection URLs.
Adds a timeout before probing for the embedded Playground iframe URL when the load event handler runs.

Ditching `wp_redirect()`

The wp_redirect() call was introduced in #1856 to handle the post-autologin redirect. Unfortunately, it strips valid sequences such as %0A and %0D from the redirection URL. This isn't useful in Playground and breaks valid use-cases such as using the ?url= parameter to initialize html-api-debugger with a valid HTML markup containing newlines.

Timeout before probing `contentWindow.location.href`

When navigating to a page with %0A sequences (encoded newlines) in the query string, the location.href property of the iframe's content window doesn't seem to reflect them. Everything else is in place, but not the %0A sequences.

Weirdly, these sequences are available after the next event loop tick – hence the setTimeout(0).

The exact cause is unclear to me. The WHATWG HTML Standard [1] has a few hints:

Current and active session history entries may get out of sync for iframes.
Documents inside iframes have "is delaying load events" set to true.

But there doesn't seem to be any concrete explanation and no recommended remediation. If anyone has a clue, please share it in a GitHub issue or start a new PR.

[1] https://html.spec.whatwg.org/multipage/document-sequences.html#nav-active-history-entry

Testing instructions

CI tests aside, try this:

Go to http://localhost:5400/website-server/?plugin=html-api-debugger&url=%2Fwp-admin%2Fadmin.php%3Fpage%3Dhtml-api-debugger%26html%3D%253Cdiv%253E%250A1%250A2%250A3%250A%253C%252Fdiv%253E&wp=6.7&php=8.0&networking=no&language=&multisite=no&random=pf3oq1y3vx
Confirm the "address bar" on the Playground page reflects the actual, correct URL: /wp-admin/admin.php?page=html-api-debugger&html=%3Cdiv%3E%0A1%0A2%0A3%0A%3C%2Fdiv%3E
Confirm you can see the following HTML

<div>
1
2
3
</div>

cc @sirreal @bgrgicak

@sirreal

This PR: * Replaces the `wp_redirect()` call with `header("Location: ")` to ensure support for all valid redirection URLs. * Adds a timeout before probing for the embedded Playground `iframe` URL when the `load` event handler runs. ### Ditching `wp_redirect()` The `wp_redirect()` call was introduced in #1856 to handle the post-autologin redirect. Unfortunately, it strips valid sequences such as `%0A` and `%0D` from the redirection URL. This isn't useful in Playground and breaks valid use-cases such as using the `?url=` parameter to initialize `html-api-debugger` with a valid HTML markup containing newlines. ### Timeout before probing `contentWindow.location.href` When navigating to a page with %0A sequences (encoded newlines) in the query string, the `location.href` property of the iframe's content window doesn't seem to reflect them. Everything else is in place, but not the %0A sequences. Weirdly, these sequences are available after the next event loop tick – hence the `setTimeout(0)`. The exact cause is unclear to me. The WHATWG HTML Standard [1] has a few hints: * Current and active session history entries may get out of sync for iframes. * Documents inside iframes have "is delaying load events" set to true. But there doesn't seem to be any concrete explanation and no recommended remediation. If anyone has a clue, please share it in a GitHub issue or start a new PR. [1] https://html.spec.whatwg.org/multipage/document-sequences.html#nav-active-history-entry ## Testing instructions CI tests aside, try this: 1. Go to http://localhost:5400/website-server/?plugin=html-api-debugger&url=%2Fwp-admin%2Fadmin.php%3Fpage%3Dhtml-api-debugger%26html%3D%253Cdiv%253E%250A1%250A2%250A3%250A%253C%252Fdiv%253E&wp=6.6&php=8.0&networking=no&language=&multisite=no&random=pf3oq1y3vx 2. Confirm the "address bar" on the Playground page reflects the actual, correct URL: `/wp-admin/admin.php?page=html-api-debugger&html=%3Cdiv%3E%0A1%0A2%0A3%0A%3C%2Fdiv%3E` 3. Confirm you can see the following HTML ``` <div> 1 2 3 </div> ``` cc @sirreal @bgrgicak

sirreal · 2024-10-25T19:00:57Z

+					 */
+					// Get the content window while e.currentTarget is available.
+					// It will be undefined on the next event loop tick.
+					const contentWindow = e.currentTarget!.contentWindow;


Could this be related to the async event handler?

I've done some testing (not on playground) and with an async event handler the behavior seemed less reliable. However, with the sync event handler, e.target.contentWindow.location.href seemed to reliably be the expected value.

I don't know whether target/currentTarget matters, I suspect not, but target seemed to work well in my testing the load event on the iframe.

Yeah, the information is lost after the await. I was thinking it's related to React and how it clears currentTarget after the synchronous event handler returns, but now that I think about it, React shouldn't be in the mix here. You may be right about the target/currentTarget – was the latter affected by bubbling?

bgrgicak · 2024-10-28T06:41:36Z

+					// Get the content window while e.currentTarget is available.
+					// It will be undefined on the next event loop tick.
+					const contentWindow = e.currentTarget!.contentWindow;
+					await new Promise((resolve) => setTimeout(resolve, 0));


This doesn't work in Safari and Firefox.
Locally in Safari, it started working when I increased the timeout to 10, but that doesn't mean the exact timeout will always work.

Good spot! I wonder what's the root cause of this behavior. A setInterval and polling twice per second would likely solve this, but I'm hesitant of just throwing it in without fully understanding the problem.

adamziel · 2024-10-28T08:41:40Z

Oh wow, synchronizing navigables histories is an entire thing:

https://html.spec.whatwg.org/dev/browsing-the-web.html#centralized-modifications-of-session-history

adamziel · 2024-10-28T08:47:13Z

What that boils down to is:

Synchronizing URLs across navigables involves a sophisticated algorithm.
There's no specific event we can listen on and the timing may vary.
Detecting URL changes requires either polling in the parent context or calling postMessage() from the nested context.

bgrgicak · 2024-11-14T18:08:18Z

+	// We need to use the html-api-debugger plugin to test this because
+	// most wp-admin pages enforce a redirect to a sanitized (broken)
+	// version of the URL.


Is this because of the login redirect Playground does, or is it a WordPress redirect that sanitizes the URL?

bgrgicak · 2024-11-14T18:10:30Z

-				home_url($redirect_url),
-				302
-			);
+			$redirect_url = home_url($redirect_url);


@adamziel we don't need to force this to a absolute URL anymore because we have a a fix for the relative URL redirects in Safari.

Suggested change

$redirect_url = home_url($redirect_url);

I hope that you don't mind, that I merged trunk into your PR and resolved the conflict with the Safari redirect fix.

bgrgicak

@adamziel your test blueprint is throwing a boot error because of the HTML in the URL. I also checked trunk an the same is happening.

adamziel · 2024-11-14T23:43:22Z

Thanks for rebasing @bgrgicak! The test link is failing indeed, not good!

your test blueprint is throwing a boot error because of the HTML in the URL

I'm confused by the "because of the HTML in the URL" part. What do you mean? How is that causing a failure?

adamziel · 2024-11-14T23:45:36Z

Bumping wp=6.6 to wp=6.7 in the test URL solved the error, I assume the latest HTML debugger plugin isn't backwards compatible with WordPress 6.7. Can you confirm @sirreal ?

bgrgicak · 2024-11-15T05:27:38Z

I'm confused by the "because of the HTML in the URL" part. What do you mean? How is that causing a failure?

Ignore that part, when I tested the blueprint, removing the html=%3Cdiv%3E%0A1%0A2%0A3%0A%3C%2Fdiv%3E argument seemed to make it work for me on Playground.WordPress.net, but it's unrelated.

Later I also realized it was related to the WP versions, but it was late and I wanted to do more research in the morning before commenting anything.

I ran the Webkit test locally a couple of times and it passes locally.

sirreal · 2024-11-15T05:54:57Z

I assume the latest HTML debugger plugin isn't backwards compatible with WordPress 6.7. Can you confirm @sirreal ?

Its minimum version was changed to latest (6.7) and does rely on new functionality. Sorry it broke the test!

adamziel · 2024-11-15T22:37:41Z

I've disabled that e2e test also in Safari. Let's get the fix in and keep monitoring the problem in Firefox and Safari. It's such a weird issue with close to no reading material out there and I can't think of any other solution anyway.

adamziel added [Type] Bug An existing feature does not function as intended [Package][@wp-playground] Blueprints [Aspect] Website labels Oct 25, 2024

adamziel requested a review from a team as a code owner October 25, 2024 17:19

adamziel added 4 commits October 25, 2024 19:21

Explain why assign contentWindow to a new variable

ee3b134

Adjust the Playwright test

2f71e1b

Use baesURI in the test

bce3918

Revert back to ownerDocument.location.href

7f7a459

sirreal reviewed Oct 25, 2024

View reviewed changes

bgrgicak approved these changes Oct 28, 2024

View reviewed changes

bgrgicak self-requested a review October 28, 2024 06:26

bgrgicak reviewed Oct 28, 2024

View reviewed changes

bgrgicak self-requested a review November 5, 2024 06:17

adamziel added 2 commits November 13, 2024 20:35

Disable the test in Firefox

421948d

Update query-api.spec.ts

0562768

bgrgicak reviewed Nov 14, 2024

View reviewed changes

Merge branch 'trunk' into fix-query-api-redirects

8788f02

bgrgicak self-requested a review November 14, 2024 19:48

bgrgicak reviewed Nov 14, 2024

View reviewed changes

Disable the e2e test in Safari

0d96f68

adamziel merged commit 7530f12 into trunk Nov 15, 2024

adamziel deleted the fix-query-api-redirects branch November 15, 2024 23:55

Conversation

adamziel commented Oct 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ditching wp_redirect()

Timeout before probing contentWindow.location.href

Testing instructions

Uh oh!

sirreal Oct 25, 2024

Choose a reason for hiding this comment

Uh oh!

adamziel Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgrgicak Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

adamziel Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamziel commented Oct 28, 2024

Uh oh!

adamziel commented Oct 28, 2024

Uh oh!

bgrgicak Nov 14, 2024

Choose a reason for hiding this comment

Uh oh!

bgrgicak Nov 14, 2024

Choose a reason for hiding this comment

Uh oh!

bgrgicak Nov 14, 2024

Choose a reason for hiding this comment

Uh oh!

bgrgicak left a comment

Choose a reason for hiding this comment

Uh oh!

adamziel commented Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamziel commented Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bgrgicak commented Nov 15, 2024

Uh oh!

sirreal commented Nov 15, 2024

Uh oh!

adamziel commented Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adamziel commented Oct 25, 2024 •

edited

Loading

Ditching `wp_redirect()`

Timeout before probing `contentWindow.location.href`

adamziel Nov 4, 2024 •

edited

Loading

adamziel Oct 28, 2024 •

edited

Loading

adamziel commented Nov 14, 2024 •

edited

Loading

adamziel commented Nov 14, 2024 •

edited

Loading

adamziel commented Nov 15, 2024 •

edited

Loading