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

Correctly handle siblings_disallowed from spec #7597

Merged
merged 15 commits into from
Aug 3, 2023
Merged

Correctly handle siblings_disallowed from spec #7597

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
3156ae7
Correctly handle `siblings_disallowed` from spec
swissspidy Aug 3, 2023
b5d6465
Remove debug cruft
swissspidy Aug 3, 2023
f7438c2
Lint fixes
swissspidy Aug 3, 2023
5f14a8a
Use yoda conditions
swissspidy Aug 3, 2023
c191f11
Undo some of the changes
swissspidy Aug 3, 2023
47d384e
Merge branch 'develop' into fix/siblings-disallowed
swissspidy Aug 3, 2023
710c0c4
Undo more changes
swissspidy Aug 3, 2023
b3be692
Use `previousSibling`/`nextSibling`
swissspidy Aug 3, 2023
5dd2342
Lint fixes
swissspidy Aug 3, 2023
891477c
Lint fix
swissspidy Aug 3, 2023
0df4b5d
Apply suggestions from code review
swissspidy Aug 3, 2023
21f8f6f
Fix condition
swissspidy Aug 3, 2023
7b6ce5f
Capture first
swissspidy Aug 3, 2023
588e7ff
Fix condition
swissspidy Aug 3, 2023
7fca058
Remove unnecessary check
swissspidy Aug 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions bin/amphtml-update.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -799,6 +799,9 @@ def GetTagRules(tag_spec):
if tag_spec.HasField('unique_warning'):
tag_rules['unique_warning'] = tag_spec.unique_warning

if tag_spec.HasField('siblings_disallowed'):
tag_rules['siblings_disallowed'] = tag_spec.siblings_disallowed

if tag_spec.HasField('child_tags'):
child_tags = collections.defaultdict( lambda: [] )
for field in tag_spec.child_tags.ListFields():
Expand Down
1 change: 1 addition & 0 deletions bin/latest-extension-versions.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@
"amp-sticky-ad" : "1.0",
"amp-story" : "1.0",
"amp-story-360" : "0.1",
"amp-story-audio-sticker" : "0.1",
"amp-story-auto-ads" : "0.1",
"amp-story-auto-analytics" : "0.1",
"amp-story-captions" : "0.1",
Expand Down
103 changes: 101 additions & 2 deletions includes/sanitizers/class-amp-allowed-tags-generated.php
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions includes/sanitizers/class-amp-rule-spec.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ abstract class AMP_Rule_Spec {
const MANDATORY_PARENT = 'mandatory_parent';
const DESCENDANT_TAG_LIST = 'descendant_tag_list';
const CHILD_TAGS = 'child_tags';
const SIBLINGS_DISALLOWED = 'siblings_disallowed';

/*
* HTML Element Attribute rule names
Expand Down
46 changes: 46 additions & 0 deletions includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ class AMP_Tag_And_Attribute_Sanitizer extends AMP_Base_Sanitizer {
const DISALLOWED_CHILD_TAG = 'DISALLOWED_CHILD_TAG';
const DISALLOWED_DESCENDANT_TAG = 'DISALLOWED_DESCENDANT_TAG';
const DISALLOWED_FIRST_CHILD_TAG = 'DISALLOWED_FIRST_CHILD_TAG';
const DISALLOWED_SIBLING_TAG = 'DISALLOWED_SIBLING_TAG';
const DISALLOWED_PROCESSING_INSTRUCTION = 'DISALLOWED_PROCESSING_INSTRUCTION';
const DISALLOWED_PROPERTY_IN_ATTR_VALUE = 'DISALLOWED_PROPERTY_IN_ATTR_VALUE';
const DISALLOWED_RELATIVE_URL = 'DISALLOWED_RELATIVE_URL';
Expand Down Expand Up @@ -749,6 +750,11 @@ static function ( $validation_error ) {
}
}

// If the node disallows any siblings, remove them.
if ( ! empty( $tag_spec[ AMP_Rule_Spec::SIBLINGS_DISALLOWED ] ) ) {
$this->remove_disallowed_siblings( $node, $this->get_spec_name( $node, $tag_spec ) );
}

// After attributes have been sanitized (and potentially removed), if mandatory attribute(s) are missing, remove the element.
$missing_mandatory_attributes = $this->get_missing_mandatory_attributes( $attr_spec_list, $node );
if ( ! empty( $missing_mandatory_attributes ) ) {
Expand Down Expand Up @@ -2453,6 +2459,46 @@ private function remove_disallowed_descendants( DOMElement $node, $allowed_desce
}
}

/**
* Loop through node's siblings and remove them.
*
* @param DOMElement $node Node.
* @param string $spec_name Spec name.
*/
private function remove_disallowed_siblings( DOMElement $node, $spec_name ) {
swissspidy marked this conversation as resolved.
Show resolved Hide resolved
$prev_sibling = $node->previousSibling;
while ( null !== $prev_sibling ) {
$prev_prev_sibling = $prev_sibling->previousSibling;
if ( $prev_sibling instanceof Element ) {
$this->remove_invalid_child(
$prev_sibling,
[
'code' => self::DISALLOWED_SIBLING_TAG,
'sibling' => $node->nodeName,
'spec_name' => $spec_name,
]
);
}
$prev_sibling = $prev_prev_sibling;
}

$next_sibling = $node->nextSibling;
while ( null !== $next_sibling ) {
$next_next_sibling = $next_sibling->nextSibling;
if ( $next_sibling instanceof Element ) {
$this->remove_invalid_child(
$next_sibling,
[
'code' => self::DISALLOWED_SIBLING_TAG,
'sibling' => $node->nodeName,
'spec_name' => $spec_name,
]
);
}
$next_sibling = $next_next_sibling;
}
}

/**
* Check whether the node validates the constraints for children.
*
Expand Down
42 changes: 42 additions & 0 deletions tests/php/test-tag-and-attribute-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1397,6 +1397,48 @@ static function() {
[ 'amp-sidebar', 'amp-story' ],
],

'amp_story_with_disallowed_sibling' => [
str_replace(
[ "\n", "\t" ],
'',
'
<b id="disallowed-before1"></b>
<i id="disallowed-before2"></i>
<amp-story standalone="standalone" title="Stories in AMP - Hello World" publisher="AMP Project" publisher-logo-src="https://ampbyexample.com/favicons/coast-228x228.png" poster-portrait-src="https://ampbyexample.com/img/story_dog2_portrait.jpg">
<amp-story-page id="cover">
<amp-story-grid-layer template="fill">
<h1>Hello World</h1>
<p>This is the cover page of this story.</p>
</amp-story-grid-layer>
</amp-story-page>
</amp-story>
<span id="disallowed-after1"></span>
<div id="disallowed-after2"></div>
'
),
str_replace(
[ "\n", "\t" ],
'',
'
<amp-story standalone="standalone" title="Stories in AMP - Hello World" publisher="AMP Project" publisher-logo-src="https://ampbyexample.com/favicons/coast-228x228.png" poster-portrait-src="https://ampbyexample.com/img/story_dog2_portrait.jpg">
<amp-story-page id="cover">
<amp-story-grid-layer template="fill">
<h1>Hello World</h1>
<p>This is the cover page of this story.</p>
</amp-story-grid-layer>
</amp-story-page>
</amp-story>
'
),
[ 'amp-story' ],
[
AMP_Tag_And_Attribute_Sanitizer::DISALLOWED_SIBLING_TAG,
AMP_Tag_And_Attribute_Sanitizer::DISALLOWED_SIBLING_TAG,
AMP_Tag_And_Attribute_Sanitizer::DISALLOWED_SIBLING_TAG,
AMP_Tag_And_Attribute_Sanitizer::DISALLOWED_SIBLING_TAG,
],
],

'amp_sidebar_with_autoscroll' => [
str_replace(
[ "\n", "\t" ],
Expand Down