From e57f55be6962ab7a2eb52c8f15be881bf5731261 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Zieli=C5=84ski?= Date: Fri, 25 Oct 2024 19:03:54 +0200 Subject: [PATCH 1/5] [Query API] Use the exact redirect URL provided in the ?url= query param MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ```
1 2 3
``` cc @sirreal @bgrgicak --- .../remote/src/lib/boot-playground-remote.ts | 27 ++- .../website/playwright/e2e/query-api.spec.ts | 19 +- .../src/lib/state/redux/slice-clients.ts | 9 - packages/playground/wordpress/src/index.ts | 14 +- tsconfig.base.json | 189 ++++++++---------- 5 files changed, 129 insertions(+), 129 deletions(-) diff --git a/packages/playground/remote/src/lib/boot-playground-remote.ts b/packages/playground/remote/src/lib/boot-playground-remote.ts index 657c0b40f0..b23c6890ed 100644 --- a/packages/playground/remote/src/lib/boot-playground-remote.ts +++ b/packages/playground/remote/src/lib/boot-playground-remote.ts @@ -129,8 +129,33 @@ 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 + */ + const contentWindow = e.currentTarget!.contentWindow; + await new Promise((resolve) => setTimeout(resolve, 0)); const path = await playground.internalUrlToPath( - e.currentTarget!.contentWindow.location.href + contentWindow.location.href ); fn(path); } catch (e) { diff --git a/packages/playground/website/playwright/e2e/query-api.spec.ts b/packages/playground/website/playwright/e2e/query-api.spec.ts index 7f21ff93ae..67df8f74b4 100644 --- a/packages/playground/website/playwright/e2e/query-api.spec.ts +++ b/packages/playground/website/playwright/e2e/query-api.spec.ts @@ -92,11 +92,20 @@ 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'], + /** + * 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. + */ + [ + '/wp-admin/?control-characters=%0A%0D', + 'should retain encoded control characters in the URL', + ], +].forEach(([path, description]) => { + test(description, async ({ website, wordpress }) => { await website.goto(`./?url=${encodeURIComponent(path)}`); expect( await wordpress.locator('body').evaluate((body) => body.baseURI) diff --git a/packages/playground/website/src/lib/state/redux/slice-clients.ts b/packages/playground/website/src/lib/state/redux/slice-clients.ts index 942b6443e0..c295513d8f 100644 --- a/packages/playground/website/src/lib/state/redux/slice-clients.ts +++ b/packages/playground/website/src/lib/state/redux/slice-clients.ts @@ -40,15 +40,6 @@ const clientsSlice = createSlice({ name: 'clients', initialState, reducers: { - // addClientInfo: (state, action: PayloadAction) => { - // return clientsAdapter.addOne(state, action.payload); - // }, - // updateClientInfo: (state, action: PayloadAction) => { - // return clientsAdapter.updateOne(state, { - // id: action.payload.siteSlug, - // changes: action.payload, - // }); - // }, addClientInfo: clientsAdapter.addOne, updateClientInfo: ( state, diff --git a/packages/playground/wordpress/src/index.ts b/packages/playground/wordpress/src/index.ts index ac0dfb13b6..f2d5a66d9d 100644 --- a/packages/playground/wordpress/src/index.ts +++ b/packages/playground/wordpress/src/index.ts @@ -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); + + /** + * 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; } /** diff --git a/tsconfig.base.json b/tsconfig.base.json index 89722ad56c..658b6a1796 100644 --- a/tsconfig.base.json +++ b/tsconfig.base.json @@ -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"] } From ee3b1344cc8dadfea15164c1323da10be74699ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Zieli=C5=84ski?= Date: Fri, 25 Oct 2024 19:21:35 +0200 Subject: [PATCH 2/5] Explain why assign contentWindow to a new variable --- packages/playground/remote/src/lib/boot-playground-remote.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/playground/remote/src/lib/boot-playground-remote.ts b/packages/playground/remote/src/lib/boot-playground-remote.ts index b23c6890ed..593c5bb9fb 100644 --- a/packages/playground/remote/src/lib/boot-playground-remote.ts +++ b/packages/playground/remote/src/lib/boot-playground-remote.ts @@ -152,6 +152,8 @@ export async function bootPlaygroundRemote() { * * [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)); const path = await playground.internalUrlToPath( From 2f71e1b49e6a558191a8f4cd51087bbf76834e8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Zieli=C5=84ski?= Date: Fri, 25 Oct 2024 20:00:39 +0200 Subject: [PATCH 3/5] Adjust the Playwright test --- .../website/playwright/e2e/query-api.spec.ts | 37 ++++++++++++++----- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/packages/playground/website/playwright/e2e/query-api.spec.ts b/packages/playground/website/playwright/e2e/query-api.spec.ts index 67df8f74b4..18dfe61994 100644 --- a/packages/playground/website/playwright/e2e/query-api.spec.ts +++ b/packages/playground/website/playwright/e2e/query-api.spec.ts @@ -95,20 +95,37 @@ test('should not login the user in if the login query parameter is set to no', a [ ['/wp-admin/', 'should redirect to wp-admin'], ['/wp-admin/post.php?post=1&action=edit', 'should redirect to post editor'], - /** - * 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. - */ - [ - '/wp-admin/?control-characters=%0A%0D', - 'should retain encoded control characters in the URL', - ], ].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. + await website.goto( + `./?url=${encodeURIComponent(path)}&plugin=html-api-debugger` + ); + expect( + await wordpress + .locator('body') + .evaluate((body) => body.ownerDocument.location.href) + ).toContain(path); +}); From bce3918d63856ec455d0890c3a4190cd1c9c0ff2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Zieli=C5=84ski?= Date: Fri, 25 Oct 2024 20:51:14 +0200 Subject: [PATCH 4/5] Use baesURI in the test --- packages/playground/website/playwright/e2e/query-api.spec.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/playground/website/playwright/e2e/query-api.spec.ts b/packages/playground/website/playwright/e2e/query-api.spec.ts index 18dfe61994..20b677e606 100644 --- a/packages/playground/website/playwright/e2e/query-api.spec.ts +++ b/packages/playground/website/playwright/e2e/query-api.spec.ts @@ -124,8 +124,6 @@ test('should retain encoded control characters in the URL', async ({ `./?url=${encodeURIComponent(path)}&plugin=html-api-debugger` ); expect( - await wordpress - .locator('body') - .evaluate((body) => body.ownerDocument.location.href) + await wordpress.locator('body').evaluate((body) => body.baseURI) ).toContain(path); }); From 7f7a4595ad6afdceea953a10552df15c35214ff7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Zieli=C5=84ski?= Date: Fri, 25 Oct 2024 20:52:58 +0200 Subject: [PATCH 5/5] Revert back to ownerDocument.location.href --- packages/playground/website/playwright/e2e/query-api.spec.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/playground/website/playwright/e2e/query-api.spec.ts b/packages/playground/website/playwright/e2e/query-api.spec.ts index 20b677e606..18dfe61994 100644 --- a/packages/playground/website/playwright/e2e/query-api.spec.ts +++ b/packages/playground/website/playwright/e2e/query-api.spec.ts @@ -124,6 +124,8 @@ test('should retain encoded control characters in the URL', async ({ `./?url=${encodeURIComponent(path)}&plugin=html-api-debugger` ); expect( - await wordpress.locator('body').evaluate((body) => body.baseURI) + await wordpress + .locator('body') + .evaluate((body) => body.ownerDocument.location.href) ).toContain(path); });