From ef2fb9d70cac8a019eaa754ee87eb11b5a274473 Mon Sep 17 00:00:00 2001 From: Daryll Doyle Date: Thu, 26 Sep 2024 16:30:25 +0100 Subject: [PATCH 1/8] Only add SVG as a mime when someone uploads an SVG via an approved method --- safe-svg.php | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/safe-svg.php b/safe-svg.php index 38b86c15..9546704c 100644 --- a/safe-svg.php +++ b/safe-svg.php @@ -129,10 +129,8 @@ public function __construct() { $this->sanitizer->minify( true ); 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 ); @@ -242,7 +240,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 +275,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 * From d421dadfe4c8c8a184a61d08e25a0a32e527f5c4 Mon Sep 17 00:00:00 2001 From: Daryll Doyle Date: Thu, 26 Sep 2024 17:10:24 +0100 Subject: [PATCH 2/8] Unsure we have access to remove_filter --- tests/unit/bootstrap.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/unit/bootstrap.php b/tests/unit/bootstrap.php index 24bf368f..703ac9cd 100644 --- 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'; From 439d6c49f9440f55ee64589925c8040ff686ff25 Mon Sep 17 00:00:00 2001 From: Daryll Doyle Date: Thu, 26 Sep 2024 17:10:33 +0100 Subject: [PATCH 3/8] Update tests --- tests/unit/test-safe-svg.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/unit/test-safe-svg.php b/tests/unit/test-safe-svg.php index 061c175b..70700f53 100644 --- a/tests/unit/test-safe-svg.php +++ b/tests/unit/test-safe-svg.php @@ -41,9 +41,8 @@ public function tearDown(): void { * Test constructor. */ public function test_constructor() { - \WP_Mock::expectFilterAdded( 'upload_mimes', array( $this->instance, 'allow_svg' ) ); \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 +158,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 From 09dcdfb32f4b1ccb0472f2c35be1fb85ee6e854c Mon Sep 17 00:00:00 2001 From: Daryll Doyle Date: Mon, 30 Sep 2024 11:39:49 +0100 Subject: [PATCH 4/8] =?UTF-8?q?Add=20SVGs=20to=20the=20allowed=20mimes=20w?= =?UTF-8?q?hen=20on=20the=20upload.php=20page,=20otherwise=20the=20media?= =?UTF-8?q?=20grid=20doesn=E2=80=99t=20work?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- safe-svg.php | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/safe-svg.php b/safe-svg.php index 9546704c..13702709 100644 --- a/safe-svg.php +++ b/safe-svg.php @@ -129,6 +129,7 @@ public function __construct() { $this->sanitizer->minify( true ); add_action( 'init', array( $this, 'setup_blocks' ) ); + add_action( 'load-upload.php', array( $this, 'allow_svg_from_upload' ) ); 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_prepare_attachment_for_js', array( $this, 'fix_admin_preview' ), 10, 3 ); @@ -143,6 +144,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. * From 147ce38a30d4e2c0b1758e9d12125c864a0932e5 Mon Sep 17 00:00:00 2001 From: Daryll Doyle Date: Mon, 30 Sep 2024 11:40:00 +0100 Subject: [PATCH 5/8] Ensure the actions are woring --- tests/unit/test-safe-svg.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/unit/test-safe-svg.php b/tests/unit/test-safe-svg.php index 70700f53..dd4881a6 100644 --- a/tests/unit/test-safe-svg.php +++ b/tests/unit/test-safe-svg.php @@ -41,6 +41,7 @@ public function tearDown(): void { * Test constructor. */ public function test_constructor() { + \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_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 ); From 520f87fd875ab779a68b82c025569748342d860d Mon Sep 17 00:00:00 2001 From: Daryll Doyle Date: Mon, 30 Sep 2024 11:40:38 +0100 Subject: [PATCH 6/8] Add function to upload via media grid view --- tests/cypress/support/commands.js | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/tests/cypress/support/commands.js b/tests/cypress/support/commands.js index 5b5692aa..15959ae2 100644 --- 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' }); -}); \ No newline at end of file +}); + +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); + }) +}); From fd6901011979c7d506e3a5d8cac9eaaa0a406132 Mon Sep 17 00:00:00 2001 From: Daryll Doyle Date: Mon, 30 Sep 2024 11:40:50 +0100 Subject: [PATCH 7/8] Add test for grid view uploads --- tests/cypress/e2e/safe-svg.cy.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/cypress/e2e/safe-svg.cy.js b/tests/cypress/e2e/safe-svg.cy.js index 7f95718f..601ff02e 100644 --- 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. * From 8af17da42e611ab1137042d18dc83fd197210579 Mon Sep 17 00:00:00 2001 From: Daryll Doyle Date: Mon, 30 Sep 2024 11:47:34 +0100 Subject: [PATCH 8/8] Add additional contexts --- safe-svg.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/safe-svg.php b/safe-svg.php index 13702709..30b17278 100644 --- a/safe-svg.php +++ b/safe-svg.php @@ -128,8 +128,14 @@ public function __construct() { $this->sanitizer = new Sanitizer(); $this->sanitizer->minify( true ); - add_action( 'init', array( $this, 'setup_blocks' ) ); + // 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( 'wp_handle_sideload_prefilter', array( $this, 'check_for_svg' ) ); add_filter( 'wp_handle_upload_prefilter', array( $this, 'check_for_svg' ) ); add_filter( 'wp_prepare_attachment_for_js', array( $this, 'fix_admin_preview' ), 10, 3 );