Skip to content
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion public/theme/skeleton/partials/_image.twig
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{% if image %}
<figure>
<a href="{{ record|image }}">
<img src="{{ thumbnail(record, 1368, 1026) }}" alt="{{ (record|image).alt|default(record|title) }}">
<img src="{{ thumbnail(record, 1368, 1026, quality = 70, format = 'avif') }}" alt="{{ (record|image).alt|default(record|title) }}">
</a>
{% if image.alt|default() %}
<figcaption>{{ image.alt }}</figcaption>
Expand Down
86 changes: 63 additions & 23 deletions src/Controller/ImageController.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@

class ImageController
{
private const SUPPORTED_FORMATS = ['jpg', 'webp', 'png', 'gif', 'avif'];

private Server $server;

/**
Expand All @@ -45,17 +47,20 @@ public function thumbnail(Request $request, string $paramString, string $filenam
return $this->sendErrorImage();
}

$this->parseParameters($paramString);
$urlFilename = $filename;
$sourceFilename = $this->parseFormatFromFilename($filename);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Direct usage of the filename will cause a regression into a security incident (#3661) we had in the past, namely a path traversal. Paths can only be used after a canonicalize call.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The filename is still used directly without sanitation, this needs to be resolved first.


try {
$filename = PathCanonicalize::canonicalize($this->getPath($request), $filename, true);
$sourceFilename = PathCanonicalize::canonicalize($this->getPath($request), $sourceFilename, true);
} catch (Exception) {
return $this->sendErrorImage();
}

$this->parseParameters($paramString);
$this->createServer($request);
$this->saveThumb($request, $filename);
$this->saveThumb($request, $sourceFilename, $urlFilename);

return $this->buildResponse($request, $filename);
return $this->buildResponse($request, $sourceFilename);
}

private function createServer(Request $request): void
Expand All @@ -82,7 +87,19 @@ private function getPath(Request $request, ?string $path = null, bool $absolute
return $this->config->getPath($path, $absolute, $additional);
}

private function saveThumb(Request $request, string $filename): void
private function parseFormatFromFilename(string $filename): string
{
$ext = mb_strtolower(pathinfo($filename, PATHINFO_EXTENSION));
if ($this->isSupportedFormat($ext) && pathinfo(pathinfo($filename, PATHINFO_FILENAME), PATHINFO_EXTENSION) !== '') {
$this->parameters['fm'] = $ext;

return mb_substr($filename, 0, -(mb_strlen($ext) + 1));
}

return $filename;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, the goal is to retrieve the last part of the path here. Why not just explode on ., and validate if the match is one of the supported extensions?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not able to allow maintainers to edit the PR ???
I switch the PR to 6.2 and fix both issues.

}

private function saveThumb(Request $request, string $filename, string $urlFilename = ''): void
{
if (! $this->config->get('general/thumbnails/save_files', true)) {
return;
Expand All @@ -95,7 +112,7 @@ private function saveThumb(Request $request, string $filename): void
$thumbPath = Path::join(
$this->getPath($request, 'thumbs'),
$this->parameterPath(),
$filename
$urlFilename ?: $filename
);

try {
Expand Down Expand Up @@ -162,21 +179,38 @@ private function parseParameters(string $paramString): void
$this->parameters = [
'w' => (isset($raw[0]) && is_numeric($raw[0])) ? (int) $raw[0] : 400,
'h' => (isset($raw[1]) && is_numeric($raw[1])) ? (int) $raw[1] : 300,
'fit' => $raw[2] ?? $this->config->get('general/thumbnails/default_cropping', 'default'),
Comment thread
kouz75 marked this conversation as resolved.
'fm' => '',
'fit' => '',
'location' => 'files',
'q' => (! empty($raw[2]) && 0 <= $raw[2] && $raw[2] <= 100) ? (int) $raw[2] : 80,
'q' => 80,
];

if (isset($raw[4])) {
$this->parameters['fit'] = $this->parseFit($raw[3]);
$this->parameters['location'] = $raw[4];
} elseif (isset($raw[3])) {
$possibleFit = $this->parseFit($raw[3]);
$remaining = array_values(array_filter(
array_slice($raw, 2),
static fn (int|string $value): bool => $value !== ''
));

if (isset($remaining[0]) && is_numeric($remaining[0]) && 0 <= (int) $remaining[0] && (int) $remaining[0] <= 100) {
$this->parameters['q'] = (int) array_shift($remaining);
}

foreach ($remaining as $token) {
$token = (string) $token;
$normalizedToken = mb_strtolower($token);

if ($this->parameters['fm'] === '' && $this->isSupportedFormat($normalizedToken)) {
$this->parameters['fm'] = $normalizedToken;
continue;
}

$fit = $this->parseFit($normalizedToken);
if ($this->testFit($fit)) {
$this->parameters['fit'] = $fit;
continue;
}

if ($this->testFit($possibleFit)) {
$this->parameters['fit'] = $possibleFit;
} else {
$this->parameters['location'] = $raw[3];
if ($this->parameters['location'] === 'files') {
$this->parameters['location'] = $token;
}
}
}
Expand All @@ -203,6 +237,11 @@ private function testFit(string $fit): bool
return (bool) preg_match('/^(contain|max|fill|stretch|crop)(-.+)?/', $fit);
}

private function isSupportedFormat(string $format): bool
{
return in_array($format, self::SUPPORTED_FORMATS, true);
}

public function parseFit(string $fit): string
{
return match ($fit) {
Expand All @@ -217,14 +256,15 @@ public function parseFit(string $fit): string

private function parameterPath(): string
{
return sprintf(
'%d_%d_%d_%s_%s',
$parts = array_filter([
$this->parameters['w'] ?? 0,
$this->parameters['h'] ?? 0,
$this->parameters['q'] ?? 0,
$this->parameters['fit'] ?? '',
$this->parameters['location'] ?? ''
);
$this->parameters['q'] ?? 80,
$this->parameters['fit'] ?? null,
$this->parameters['location'] ?? 'files',
], fn (int|string|null $v): bool => $v !== null && $v !== '' && $v !== 0);

return implode('×', $parts);
}

public function sendErrorImage(): Response
Expand Down
4 changes: 2 additions & 2 deletions src/Twig/ImageExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -102,11 +102,11 @@ public function showImage($image, ?int $width = null, ?int $height = null, ?bool
/**
* @param ImageField|array|string $image
*/
public function thumbnail($image, ?int $width = null, ?int $height = null, ?string $location = null, ?string $path = null, ?string $fit = null, ?int $quality = null): string
public function thumbnail($image, ?int $width = null, ?int $height = null, ?string $location = null, ?string $path = null, ?string $fit = null, ?int $quality = null, ?string $format = null): string
{
$filename = $this->getFilename($image, true);

return $this->thumbnailHelper->path($filename, $width, $height, $location, $path, $fit, $quality);
return $this->thumbnailHelper->path($filename, $width, $height, $location, $path, $fit, $quality, $format);
}

/**
Expand Down
10 changes: 6 additions & 4 deletions src/Utils/ThumbnailHelper.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ private function parameters(?int $width = null, ?int $height = null, ?string $fi
$height = 10000;
}

if ($location === 'files') {
$location = null;
if ($location === null) {
$location = 'files';
}

if (! $quality && $this->config instanceof Config) {
Expand All @@ -38,7 +38,7 @@ private function parameters(?int $width = null, ?int $height = null, ?string $fi
return implode('×', array_filter([$width, $height, $quality, $fit, $location]));
}

public function path(?string $filename = null, ?int $width = null, ?int $height = null, ?string $location = null, ?string $path = null, ?string $fit = null, ?int $quality = null): string
public function path(?string $filename = null, ?int $width = null, ?int $height = null, ?string $location = null, ?string $path = null, ?string $fit = null, ?int $quality = null, ?string $format = null): string
{
if (! $filename) {
return '/assets/images/placeholder.png';
Expand All @@ -47,7 +47,9 @@ public function path(?string $filename = null, ?int $width = null, ?int $height
if ($path) {
$filename = $path . '/' . $filename;
}

if ($format) {
$filename .= '.' . $format;
}
$paramString = $this->parameters($width, $height, $fit, $location, $quality);
$filename = Str::ensureStartsWith($filename, '/');

Expand Down
Loading