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

Better CSS Sanitization #610

Closed

Conversation

delputnam
Copy link
Contributor

@mjangda, This PR is to address issue #604.

Summary:

Replaced safecss_filter_attr() with sanitize_amp_custom_style() which uses AMP_Allowed_Styles_Generated to validate CSS. Also replaced use of explode() with a custom function css_explode() which ignores delimiters in parentheses and quotation marks. Included amp_wp_build_styles.py to generate AMP_Allowed_Styles_Generated class.

Detail:

The original issue was caused by the following:

  • The sanitizer used 'safecss_filter_attr()' from kses which would filter out any declaration with a ( in it. For example, this would incorrectly remove a declaration that uses background:url(...)

    safe_filter_attr() has been replaced with a new function: sanitize_amp_custom_style() which uses a new class called AMP_Allowed_Styles_Generated to validate the amp-custom CSS. This new class is generated from the same protocol buffers that AMP uses to produce the official validator.

    The only rules in this version of the specification that apply to inline styles are:

    • A list of regular expressions that will flag blacklisted data in the amp-custom styles section. Note that !important is included in this blacklist, but there is no rule that disallows variants with whitespace between the ! and important, thus tests that relate to strings such as ! important have been removed.

    • A set of rules that specify requirements of image URLs present in the styles.

    Notably, the current AMP spec does not blacklist overflow and it does not specify that width should be replaced by max-width, thus these rules are no longer enforced.

    Note: Even though the AMP website mentions that overflow may not be styled as auto or scroll, there is nothing in the AMP validator specification that enforces this and the current version of the JS validator does not flag this use of overflow as an AMP error. (Perhaps the AMP web page needs to be updated??) The use of overflow seems to be handled by the AMP JS now rather than explicitly disallowing it in CSS.

  • Additionally, the sanitizer was using explode( ';', $string ) to split apart CSS directives in a CSS directive block. This would incorrectly split something like:

    height:44px;background:url(data:image/png;base64,iVBORw0K...SuQmCC);
    

    into three tokens instead of two:

    height:44px
    background:url(data:image/png
    base64,iVBORw0K...SuQmCC)
    

    The changes in this PR include a new function css_explode() that works in a manner similar to php's explode(), but it ignores delimiters inside parentheses and quoted strings. This new function is used to break apart CSS directive blocks as well as split individual directives into their property/value pairs in process_style().

…_Allowed_Styles_Generated to validate CSS. Also replaced use of with a custom function which ignores delimiters in parentheses and quotation marks.
@mjangda mjangda changed the title Changes to address issue #604. Better CSS Sanitization Apr 24, 2017
@westonruter
Copy link
Member

Closing in favor of #1048.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants