Make WordPress Core

Opened 3 years ago

Last modified 3 months ago

#51407 assigned enhancement

Remove inline event handlers and JavaScript URIs for Strict CSP-compatibility

Reported by: enricocarraro's profile enricocarraro Owned by: adamsilverstein's profile adamsilverstein
Milestone: Future Release Priority: normal
Severity: normal Version: 4.8
Component: Security Keywords: has-patch 2nd-opinion has-unit-tests
Focuses: javascript Cc:

Description

Content Security Policy is a mechanism designed to make applications more secure against common web vulnerabilities, particularly cross-site scripting. It is enabled by setting the Content-Security-Policy HTTP response header.
An application can add a critical defense-in-depth layer against markup injection attacks by adopting a strict policy that prevents the loading of untrusted scripts or plugins.
A basic policy (nonce + strict-dynamic + unsafe-eval) would block more than 40% of the XSS sinks.

To make an application compatible with strict CSP, it is necessary to make changes to HTML templates and client-side code and add the policy header:

  1. Add nonces to <script> elements
  2. Refactor inline event handlers and javascript: URIs
  3. Refactor calls to JS APIs incompatible with CSP
  4. Serve the Content-Security-Policy header

More on CSP.

Since these PRs are quite significant, I separated the one regarding script nonces (#39941) and the one on inline event handlers and JavaScript URIs to facilitate code reviews.
This patch builds on top of this one (use this link to compare the two branches easily); I introduced some new JavaScript files where I moved inline event handlers and JavaScript URIs to, in particular:

  • src/js/_enqueues/admin/link-manager.js: handles link deletion confirmation;
  • src/js/_enqueues/admin/media-events.js: events related to the media library and media details;
  • src/js/_enqueues/admin/setup-config.js: event handler for try again button;
  • src/js/_enqueues/admin/themes-list.js: theme deletion confirm event handler;
  • src/js/_enqueues/lib/metabox-events.js: event handlers for various meta boxes.

I also moved various event handlers to already existing JavaScript files.
In some cases, I couldn't move the inline event handlers to a file because 'user-defined' ajax calls could load the generated HTML, so I moved them to an inline script.

Change History (17)

This ticket was mentioned in PR #551 on WordPress/wordpress-develop by enricocarraro.


3 years ago
#1

  • Keywords has-unit-tests added

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


3 years ago

#3 @adamsilverstein
3 years ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

#4 @enricocarraro
3 years ago

As suggested by @adamsilverstein, here's a list of page changes:

  1. Invalid Comment ID 'Go Back' button and MultiSite 'Back' button
    • Originally in src/wp-admin/comment.php] moved to src/js/_enqueues/admin/common.js
    • commit
  2. Custom Image Header: 'Blog Name' anchor
    • Originally in src/wp-admin/includes/class-custom-image-header.php moved to src/js/_enqueues/admin/custom-header.js
    • commit
  3. Link Manager: 'Delete' button
    • Originally in src/wp-admin/includes/class-wp-links-list-table.php moved to src/js/_enqueues/admin/link-manager.js
    • commit
  4. Link details: 'Delete' button
    • Originally in src/wp-admin/includes/meta-boxes.php moved to src/js/_enqueues/admin/link.js
    • commit
  5. Media : 'Delete' button, 'Edit Image' button, 'Cancel' delete button, Use Featured Image, 'Cancel' button, 'Insert gallery', 'Update gallery settings'
    • Originally in src/wp-admin/includes/media.php moved to src/js/_enqueues/admin/media-events.js
    • commit
  6. Media Insert URL Form: src 4 radio inputs for alignment, "Go", "Link to image" and "None" buttons
    • Originally in src/wp-admin/includes/media.php moved to src/js/_enqueues/admin/media-events.js
    • same as above
  7. Media Library List View: 'Delete' and 'Attach' button
    • Originally in src/wp-admin/includes/class-wp-media-list-table.php moved to src/js/_enqueues/admin/media.js
    • commit
  8. Advanced Edit form: "Get shortlink"
    • Originally in src/wp-admin/edit-form-advanced.php moved to src/js/_enqueues/admin/post.js
    • commit
  9. Setup Config: "Go back" button on config failure
    • Originally in src/wp-admin/setup-config.php moved to src/js/_enqueues/admin/setup-config.js
    • commit
  10. Theme List: Delete button
    • Originally in src/wp-admin/includes/class-wp-themes-list-table.php moved to src/js/_enqueues/admin/themes-list.js
    • commit
  11. Image Edit: help toggles, text fields for crop and scale and buttons for Preview, Crop, Rotate, Flip, Undo, Redo, Cancel, Submit and Scale
    • Originally in src/wp-admin/includes/image-edit.php moved to src/js/_enqueues/lib/image-edit.js
    • commit
  12. Block Editor: metabox location form; Media Details : "Delete" button, "Add new comment" button, "Reply" button, "Show comments" button
    • Originally in src/wp-admin/includes/post.php and src/js/_enqueues/lib/metabox-events.js moved to src/js/_enqueues/lib/metabox-events.js
    • commit
  13. Dashboard: reply to comment button
    • Originally in src/wp-admin/includes/dashboard.php and src/js/_enqueues/admin/edit-comments.js
    • commit
  14. Customize Menu view: "widget areas" and "Widgets panel" button
    • Originally in src/wp-includes/class-wp-customize-nav-menus.php moved to src/js/_enqueues/wp/customize/nav-menus.js
    • commit
  15. Customize Widgets: Nav Menu widget "Create some" button when no menus exist
    • Originally in src/wp-includes/widgets/class-wp-nav-menu-widget.php moved to src/js/_enqueues/wp/customize/widgets.js
    • commit
  16. Theme and Plugin Editor: "Docs Lookup" button
    • Originally in src/wp-admin/plugin-editor.php and src/wp-admin/theme-editor.php
    • commit
  17. Async Upload: "Dismiss" button
    • Target src/wp-admin/async-upload.php
    • commit
  18. Theme List Install: "Try Again" Button
    • Target: src/wp-admin/includes/class-wp-theme-install-list-table.php
    • commit
  19. Template Meta Form: "Enter" new
    • Target: src/wp-admin/includes/template.php
    • commit
  20. Comment Template: "Reply" button
    • Target: src/wp-includes/comment-template.php
    • commit
  21. Default wp_die handler: "Go back" button
    • Target: src/wp-includes/functions.php
    • commit
  22. ThickBox: iframe loading
    • Target: src/js/_enqueues/vendor/thickbox/thickbox.js
    • commit

