Skip to content

Commit

Permalink
Bail out gracefully on non-SVG files
Browse files Browse the repository at this point in the history
Bug: T229459
  • Loading branch information
MaxSem committed Aug 2, 2019
1 parent 577d18c commit 683bfad
Show file tree
Hide file tree
Showing 9 changed files with 75 additions and 9 deletions.
1 change: 1 addition & 0 deletions .phpcs.xml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
<exclude-pattern>node_modules/</exclude-pattern>
<exclude-pattern>var/</exclude-pattern>
<exclude-pattern>public/assets</exclude-pattern>
<exclude-pattern>.docker</exclude-pattern>

<rule ref="SlevomatCodingStandard.Files.TypeNameMatchesFileName">
<properties>
Expand Down
1 change: 1 addition & 0 deletions i18n/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"translate-button": "Translate",
"no-translations": "This file does not have any labels available for translation. Please pick another image.",
"not-found": "The file you requested could not be found. Please pick another image.",
"invalid-format": "Only SVG files are supported.",

"pick-another": "← pick another file",
"view-on-commons": "View on Commons",
Expand Down
1 change: 1 addition & 0 deletions i18n/qqq.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"translate-button": "Search button text.\n{{Identical|Translate}}",
"no-translations": "Error message displayed for SVGs that have nothing to translate.",
"not-found": "Error message displayed if the requested file could not be found.",
"invalid-format": "Error message displayed if the requested file is not an SVG image.",
"pick-another": "Label for backlink from the translate page to the search page.",
"view-on-commons": "Link to view the file on Commons. The whole phrase is linked.",
"opens-in-new-tab": "Tooltip text for links that will open in a new browser tab.",
Expand Down
1 change: 1 addition & 0 deletions public/assets/i18n/app/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"translate-button": "Translate",
"no-translations": "This file does not have any labels available for translation. Please pick another image.",
"not-found": "The file you requested could not be found. Please pick another image.",
"invalid-format": "Only SVG files are supported.",

"pick-another": "← pick another file",
"view-on-commons": "View on Commons",
Expand Down
1 change: 1 addition & 0 deletions public/assets/i18n/app/qqq.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"translate-button": "Search button text.\n{{Identical|Translate}}",
"no-translations": "Error message displayed for SVGs that have nothing to translate.",
"not-found": "Error message displayed if the requested file could not be found.",
"invalid-format": "Error message displayed if the requested file is not an SVG image.",
"pick-another": "Label for backlink from the translate page to the search page.",
"view-on-commons": "Link to view the file on Commons. The whole phrase is linked.",
"opens-in-new-tab": "Tooltip text for links that will open in a new browser tab.",
Expand Down
51 changes: 42 additions & 9 deletions src/Controller/TranslateController.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
namespace App\Controller;

use App\Exception\ImageNotFoundException;
use App\Exception\InvalidFormatException;
use App\Model\Svg\SvgFile;
use App\Model\Title;
use App\OOUI\TranslationsFieldset;
Expand Down Expand Up @@ -55,18 +56,15 @@ public function translate(

// Fetch the SVG from Commons.
try {
$this->assertFileType($normalizedFilename);
$path = $cache->getPath($filename);
} catch (ImageNotFoundException $exception) {
$notFoundMessage = $this->renderView(
'error_message.html.twig',
['msg_name' => 'not-found']
);
// Flash the message to show to the user under the search form.
$this->addFlash('search-errors', (string)$notFoundMessage);
// Also flash the failed filename so we can populate the search form.
$this->addFlash('search-redirect', $normalizedFilename);
return $this->redirectToRoute('home');
return $this->showError('not-found', $normalizedFilename);
} catch (InvalidFormatException $exception) {
return $this->showError('invalid-format', $normalizedFilename);
}


$svgFile = new SvgFile($path);

// If there are no strings to translate, tell the user.
Expand Down Expand Up @@ -227,4 +225,39 @@ public function updownload(
return $this->redirectToRoute('translate', ['filename' => $filename]);
}
}

/**
* Shows an error related to the selected file
*
* @param string $messageKey
* @param string $fileName
* @return Response
*/
private function showError(string $messageKey, string $fileName): Response
{
$message = $this->renderView(
'error_message.html.twig',
['msg_name' => $messageKey]
);
// Flash the message to show to the user under the search form.
$this->addFlash('search-errors', (string)$message);
// Also flash the failed filename so we can populate the search form.
$this->addFlash('search-redirect', $fileName);

return $this->redirectToRoute('home');
}

/**
* Throws an exception if the given filename is not of an SVG file.
* Exceptions are used to unify handling with other places that might encounter this problem.
*
* @param string $fileName
*/
private function assertFileType(string $fileName): void
{
$extension = mb_strtolower(pathinfo($fileName, PATHINFO_EXTENSION));
if ('svg' !== $extension) {
throw new InvalidFormatException($extension);
}
}
}
14 changes: 14 additions & 0 deletions src/Exception/InvalidFormatException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<?php
declare(strict_types = 1);

namespace App\Exception;

use Symfony\Component\HttpKernel\Exception\UnsupportedMediaTypeHttpException;

class InvalidFormatException extends UnsupportedMediaTypeHttpException
{
public function __construct(string $format)
{
parent::__construct("Files of type '$format' are not supported");
}
}
6 changes: 6 additions & 0 deletions src/Service/Retriever.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

namespace App\Service;

use App\Exception\InvalidFormatException;
use GuzzleHttp\Client;

/**
Expand Down Expand Up @@ -41,6 +42,11 @@ protected function httpGet(string $url): string
{
$client = new Client();
$response = $client->get($url);
$contentType = $response->getHeader('Content-Type');
$contentType = reset($contentType);
if ('image/svg+xml' !== $contentType) {
throw new InvalidFormatException($contentType);
}
return $response->getBody()->getContents();
}
}
8 changes: 8 additions & 0 deletions tests/Controller/TranslateControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,12 @@ public function testExists(): void
static::assertEquals(200, $response->getStatusCode());
static::assertContains('Test.svg', $crawler->filter('h1')->text());
}

public function testNonSvgHandling(): void
{
$client = static::createClient();
$client->request('GET', '/File:Test.jpg');
$response = $client->getResponse();
static::assertEquals(302, $response->getStatusCode());
}
}

0 comments on commit 683bfad

Please sign in to comment.