Skip to content

Add patch from PHP-CSS-Parser to prevent malformed CSS properties leaking AMP validation error past the sanitizer #4290

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

Merged
merged 5 commits into from
Feb 16, 2020

Conversation

pierlon
Copy link
Contributor

@pierlon pierlon commented Feb 14, 2020

Summary

I've taken the current changes from MyIntervals/PHP-CSS-Parser#185 and added them as a patch file that will be applied via Composer. The external PR spans multiple commits and it is not guaranteed that the PR will stay the same, so the file patches/php-css-parser-pull-185.patch is created as a snapshot of the current state of the PR.

Fixes #4113.

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 Feb 14, 2020
@pierlon pierlon requested a review from schlessera February 14, 2020 06:54
@pierlon pierlon changed the title Add patch from external PR that resolves the issue Add patch from external PR to fix #4113. Feb 14, 2020
@pierlon pierlon changed the title Add patch from external PR to fix #4113. Add patch from external PR to fix #4113 Feb 14, 2020
@schlessera
Copy link
Collaborator

The code looks good to me. The only change that made me worthwhile would be to merge all patches into one. However, that might make it more difficult to adapt it as the PR changes, so it's probably not a real win.

@pierlon
Copy link
Contributor Author

pierlon commented Feb 14, 2020

#4197 also requires a bump of the cache group version, so once this gets merged I'll rebase accordingly and open a PR for it.

@pierlon pierlon requested a review from westonruter February 14, 2020 20:21
@westonruter westonruter added this to the v1.5 milestone Feb 16, 2020
@westonruter westonruter changed the title Add patch from external PR to fix #4113 Add patch from PHP-CSS-Parser to prevent malformed CSS properties leaking AMP validation error past the sanitizer Feb 16, 2020
@westonruter
Copy link
Member

This PR is failing the build process, namely: grunt shell:composer_install.

Running "shell:composer_install" (shell) task
Loading composer repositories with package information
Installing dependencies from lock file
Warning: The lock file is not up to date with the latest changes in composer.json. You may be getting outdated dependencies. It is recommended that you run `composer update` or `composer update <package name>`.
Package operations: 1 install, 0 updates, 0 removals
  - Installing cweagans/composer-patches (1.6.7): Loading from cache
Generating optimized autoload files
cweagans/composer-patches is not required in your composer.json and has not been removed
Gathering patches for root package.
Removing package sabberworm/php-css-parser so that it can be re-installed and re-patched.
  - Removing sabberworm/php-css-parser (8.3.0)
Loading composer repositories with package information
Updating dependencies
Package operations: 1 install, 0 updates, 1 removal
  - Removing cweagans/composer-patches (1.6.7)
Gathering patches for root package.
Gathering patches for dependencies. This might take a minute.
  - Installing sabberworm/php-css-parser (8.3.0): Loading from cache
  - Applying patches for sabberworm/php-css-parser
    https://github.com/sabberworm/PHP-CSS-Parser/commit/fa139f65c5b098ae652c970b25e6eb03fc495eb4.diff (Fix parsing CSS selectors which contain commas <https://github.com/sabberworm/PHP-CSS-Parser/pull/138>)
PHP Fatal error:  Uncaught Error: Class 'cweagans\Composer\PatchEvent' not found in phar:///usr/local/bin/composer/src/Composer/Plugin/PluginManager.php(196) : eval()'d code:312
Stack trace:
#0 [internal function]: cweagans\Composer\Patches_composer_tmp0->postInstall(Object(Composer\Installer\PackageEvent))
#1 phar:///usr/local/bin/composer/src/Composer/EventDispatcher/EventDispatcher.php(176): call_user_func(Array, Object(Composer\Installer\PackageEvent))
#2 phar:///usr/local/bin/composer/src/Composer/EventDispatcher/EventDispatcher.php(116): Composer\EventDispatcher\EventDispatcher->doDispatch(Object(Composer\Installer\PackageEvent))
#3 phar:///usr/local/bin/composer/src/Composer/Installer.php(622): Composer\EventDispatcher\EventDispatcher->dispatchPackageEvent('post-package-in...', false, Object(Composer\DependencyResolver\DefaultPolicy), Object(Composer\DependencyResolver\Pool), Object(Composer\Repository\CompositeRepository), Object(Composer\DependencyResolver\Request), Array, Object(Composer\DependencyResolver\Operation\ in phar:///usr/local/bin/composer/src/Composer/Plugin/PluginManager.php(196) : eval()'d code on line 312

Fatal error: Uncaught Error: Class 'cweagans\Composer\PatchEvent' not found in phar:///usr/local/bin/composer/src/Composer/Plugin/PluginManager.php(196) : eval()'d code:312
Stack trace:
#0 [internal function]: cweagans\Composer\Patches_composer_tmp0->postInstall(Object(Composer\Installer\PackageEvent))
#1 phar:///usr/local/bin/composer/src/Composer/EventDispatcher/EventDispatcher.php(176): call_user_func(Array, Object(Composer\Installer\PackageEvent))
#2 phar:///usr/local/bin/composer/src/Composer/EventDispatcher/EventDispatcher.php(116): Composer\EventDispatcher\EventDispatcher->doDispatch(Object(Composer\Installer\PackageEvent))
#3 phar:///usr/local/bin/composer/src/Composer/Installer.php(622): Composer\EventDispatcher\EventDispatcher->dispatchPackageEvent('post-package-in...', false, Object(Composer\DependencyResolver\DefaultPolicy), Object(Composer\DependencyResolver\Pool), Object(Composer\Repository\CompositeRepository), Object(Composer\DependencyResolver\Request), Array, Object(Composer\DependencyResolver\Operation\ in phar:///usr/local/bin/composer/src/Composer/Plugin/PluginManager.php(196) : eval()'d code on line 312

@schlessera thoughts?

@pierlon
Copy link
Contributor Author

pierlon commented Feb 16, 2020

@westonruter the referenced patch wasn't found in the build folder, which is why it failed (the error stack trace made that a bit obscure). Copying the patches folder resolves the error.

@westonruter
Copy link
Member

Copying the patches folder resolves the error.

Good catch. 👍

@westonruter westonruter merged commit 3b764c0 into develop Feb 16, 2020
@westonruter westonruter deleted the fix/4113-malformed-css-props branch February 16, 2020 03:08
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 CSS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Malformed CSS properties leak AMP validation error past the sanitizer
4 participants