List of efforts on WordPress JavaScript dependencies:

  • TinyMCE compat3x plugin: PR
  • MediaElement volume: PR
Last edited 3 years ago by enricocarraro (previous) (diff)

#5 @enricocarraro
3 years ago

@whyisjake Do you know anybody that could help in the review process? I would appreciate some more eyes on it since there isn't much time until the 5.6 beta phase.
I would also like to stress how important strict CSP would be in protecting WordPress users.

#6 @hellofromTonya
3 years ago

  • Version changed from trunk to 4.8

This ticket does not seem to be directly caused by Version 5.6 (ie trunk). It does reference being built upon #39941, which has a Version of 4.8. Changing the version to match that ticket.

However, if the version is different, please advise or update.

#7 @adamsilverstein
3 years ago

  • Milestone changed from Awaiting Review to 5.7

#8 in reply to: ↑ description @jornfranke
3 years ago

I think a better way would be not to use nonces (too complex to configure on the web server side etc.). I recommend to make all Javascript strictly as external files. Then, I dont need to configure the nounces in the headers.

Othrwise I strongly support strict CSP. The new block editor is a security disaster:
unsafe-inline, unsafe-eval, external references to google fonts...

A good out of the box WordPress installation must work with the following CSP for all areas (especially the admin area):

Content-Security-Policy: default-src 'none'; base-uri 'self'; script-src 'self'; style-src 'self'; img-src 'self'; connect-src 'self'; font-src 'self'; object-src 'none'; media-src 'none'; child-src 'self'; form-action 'self'; frame-ancestors 'none'; navigate-to 'self'; block-all-mixed-content

Last edited 3 years ago by jornfranke (previous) (diff)

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


3 years ago

#10 @hellofromTonya
3 years ago

Notes from last scrub:

@whyisjake raised his hand to review for 2nd opinion.

@adamsilverstein thought there's still time for this ticket to land in 5.7 Beta 1.

#11 @hellofromTonya
3 years ago

  • Milestone changed from 5.7 to 5.8

5.7 Beta 1 is happening shortly. Ran out of time to get this ticket into the release. Punting to 5.8.

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

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


3 years ago

#13 @poena
3 years ago

  • Milestone changed from 5.8 to 5.9

Punting to 5.9.

#14 @adamsilverstein
2 years ago

  • Milestone changed from 5.9 to Future Release

Marking as future release for now and will look for opportunities to circle back to this work.

#15 @jornfranke
2 years ago

I agree and strongly support that all Javascript as well as CSS should be strictly external for higher security with CSP.

#16 @wildturtles
10 months ago

agree with @jornfranke not making it lead bad dev/sys admin to do ugly stuff like remove the inline source code.

Any news about that achievment ?

Any things about good practices in dev advices (core, plugins)

#17 @jornfranke
3 months ago

Hi,

Gutenberg did some fix related to CSP:
https://github.com/WordPress/gutenberg/issues/42513

Do you have an idea when this could be included?

Thank you.

Best regards

Note: See TracTickets for help on using tickets.