Make WordPress Core

Opened 2 years ago

Closed 19 months 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 23 months ago.
53626-change-media-upload-failure-string-no-patch.gif (335.5 KB) - added by iluy 20 months ago.
Showing the issue reported in 53626 in action
53626-change-media-upload-failure-string-with-patch.gif (331.0 KB) - added by iluy 20 months ago.
Showing the patch (53626.diff) applied and in action
53626.2.diff (2.1 KB) - added by iluy 20 months 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 20 months 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 20 months ago.

Download all attachments as: .zip

Change History (22)

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

  • Milestone changed from Awaiting Review to 5.8.1

@Presskopp
23 months ago

#3 @Presskopp
23 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
22 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
21 months 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
20 months ago

Showing the issue reported in 53626 in action

@iluy
20 months ago

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

#6 @iluy
20 months 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
20 months 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
20 months 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
20 months 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
20 months 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
19 months ago

  • Keywords needs-testing added

#10 @desrosj
19 months 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.


19 months ago

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


19 months ago

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


19 months ago

#14 @antpb
19 months ago

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

#15 @mikeschroder
19 months 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
19 months 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.