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

Add support for custom non-AMP scripts #6528

Merged
merged 29 commits into from
Aug 19, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
65410c6
WIP: Start to allow script sanitizer to actually sanitize scripts
westonruter Aug 11, 2021
f42d11c
Prevent unwrapping `noscript` elements when there were scripts kept.
westonruter Aug 17, 2021
6d6128a
Use sanitize_scripts as sanitizer arg; remove obsolete todos
westonruter Aug 17, 2021
860ab05
Add sanitization of event handler attributes
westonruter Aug 17, 2021
af98e66
Prevent including AMP event handlers as custom event handlers
westonruter Aug 17, 2021
b574010
Remove sanitizing JSON scripts in script sanitizer
westonruter Aug 17, 2021
cec9be0
Catch MaxCssByteCountExceeded exception when attempting to set style …
westonruter Aug 17, 2021
cc0bdb1
Update amp-toolbox-php to https://github.com/ampproject/amp-toolbox-p…
westonruter Aug 17, 2021
aa872f8
Add style sanitizer arg to skip_tree_shaking
westonruter Aug 17, 2021
bdc673a
Prevent enforcing CSS max byte count on dev mode documents
westonruter Aug 17, 2021
1578aba
Run AMP_Script_Sanitizer first
westonruter Aug 17, 2021
e50e6a0
Prevent false positive event handler attribute detection with `amp-po…
westonruter Aug 17, 2021
5c7eb99
Make amp-position-observer and amp-font attribute exceptions more spe…
westonruter Aug 17, 2021
b88d876
Turn off tree shaking in style sanitizer when script sanitizer keeps …
westonruter Aug 17, 2021
880eb18
Defer gathering selector conversion mappings until sanitizing
westonruter Aug 17, 2021
c16126c
Add titles for script sanitizer error codes
westonruter Aug 17, 2021
842b8b2
Fix commit ref for ampproject/amp-toolbox
westonruter Aug 17, 2021
ab30f62
Run composer update
westonruter Aug 17, 2021
2ced838
Include script basename in external script error title
westonruter Aug 17, 2021
d918ba2
Add todos
westonruter Aug 17, 2021
5fb6041
Use native img when custom scripts are kept
westonruter Aug 17, 2021
170309a
Allow native post forms when custom scripts are kept
westonruter Aug 17, 2021
dae21b9
Fix omission of amp-form script given native_post_forms_allowed arg c…
westonruter Aug 17, 2021
f0f613a
Improve PHP comments
westonruter Aug 18, 2021
681233e
Improve test coverage
westonruter Aug 18, 2021
dd49403
Rename sanitize_script_elements() back to sanitize_js_script_elements()
westonruter Aug 18, 2021
3070b83
Rename sanitize_scripts arg to sanitize_js_scripts
westonruter Aug 18, 2021
03fb8b7
Avoid false positive sanitization of non-JS scripts
westonruter Aug 19, 2021
f35c4e9
Use FQCN in docblock
pierlon Aug 19, 2021
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
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
"ext-json": "*",
"ext-libxml": "*",
"ext-spl": "*",
"ampproject/amp-toolbox": "dev-main#a155c37",
"ampproject/amp-toolbox": "dev-add/enforced-css-max-byte-count-transformed-identifier-config#718d9dad",
"cweagans/composer-patches": "~1.0",
"fasterimage/fasterimage": "1.5.0",
"sabberworm/php-css-parser": "dev-master#bfdd976"
Expand Down
51 changes: 25 additions & 26 deletions composer.lock

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

10 changes: 8 additions & 2 deletions includes/amp-helper-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -1478,6 +1478,11 @@ function amp_get_content_sanitizers( $post = null ) {
$native_post_forms_allowed = amp_is_native_post_form_allowed();

$sanitizers = [
// The AMP_Script_Sanitizer runs first because based on whether it allows custom scripts
// to be kept, it may impact the behavior of other sanitizers. For example, if custom
// scripts are kept then this is a signal that tree shaking in AMP_Style_Sanitizer cannot be
// performed.
AMP_Script_Sanitizer::class => [],
AMP_Embed_Sanitizer::class => [
'amp_to_amp_linking_enabled' => $amp_to_amp_linking_enabled,
],
Expand Down Expand Up @@ -1515,8 +1520,9 @@ function amp_get_content_sanitizers( $post = null ) {
'native_img_used' => $native_img_used,
],
AMP_Block_Sanitizer::class => [], // Note: Block sanitizer must come after embed / media sanitizers since its logic is using the already sanitized content.
AMP_Script_Sanitizer::class => [],
AMP_Style_Sanitizer::class => [],
AMP_Style_Sanitizer::class => [
'skip_tree_shaking' => is_customize_preview(),
],
AMP_Meta_Sanitizer::class => [],
AMP_Layout_Sanitizer::class => [],
AMP_Accessibility_Sanitizer::class => [],
Expand Down
16 changes: 14 additions & 2 deletions includes/class-amp-theme-support.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
use AmpProject\Dom\Document;
use AmpProject\Extension;
use AmpProject\Optimizer;
use AmpProject\Optimizer\Configuration\TransformedIdentifierConfiguration;
use AmpProject\Optimizer\Transformer\TransformedIdentifier;
use AmpProject\RequestDestination;
use AmpProject\Tag;
use AmpProject\AmpWP\Optimizer\Transformer\AmpSchemaOrgMetadata;
Expand Down Expand Up @@ -1531,7 +1533,11 @@ public static function ensure_required_markup( Document $dom, $script_handles =
}

// When opting-in to POST forms, omit the amp-form component entirely since it blocks submission.
if ( amp_is_native_post_form_allowed() && $dom->xpath->query( '//form[ @action and @method and translate( @method, "POST", "post" ) = "post" ]' )->length > 0 ) {
if (
Copy link
Contributor

Choose a reason for hiding this comment

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

No more checking if amp_is_native_post_form_allowed()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, because now that the AMP_Script_Sanitizer can modify the args of AMP_Form_Sanitizer to enable the native_post_forms_allowed arg, the return value of amp_is_native_post_form_allowed() may not be accurate at this point. It's return value is used as the initial value for native_post_forms_allowed, but since it can be changed by a sanitizer then it's only relevant for setting the initial value in amp_get_content_sanitizers().

In fact, there's not really a need for a global amp_is_native_post_form_allowed() function anymore.

The only reason why the function was here was to offer a slight performance improvement to skip doing the XPath query. But now we are checking to see if the amp-form extension was identified instead.

in_array( Extension::FORM, $script_handles, true )
&&
$dom->xpath->query( '//form[ @action and @method and translate( @method, "POST", "post" ) = "post" ]' )->length > 0
) {
$superfluous_script_handles[] = Extension::FORM;
}

Expand Down Expand Up @@ -2075,6 +2081,9 @@ public static function prepare_response( $response, $args = [] ) {
do_action( 'amp_server_timing_start', 'amp_optimizer' );

$errors = new Optimizer\ErrorCollection();

// @todo The dev mode attribute is being overloaded here. We should use something else.
$args['skip_css_max_byte_count_enforcement'] = $dom->documentElement->hasAttribute( DevMode::DEV_MODE_ATTRIBUTE );
self::get_optimizer( $args )->optimizeDom( $dom, $errors );

if ( count( $errors ) > 0 ) {
Expand Down Expand Up @@ -2164,10 +2173,13 @@ static function () use ( $args ) {
// Supply the Schema.org metadata, previously obtained just before output buffering began, to the AmpSchemaOrgMetadataConfiguration.
add_filter(
'amp_optimizer_config',
function ( $config ) {
function ( $config ) use ( $args ) {
if ( is_array( self::$metadata ) ) {
$config[ AmpSchemaOrgMetadata::class ][ AmpSchemaOrgMetadataConfiguration::METADATA ] = self::$metadata;
}
if ( ! empty( $args['skip_css_max_byte_count_enforcement'] ) ) {
$config[ TransformedIdentifier::class ][ TransformedIdentifierConfiguration::ENFORCED_CSS_MAX_BYTE_COUNT ] = false;
}
return $config;
},
defined( 'PHP_INT_MIN' ) ? PHP_INT_MIN : ~PHP_INT_MAX // phpcs:ignore PHPCompatibility.Constants.NewConstants.php_int_minFound
Expand Down
11 changes: 11 additions & 0 deletions includes/sanitizers/class-amp-base-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,17 @@ public function get_selector_conversion_mapping() {
return [];
}

/**
* Update args.
*
* Merges the supplied args with the existing args.
*
* @param array $args Args.
*/
public function update_args( $args ) {
$this->args = array_merge( $this->args, $args );
}

/**
* Run logic before any sanitizers are run.
*
Expand Down
Loading