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

Escape instances of unescapeed output in AMP settings screen code #3703

Merged
merged 3 commits into from
Nov 10, 2019
Merged

Escape instances of unescapeed output in AMP settings screen code #3703

merged 3 commits into from
Nov 10, 2019

Conversation

pierlon
Copy link
Contributor

@pierlon pierlon commented Nov 8, 2019

Summary

  • Escape the translated strings in the warning message
  • Escape the settings errors are stored without sanitization

Fixes #3217.

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@googlebot googlebot added the cla: yes Signed the Google CLA label Nov 8, 2019
schlessera
schlessera requested changes Nov 9, 2019
View reviewed changes
includes/options/class-amp-options-manager.php Outdated
wp_kses(
sprintf(
/* translators: %s: Persistent object cache support URL */
__( 'The AMP plugin performs at its best when persistent object cache is enabled. <a href="%s">More details</a>', 'amp' ), // phpcs:ignore WordPress.Security.EscapeOutput
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we have now added escaping, we don't need the PHPCS exceptions anymore.

Suggested change
__( 'The AMP plugin performs at its best when persistent object cache is enabled. <a href="%s">More details</a>', 'amp' ), // phpcs:ignore WordPress.Security.EscapeOutput
__( 'The AMP plugin performs at its best when persistent object cache is enabled. <a href="%s">More details</a>', 'amp' ),

includes/options/class-amp-options-manager.php Outdated
wp_kses(
sprintf(
/* translators: %s: post-processor cache support URL */
__( 'The AMP plugin&lsquo;s post-processor cache was disabled due to the detection of highly-variable content. <a href="%s">More details</a>', 'amp' ), // phpcs:ignore WordPress.Security.EscapeOutput
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
__( 'The AMP plugin&lsquo;s post-processor cache was disabled due to the detection of highly-variable content. <a href="%s">More details</a>', 'amp' ), // phpcs:ignore WordPress.Security.EscapeOutput
__( 'The AMP plugin&lsquo;s post-processor cache was disabled due to the detection of highly-variable content. <a href="%s">More details</a>', 'amp' ),

includes/options/class-amp-options-manager.php Outdated
wp_kses(
sprintf(
/* translators: %s: path to the conflicting library */
__( 'A conflicting version of PHP-CSS-Parser appears to be installed by another plugin or theme (located in %s). Because of this, CSS processing will be limited, and tree shaking will not be available.', 'amp' ), // phpcs:ignore WordPress.Security.EscapeOutput
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
__( 'A conflicting version of PHP-CSS-Parser appears to be installed by another plugin or theme (located in %s). Because of this, CSS processing will be limited, and tree shaking will not be available.', 'amp' ), // phpcs:ignore WordPress.Security.EscapeOutput
__( 'A conflicting version of PHP-CSS-Parser appears to be installed by another plugin or theme (located in %s). Because of this, CSS processing will be limited, and tree shaking will not be available.', 'amp' ),

@pierlon @schlessera
Revert ignoring PHPCS exceptions
4c25cd4 
Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no Has not signed the Google CLA and removed cla: yes Signed the Google CLA labels Nov 9, 2019
@pierlon
Copy link
Contributor Author

pierlon commented Nov 9, 2019

Hey @schlessera could you also give consent when you have the time, thanks.

@schlessera
Copy link
Collaborator

@googlebot I consent.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes Signed the Google CLA and removed cla: no Has not signed the Google CLA labels Nov 9, 2019
@westonruter westonruter modified the milestones: v1.5, v1.4.1 Nov 9, 2019
@westonruter westonruter merged commit cf89b7b into develop Nov 10, 2019
@westonruter westonruter deleted the fix/3217-escape-amp-settings-screen-code branch November 10, 2019 01:23
westonruter pushed a commit that referenced this pull request Nov 10, 2019
@pierlon @schlessera @westonruter
Escape instances of unescapeed output in AMP settings screen code (#3703
78adcc6 
)

* Filter text and strip disallowed HTML

* Escape setting error text

* Revert ignoring PHPCS exceptions

Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>
westonruter added a commit that referenced this pull request Nov 11, 2019
@westonruter
Merge branch 'develop' of github.com:ampproject/amp-wp into add/block…
391b9a0 
…-editor-amp-preview

* 'develop' of github.com:ampproject/amp-wp: (30 commits)
  Fix identifying sources for validation errors coming child themes (#3708)
  Fix failing E2E tests (#3707)
  Remove amp_validate query var from Validated URL 'View' row action
  Re-factor get_html_attribute_pattern as match_element_attributes
  Quote variables added to regex pattern
  Escape instances of unescapeed output in AMP settings screen code (#3703)
  Replace incorrect usage of esc_url() with esc_url_raw()
  Remove empty alt attributes
  Add object-fit=contain to amp-youtube placeholder image
  Prevent copying empty title/alt in WP<5.2
  Extract HTML attribute parsing logic into base class, and use for YouTube embeds
  Update doc comment for `get_video_id_from_url`
  Merge separate regexes used to retrieve props into one
  Replace ambiguous `$fallback` with `$fallback_for_expected`
  Refactor `get_video_id_from_url` function
  Replace the fallback with an image placeholder for the iframe
  Remove todo
  HTML entity encoding will be handled by the DOM instead
  Bump `oembed` function deprecation version to 1.5.0
  Bump develop to v1.5.0-alpha
  ...
westonruter added a commit that referenced this pull request Nov 11, 2019
@westonruter
Merge branch 'develop' of github.com:ampproject/amp-wp into add/galle…
22ae686 
…ry-shortcode-captions

* 'develop' of github.com:ampproject/amp-wp: (61 commits)
  Improve specificity of JS doc
  Fix identifying sources for validation errors coming child themes (#3708)
  Fix failing E2E tests (#3707)
  Remove amp_validate query var from Validated URL 'View' row action
  Re-factor get_html_attribute_pattern as match_element_attributes
  Quote variables added to regex pattern
  Escape instances of unescapeed output in AMP settings screen code (#3703)
  Replace incorrect usage of esc_url() with esc_url_raw()
  Remove empty alt attributes
  Add object-fit=contain to amp-youtube placeholder image
  Prevent copying empty title/alt in WP<5.2
  Extract HTML attribute parsing logic into base class, and use for YouTube embeds
  Update doc comment for `get_video_id_from_url`
  Merge separate regexes used to retrieve props into one
  Replace ambiguous `$fallback` with `$fallback_for_expected`
  Refactor `get_video_id_from_url` function
  Replace the fallback with an image placeholder for the iframe
  Remove todo
  HTML entity encoding will be handled by the DOM instead
  Bump `oembed` function deprecation version to 1.5.0
  ...
westonruter added a commit that referenced this pull request Nov 14, 2019
@westonruter
Merge tag '1.4.1'
3133665 
* tag '1.4.1': (26 commits)
  Bump 1.4.1
  Update screenshots for 1.4.1
  Fix expected image name after upstream change (#3749)
  Use length property instead of count() method on DOMNodeList (#3727)
  Improve display of validation errors for scripts (#3722)
  Conditionally run E2E tests (#3723)
  Tidy up validation error details (#3721)
  Bump 1.4.1-RC1
  Default to the homepage instead of fetching the first AMP compatible post to customize (#3715)
  Add missing space after sentence (#3720)
  Include text content of style element in validation error (#3717)
  Use bitwise operator.
  Check if element is not in top toolbar.
  Fix user select for meta date and author
  Allow right click for meta blocks
  Fix summarizing error sources both parent theme and child theme (#3709)
  Fix identifying sources for validation errors coming child themes (#3708)
  Fix failing E2E tests (#3707)
  Remove amp_validate query var from Validated URL 'View' row action (#3706)
  Escape instances of unescapeed output in AMP settings screen code (#3703)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schlessera schlessera schlessera approved these changes

Assignees
No one assigned
Labels
cla: yes Signed the Google CLA
Projects
None yet
Milestone
v1.4.1
Development

Successfully merging this pull request may close these issues.

Missing escaping in AMP settings screen code
4 participants
@pierlon @googlebot @schlessera @westonruter