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.

Sign up for GitHub

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

Jump to bottom

Leaked validation errors for implicit layout "container" without width/height #4465

Closed
westonruter opened this issue Mar 27, 2020 · 15 comments · Fixed by #4541
Closed

Leaked validation errors for implicit layout "container" without width/height #4465

westonruter opened this issue Mar 27, 2020 · 15 comments · Fixed by #4541
Assignees
@schlessera
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. Validation WS:Core Work stream for Plugin core
Milestone
v2.0

Comments

@westonruter
Copy link
Member

westonruter commented Mar 27, 2020 •
edited by schlessera
Loading

Bug Description

Given this markup in a Custom HTML block:

<amp-ad type="adsense" data-ad-client="ca-pub-123" data-ad-slot="456" data-ad-format="auto" data-full-width-responsive="true"><span overflow=""></span></amp-ad>

This is leaking a validation error to the frontend:

Incomplete layout attributes specified for tag 'amp-ad'. For example, provide attributes 'width' and 'height'.

I thought that this would have been prevented by #3728.

This was discovered in a support topic.

Expected Behaviour

AMP components that have neither a layout nor width/height attributes should be removed entirely. An appropriate validation error should be raised in the plugin.

The corresponding AMP validation error constant is MISSING_LAYOUT_ATTRIBUTES.

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

Acceptance criteria

Implementation brief

QA testing instructions

  1. Create a new post.
  2. Add the following markup into a "Custom HTML" block on the page: 
    <amp-img src="https://placeholder.com/350"></amp-img>
  3. Publish the page and visit its AMP version.
  4. Verify: The page should not show any validation errors (except for the "dev-mode" one) for the page.
  5. Go to the "Validate" screen for the page.
  6. Verify: The page should have 1 validation error with the following text:

    Incomplete layout attributes specified for tag amp-img. For example, provide attributes width and height.

Demo

Before

Image 2020-04-07 at 5 44 53 AM
The plugin's validation doesn't show any errors for the post.

Image 2020-04-07 at 5 45 30 AM
However, the AMP validator complains about the post.

After

Image 2020-04-07 at 5 45 52 AM
The plugin's validation catches the two offending elements and removes them.

Image 2020-04-07 at 5 46 04 AM
The validation issues are not leaked into the AMP validator anymore.

Changelog entry
@westonruter westonruter added Bug Something isn't working Validation labels Mar 27, 2020
@schlessera schlessera self-assigned this Apr 6, 2020
@schlessera
Copy link
Collaborator

It looks like this is not part of the protoascii specs, but hard-coded logic within the validator(s): https://github.com/ampproject/amphtml/blob/6eb58814bebd917d3665ccf81f9db4cf641415c5/validator/engine/validator.js#L4124

@schlessera schlessera mentioned this issue Apr 6, 2020
Closed
@schlessera schlessera changed the title Leaked validation errors for amp-ad without width/height Leaked validation errors for implicit layout "container" without width/height Apr 6, 2020
@schlessera
Copy link
Collaborator

This has nothing to do with amp-ad, as you get the exact same error for an <amp-img> that is missing both the layout and the width/height attributes.

The reason for the validation error is that a missing layout falls back to the implicit layout "container", and that implicit layout requires width and height attributes to be set.

@westonruter westonruter added this to the v1.6 milestone Apr 6, 2020
@westonruter
Copy link
Member Author

The reason for the validation error is that a missing layout falls back to the implicit layout "container", and that implicit layout requires width and height attributes to be set.

That doesn't seem to be quite right, as container layout actually does not require width/height per the docs:

image

@schlessera
Copy link
Collaborator

Yes, I misphrased this. From the validator:

// Special case. If no layout related attributes were provided, this implies
// the CONTAINER layout. However, telling the user that the implied layout
// is unsupported for this tag is confusing if all they need is to provide
// width and height in, for example, the common case of creating
// an AMP-IMG without specifying dimensions. In this case, we emit a
// less correct, but simpler error message that could be more useful to
// the average user.

@westonruter
Copy link
Member Author

It seems we need to implement that entire code block in \AMP_Tag_And_Attribute_Sanitizer::is_valid_layout(): https://github.com/ampproject/amphtml/blob/6eb58814bebd917d3665ccf81f9db4cf641415c5/validator/engine/validator.js#L4107-L4134

@schlessera
Copy link
Collaborator

Fixing this bug actually creates lots of failures in our test suite. I have 17 test failures right now. I haven't verified them all, yet, but it seems they are legit.

Here's an example:

