WordPress.org

Make WordPress Core

Opened 4 months ago

Last modified 9 days ago

#53626 new enhancement

Change Media upload failures string to not include "Security" messaging for each failure

Reported by: antpb Owned by:
Milestone: 5.8.2 Priority: normal
Severity: normal Version: 5.7
Component: Media Keywords: good-first-bug has-patch
Focuses: Cc:

Description

Currently when uploading a media item that is not supported, the default message claims that the reason it cannot upload is due to security reasons. This is not always true. We should expand on this to be either conditional to show security message on valid security risks, or just a general "this file type is not supported" message for all types uploaded. I lean more toward the general message.

Attachments (6)

53626.diff (2.0 KB) - added by Presskopp 3 months ago.
53626-change-media-upload-failure-string-no-patch.gif (335.5 KB) - added by iluy 10 days ago.
Showing the issue reported in 53626 in action
53626-change-media-upload-failure-string-with-patch.gif (331.0 KB) - added by iluy 10 days ago.
Showing the patch (53626.diff) applied and in action
53626.2.diff (2.1 KB) - added by iluy 10 days ago.
In case we decide to go with @peterwilsoncc 's suggestion... I went ahead and created a patch. Hope I'm not cluttering this ticket.
53626.2.2.diff (2.1 KB) - added by iluy 10 days ago.
In case we decide to go with @peterwilsoncc 's suggestion... I went ahead and created a patch. Hope I'm not cluttering this ticket. It might be necessary to coordinate with the Gutenberg team to update this string in their codebase as mentioned by @desrosj below.
53626.3.diff (6.1 KB) - added by peterwilsoncc 9 days ago.

Download all attachments as: .zip

Change History (14)

#1 @Presskopp
4 months ago

We already have a string "Sorry, this file type is not supported here." in

wp-includes/js/dist/media-utils.js:663

#2 @antpb
4 months ago

  • Milestone changed from Awaiting Review to 5.8.1

@Presskopp
3 months ago

#3 @Presskopp
3 months ago

  • Keywords has-patch added; needs-patch removed

I found 3 occurrences and changed them to the string which is already there

#4 @peterwilsoncc
3 months ago

The "not permitted for security reasons" version of the string also appears in Gutenberg, so an upstream PR to change that would be good.

"Sorry, this file type is not supported here." looks to be about someone trying to upload an incompatible file to a block (eg an image to the audio block).

For file types the user is prevented from uploading, I'm wondering if a more direct "Sorry, you are not allowed to upload this file type." is better. No security FUD if it's prevented because the site owner simply doesn't want to use a particular mime type on their site.

Additionally...

Is there a compelling reason this can't be held off to 5.9? Typically string changes are avoided as much as possible in minor releases to avoid the additional translation load on the polyglots.

If it is a must have, then I suggest going with 53626.diff in the 5.8 branch and creating a follow up ticket if you wish to go with a new string for 5.9.

#5 @desrosj
8 weeks ago

  • Milestone changed from 5.8.1 to 5.8.2

Ideally, once the desired string is found, the Gutenberg string is fixed at the same time. With 5.8.1 RC in less than 24 hours, I'm going to punt this to allow for more time.

@iluy
10 days ago

Showing the issue reported in 53626 in action

@iluy
10 days ago

Showing the patch (53626.diff) applied and in action

#6 @iluy
10 days ago

Not sure if a decision was made about the right string for this ticket @peterwilsoncc. I like your proposed version "Sorry, you are not allowed to upload this file type." but I'm not sure if I should make a patch. (?) I'm brand new to contributing.

Just adding that I was able to reproduce the issue reported by @antpb with the following settings:

  • Updated my local wordpress-svn copy to revision 51905 (14 Oct 2021)
  • Ran MAMP version 6.6 (1211) (on MacOS Catalina 10.15.7)
  • Tested the bug and applied the patch (53626.diff) running PHP version 7.4.21 AND version 8.0.8

GIF Animation Images attached to the ticket

@iluy
10 days ago

In case we decide to go with @peterwilsoncc 's suggestion... I went ahead and created a patch. Hope I'm not cluttering this ticket.

@iluy
10 days ago

In case we decide to go with @peterwilsoncc 's suggestion... I went ahead and created a patch. Hope I'm not cluttering this ticket. It might be necessary to coordinate with the Gutenberg team to update this string in their codebase as mentioned by @desrosj below.

@peterwilsoncc
9 days ago

#7 @peterwilsoncc
9 days ago

@iluy

Thank you for making a patch, you're always welcome to do so on any ticket and it will never be considered cluttering, I assure you! Let's see if we can get this committed and get a core contributor badge against your profile :)

In 53626.3.diff I've made a minor change to include a forth occurrence of the string in the wordpress-develop repository. Otherwise it's identical to 53626.2.2.diff.

#8 @iluy
9 days ago

@peterwilsoncc thanks for the feedback and the support words. :)

Hope we'll get this ticket committed soon. I'll keep looking for some more good first bugs in the coming days. Cheers!

Note: See TracTickets for help on using tickets.