Fix critical security vulnerabilities, improve error handling, and enhance code quality in ActivityPub plugin admin files #2220

Mukeshoad · 2025-09-21T08:31:39Z

Proposed changes:

Security Enhancements: Fixed multiple XSS vulnerabilities by adding proper output escaping (esc_html(), esc_attr(), esc_url()) throughout all admin files
CSRF Protection: Enhanced nonce verification and moved validation to the beginning of AJAX handlers for better security
Input Validation: Added comprehensive sanitization and validation for all user inputs, including file uploads and form data
Error Handling: Implemented robust error handling for WordPress functions that can return WP_Error objects
File Security: Added proper file existence checks before including templates and asset files
Database Safety: Enhanced user capability checks and added validation for user existence before operations
URL Validation: Added wp_http_validate_url() checks for external URL imports
Type Safety: Added null/empty checks and proper type validation before processing arrays and objects

Other information:

Have you written new tests for your changes, if applicable?

Testing instructions:

Security Testing:

XSS Prevention Testing:

Go to wp-admin/users.php and test bulk user operations
Try to inject script tags in form fields - they should be properly escaped
Test comment and post admin pages for proper output escaping

File Upload Security:

Go to Tools > Import > Mastodon or Starter Kit
Try uploading malicious files (non-ZIP for Mastodon, non-JSON for Starter Kit)
Verify proper error messages and no execution of malicious content

AJAX Security:

Open browser dev tools and go to ActivityPub settings
Test moderation settings AJAX calls - verify nonce validation works
Try requests without proper nonces - should fail gracefully

Functionality Testing:

Import Functionality:

Test Mastodon archive import with valid ZIP file
Test Starter Kit import with both file upload and URL methods
Verify proper error handling with corrupted/invalid files

Admin Interface:

Test ActivityPub settings pages and dashboard widgets
Verify all admin notices display correctly
Test screen options functionality

User Management:

Test bulk enable/disable ActivityPub for users
Verify proper capability checks and user validation
Test the confirmation flow for removing users from fediverse

Error Handling Testing:

File System Errors:

Test with insufficient file permissions
Test with full disk scenarios
Verify graceful error messages

Network Errors:

Test Starter Kit URL import with invalid/unreachable URLs
Verify proper timeout handling

Before/After Comparison:

Before: XSS vulnerabilities, potential CSRF attacks, poor error handling
After: Secure output escaping, robust nonce validation, comprehensive error handling

Changelog entry

Automatically create a changelog entry from the details below.

Changelog Entry Details

Significance

Minor

Type

Security - in case of vulnerabilities
Fixed - for any bug fixes

pfefferle · 2025-09-21T09:00:56Z

includes/wp-admin/import/class-mastodon.php

 		\check_admin_referer( 'import-upload' );

