Bulk scan/fix for insecure content from the admin #112

kmgalanakis · 2023-03-23T16:25:50Z

Description of the Change

Closes #58

How to test the Change

Create a few posts with the following WP cli command echo '<figure class="wp-block-image size-large"><img src="http://cdn.pixabay.com/photo/2015/06/19/21/24/avenue-815297_960_720.jpg" alt=""/></figure><figure class="wp-block-image size-large"><img src="http://cdn.pixabay.com/photo/2015/06/19/21/24/avenue-815297_960_720.jpg" alt=""/></figure><figure class="wp-block-image size-large"><img src="http://cdn.pixabay.com/photo/2015/06/19/21/24/avenue-815297_960_720.jpg" alt=""/></figure>' | wp post generate --count=1 --post_type=car
The newly created posts will contain insecure content (image). In the Admin, go to Tools -> Insecure Content Warning and run (and dry-run) the process to fix the newly created posts. Play with all the options.
Verify the the already existing WP cli command to fix insecure content in posts still works.

Changelog Entry

Added - New admin screen to fix insecure content in posts.

Checklist:

I agree to follow this project's Code of Conduct.
I have updated the documentation accordingly.
I have added tests to cover my change.
All new and existing tests pass.

peterwilsoncc · 2023-04-11T01:06:48Z

@kmgalanakis Feel free to ping me on the ticket once this is ready for review so it doesn't get lost.

I'm happy to do an early scan if you wish, too, just say the word.

kmgalanakis · 2023-04-15T14:02:12Z

@kmgalanakis Feel free to ping me on the ticket once this is ready for review so it doesn't get lost.

I'm happy to do an early scan if you wish, too, just say the word.

@peterwilsoncc the biggest part of the work here is done but the problem is that I realized my current approach is not performing very well when there is a large number of posts with insecure content to be updated. I want to change that and as soon as it's ready I will let you know.

kmgalanakis · 2023-04-28T21:54:57Z

@kmgalanakis Feel free to ping me on the ticket once this is ready for review so it doesn't get lost.

I'm happy to do an early scan if you wish, too, just say the word.

@peterwilsoncc the fix is ready for review. I can still see the E2E tests failing but they are also failing on develop and I don't know how to fix them. I would be more than happy to also add the E2E test for this new feature but I don't know when I will be able to focus on this again, so I'm ok to do it after this feature is merged.

Please let me know what you think about the approach and the code.

Thank you.

peterwilsoncc

I've added some notes inline, there's a couple of security pick ups in there that are blockers.

There's a lot of unrelated changes in this PR due to the eslint fixes, while that should probably be done, I'd rather do it on a separate ticket to avoid confiscation. Especially given the introduction of bulk edits/rest endpoints that need a good security review.

peterwilsoncc · 2023-05-08T03:02:57Z

includes/admin.php