3) AMP_Tag_And_Attribute_Sanitizer_Test::test_sanitize with data set "attribute_requirements_overriden_by_placeholders_within_template" ('<html amp><head><meta charset.../html>', null, array('amp-mustache', 'amp-timeago'))
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
     9 => '<body>'
     11 => '<!-- before -->'
     13 => '<template type="amp-mustache">'
-    15 => '<amp-timeago datetime="{{iso}}">'
-    17 => '</amp-timeago>'
-    19 => '</template>'
-    21 => '<!-- after -->'
-    23 => '</body>'
-    25 => '</html>'
+    15 => '</template>'
+    17 => '<!-- after -->'
+    19 => '</body>'
+    21 => '</html>'
 )

This is what the official validator shows for this test case:
Image 2020-04-07 at 3 50 54 AM

@schlessera
Copy link
Collaborator

I could reproduce 15 out of 17 test failures in the official validator and fix by adding width="100" height="100".

The two remaining test failures are related to <amp-list>, which doesn't seem to fall into this case according to the official validator. I'll push the current code for now as a WIP PR while I'm investigating the 2 remaining failures.

@schlessera
Copy link
Collaborator

I think the two remaining test failures are because we are also missing this part of the logic: https://github.com/ampproject/amphtml/blob/6eb58814bebd917d3665ccf81f9db4cf641415c5/validator/engine/validator.js#L4045-L4054

So, validating the layout should be skipped as soon as template syntax is discovered.

@schlessera
Copy link
Collaborator

Ah, I forgot we already skip sanitization completely through this code: https://github.com/ampproject/amp-wp/pull/3243/files#diff-ee41b32e64c90445a71328ca93cfa799R1005-R1009

I'll have to find another culprit then...

@schlessera schlessera mentioned this issue Apr 7, 2020
Merged
3 tasks
@schlessera
Copy link
Collaborator

Here's what I've got for the two remaining test failures.

  • They seem to have the exact same cause: <amp-list> being detected as falling back to implicit container layout and missing width and height.
  • According to our current spec version active in the plugin, <amp-list> does not support the container layout.
  • Therefore, we compute the MISSING_LAYOUT_ATTRIBUTES validation error and strip it.
  • However, in the AMP Validator, the same AMP markup passes without validation errors.
  • In the documentation for the amp-list component, it states that <amp-list> does support the container layout.
  • There was a very recent effort to add support for the container layout to the amp-list component, but it was reverted recently afterwards because it broke existing sites.

My current conclusion is that our interpretation (throw a MISSING_LAYOUT_ATTRIBUTES validation error if width & height are missing) is currently the correct one, and the official AMP validator is stuck between the PR for adding support and the PR for reverting it again, making the container layout supported and thus eliminating one of the prerequisites for issuing the MISSING_LAYOUT_ATTRIBUTES validation error.

@westonruter, @sebastianbenz Is this plausible? Are you able to confirm this internally?

@sebastianbenz
Copy link

@schlessera sounds reasonable. @caroqliu to confirm.

@caroqliu
Copy link

Looks like amp-list with [layout="container"] is passing the validation at https://validator.ampproject.org/ This should have been reverted as cited earlier.

cc @ampproject/wg-caching

@westonruter
Copy link
Member Author

  • According to our current spec version active in the plugin, <amp-list> does not support the container layout.

Correct. But the spec in the plugin is a bit stale. I have a pending PR to update it to 2004041903580 which will add this layout: #4548 (comment)

So the validator spec should actually be deleting supported_layouts: CONTAINER from amp-list?

@caroqliu
Copy link

So the validator spec should actually be deleting supported_layouts: CONTAINER from amp-list?

Yes, in the line referenced above as well as here and here. This commit specifically covers the extent of the validator changes that should be undone.

@westonruter westonruter modified the milestones: v1.6, Sprint 27 May 2, 2020
@pierlon
Copy link
Contributor

pierlon commented May 19, 2020

QA passed:

With the following markup on the AMP page:

<amp-img src="https://placeholder.com/350"></amp-img>

  • The AMP Validator only complains of dev mode being enabled:
    image

  • The only validation error shown is for incomplete layout attributes not being specified:
    image

@kmyram kmyram modified the milestones: Sprint 27, v1.6 May 27, 2020
@kmyram kmyram closed this as completed Jun 2, 2020
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Jul 17, 2020
@kmyram kmyram added the WS:Core Work stream for Plugin core label Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@schlessera schlessera

Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. Validation WS:Core Work stream for Plugin core
Projects
None yet
Milestone
v2.0
Development

Successfully merging a pull request may close this issue.

Catch validation errors for implicit layout missing width and height ampproject/amp-wp
6 participants
@schlessera @westonruter @sebastianbenz @kmyram @caroqliu @pierlon