-
Notifications
You must be signed in to change notification settings - Fork 274
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
[Query API] Use the exact redirect URL provided in the ?url= query param #1945
Changes from 5 commits
e57f55b
ee3b134
2f71e1b
bce3918
7f7a459
421948d
0562768
8788f02
0d96f68
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -129,8 +129,35 @@ export async function bootPlaygroundRemote() { | |
// Manage the address bar | ||
wpFrame.addEventListener('load', async (e: any) => { | ||
try { | ||
/** | ||
* 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 at the moment of writing of this | ||
* comment. 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 | ||
*/ | ||
// 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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't work in Safari and Firefox. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good spot! I wonder what's the root cause of this behavior. A |
||
const path = await playground.internalUrlToPath( | ||
e.currentTarget!.contentWindow.location.href | ||
contentWindow.location.href | ||
); | ||
fn(path); | ||
} catch (e) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,14 +92,40 @@ test('should not login the user in if the login query parameter is set to no', a | |
); | ||
}); | ||
|
||
['/wp-admin/', '/wp-admin/post.php?post=1&action=edit'].forEach((path) => { | ||
test(`should correctly redirect encoded wp-admin url to ${path}`, async ({ | ||
website, | ||
wordpress, | ||
}) => { | ||
[ | ||
['/wp-admin/', 'should redirect to wp-admin'], | ||
['/wp-admin/post.php?post=1&action=edit', 'should redirect to post editor'], | ||
].forEach(([path, description]) => { | ||
test(description, async ({ website, wordpress }) => { | ||
await website.goto(`./?url=${encodeURIComponent(path)}`); | ||
expect( | ||
await wordpress.locator('body').evaluate((body) => body.baseURI) | ||
await wordpress | ||
.locator('body') | ||
.evaluate((body) => body.ownerDocument.location.href) | ||
).toContain(path); | ||
}); | ||
}); | ||
|
||
/** | ||
* There is no reason to remove encoded control characters from the URL. | ||
* For example, the html-api-debugger accepts markup with newlines encoded | ||
* as %0A via the query string. | ||
*/ | ||
test('should retain encoded control characters in the URL', async ({ | ||
website, | ||
wordpress, | ||
}) => { | ||
const path = | ||
'/wp-admin/admin.php?page=html-api-debugger&html=%3Cdiv%3E%0A1%0A2%0A3%0A%3C%2Fdiv%3E'; | ||
// 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. | ||
Comment on lines
+127
to
+129
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this because of the login redirect Playground does, or is it a WordPress redirect that sanitizes the URL? |
||
await website.goto( | ||
`./?url=${encodeURIComponent(path)}&plugin=html-api-debugger` | ||
); | ||
expect( | ||
await wordpress | ||
.locator('body') | ||
.evaluate((body) => body.ownerDocument.location.href) | ||
).toContain(path); | ||
}); |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -171,10 +171,16 @@ export async function setupPlatformLevelMuPlugins(php: UniversalPHP) { | |||
$parts = explode('/', $redirect_url); | ||||
$redirect_url = '/' . implode('/', array_slice($parts, 2)); | ||||
} | ||||
wp_redirect( | ||||
home_url($redirect_url), | ||||
302 | ||||
); | ||||
$redirect_url = home_url($redirect_url); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I hope that you don't mind, that I merged trunk into your PR and resolved the conflict with the Safari redirect fix. |
||||
|
||||
/** | ||||
* Intentionally do not use wp_redirect() here. It removes | ||||
* %0A and %0D sequences from the URL, which we don't want. | ||||
* There are valid use-cases for encoded newlines in the query string, | ||||
* for example html-api-debugger accepts markup with newlines | ||||
* encoded as %0A via the query string. | ||||
*/ | ||||
header( "Location: $redirect_url", true, 302 ); | ||||
exit; | ||||
} | ||||
/** | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,112 +1,81 @@ | ||
{ | ||
"compileOnSave": false, | ||
"compilerOptions": { | ||
"rootDir": ".", | ||
"sourceMap": true, | ||
"declaration": false, | ||
"moduleResolution": "node", | ||
"esModuleInterop": true, | ||
"emitDecoratorMetadata": true, | ||
"experimentalDecorators": true, | ||
"importHelpers": true, | ||
"resolveJsonModule": true, | ||
"jsx": "react", | ||
"target": "ES2021", | ||
"module": "esnext", | ||
"lib": [ | ||
"ES2022", | ||
"esnext.disposable", | ||
"dom" | ||
], | ||
"skipLibCheck": true, | ||
"skipDefaultLibCheck": true, | ||
"baseUrl": ".", | ||
"paths": { | ||
"@php-wasm/cli": [ | ||
"packages/php-wasm/cli/src/main.ts" | ||
], | ||
"@php-wasm/fs-journal": [ | ||
"packages/php-wasm/fs-journal/src/index.ts" | ||
], | ||
"@php-wasm/logger": [ | ||
"packages/php-wasm/logger/src/index.ts" | ||
], | ||
"@php-wasm/node": [ | ||
"packages/php-wasm/node/src/index.ts" | ||
], | ||
"@php-wasm/node-polyfills": [ | ||
"packages/php-wasm/node-polyfills/src/index.ts" | ||
], | ||
"@php-wasm/private": [ | ||
"packages/php-wasm/private/src/index.ts" | ||
], | ||
"@php-wasm/progress": [ | ||
"packages/php-wasm/progress/src/index.ts" | ||
], | ||
"@php-wasm/scopes": [ | ||
"packages/php-wasm/scopes/src/index.ts" | ||
], | ||
"@php-wasm/stream-compression": [ | ||
"packages/php-wasm/stream-compression/src/index.ts" | ||
], | ||
"@php-wasm/universal": [ | ||
"packages/php-wasm/universal/src/index.ts" | ||
], | ||
"@php-wasm/util": [ | ||
"packages/php-wasm/util/src/index.ts" | ||
], | ||
"@php-wasm/web": [ | ||
"packages/php-wasm/web/src/index.ts" | ||
], | ||
"@php-wasm/web-service-worker": [ | ||
"packages/php-wasm/web-service-worker/src/index.ts" | ||
], | ||
"@wp-playground/blueprints": [ | ||
"packages/playground/blueprints/src/index.ts" | ||
], | ||
"@wp-playground/cli": [ | ||
"packages/playground/cli/src/cli.ts" | ||
], | ||
"@wp-playground/client": [ | ||
"packages/playground/client/src/index.ts" | ||
], | ||
"@wp-playground/common": [ | ||
"packages/playground/common/src/index.ts" | ||
], | ||
"@wp-playground/components": [ | ||
"packages/playground/components/src/index.ts" | ||
], | ||
"@wp-playground/nx-extensions": [ | ||
"packages/nx-extensions/src/index.ts" | ||
], | ||
"@wp-playground/remote": [ | ||
"packages/playground/remote/src/index.ts" | ||
], | ||
"@wp-playground/storage": [ | ||
"packages/playground/storage/src/index.ts" | ||
], | ||
"@wp-playground/sync": [ | ||
"packages/playground/sync/src/index.ts" | ||
], | ||
"@wp-playground/unit-test-utils": [ | ||
"packages/playground/unit-test-utils/src/index.ts" | ||
], | ||
"@wp-playground/website": [ | ||
"packages/playground/website/src/index.ts" | ||
], | ||
"@wp-playground/wordpress": [ | ||
"packages/playground/wordpress/src/index.ts" | ||
], | ||
"@wp-playground/wordpress-builds": [ | ||
"packages/playground/wordpress-builds/src/index.ts" | ||
], | ||
"isomorphic-git": [ | ||
"./isomorphic-git/src" | ||
] | ||
} | ||
}, | ||
"exclude": [ | ||
"node_modules", | ||
"tmp" | ||
] | ||
"compileOnSave": false, | ||
"compilerOptions": { | ||
"rootDir": ".", | ||
"sourceMap": true, | ||
"declaration": false, | ||
"moduleResolution": "node", | ||
"esModuleInterop": true, | ||
"emitDecoratorMetadata": true, | ||
"experimentalDecorators": true, | ||
"importHelpers": true, | ||
"resolveJsonModule": true, | ||
"jsx": "react", | ||
"target": "ES2021", | ||
"module": "esnext", | ||
"lib": ["ES2022", "esnext.disposable", "dom"], | ||
"skipLibCheck": true, | ||
"skipDefaultLibCheck": true, | ||
"baseUrl": ".", | ||
"paths": { | ||
"@php-wasm/cli": ["packages/php-wasm/cli/src/main.ts"], | ||
"@php-wasm/fs-journal": [ | ||
"packages/php-wasm/fs-journal/src/index.ts" | ||
], | ||
"@php-wasm/logger": ["packages/php-wasm/logger/src/index.ts"], | ||
"@php-wasm/node": ["packages/php-wasm/node/src/index.ts"], | ||
"@php-wasm/node-polyfills": [ | ||
"packages/php-wasm/node-polyfills/src/index.ts" | ||
], | ||
"@php-wasm/private": ["packages/php-wasm/private/src/index.ts"], | ||
"@php-wasm/progress": ["packages/php-wasm/progress/src/index.ts"], | ||
"@php-wasm/scopes": ["packages/php-wasm/scopes/src/index.ts"], | ||
"@php-wasm/stream-compression": [ | ||
"packages/php-wasm/stream-compression/src/index.ts" | ||
], | ||
"@php-wasm/universal": ["packages/php-wasm/universal/src/index.ts"], | ||
"@php-wasm/util": ["packages/php-wasm/util/src/index.ts"], | ||
"@php-wasm/web": ["packages/php-wasm/web/src/index.ts"], | ||
"@php-wasm/web-service-worker": [ | ||
"packages/php-wasm/web-service-worker/src/index.ts" | ||
], | ||
"@wp-playground/blueprints": [ | ||
"packages/playground/blueprints/src/index.ts" | ||
], | ||
"@wp-playground/cli": ["packages/playground/cli/src/cli.ts"], | ||
"@wp-playground/client": [ | ||
"packages/playground/client/src/index.ts" | ||
], | ||
"@wp-playground/common": [ | ||
"packages/playground/common/src/index.ts" | ||
], | ||
"@wp-playground/components": [ | ||
"packages/playground/components/src/index.ts" | ||
], | ||
"@wp-playground/nx-extensions": [ | ||
"packages/nx-extensions/src/index.ts" | ||
], | ||
"@wp-playground/remote": [ | ||
"packages/playground/remote/src/index.ts" | ||
], | ||
"@wp-playground/storage": [ | ||
"packages/playground/storage/src/index.ts" | ||
], | ||
"@wp-playground/sync": ["packages/playground/sync/src/index.ts"], | ||
"@wp-playground/unit-test-utils": [ | ||
"packages/playground/unit-test-utils/src/index.ts" | ||
], | ||
"@wp-playground/website": [ | ||
"packages/playground/website/src/index.ts" | ||
], | ||
"@wp-playground/wordpress": [ | ||
"packages/playground/wordpress/src/index.ts" | ||
], | ||
"@wp-playground/wordpress-builds": [ | ||
"packages/playground/wordpress-builds/src/index.ts" | ||
], | ||
"isomorphic-git": ["./isomorphic-git/src"] | ||
} | ||
}, | ||
"exclude": ["node_modules", "tmp"] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, buttarget
seemed to work well in my testing the load event on the iframe.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the information is lost after the
await
. I was thinking it's related to React and how it clearscurrentTarget
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 thetarget
/currentTarget
– was the latter affected by bubbling?