[4.2] Return messages from the media helper upload on failure

[obuisard]

When coming back from the Media Helper 'canUpload' function, error messages are lost.

Pull Request for Issue #38068.

Summary of Changes

When a file is denied upload, the message 'Unable to upload file.' is returned.
In the case of SVGs, for instance, a file can be denied for security reasons but no feedback is returned to the user, although all messages are present in the Media Helper.

This PR uses the last entry returned by the Media Helper rather than the default message.

Testing Instructions

Upload a file that is not part of the allowed file types, a file that is too large or an SVG file that is complex and contains suspicious tags (like DOCTYPE). Or remove SVG from the allowed extensions... Anything that can fail the upload of an image or SVG file.

Actual result BEFORE applying this Pull Request

The error message is 'Unable to upload file.'

Expected result AFTER applying this Pull Request

In the case of a SVG file, the message returned should be 'Possible IE XSS Attack found.'

Documentation Changes Required

None

[brianteeman]

I like the idea of this PR - a lot

It is not a replacement however to resolving the issue of why svg can no longer be uploaded

[richard67]

@obuisard Please fix the code style errors reported here: https://ci.joomla.org/joomla/joomla-cms/57037/1/6

[laoneo]

Getting the exception from the message queue is problematic, what when a plugin added a message before to the queue. Instead of I would add an optional parameter to the canUpload function to throw an exception which is by default false. Then you are on the safe side and doesn't have to deal with the apps message list.

[zero-24]

And I would say the message should be limited to Superusers as they are the only one who could do anything about it. This way we also not give a to specific error messages to the possible public user.

[zero-24]

Maybe its worth adding 'please contact the site admin for a detailed error analyse' in that case too.

[Fedik]

Sorry the fix is incorrect.

The MessageQueue already sent with response:

joomla-cms/administrator/components/com_media/src/Controller/ApiController.php

Lines 309 to 310 in dd91072

	// Send the data
	echo new JsonResponse($data);

By default JsonResponse include MessageQueue, it just need to be rendered on client side.

joomla-cms/libraries/src/Response/JsonResponse.php

Lines 74 to 93 in dd91072

	// Get the message queue if requested and available
	$app = Factory::getApplication();
	if (!$ignoreMessages && $app !== null && \is_callable(array($app, 'getMessageQueue'))) {
	$messages = $app->getMessageQueue();
	// Build the sorted messages list
	if (\is_array($messages) && \count($messages)) {
	foreach ($messages as $message) {
	if (isset($message['type']) && isset($message['message'])) {
	$lists[$message['type']][] = $message['message'];
	}
	}
	}
	// If messages exist add them to the output
	if (isset($lists) && \is_array($lists)) {
	$this->messages = $lists;
	}
	}

@zero-24 I do not think that need any restriction here, maybe a better message string.
Insetad of technical Possible IE XSS Attack found., need something more clear, kind of This file looks suspicious, therefore cannot be uploaded, or something similar.

[obuisard]

I like the idea of this PR - a lot

It is not a replacement however to resolving the issue of why svg can no longer be uploaded

Indeed Brian, but I thought this was the first step that could be handled as a fix in 4.2 so people don't wait until 4.3 for the full resolution.

[obuisard]

Getting the exception from the message queue is problematic, what when a plugin added a message before to the queue. Instead of I would add an optional parameter to the canUpload function to throw an exception which is by default false. Then you are on the safe side and doesn't have to deal with the apps message list.

I totally agree. But I wanted a working fix available as soon as possible without modifying any function's signature.

[obuisard]

And I would say the message should be limited to Superusers as they are the only one who could do anything about it. This way we also not give a to specific error messages to the possible public user.

I disagree. All users should be able to know that a file has a wrong extension type other than just a message telling them the file cannot be upload.

[obuisard]

Sorry the fix is incorrect.

The MessageQueue already sent with response:

joomla-cms/administrator/components/com_media/src/Controller/ApiController.php

Lines 309 to 310 in dd91072

	// Send the data
	echo new JsonResponse($data);

By default JsonResponse include MessageQueue, it just need to be rendered on client side.

joomla-cms/libraries/src/Response/JsonResponse.php

Lines 74 to 93 in dd91072

	// Get the message queue if requested and available
	$app = Factory::getApplication();
	if (!$ignoreMessages && $app !== null && \is_callable(array($app, 'getMessageQueue'))) {
	$messages = $app->getMessageQueue();
	// Build the sorted messages list
	if (\is_array($messages) && \count($messages)) {
	foreach ($messages as $message) {
	if (isset($message['type']) && isset($message['message'])) {
	$lists[$message['type']][] = $message['message'];
	}
	}
	}
	// If messages exist add them to the output
	if (isset($lists) && \is_array($lists)) {
	$this->messages = $lists;
	}
	}

Great if there is a better way to do this! Do you mind providing your solution? Thank you @Fedik

@zero-24 I do not think that need any restriction here, maybe a better message string. Insetad of technical Possible IE XSS Attack found., need something more clear, kind of This file looks suspicious, therefore cannot be uploaded, or something similar.

Totally agree.

[Fedik]

Do you mind providing your solution?

I will look

[obuisard]

Do you mind providing your solution?

I will look

Super! Thank you Fedir

[Fedik]

please test #38536

[laoneo]

Closing this in favor of #38536. Thanks @obuisard for bringing this issue on the table.