From 5d6e5ffdd8d1239149bae1b2e3a2474ad9a6b9ea Mon Sep 17 00:00:00 2001 From: Bogdan Date: Mon, 7 Sep 2026 22:00:49 +0200 Subject: [PATCH] fix(HTTP): strip CRLF from DownloadResponse filename to prevent header injection Sanitize carriage return and newline characters from filename in DownloadResponse to prevent HTTP response splitting and invalid header values per RFC 7230. --- system/HTTP/DownloadResponse.php | 8 +++---- tests/system/HTTP/DownloadResponseTest.php | 24 +++++++++++++++++++++ user_guide_src/source/changelogs/v4.7.5.rst | 1 + 3 files changed, 29 insertions(+), 4 deletions(-) diff --git a/system/HTTP/DownloadResponse.php b/system/HTTP/DownloadResponse.php index b7ce79b8309a..997ab90529d5 100644 --- a/system/HTTP/DownloadResponse.php +++ b/system/HTTP/DownloadResponse.php @@ -71,7 +71,7 @@ public function __construct(string $filename, bool $setMime) { parent::__construct(config(App::class)); - $this->filename = $filename; + $this->filename = str_replace(["\r", "\n"], '', $filename); $this->setMime = $setMime; // Make sure the content type is either specified or detected @@ -113,7 +113,7 @@ public function setFilePath(string $filepath) */ public function setFileName(string $filename) { - $this->filename = $filename; + $this->filename = str_replace(["\r", "\n"], '', $filename); return $this; } @@ -161,8 +161,8 @@ private function setContentTypeByMimeType(): void */ private function getDownloadFileName(): string { - $filename = $this->filename; - $x = explode('.', $this->filename); + $filename = str_replace(["\r", "\n"], '', $this->filename); + $x = explode('.', $filename); $extension = end($x); /* It was reported that browsers on Android 2.1 (and possibly older as well) diff --git a/tests/system/HTTP/DownloadResponseTest.php b/tests/system/HTTP/DownloadResponseTest.php index d5fea35e380c..b0f34f2c1f4f 100644 --- a/tests/system/HTTP/DownloadResponseTest.php +++ b/tests/system/HTTP/DownloadResponseTest.php @@ -154,6 +154,30 @@ public function testDispositionInlineWithSetFileName(): void $this->assertSame('inline; filename="my\"quoted\"File.txt"; filename*=UTF-8\'\'my%22quoted%22File.txt', $response->getHeaderLine('Content-Disposition')); } + public function testContentDispositionSanitizesCRLF(): void + { + $response = new DownloadResponse("test\r\nfile.txt", false); + $response->buildHeaders(); + + $header = $response->getHeaderLine('Content-Disposition'); + $this->assertStringNotContainsString("\r", $header); + $this->assertStringNotContainsString("\n", $header); + $this->assertSame('attachment; filename="testfile.txt"; filename*=UTF-8\'\'testfile.txt', $header); + + $response = new DownloadResponse('report.pdf', false); + $response->setFileName("report\r\nSet-Cookie: evil=1\r\n.pdf"); + + // The filename must be sanitized to strip carriage returns and newlines, + // preventing HTTP response splitting / header injection without throwing an RFC 7230 error. + $response->buildHeaders(); + + $header = $response->getHeaderLine('Content-Disposition'); + $this->assertStringNotContainsString("\r", $header); + $this->assertStringNotContainsString("\n", $header); + $this->assertFalse($response->hasHeader('Set-Cookie')); + $this->assertSame('attachment; filename="reportSet-Cookie: evil=1.pdf"; filename*=UTF-8\'\'reportSet-Cookie%3A%20evil%3D1.pdf', $header); + } + public function testNoCache(): void { $response = new DownloadResponse('unit-test.txt', true); diff --git a/user_guide_src/source/changelogs/v4.7.5.rst b/user_guide_src/source/changelogs/v4.7.5.rst index f13a3e4b2b49..db1bc68d3875 100644 --- a/user_guide_src/source/changelogs/v4.7.5.rst +++ b/user_guide_src/source/changelogs/v4.7.5.rst @@ -43,6 +43,7 @@ Bugs Fixed - **Content Security Policy:** Fixed a bug where empty ``Content-Security-Policy``, ``Content-Security-Policy-Report-Only``, and ``Reporting-Endpoints`` response headers were generated when no corresponding values existed. - **Cookie:** Fixed a bug where ``Cookie`` instances created with ``raw: true`` allowed invalid characters in cookie values rejected by ``setrawcookie()``. - **Database:** Fixed a bug where rebuilding a SQLite3 table (e.g., ``Forge::dropColumn()``, ``Forge::modifyColumn()``, ``Forge::dropForeignKey()`` and ``Forge::dropPrimaryKey()``) corrupted the table names referenced by its foreign keys when ``DBPrefix`` was set. +- **DownloadResponse:** Fixed a bug where filenames containing carriage returns or newlines were not sanitized, potentially leading to HTTP response splitting (CRLF / header injection). - **Files:** Fixed a bug where ``File::move()`` and ``UploadedFile::move()`` set executable and overly permissive file permissions (``0777 & ~umask()`` instead of ``0666 & ~umask()``), and ``UploadedFile::move()`` targeted the parent directory instead of the destination file for ``chmod()``. - **Helpers:** Fixed a bug where ``get_dir_file_info()`` returned incomplete entries for subdirectories and missing files instead of omitting them. - **Honeypot:** Fixed a bug where bot detection returned an HTTP 500 response instead of 403 (Forbidden).