+	$hook = add_management_page(
+		__( 'Insecure Content Warning Admin', 'insecure-content-warning' ),
+		__( 'Insecure Content Warning', 'insecure-content-warning' ),
+		'install_plugins',


Suggested change

'install_plugins',

'edit_posts',

As a general capability, this seems closes to what the page is doing.

Good point, thank you!

peterwilsoncc · 2023-05-08T03:09:03Z

includes/classes/class-fixinsecurecontent.php

+				// Wait for a while before fixing the rest.
+				usleep( 1000 );


I don't understand why the wait, what is the intent?

I can't remember exactly because it's been a while but I'm guessing this was a leftover from my initial approach. I removed it.

peterwilsoncc · 2023-05-08T03:12:26Z

includes/classes/class-fixinsecurecontent.php

+
+			foreach ( $this->warning_count as $warning ) {
+				$warning_message = array_shift( $warning );
+				$output         .= '<span class="warning">' . __( 'Warning', 'insecure-content-warning' ) . '</span>: ' . $warning_message . PHP_EOL;


Suggested change

$output .= '<span class="warning">' . __( 'Warning', 'insecure-content-warning' ) . '</span>: ' . $warning_message . PHP_EOL;

$output .= '<span class="warning">' . __( 'Warning', 'insecure-content-warning' ) . '</span>: ' . wp_strip_all_tags( $warning_message ) . PHP_EOL;

I know it isn't needed now but it protects ourselves from our future selves.

Better safe than sorry, thank you.

peterwilsoncc · 2023-05-08T03:15:00Z

includes/classes/class-fixinsecurecontent.php

+			// translators: Message to show when the system is unable to fetch details for the post.
+			$message = sprintf( __( 'Unable to fetch details for post %s', 'insecure-content-warning' ), $post_id );


Translator notes are intended to describe the placeholder rather than the string.

Suggested change

// translators: Message to show when the system is unable to fetch details for the post.

$message = sprintf( __( 'Unable to fetch details for post %s', 'insecure-content-warning' ), $post_id );

// translators: %d The Post ID being scanned for insecure content.

$message = sprintf( __( 'Unable to fetch details for post %d', 'insecure-content-warning' ), $post_id );

Done, thank you.

peterwilsoncc · 2023-05-08T03:15:18Z

includes/classes/class-fixinsecurecontent.php

+			// translators: Message to show when the system is unable to fetch details for the post.
+			$message = sprintf( __( 'Unable to fetch details for post %s', 'insecure-content-warning' ), $post_id );
+			if ( defined( 'WP_CLI' ) && WP_CLI ) {
+				// translators: Message to show when the system is unable to fetch details for the post.


Suggested change

// translators: Message to show when the system is unable to fetch details for the post.

Done, thank you.

peterwilsoncc · 2023-05-08T03:24:59Z

includes/classes/class-fixinsecurecontent.php

+			}
+			$this->fixed_post_count[] = array( 'URL(s) fixed summary' => $dry_run_message );
+		} else {
+			// translators: Summary to show on fix.


Done, thank you.

peterwilsoncc · 2023-05-08T03:27:45Z

includes/partials/admin-page.php

+						<option selected="selected" value="<?php echo esc_attr( 'all' ); ?>"><?php esc_html_e( 'All', 'insecure-content-warning' ); ?></option>
+						<option value="<?php echo esc_attr( 'posts' ); ?>"><?php esc_html_e( 'Individual post(s)', 'insecure-content-warning' ); ?></option>
+						<option value="<?php echo esc_attr( 'all_from_post_type' ); ?>"><?php esc_html_e( 'All from post type', 'insecure-content-warning' ); ?></option>


No need to escape the constants, just variables and translations/

Suggested change

<option selected="selected" value="<?php echo esc_attr( 'all' ); ?>"><?php esc_html_e( 'All', 'insecure-content-warning' ); ?></option>

<option value="<?php echo esc_attr( 'posts' ); ?>"><?php esc_html_e( 'Individual post(s)', 'insecure-content-warning' ); ?></option>

<option value="<?php echo esc_attr( 'all_from_post_type' ); ?>"><?php esc_html_e( 'All from post type', 'insecure-content-warning' ); ?></option>

<option selected="selected" value="all"><?php esc_html_e( 'All', 'insecure-content-warning' ); ?></option>

<option value="posts"><?php esc_html_e( 'Individual post(s)', 'insecure-content-warning' ); ?></option>

<option value="all_from_post_type"><?php esc_html_e( 'All from post type', 'insecure-content-warning' ); ?></option>

Good point, thank you.

peterwilsoncc · 2023-05-08T03:33:23Z

includes/rest.php

+		array(
+			'methods'             => 'POST',
+			'callback'            => __NAMESPACE__ . '\\fix_endpoint',
+			'permission_callback' => '__return_true',


Suggested change

'permission_callback' => '__return_true',

'permission_callback' => 'edit_posts',

This might end up needing to be a little more complex to allow for CPTs registered with custom capabilities.

As it's editing the posts, it will definitely need to include a capability check and should probably include a nonce check for users without an application password.

Should I take the time to research this now?

peterwilsoncc · 2023-05-08T03:34:01Z

includes/rest.php

+		array(
+			'methods'             => 'POST',
+			'callback'            => __NAMESPACE__ . '\\count_for_fix_endpoint',
+			'permission_callback' => '__return_true',


Suggested change

'permission_callback' => '__return_true',

'permission_callback' => 'edit_posts',

Reveals data.

Good point, thank you.

peterwilsoncc · 2023-05-08T03:37:13Z

includes/rest.php

+ *
+ * @return array
+ */
+function prepare_fix_params( WP_REST_Request $request ): array {


I suggest using the REST APIs schema for this.

This method does nothing more than prepare the params coming from the request for the methods that implement that counting and the fixing of posts with insecure content. How do you think we could use the REST APIs schema for this?

Generally, the goal is to do it the WordPress way -- using schema gets a lot of things for free.

Setting the parameters in the schema will use the validation and sanitization built in to WP, so specifying the expected types there will automatically throw warnings and validation errors for invalid values.

If WP discovers a security error in the RES API, by using the schema we can ensure that the upstream fixes are applied to the endpoints here too.

For post types, you'll probably need to do a little more validation -- see this line onwards in distributor -- to make sure the post type is intended to be visible in the rest api, the user has access, etc.

The functionality to bulk scan/fix insecure content from the admin is actually a replication of the WP CLI functionality. The WP CLI functionality can properly scan/fix post types that are not shown in REST, so in my opinion, the admin functionality should behave exactly the same. According to my tests, neither the WP CLI nor the built admin functionality has problems scanning/fixing posts from a post type that is not shown in REST.

What I don't understand from your comment is how this specific function here could be avoided by the use of the WP REST API. Let me explain once again here that what this method does is "transform" the parameters of the WP REST API endpoint call into suitable parameters for the methods that count/scan/fix posts with insecure content. You see, these methods are also used from the WP CLI command and it wouldn't make too much sense to have a 1:1 relation between the REST API endpoints parameters with the parameters fo those methods, that is why I'm using this "transformation" method.

Still, I took the time to implement some more validation and sanitization callbacks for the actual REST API endpoint parameters to play more on the safe side.

peterwilsoncc · 2023-06-13T00:19:12Z

includes/rest.php

+ *
+ * @return array
+ */
+function prepare_fix_params( WP_REST_Request $request ): array {


Generally, the goal is to do it the WordPress way -- using schema gets a lot of things for free.

Setting the parameters in the schema will use the validation and sanitization built in to WP, so specifying the expected types there will automatically throw warnings and validation errors for invalid values.

If WP discovers a security error in the RES API, by using the schema we can ensure that the upstream fixes are applied to the endpoints here too.

For post types, you'll probably need to do a little more validation -- see this line onwards in distributor -- to make sure the post type is intended to be visible in the rest api, the user has access, etc.

peterwilsoncc · 2023-06-13T00:52:52Z

src/js/classic-editor.js

@@ -2,27 +2,26 @@ import blurInsecure from './utils/blur-insecure';
 import checkContent from './utils/check-content';
 import findElements from './utils/find-elements';
 import replaceContent from './utils/replace';
-import $ from 'jquery';


At the start of the file you'll need to add:

// eslint-disable-next-line import/no-unresolved import { jQuery } from 'jquery';

This will ensure the dependency is included in dist/js/classic-editor.asset.php by @wordpress/dependency-extraction-webpack-plugin

I think you mean import jQuery from 'jquery';. Initially, this didn't play nicely with ESLint but I ended up copying the way it's done in Distributor and now it's fine.

Thank you.

peterwilsoncc · 2023-06-13T00:57:05Z

src/js/gutenberg.js

 import { debounce } from 'underscore';
 import { getScrollContainer } from '@wordpress/dom';
 import apiRequest from '@wordpress/api-request';
 import domReady from '@wordpress/dom-ready';
 import { dispatch, select, subscribe } from '@wordpress/data';
-import $ from 'jquery';


Good catch, thank you.

peterwilsoncc · 2023-06-13T00:57:18Z

src/js/utils/blur-insecure.js

@@ -1,23 +1,21 @@
-import $ from 'jquery';


Good catch, thank you.

peterwilsoncc · 2023-06-13T00:57:40Z

src/js/utils/check-content.js

 import blurInsecure from './blur-insecure';
 import { scanElements } from './scan-elements';

-import { __, _nx, sprintf } from '@wordpress/i18n';
-import $ from 'jquery';


Good catch, thank you.

peterwilsoncc · 2023-06-13T00:57:53Z

src/js/utils/find-elements.js

@@ -1,29 +1,27 @@
 import { get } from 'underscore';

-import $ from 'jquery';


Good catch, thank you.

peterwilsoncc · 2023-06-13T00:58:10Z

src/js/utils/replace.js

@@ -1,5 +1,3 @@
-import $ from 'jquery';


Good catch, thank you.

peterwilsoncc · 2023-06-13T01:05:49Z

includes/classes/class-fixinsecurecontent.php

+	public function count_post_to_check( string $post_type, int $posts_per_page, int $post_offset ): int {
+		$args = array(
+			'posts_per_page' => $posts_per_page,
+			'post_type'      => 'all' === $post_type ? 'any' : $post_type,


This should be limited to only posts available via the rest api -- per note re:schema. Many e-commerce plugins use post types for orders sow we definitely need to exclude those.

As I commented on the other conversation, the functionality to bulk scan/fix insecure content from the admin is actually a replication of the WP CLI functionality. The WP CLI functionality can properly scan/fix post types that are not shown in REST, so in my opinion, the admin functionality should behave exactly the same. According to my tests, neither the WP CLI nor the built admin functionality has problems scanning/fixing posts from a post type that is not shown in REST.

The WP CLI functionality can properly scan/fix post types that are not shown in REST, so in my opinion, the admin functionality should behave exactly the same.

The difference between the two is that the WP-CLI is basically the equivalent of running as the root user on a server. Once someone has access to the server they can do anything, such as running wp db clean --yes so permissions don't apply.

For users with access to the dashboard only, we need to check that they have full permission to make any changes to a post. If a post type isn't editable via the rest API then we need to assume the developer has reasons for preventing this and respect their decision

What you say makes perfect sense @peterwilsoncc. I've modified the method that counts posts with and the method that fixes insecure content so that posts from post types that are not shown in RFEST are excluded from the process.

Please let me know if this is what you meant. Thank you for your feedback and patience.

peterwilsoncc · 2023-06-13T01:13:59Z

includes/classes/class-fixinsecurecontent.php

+		// Check if a https version of the URL exists.
+		$secure_version_exists = false;
+		$ssl                   = preg_replace( '/^http:/i', 'https:', $url );
+		$response              = wp_remote_get( $ssl );


maybe, maybe, maybe???

Per MDN, this should tell you whether the file exists without having to download the full content. It should return the HTTP headers as-though you were making a GET request.

Suggested change

$response = wp_remote_get( $ssl );

$response = wp_remote_head( $ssl );

This is part of the code from the implementation of the WP CLI command but I can confirm that your point is right, it checks if the URL resolves to an existing resource without downloading it just by checking the returned HTTP header.

Good point, thank you.

…EST API endpoint args with validation and sanitization callbacks and fix an issue with the WP CLI command generating a PHP fatal error.

…content from the admin

peterwilsoncc

Looks good with a minor nitpcik.

peterwilsoncc · 2023-08-06T23:39:12Z

includes/classes/class-fixinsecurecontent.php

+	public function get_wp_query_post_type_args(): array {
+		return array(
+			'show_ui'      => true,
+			'public'       => true,


Suggested change

'public' => true,

I think the other two will be fine, the user may have permission to edit some private post types

peterwilsoncc · 2023-08-06T23:40:16Z

includes/classes/class-fixinsecurecontent.php

+			$total = 0;
+
+			// Exclude post from post types not shown in REST when fixing insecure content from the admin UI.
+			if ( ! defined( 'WP_CLI' ) && 'any' === $post_type ) {


you've used all elsewhere and any here.

Good point, fixed.

peterwilsoncc

LGTM, thank you!

# Conflicts: # package-lock.json

peterwilsoncc

Resolving merge conflicts dismissed my review, reapproving.

kmgalanakis self-assigned this Mar 23, 2023

kmgalanakis force-pushed the feature/bulk-scan-fix-from-admin branch 2 times, most recently from 41967ab to 292e8c9 Compare March 24, 2023 08:45

kmgalanakis changed the title ~~Initial commit~~ Bulk scan/fix for insecure content from the admin Mar 24, 2023

kmgalanakis force-pushed the feature/bulk-scan-fix-from-admin branch from 292e8c9 to 853d690 Compare March 24, 2023 19:36

jeffpaul added this to the 1.1.0 milestone Apr 10, 2023

jeffpaul requested review from a team and peterwilsoncc and removed request for a team April 10, 2023 21:44

kmgalanakis force-pushed the feature/bulk-scan-fix-from-admin branch 3 times, most recently from d13b79a to 8837f00 Compare April 17, 2023 21:31

kmgalanakis force-pushed the feature/bulk-scan-fix-from-admin branch 13 times, most recently from 268e858 to c0be893 Compare April 28, 2023 18:36

kmgalanakis added 2 commits April 28, 2023 21:37

Initial commit

9416685

Fix eslint errors

50b2b85

kmgalanakis force-pushed the feature/bulk-scan-fix-from-admin branch from c0be893 to 6cba4df Compare April 28, 2023 18:41

kmgalanakis force-pushed the feature/bulk-scan-fix-from-admin branch from 6cba4df to 50b2b85 Compare April 28, 2023 21:49

kmgalanakis marked this pull request as ready for review April 28, 2023 21:51

kmgalanakis requested a review from a team as a code owner April 28, 2023 21:51

Merge branch 'develop' into feature/bulk-scan-fix-from-admin

4ec530d

peterwilsoncc requested changes May 8, 2023

View reviewed changes

Fix suggestion in the PR

459e817

jeffpaul requested a review from peterwilsoncc June 12, 2023 15:51

peterwilsoncc reviewed Jun 13, 2023

View reviewed changes

kmgalanakis added 2 commits June 16, 2023 11:09

Merge branch 'develop' into feature/bulk-scan-fix-from-admin

1820101

Include jQuery to any module that needs it, further booletproof the R…

df6ab38

…EST API endpoint args with validation and sanitization callbacks and fix an issue with the WP CLI command generating a PHP fatal error.

kmgalanakis force-pushed the feature/bulk-scan-fix-from-admin branch from 7226db7 to df6ab38 Compare June 16, 2023 11:15

dkotter modified the milestones: 1.1.0, 1.2.0 Jun 16, 2023

jeffpaul requested a review from peterwilsoncc July 10, 2023 15:28

kmgalanakis force-pushed the feature/bulk-scan-fix-from-admin branch from a33d13a to df6ab38 Compare August 2, 2023 06:28

Exclude posts from post types not shown in REST when fixing insecure …

d2b6321

…content from the admin

kmgalanakis force-pushed the feature/bulk-scan-fix-from-admin branch from 58a0cdd to d2b6321 Compare August 2, 2023 07:09

peterwilsoncc reviewed Aug 6, 2023

View reviewed changes

Fixing further suggestions in the PR.

0bda620

kmgalanakis requested a review from peterwilsoncc August 7, 2023 09:50

peterwilsoncc previously approved these changes Aug 7, 2023

View reviewed changes

Merge branch 'develop' into feature/bulk-scan-fix-from-admin

bf5af8e

# Conflicts: # package-lock.json

peterwilsoncc dismissed their stale review via bf5af8e August 7, 2023 23:09

peterwilsoncc approved these changes Aug 7, 2023

View reviewed changes

jeffpaul merged commit 494aa7d into develop Aug 8, 2023
5 of 6 checks passed

jeffpaul deleted the feature/bulk-scan-fix-from-admin branch August 8, 2023 02:03

	$output .= '<span class="warning">' . __( 'Warning', 'insecure-content-warning' ) . '</span>: ' . $warning_message . PHP_EOL;
	$output .= '<span class="warning">' . __( 'Warning', 'insecure-content-warning' ) . '</span>: ' . wp_strip_all_tags( $warning_message ) . PHP_EOL;

		// translators: Message to show when the system is unable to fetch details for the post.
		$message = sprintf( __( 'Unable to fetch details for post %s', 'insecure-content-warning' ), $post_id );

	'permission_callback' => '__return_true',
	'permission_callback' => 'edit_posts',

		@@ -1,29 +1,27 @@
		import { get } from 'underscore';

		import $ from 'jquery';

	$response = wp_remote_get( $ssl );
	$response = wp_remote_head( $ssl );

Bulk scan/fix for insecure content from the admin #112

Bulk scan/fix for insecure content from the admin #112

Conversation

kmgalanakis commented Mar 23, 2023 • edited Loading

Description of the Change

How to test the Change

Changelog Entry

Checklist:

peterwilsoncc commented Apr 11, 2023

kmgalanakis commented Apr 15, 2023

kmgalanakis commented Apr 28, 2023

peterwilsoncc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterwilsoncc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterwilsoncc left a comment

Choose a reason for hiding this comment

peterwilsoncc left a comment

Choose a reason for hiding this comment

kmgalanakis commented Mar 23, 2023 •

edited

Loading