WordPress.org

Make WordPress Core

Opened 6 years ago

Last modified 8 weeks ago

#32067 assigned feature request

Remove inline javascript from WP-Core to allow CSP protection

Reported by: tdelmas Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Security Keywords:
Focuses: javascript Cc:

Description

To avoid catastrophic effects of XSS, it would be safe to allow user to add a Content Security Policy (CSP) header.

To be effective, a CSP must at least disallow inline javascript.

Change History (20)

#1 @johnbillion
6 years ago

  • Type changed from enhancement to feature request

Thanks for the ticket, tdelmas.

The majority of inline JS in WordPress is there to deliver localised strings (via wp_localize_script()).

The only way we could eliminate these particular inline scripts is to generate external JS files on the fly which contain the contents of the scripts that would otherwise be output inline. We could do this, but it would need a completely new approach to localised strings. This has been mentioned before but I can't currently find the ticket in question.

#2 @dd32
6 years ago

Unfortunately this just isn't going to be possible for WordPress to add. WordPress historically has support for inline JS, both being emitted by core (Emoji in 4.2 is a good example) and user-added (inline Javascript in posts is allowed if you're an administrator).

While CSP is a great mechanism, and should definitely be used on sites that need it (I'd suggest a plugin), it doesn't make sense by default in WordPress IMHO.

#3 @tdelmas
6 years ago

Thank you @johnbillion and @dd32 for your answers.

To add some clarification :
I was not asking of a way to add a CSP (a plugin, or by default), It's easy enough to add one configuration line in your favorite web-server.
My point was really about inline javascript. In my opinion, a CSP blocking inline javacript is a really improvement. With this kind of mechanism, an XSS (like the last one) is not anymore a catastrophic problem.
To allow this kind of security, we need to get rid of inline javascript.

I know WordPress have a lot of technical debts, but removing inline javascript from WP-Core can be an (long term) objective ?

From an administrator point of view, if none of my post contain inline JS, and none of my plugin too, I would feel more secure if I could add a proper CSP. (Of course today I can add one, but I will possibly break a lot of things !)

For example, for wp_localize_script (example taken from https://pippinsplugins.com/use-wp_localize_script-it-is-awesome/ )

	wp_enqueue_script('pw-script', plugin_dir_url( __FILE__ ) . 'js/pw-script.js');
	wp_localize_script('pw-script', 'pw_script_vars', array(
			'alert' => __('Hey! You have clicked the button!', 'pippin')
		)
	);

Could generate something like :

<span data-translation="Hey! You have clicked the button!" data-context="pippin" data-translated="(The same text, localized)"></span>
<script src="js/wp-localize.js"></script>
<script src="js/pw-script.js"></script>

And wp-localize.js will read the dom and load all translations, before the execution of pw-script.js

It could be done without breaking the API compatibility.

To conclude : IMHO, adding a CSP must not be an obligation, but, allowing administrator to strengthened the security of their WP with a CSP will be a huge step from a security point of view.

#4 @JonathanKingston
5 years ago

Without the core of WordPress being CSP safe then plugins will have a very hard time fixing all the bad practices after the fact.
I suggest actually getting the scripts to load over XHR/fetch the localised scripts data as this would allow all the code to be static thus allowing the page to generate SRI hashes which adds further script safety.

WordPress developers should be going out of its way to advocate security such that it's plugin authors can follow from their example.

Adding the CSP could be an addon (which should really be enabled by default) but the actual task here is getting the default installs to not require inline JavaScript.

#5 @swissspidy
4 years ago

Related: #20491. That ticket currently aims to use .json files for JavaScript i18n.

#7 @jdgrimes
4 years ago

  • Focuses javascript added

Even setting aside CSP entirely, there are still many reasons to remove all inline scripts and styles. Automatic syntax checking, code sniffing, minification, RTLificating, etc., is much harder—if not impossible—when JS and CSS are inline. So I think removing all inline scripts and styles is a worthy goal, and the fact that it may lead to us one day being able to use a CSP with WordPress to mitigate persistent XSS/Cross-Site Styling is just the icing on the cake.

This ticket was mentioned in Slack in #meta by pento. View the logs.


4 years ago

#9 @Phil McKerracher
4 years ago

Can I request an increase in priority for this, as is appropriate for a security issue? Attacks are becoming more sophisticated and frequent, more people are using SSL now, and security ratings on places like https://observatory.mozilla.org and https://securityheaders.io are beginning to matter.

I don't mind disabling or replacing insecure plugins or features temporarily, or adding some named exceptions to a CSP header to allow for legacy code. But problems in WP core are more difficult to fix without them being overwritten by the next update. Hashes and nonces are sometimes a workaround but are difficult to implement and maintain.

On the other hand, removing inline scripts and styles from WP core seems like a fairly routine task (though not trivial because there are many) that would have additional benefits, as others have said.

#10 @tomdxw
4 years ago

Related: #39941.

#11 @scotthelme
3 years ago

Given the increase in adoption and requirement for a strong CSP to protect websites and their visitors, could we can get an update on the status of this issue?

#12 @melodiouscode
3 years ago

  • Focuses coding-standards added

A further bump on top of @scotthelme's bump!

#13 follow-up: @johnbillion
3 years ago

  • Focuses coding-standards removed
  • Milestone changed from Awaiting Review to 5.0
  • Owner set to johnbillion
  • Status changed from new to accepted

I think the solution proposed in #39941 is the most viable one. Some feedback on that approach would be greatly appreciated!

#14 in reply to: ↑ 13 @jdgrimes
3 years ago

Replying to scotthelme:

Hey Scott! Thanks for dropping in! I am a big fan of Troy Hunt, and through his blog I found out about your site https://report-uri.com/, and that has made adding a CSP to my own WordPress-powered sites actually doable rather than seeming like an impossible chore. So thank you so much for that!

Of course, up to now, I've had to use a CSP with unsafe-inline. However, with the increased flexibility that CSPs have with hashes and nonces now, I think that getting to a place where WordPress can run without the need for unsafe-inline is achievable.

Replying to johnbillion:

I think the solution proposed in #39941 is the most viable one. Some feedback on that approach would be greatly appreciated!

I agree that in the short-term, using nonces is going to be necessary, since it requires the least amount of refactoring. We have to keep in mind that to really be useful, whatever happens here needs to be embraced by the WordPress ecosystem as a whole, if you want to use a non-unsafe CSP on a site with any plugins and a non-default theme. It is unfortunately not realistic to think that many plugins and themes are going to completely remove inline scripts anytime soon, and core may also need to keep using them at least in some places, for backward compatibility if for no other reason.

That said, longer-term, I think moving away from inline scripts as much as possible should still be the goal. The downside of the nonces approach is that it still allows XSS, if untrusted input is being output within those nonced script tags unescaped. We should really be pushing plugin developers to pass data to scripts in a more fail-safe manner. Just replacing all of their inline script tags with a call to an inline_js() function is not going to magically make that script itself safer.

Despite that caveat, I think it is still a worthwhile pursuit and a considerable improvement, because sites running a non-unsafe CSP would still eliminate other whole classes of more common XSS.

But what I think should ideally happen, is that we do something along the lines of #39941 now, but in the future it would be deprecated/discouraged, in favor of moving away from inline scripts altogether.

#15 @johnbillion
2 years ago

  • Milestone changed from 5.0 to 5.1

Related: #39941

#16 @pento
23 months ago

  • Milestone changed from 5.1 to Future Release

Along with #39941, this ticket needs further investigation and work before it will be ready.

#17 @enricocarraro
2 months ago

I worked on refactoring all script tags so that their attributes can be controlled in this pull request for #39941.
Based on that, I’m now working on another PR for this ticket here, focusing on refactoring inline event handlers and JavaScript URIs in a way that is as clean as possible, and that doesn't impact the page rendering time.
Page rendering time becomes a slow when there are many inline script tags spread across the page; solutions to this could be:

  • Grouping all event handlers and JavaScript URIs calls, and printing their equivalents together at the end of the page as one inline script:
    • advantages:
      • straightforward;
    • disadvantages:
      • low code readability;
      • needs some logic to aggregate and print scripts (WP_Scripts could be used for this, but it would be a bit of a stretch).
  • Moving them to JavaScript files (current effort):
    • advantages:
      • better code readability;
    • disadvantages:
      • not an option for all, as some receive data from PHP (this can be overcome using data attributes);
      • it's tricky to understand which pages can be grouped together to include the same JavaScript file.
  • Substituting them with inline scripts with the defer attribute so that they can only be executed after the document parsing is completed:
    • advantages:
      • very straightforward;
    • disadvantages:
      • low code readability.

I would be interested in knowing what do you think is the most appropriate solution.

Edit:
I opened a new ticket (#51407) focused on inline event handlers and javascript: URIs refactoring, in which I use a mix of the last two solutions I proposed.

Last edited 8 weeks ago by enricocarraro (previous) (diff)

This ticket was mentioned in Slack in #core by laura. View the logs.


2 months ago

#19 @johnbillion
2 months ago

  • Owner johnbillion deleted
  • Status changed from accepted to assigned

This ticket was mentioned in Slack in #core-js by enricocarraro. View the logs.


8 weeks ago

Note: See TracTickets for help on using tickets.