Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#53626 closed enhancement (fixed)

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

Reported by: antpb's profile antpb Owned by: antpb's profile antpb
Milestone: 5.9 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 years ago.
53626-change-media-upload-failure-string-no-patch.gif (335.5 KB) - added by iluy 3 years ago.
Showing the issue reported in 53626 in action
53626-change-media-upload-failure-string-with-patch.gif (331.0 KB) - added by iluy 3 years ago.
Showing the patch (53626.diff) applied and in action
53626.2.diff (2.1 KB) - added by iluy 3 years 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 3 years 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 3 years ago.

Download all attachments as: .zip

Change History (22)

#1 @Presskopp
3 years 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
3 years ago

  • Milestone changed from Awaiting Review to 5.8.1

@Presskopp
3 years ago

#3 @Presskopp
3 years 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 years 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
3 years 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
3 years ago

Showing the issue reported in 53626 in action

@iluy
3 years ago

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

#6 @iluy
3 years 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
3 years 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
3 years 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.

#7 @peterwilsoncc
3 years 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
3 years 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!

#9 @circlecube
3 years ago

  • Keywords needs-testing added

#10 @desrosj
3 years ago

  • Keywords needs-testing removed
  • Milestone changed from 5.8.2 to 5.9

This is looking good, but it still needs a pull request accompanying it for Gutenberg.

Since 5.8.1 RC is later today, I'm going to punt this to 5.9 so both can be merged at the same time.

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


3 years ago

#14 @antpb
3 years ago

I opened a PR for the Gutenberg side changes: https://github.com/WordPress/gutenberg/pull/36226

#15 @kirasong
3 years ago

Tossed a review along with @joedolson's + merged over on the Gutenberg side to get it included in the upcoming 11.9 RC.

#16 @antpb
3 years ago

  • Owner set to antpb
  • Resolution set to fixed
  • Status changed from new to closed

In 52032:

Media: Remove security messaging in media upload failures.

Previously, when uploading a media item type that is not supported, the default error message claims that the reason it cannot upload is due to security reasons. This is not always true. Now the warning says that the type is not allowed, which is always true.

Props antpb, Presskopp, peterwilsoncc, desrosj, iluy, circlecube, mikeschroder.
Fixes #53626.

Note: See TracTickets for help on using tickets.