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

Add sanitization of JSON to prevent AMP validation errors #4320

Closed
westonruter opened this issue Feb 20, 2020 · 5 comments · Fixed by #4340
Closed

Add sanitization of JSON to prevent AMP validation errors #4320

westonruter opened this issue Feb 20, 2020 · 5 comments · Fixed by #4340
Labels
Bug Something isn't working P0 High priority QA passed Has passed QA and is done Validation
Milestone

Comments

@westonruter
Copy link
Member

westonruter commented Feb 20, 2020

Bug Description

I found out that the AMP validator validates JSON for parse errors, but we are not.

Given this markup:

<script type="application/ld+json">
	{BAD  🚫 🚫 🚫
</script>
<amp-analytics type="nielsen">
	<script type="application/json">
		{
			"vars": {
				"apid": "XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX",
				"apv": "1.0",
				"ap BAD  🚫 🚫 🚫
	</script>
</amp-analytics>

No validation errors are reported by the AMP plugin, and yet the AMP validator does report them:

image

Such invalid JSON should be sanitized while also reporting a validation error to the user to prevent GSC from alerting users to an error when it crawls next that we can proactively alert them of immediately.

Note that the AMP validator does not yet validate application/ld+json data, but it should after ampproject/amphtml#25610.

One way that this JSON parse error can occur is if JSON contains emoji. See #4318.

The code needing to be patched here is AMP_Tag_And_Attribute_Sanitizer::validate_cdata_for_node(). Note this code will also need to be modified to fix a separate bug with how blacklisted_cdata_regex is handled, so beware of merge conflicts; see #4319.

The logic can be adopted from the amphtml-validator here: https://github.com/ampproject/amphtml/blob/e1fd0e15b85e6f80f5d6382600b0765875d37b23/validator/engine/validator.js#L6226-L6237

Note the INVALID_JSON_CDATA error code. The error message should be: “The script tag contains invalid JSON that cannot be parsed.”

Related support topics:

Expected Behaviour

Steps to reproduce

  1. Go to '...'
  2. Click on '....'
  3. Scroll down to '....'
  4. See error

Screenshots

Additional context

  • WordPress version:
  • Plugin version:
  • Gutenberg plugin version (if applicable):
  • AMP plugin template mode:
  • PHP version:
  • OS:
  • Browser: [e.g. chrome, safari]
  • Device: [e.g. iPhone6]

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

@westonruter westonruter added Bug Something isn't working Validation labels Feb 20, 2020
@westonruter westonruter added this to the v1.5 milestone Feb 20, 2020
@westonruter westonruter added the P0 High priority label Feb 20, 2020
@kienstra kienstra self-assigned this Feb 28, 2020
@kienstra
Copy link
Contributor

kienstra commented Mar 3, 2020

This is probably a small size.

@csossi
Copy link

csossi commented Mar 10, 2020

Any QA instructions to add here, @kienstra ?

@kienstra
Copy link
Contributor

Hi @csossi,
Could you please use the testing instructions in #4340 (comment), but ignore step 1 (copying code into a plugin)?

In Step 4, there should be 1 expected validation error.

Thanks, Claudio!

@csossi
Copy link

csossi commented Mar 16, 2020

Seeing this now, @kienstra - this the description expected here?:
image

@kienstra
Copy link
Contributor

Hi @csossi,
Thanks, that looks as expected. Sorry, I should have updated the testing instructions for the new message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working P0 High priority QA passed Has passed QA and is done Validation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants