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

Normalize frameborder=no|yes to frameborder=0|1 in iframe sanitizer #3064

Merged
merged 3 commits into from
Aug 22, 2019

Conversation

schlessera
Copy link
Collaborator

@schlessera schlessera commented Aug 21, 2019

Fixes #2335

The logic to normalize the frameborder attribute is based on the text from https://html.spec.whatwg.org/#frames-and-framesets:

A frameset or frame element has a border if the following algorithm returns true:

If the element has a frameborder attribute whose value is not the empty string and whose first character is either a U+0031 DIGIT ONE (1) character, a U+0079 LATIN SMALL LETTER Y character (y), or a U+0059 LATIN CAPITAL LETTER Y character (Y), then return true.

Open questions:

  • The actual normalization logic is a method called sanitize_boolean_digit() in the AMP_Base_Sanitizer parent class, to mirror the behavior of sanitize_dimension() which is in there, too. Should the base class regroup such logic that might be needed across sanitizer? Or would it make sense to extract them into either separate helper objects or traits?
  • The name of the method is sanitize_boolean_digit()... is there a name that is clearer here? Maybe an official HTML "attribute type" name I don't know about?

@googlebot googlebot added the cla: yes Signed the Google CLA label Aug 21, 2019
@westonruter westonruter modified the milestones: v1.2.1, v1.2.2 Aug 21, 2019
@westonruter
Copy link
Member

@schlessera Since sanitize_boolean_digit() is only being used in the AMP_Iframe_Sanitizer, perhaps best to just make it a private method on that class for now? Figuring out the best place for these methods and whether traits should be used will be part of the larger refactoring effort in #2315.

@westonruter westonruter merged commit 575ad43 into develop Aug 22, 2019
@westonruter westonruter deleted the fix/2335-normalize-iframe-frameborder branch August 22, 2019 18:40
@westonruter westonruter modified the milestones: v1.2.2, v1.3 Aug 27, 2019
@westonruter westonruter modified the milestones: v1.3, v1.2.2 Aug 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA Sanitizers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Normalize frameborder=no|yes to frameborder=0|1 in iframe sanitizer
3 participants