Skip to content
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

Scripts added by PWA plugin on error template are removed by sanitizer #7122

Closed
westonruter opened this issue May 26, 2022 · 9 comments · Fixed by #7141
Closed

Scripts added by PWA plugin on error template are removed by sanitizer #7122

westonruter opened this issue May 26, 2022 · 9 comments · Fixed by #7141
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. Integration: PWA
Milestone

Comments

@westonruter
Copy link
Member

westonruter commented May 26, 2022

Bug Description

The PWA plugin adds a couple scripts to the error template:

<script id="wp-navigation-request-properties" type="application/json">{{{WP_NAVIGATION_REQUEST_PROPERTIES}}}</script>	<script type="module">
		const shouldRetry = () => {
			if (
				new URLSearchParams(location.search.substring(1)).has(
					'wp_error_template'
				)
			) {
				return false;
			}

			const navigationRequestProperties = JSON.parse(
				document.getElementById('wp-navigation-request-properties').text
			);
			if ('GET' !== navigationRequestProperties.method) {
				return false;
			}

			return true;
		};

		if (shouldRetry()) {
			/**
			 * Listen to changes in the network state, reload when online.
			 * This handles the case when the device is completely offline.
			 */
			window.addEventListener('online', () => {
				window.location.reload();
			});

			// Create a counter to implement exponential backoff.
			let count = 0;

			/**
			 * Check if the server is responding and reload the page if it is.
			 * This handles the case when the device is online, but the server is offline or misbehaving.
			 */
			async function checkNetworkAndReload() {
				try {
					const response = await fetch(location.href, {
						method: 'HEAD',
					});
					// Verify we get a valid response from the server
					if (response.status >= 200 && response.status < 500) {
						window.location.reload();
						return;
					}
				} catch {
					// Unable to connect so do nothing.
				}
				window.setTimeout(
					checkNetworkAndReload,
					Math.pow(2, count++) * 2500
				);
			}

			checkNetworkAndReload();
		}
	</script>

These are getting stripped from the page when in Moderate/Strict standboxing, and in Loose they are getting retained with a data-amp-unvalidated-tag attribute added to them. In Loose mode, this is causing the CSS processing to be disabled, resulting in no style[amp-custom] to be added to the page. Since the PWA plugin relies on the runtime cache to store CSS files, this means that when the Offline page is served to the user, no stylesheets will be available in the cache.

The fix is simple: we need to add a PWA sanitizer which adds data-px-verified-tag to both of these scripts.

Expected Behaviour

Scripts on PWA error template should not trigger the Loose sandboxing level.

Screenshots

image

PHP Version

No response

Plugin Version

2.3-alpha

AMP plugin template mode

Standard

WordPress Version

6.0

Site Health

No response

Gutenberg Version

No response

OS(s) Affected

No response

Browser(s) Affected

No response

Device(s) Affected

No response

Acceptance Criteria

No response

Implementation Brief

No response

QA Testing Instructions

No response

Demo

No response

Changelog Entry

No response

@westonruter westonruter added Bug Something isn't working Integration: PWA labels May 26, 2022
@westonruter westonruter added this to the v2.3 milestone May 26, 2022
@maitreyie-chavan
Copy link
Collaborator

Having @thelovekesh pick this one up

@thelovekesh
Copy link
Collaborator

Thanks, @maitreyie-chavan! I’ve begun working on this and will aim to send over the PR by Monday.

@thelovekesh
Copy link
Collaborator

@westonruter I think sanitizer should work on both offline and 500 pages as we have added reload script to both.

It will result something like:

class AMP_PWA_Plugin_Sanitizer extends AMP_Base_Sanitizer {

	public function sanitize() {
		if (
			! ( function_exists( 'is_offline' ) && is_offline() ) &&
			! ( function_exists( 'is_500' ) && is_500() )
		) {
			return;
		}

		$scripts = $this->dom->getElementsByTagName( Tag::SCRIPT );

		foreach ( $scripts as $script ) {
			if (
				( 'wp-navigation-request-properties' === $script->getAttribute( Attribute::ID ) ) ||
				( 'module' === $script->getAttribute( Attribute::TYPE ) && false !== strpos( $script->textContent, 'checkNetworkAndReload()' ) )
			) {
				ValidationExemption::mark_node_as_px_verified( $script );
			}
		}
	}
}

@westonruter
Copy link
Member Author

@thelovekesh That looks like it will work. I think it could be optimized a bit further by using XPath to query for the two scripts as opposed to looping over all scripts on the page.

@thelovekesh
Copy link
Collaborator

That looks like it will work.

I have tested it and it's working fine. I will optimize it further by using XPath to query.

I have named the file class-amp-pwa-plugin-sanitizer.php. Does it look right to you?

@westonruter
Copy link
Member Author

I have named the file class-amp-pwa-plugin-sanitizer.php. Does it look right to you?

Let's call it rather class-amp-pwa-script-sanitizer.php since if PWA is merged into core then it wouldn't be a plugin anymore.

@thelovekesh
Copy link
Collaborator

@westonruter Should I introduce a filter to generate an XPath query?
e.g.

$scripts = array(
    'script-id-1',
    'script-id-2'
)

$scripts = apply_filters( 'amp-pwa-script-sanitizer', $scripts )

There can be use cases when users want to add more scripts in offline or 500 error pages. Thoughts?

@westonruter
Copy link
Member Author

@thelovekesh nah, I think that would be overkill.

@westonruter
Copy link
Member Author

QA Passed

I deployed to weston.ruter.net and when I was offline I saw:

image

The scripts have data-px-verified-tag as expected:

image

And because all PX-verified, stylesheet processing is retained:

image

Aside: Because a Mustache template tag is used in place of the JSON script for the offline template, the AMP optimizer transformer complains about invalid JSON not allowing it to optimize it:

image

In this case it's no problem, except for the error causing some unnecessary noise. That's more something to think about for the PWA plugin than AMP here.

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. Integration: PWA
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants