Automattic / wp-calypso

The JavaScript and API powered WordPress.com
https://developer.wordpress.com
GNU General Public License v2.0
12.43k stars 1.99k forks source link

Router: trim leading slashes to avoid interpreting path as external link #96536

Closed jsnajdr closed 3 days ago

jsnajdr commented 3 days ago

Fixes a bug where link in Calypso UI like:

<a href="https://wordpress.com//jetpack.com">Navigate</a>

is incorrectly interpreted by the Calypso router and causes navigation to https://jetpack.com.

The Calypso router has a clickHandler that captures all clicks on a href elements and tries to do an SPA navigation. The pathname of the offending URL is //jetpack.com, and because no matching route is registered in Calypso for this path, eventually the router does a full-page navigation by assigning to window.location.href:

window.location.href = '//jetpack.com';

But here the path is not interpreted as a relative pathname, but as a scheme-relative URL to jetpack.com.

This is a counterintutitive behavior of URLs because when parsing and composing an URL with the native URL class is not "symmetrical":

new URL( 'https://wordpress.com//jetpack.com' )

produces a parsed object with { host: 'wordpress.com', pathname: '//jetpack.com' }. But trying to use the pathname as a relative URL leads to something different:

new URL( '//jetpack.com', 'https://wordpress.com' /* base url */ )

produces a parsed object with { host: 'jetpack.com' }. Only the https scheme is used from the base URL.

I'm fixing this by removing extra leading slashes from the path when producing a router Context object. This way the offending path is never assigned to window.location and is also never passed to window.history.pushState.

How to test: Put a <a href> element somewhere in Calypso React UI and click on it. Unpatched Calypso will navigate to http://jetpack.com. Patched Calypso will navigate to local /jetpack.com.

github-actions[bot] commented 3 days ago
Calypso Live (direct link)
https://calypso.live?image=registry.a8c.com/calypso/app:build-125051
Jetpack Cloud live (direct link)
https://calypso.live?image=registry.a8c.com/calypso/app:build-125051&env=jetpack
Automattic for Agencies live (direct link)
https://calypso.live?image=registry.a8c.com/calypso/app:build-125051&env=a8c-for-agencies
matticbot commented 3 days ago

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~17 bytes added 📈 [gzipped])

``` name parsed_size gzip_size entry-subscriptions +31 B (+0.0%) +17 B (+0.0%) entry-stepper +31 B (+0.0%) +17 B (+0.0%) entry-main +31 B (+0.0%) +17 B (+0.0%) entry-login +31 B (+0.0%) +17 B (+0.0%) ``` Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Legend

What is parsed and gzip size? **Parsed Size:** Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. **Gzip Size:** Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

matticbot commented 3 days ago

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

To test WordPress.com changes, run install-plugin.sh $pluginSlug fix/router-double-leading-slash on your sandbox.