Merge pull request #228 from 10up/fix/only-allow-svg-when-uploading

Fix/only allow svg when uploading
10up · Oct 16, 2024 · 718e445 · 718e445
2 parents 89d900c + 8af17da
commit 718e445
Show file tree

Hide file tree

Showing 5 changed files with 74 additions and 6 deletions.
diff --git a/safe-svg.php b/safe-svg.php
@@ -128,11 +128,16 @@ public function __construct() {
 			$this->sanitizer = new Sanitizer();
 			$this->sanitizer->minify( true );
 
+			// Allow SVG uploads from specific contexts.
+			add_action( 'load-upload.php', array( $this, 'allow_svg_from_upload' ) );
+			add_action( 'load-post-new.php', array( $this, 'allow_svg_from_upload' ) );
+			add_action( 'load-post.php', array( $this, 'allow_svg_from_upload' ) );
+			add_action( 'load-site-editor.php', array( $this, 'allow_svg_from_upload' ) );
+
+			// Init all the things.
 			add_action( 'init', array( $this, 'setup_blocks' ) );
-			add_filter( 'upload_mimes', array( $this, 'allow_svg' ) );
 			add_filter( 'wp_handle_sideload_prefilter', array( $this, 'check_for_svg' ) );
 			add_filter( 'wp_handle_upload_prefilter', array( $this, 'check_for_svg' ) );
-			add_filter( 'wp_check_filetype_and_ext', array( $this, 'fix_mime_type_svg' ), 75, 4 );
 			add_filter( 'wp_prepare_attachment_for_js', array( $this, 'fix_admin_preview' ), 10, 3 );
 			add_filter( 'wp_get_attachment_image_src', array( $this, 'one_pixel_fix' ), 10, 4 );
 			add_filter( 'admin_post_thumbnail_html', array( $this, 'featured_image_fix' ), 10, 3 );
@@ -145,6 +150,16 @@ public function __construct() {
 			new safe_svg_settings();
 		}
 
+		/**
+		 * Allow SVG uploads from the wp-admin/upload.php screen. Without this, you cannot upload SVGs from the media grid view.
+		 *
+		 * @return void
+		 */
+		public function allow_svg_from_upload() {
+			add_filter( 'upload_mimes', array( $this, 'allow_svg' ) );
+			add_filter( 'wp_check_filetype_and_ext', array( $this, 'fix_mime_type_svg' ), 75, 4 );
+		}
+
 		/**
 		 * Custom function to check if user can upload svg.
 		 *
@@ -242,7 +257,18 @@ public function check_for_svg( $file ) {
 			}
 
 			$file_name   = isset( $file['name'] ) ? $file['name'] : '';
+
+			// Allow SVGs to be uploaded when this function runs.
+			add_filter( 'upload_mimes', array( $this, 'allow_svg' ) );
+			add_filter( 'wp_check_filetype_and_ext', array( $this, 'fix_mime_type_svg' ), 75, 4 );
+
 			$wp_filetype = wp_check_filetype_and_ext( $file['tmp_name'], $file_name );
+
+			// Remove the SVG mime type after we've sanitized the file.
+			// We need to utilize the pre_move_uploaded_file filter to ensure we can remove the filters after the file has been full-processed.
+			// This is because wp_check_filetype_and_ext() is called multiple times during the upload process.
+			add_filter( 'pre_move_uploaded_file', array( $this, 'pre_move_uploaded_file' ) );
+
 			$type        = ! empty( $wp_filetype['type'] ) ? $wp_filetype['type'] : '';
 
 			if ( 'image/svg+xml' === $type ) {
@@ -266,6 +292,23 @@ public function check_for_svg( $file ) {
 			return $file;
 		}
 
+		/**
+		 * Remove the filters after the file has been processed.
+		 *
+		 * We need to utilize the pre_move_uploaded_file filter to ensure we can remove the filters after the file has been full-processed.
+		 * This is because wp_check_filetype_and_ext() is called multiple times during the upload process.
+		 *
+		 * @param string $move_new_file The new file path. We don't touch this, just return it.
+		 *
+		 * @return string
+		 */
+		public function pre_move_uploaded_file( $move_new_file ) {
+			remove_filter( 'wp_check_filetype_and_ext', array( $this, 'fix_mime_type_svg' ), 75 );
+			remove_filter( 'upload_mimes', array( $this, 'allow_svg' ) );
+
+			return $move_new_file;
+		}
+
 		/**
 		 * Sanitize the SVG
 		 *

diff --git a/tests/cypress/e2e/safe-svg.cy.js b/tests/cypress/e2e/safe-svg.cy.js
@@ -3,12 +3,18 @@ describe('Safe SVG Tests', () => {
     cy.login();
   });
 
-  it('Admin can upload SVG image', () => {
+  it('Admin can upload SVG image via add new media file', () => {
     cy.uploadMedia('.wordpress-org/icon.svg');
     cy.get('.media-item .media-list-title, .media-item .title').should('exist').contains('icon');
     cy.get('.media-item a.edit-attachment').should('exist').contains('Edit');
   });
 
+  it('Admin can upload SVG image via the media grid', () => {
+    cy.uploadMediaThroughGrid('.wordpress-org/icon.svg').then((attachmentId) => {
+      cy.get(`.attachments .attachment[data-id="${attachmentId}"]`).should('exist');
+    });
+  });
+
   /**
    * Flow for verify SVG sanitization.
    *

diff --git a/tests/cypress/support/commands.js b/tests/cypress/support/commands.js
@@ -29,4 +29,18 @@ Cypress.Commands.add('uploadMedia', (filePath) => {
     cy.get('.drag-drop').should('exist');
     cy.get('#drag-drop-area').should('exist');
     cy.get('#drag-drop-area').selectFile(filePath, { action: 'drag-drop' });
-});
+});
+
+Cypress.Commands.add('uploadMediaThroughGrid', (filePath) => {
+    cy.visit('/wp-admin/upload.php?mode=grid');
+    cy.get('.supports-drag-drop').should('exist');
+    cy.get('.uploader-window').should('exist');
+    // Intercept the upload request
+    cy.intercept('POST', '/wp-admin/async-upload.php').as('upload');
+    cy.get('.supports-drag-drop').selectFile(filePath, { action: 'drag-drop', force: true, waitForAnimations: true });
+    return cy.wait('@upload')
+        .then(({ request, response }) => {
+            cy.get('.uploader-window').trigger('dropzone:leave');
+            return cy.wrap(response.headers['x-wp-upload-attachment-id'] ?? 0);
+        })
+});
diff --git a/tests/unit/bootstrap.php b/tests/unit/bootstrap.php
@@ -15,5 +15,6 @@
 WP_Mock::bootstrap();
 
 \WP_Mock::userFunction( 'plugin_dir_url' );
+\WP_Mock::userFunction( 'remove_filter' );
 
 require TEST_PLUGIN_DIR . '/safe-svg.php';
diff --git a/tests/unit/test-safe-svg.php b/tests/unit/test-safe-svg.php
@@ -41,9 +41,9 @@ public function tearDown(): void {
 	 * Test constructor.
 	 */
 	public function test_constructor() {
-		\WP_Mock::expectFilterAdded( 'upload_mimes', array( $this->instance, 'allow_svg' ) );
+		\WP_Mock::expectActionAdded( 'load-upload.php', array( $this->instance, 'allow_svg_from_upload' ) );
 		\WP_Mock::expectFilterAdded( 'wp_handle_upload_prefilter', array( $this->instance, 'check_for_svg' ) );
-		\WP_Mock::expectFilterAdded( 'wp_check_filetype_and_ext', array( $this->instance, 'fix_mime_type_svg' ), 75, 4 );
+		\WP_Mock::expectFilterAdded( 'wp_handle_sideload_prefilter', array( $this->instance, 'check_for_svg' ) );
 		\WP_Mock::expectFilterAdded( 'wp_prepare_attachment_for_js', array( $this->instance, 'fix_admin_preview' ), 10, 3 );
 		\WP_Mock::expectFilterAdded( 'wp_get_attachment_image_src', array( $this->instance, 'one_pixel_fix' ), 10, 4 );
 		\WP_Mock::expectFilterAdded( 'admin_post_thumbnail_html', array( $this->instance, 'featured_image_fix' ), 10, 3 );
@@ -159,6 +159,10 @@ public function test_check_for_svg() {
 			'name'     => 'svgTestOne.svg',
 		);
 
+		\WP_Mock::expectFilterAdded( 'upload_mimes', array( $this->instance, 'allow_svg' ) );
+		\WP_Mock::expectFilterAdded( 'wp_check_filetype_and_ext', array( $this->instance, 'fix_mime_type_svg' ), 75, 4 );
+		\WP_Mock::expectFilterAdded( 'pre_move_uploaded_file', array( $this->instance, 'pre_move_uploaded_file' ) );
+
 		$this->instance->check_for_svg( $file );
 
 		// @phpcs:ignore WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents