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

Sanitize scripts without type #898

Closed
eduardcotmrf opened this issue Jan 24, 2018 · 4 comments
Closed

Sanitize scripts without type #898

eduardcotmrf opened this issue Jan 24, 2018 · 4 comments

Comments

@eduardcotmrf
Copy link

Related to this issue: #883.

I've encoutered some plugins (for instance custom plugins that add script advertisement tags in the body ) that forget to put the type="text/javascript" in the tag script. In this cases the script it's not removed by amp-wp so the AMP page remains invalid.

Maybe it would be a good idea to sanitize all scripts without any type as well?

At the end we only want to preserve the ones that use type='aplication/json'.

@eduardcotmrf eduardcotmrf changed the title Remove scripts without type Sanitize scripts without type Jan 24, 2018
@westonruter
Copy link
Member

Plugins that add scripts to the_content? We may have fixed the issue with script tag sanitization in the 0.6.0 release and the late change #893.

Is this still an issue in 0.6.0? If so, could you please share a PHP snippet that can be used to reproduce the problem?

@eduardcotmrf
Copy link
Author

Yes, plugins that add scripts to the_content. It's not fixed in 0.6.

What it's fixed in 0.6 it's that scripts with type="text/javascript" are sanitized. Here the problems are with scripts with no type specified. As specified in https://dev.w3.org/html5/spec-preview/the-script-element.html#attr-script-type :

The type attribute gives the language of the script or format of the data. If the attribute is present, its value must be a valid MIME type. The charset parameter must not be specified. The default, which is used if the attribute is absent, is "text/javascript".

So we should sanitize when no type it's specified.

Example of php snippet:

add_filter( 'the_content', 'get_script' );

function get_script() {
    echo "<script>console.log('Hello World');</script>"
}

@westonruter
Copy link
Member

@eduardcotmrf I'm trying to reproduce the issue but I'm not able to.

I think the problem is that your the_content filter is printing when it should be returning the string.

In both the 0.6 branch and develop (0.7-beta1), when I add the following:

add_filter( 'the_content', function ( $content ) {
	return $content . "<script>document.write('no type!');</script><script type='text/javascript'>document.write('with type!');</script>";
} );

Both script tags get removed.

Note that in 0.7 for themes that have amp theme support, the entire response is output-buffered and sent through the sanitizer. This means that if you do some echo of something illegal, such as a script in the head via the wp_head action, then it too will get removed.

@eduardcotmrf
Copy link
Author

Ok, thanks @westonruter . The echo comes from a plugin that we don't have control on it and it's messing up things with our own. When 0.7 it's out, with the output-buffered sanitization, I'll try it again to be sure we don't have this problem anymore.

Thanks!

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

No branches or pull requests

3 participants