-		if ( ! isset( $_FILES['import']['name'] ) ) {
+		if ( ! isset( $_FILES['import']['name'] ) || empty( $_FILES['import']['name'] ) ) {


Empty implicitly checks for isset, so no need to have both.

Suggested change

if ( ! isset( $_FILES['import']['name'] ) || empty( $_FILES['import']['name'] ) ) {

if ( empty( $_FILES['import']['name'] ) ) {

Thanks for catching that! You’re right — empty() already covers the isset() check. I’ll simplify it accordingly.

pfefferle · 2025-09-21T09:02:56Z

includes/class-blocks.php

+		$asset_file = ACTIVITYPUB_PLUGIN_DIR . 'build/editor-plugin/plugin.asset.php';
+		if ( ! file_exists( $asset_file ) ) {
+			return;
+		}
+
+		$asset_data = include $asset_file;


This file is part of the plugin, so there is no need to check if it is available.

Good point. Since these asset files are shipped with the plugin, existence checks aren’t really necessary. I’ll revert those changes to keep the code clean.

pfefferle · 2025-09-21T09:03:10Z

includes/class-blocks.php

+		$asset_file = ACTIVITYPUB_PLUGIN_DIR . 'build/reply-intent/plugin.asset.php';
+		if ( ! file_exists( $asset_file ) ) {
+			return;
+		}
+
+		$asset_data = include $asset_file;


same as above

pfefferle · 2025-09-21T09:04:36Z

includes/class-blocks.php

+		$blocks = array(
+			'/build/follow-me',
+			'/build/followers',
+			'/build/reactions',
 		);
+
+		foreach ( $blocks as $block ) {
+			$block_path = ACTIVITYPUB_PLUGIN_DIR . $block;
+			if ( file_exists( $block_path ) ) {
+				\register_block_type_from_metadata( $block_path );
+			}
+		}
+
+		$reply_block_path = ACTIVITYPUB_PLUGIN_DIR . '/build/reply';
+		if ( file_exists( $reply_block_path ) ) {
+			\register_block_type_from_metadata(
+				$reply_block_path,
+				array(
+					'render_callback' => array( self::class, 'render_reply_block' ),
+				)
+			);
+		}


This looks more complicated than the inital code, so I would not change it. There is also no need to check if the file exists, because the files are part of this plugin.

Understood, I’ll simplify this back to the original approach and drop the file_exists() checks. Thanks for pointing that out.

pfefferle · 2025-09-21T09:05:38Z

includes/class-blocks.php

-			return get_the_author_meta( 'ID' );
+			return get_post_field( 'post_author', $queried_object->ID );


Why this change?

That was an oversight on my end, I was aiming for consistency but I see now it changes behavior unnecessarily. I’ll revert this change to preserve the original logic.

pfefferle · 2025-09-21T09:06:54Z

includes/class-blocks.php

+			$cleaned_url = str_replace( array( 'https://', 'http://' ), '', esc_url( $attrs['url'] ) );
 			$html .= sprintf(
-				'<p><a title="%2$s" aria-label="%2$s" href="%1$s" class="u-in-reply-to" target="_blank">%3$s</a></p>',
+				'<p><a title="%2$s" aria-label="%2$s" href="%1$s" class="u-in-reply-to" target="_blank" rel="noopener noreferrer">%3$s</a></p>',
 				esc_url( $attrs['url'] ),
 				esc_attr__( 'This post is a response to the referenced content.', 'activitypub' ),
 				// translators: %s is the URL of the post being replied to.
-				sprintf( __( '&#8620;%s', 'activitypub' ), \str_replace( array( 'https://', 'http://' ), '', esc_url( $attrs['url'] ) ) )
+				sprintf( __( '&#8620;%s', 'activitypub' ), esc_html( $cleaned_url ) )


what is the improvement here?

The intent was twofold:

Add rel="noopener noreferrer" for better security when opening links in a new tab.

Use esc_html() on the cleaned URL for stricter output escaping.

But I understand this may be unnecessary if we want to keep minimal diffs. I can revert or adjust based on your preference.

pfefferle · 2025-09-21T09:09:07Z

Thanks for your contribution @Mukeshoad , I added some inline questions an feedback.

Mukeshoad · 2025-09-21T09:13:05Z

Thanks for your contribution @Mukeshoad , I added some inline questions an feedback.

Thanks a lot for the review and for the warm welcome, @pfefferle.

Just to give a bit of context, I’ve been working as a WordPress developer for about 13 years, but I’m relatively new to contributing here on GitHub. I’m still getting used to the workflow and best practices for open-source collaboration.

I really appreciate your detailed inline feedback. My main focus with this PR was to strengthen security (escaping, sanitization, nonce validation, etc.), and in some cases I may have been a little over-cautious with additional checks. Your comments are very helpful in understanding where the plugin already guarantees certain files/values, so I’ll adjust accordingly to keep the code lean and consistent.

I’ll work through your suggestions and update the PR. Thanks again for taking the time to review my changes, it’s a great learning experience to align my contributions with the project’s standards.

Mukeshoad added 5 commits September 21, 2025 13:12

Update class-blocks.php

2bd9741

Update class-admin.php

f05bef3

Update class-screen-options.php

5383ac4

Update class-mastodon.php

79f4a84

Update class-starter-kit.php

48e3c5b

github-actions bot added [Feature] Import [Feature] WP Admin [Focus] Editor Changes to the ActivityPub experience in the block editor OSS Citizen labels Sep 21, 2025

pfefferle reviewed Sep 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix critical security vulnerabilities, improve error handling, and enhance code quality in ActivityPub plugin admin files #2220

Fix critical security vulnerabilities, improve error handling, and enhance code quality in ActivityPub plugin admin files #2220

Mukeshoad commented Sep 21, 2025

Uh oh!

pfefferle Sep 21, 2025

Uh oh!

Mukeshoad Sep 21, 2025

Uh oh!

pfefferle Sep 21, 2025

Uh oh!

Mukeshoad Sep 21, 2025

Uh oh!

pfefferle Sep 21, 2025

Uh oh!

pfefferle Sep 21, 2025

Uh oh!

Mukeshoad Sep 21, 2025

Uh oh!

pfefferle Sep 21, 2025

Uh oh!

Mukeshoad Sep 21, 2025

Uh oh!

pfefferle Sep 21, 2025

Uh oh!

Mukeshoad Sep 21, 2025

Uh oh!

pfefferle commented Sep 21, 2025

Uh oh!

Mukeshoad commented Sep 21, 2025

Uh oh!

Uh oh!

	if ( ! isset( $_FILES['import']['name'] ) \|\| empty( $_FILES['import']['name'] ) ) {
	if ( empty( $_FILES['import']['name'] ) ) {

		return get_the_author_meta( 'ID' );
		return get_post_field( 'post_author', $queried_object->ID );

Fix critical security vulnerabilities, improve error handling, and enhance code quality in ActivityPub plugin admin files #2220

Are you sure you want to change the base?

Fix critical security vulnerabilities, improve error handling, and enhance code quality in ActivityPub plugin admin files #2220

Conversation

Mukeshoad commented Sep 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfefferle commented Sep 21, 2025

Uh oh!

Mukeshoad commented Sep 21, 2025

Uh oh!

Uh oh!