From 0403595045e7ab13862d34ef16a8f8c21fb5d1c1 Mon Sep 17 00:00:00 2001 From: David Losada Carballo Date: Wed, 18 Feb 2026 19:21:22 +0100 Subject: [PATCH] fix: only apply %25 URL escaping for encoded-name storage lookups The blanket str_replace('%', '%25') in formatOFilename was applied to all media URLs regardless of how the file was found. This corrupts URLs for files found by their raw name (e.g. S3 files with spaces), where the storage driver already returns a properly encoded URL. Now the %25 escaping is only applied when the file was resolved via an encoded-name fallback (2nd/3rd try), where the object name contains literal percent characters that browsers would otherwise decode. Adds unit tests covering all lookup strategies and the priority logic. --- .../V5/Http/Resources/Media/MediaResource.php | 81 ++++--- .../Resources/Media/MediaResourceTest.php | 198 ++++++++++++++++++ 2 files changed, 248 insertions(+), 31 deletions(-) create mode 100644 tests/Unit/Modules/V5/Http/Resources/Media/MediaResourceTest.php diff --git a/src/Ushahidi/Modules/V5/Http/Resources/Media/MediaResource.php b/src/Ushahidi/Modules/V5/Http/Resources/Media/MediaResource.php index a70cb564ad..9d93f7a36c 100644 --- a/src/Ushahidi/Modules/V5/Http/Resources/Media/MediaResource.php +++ b/src/Ushahidi/Modules/V5/Http/Resources/Media/MediaResource.php @@ -64,56 +64,75 @@ protected function formatOFilename($value) if (empty($value)) { return null; } - $url = $this->urlOFilename($value); - if ($url === null) { - return null; - } - // Substitute % for %25 to avoid double-encoding issues in browsers - $url = str_replace('%', '%25', $url); - return $url; + return $this->resolveMediaUrl($value); } - protected function urlOFilename($value) + /** + * Resolve the public URL for a media file, trying multiple name-encoding + * strategies to account for how the file may have been stored historically. + * + * Given a DB value like "uploads/some file.jpg", we try: + * + * 1st) Raw name: "uploads/some file.jpg" + * → Object in storage is literally named "some file.jpg" + * → S3 URL: https://s3.../uploads/some%20file.jpg (S3 encodes it) + * → Return as-is. The URL is already correct. + * + * 2nd) Single-encoded: "uploads/some%20file.jpg" + * → Object in storage is literally named "some%20file.jpg" + * (old code used rawurlencode before saving to storage) + * → CDN URL: https://cdn.../uploads/some%20file.jpg + * → Problem: browser decodes %20 → requests "some file.jpg" → 404 + * → Fix: escape % → %25 so URL becomes .../some%2520file.jpg + * browser decodes %25 → "%" and requests "some%20file.jpg" ✓ + * + * 3rd) Double-encoded: "uploads/some%2520file.jpg" + * → Object in storage is literally named "some%2520file.jpg" + * → Same browser-decoding issue, same %25 fix applied. + * + * @param string $value The o_filename value from the database + * @return string|null The public URL, or null if not found + */ + protected function resolveMediaUrl($value) { - // Removes path from image file name, encodes the filename, and joins the path and filename together $url_path = explode("/", $value); $filename = array_pop($url_path); - array_push($url_path, $filename); - $path = implode("/", $url_path); + // 1st try: raw filename as stored in the DB + // e.g. "some file.jpg" → look for object "some file.jpg" + $path = implode("/", array_merge($url_path, [$filename])); $result = Storage::url($path); - // If the result is a non-empty string if (is_string($result) && !empty($result)) { - // Success + // URL from storage is already properly encoded (e.g. S3 returns %20 for spaces) return $result; } - // For some time, we would store files with - // URL-encoded names. Try that as a fallback - $filename = rawurlencode((array_pop($url_path))); - array_push($url_path, $filename); - $path = implode("/", $url_path); - + // 2nd try: single rawurlencode — for files stored with encoded names + // e.g. "some file.jpg" → rawurlencode → "some%20file.jpg" + // look for object literally named "some%20file.jpg" + $encodedOnce = rawurlencode($filename); + $path = implode("/", array_merge($url_path, [$encodedOnce])); $result = Storage::url($path); - // If the result is a non-empty string if (is_string($result) && !empty($result)) { - // Success - return $result; + // The object name has literal "%" chars (e.g. "some%20file.jpg"). + // The CDN URL contains those raw %, which browsers would decode. + // Escape % → %25 so browsers preserve the literal percent sign. + // e.g. ".../some%20file.jpg" → ".../some%2520file.jpg" + // browser decodes %25→% and correctly requests "some%20file.jpg" + return str_replace('%', '%25', $result); } - // Try one more round of urlencoding, just in case - $filename = rawurlencode((array_pop($url_path))); - array_push($url_path, $filename); - $path = implode("/", $url_path); - + // 3rd try: double rawurlencode — for doubly-encoded legacy names + // e.g. "some%20file.jpg" → rawurlencode → "some%2520file.jpg" + // look for object literally named "some%2520file.jpg" + $encodedTwice = rawurlencode($encodedOnce); + $path = implode("/", array_merge($url_path, [$encodedTwice])); $result = Storage::url($path); - // If the result is a non-empty string if (is_string($result) && !empty($result)) { - // Success - return $result; + // Same %-escaping logic as the 2nd try + return str_replace('%', '%25', $result); } - // If we reach here, we failed to find the file return null; } } diff --git a/tests/Unit/Modules/V5/Http/Resources/Media/MediaResourceTest.php b/tests/Unit/Modules/V5/Http/Resources/Media/MediaResourceTest.php new file mode 100644 index 0000000000..e26c45a659 --- /dev/null +++ b/tests/Unit/Modules/V5/Http/Resources/Media/MediaResourceTest.php @@ -0,0 +1,198 @@ +setAccessible(true); + return $method->invoke($resource, $value); + } + + /** + * Helper: call the protected formatOFilename method via reflection. + */ + private function callFormatOFilename($value): ?string + { + $resource = new MediaResource((object) []); + $method = new \ReflectionMethod($resource, 'formatOFilename'); + $method->setAccessible(true); + return $method->invoke($resource, $value); + } + + /** + * Mock Storage::url() so it returns a URL only for the given $knownPaths. + * + * @param array $knownPaths map of storage path => returned URL + */ + private function mockStorageUrl(array $knownPaths): void + { + Storage::shouldReceive('url') + ->andReturnUsing(function (string $path) use ($knownPaths) { + return $knownPaths[$path] ?? null; + }); + } + + // --------------------------------------------------------------- + // formatOFilename: empty / null handling + // --------------------------------------------------------------- + + public function testFormatOFilenameReturnsNullForEmptyString(): void + { + $this->assertNull($this->callFormatOFilename('')); + } + + public function testFormatOFilenameReturnsNullForNull(): void + { + $this->assertNull($this->callFormatOFilename(null)); + } + + // --------------------------------------------------------------- + // 1st try: raw filename found — URL returned as-is + // --------------------------------------------------------------- + + public function testRawFilenameFoundReturnsUrlAsIs(): void + { + // File "uploads/photo.jpg" exists with its raw name in storage. + // Storage returns a properly-encoded URL — no % escaping needed. + $this->mockStorageUrl([ + 'uploads/photo.jpg' => 'https://s3.example.com/uploads/photo.jpg', + ]); + + $result = $this->callResolveMediaUrl('uploads/photo.jpg'); + + $this->assertSame('https://s3.example.com/uploads/photo.jpg', $result); + } + + public function testRawFilenameWithSpacesFoundReturnsUrlAsIs(): void + { + // File "uploads/some file.jpg" exists with spaces in its name. + // S3 returns the URL with %20 already encoded — no extra escaping. + $this->mockStorageUrl([ + 'uploads/some file.jpg' => 'https://s3.example.com/uploads/some%20file.jpg', + ]); + + $result = $this->callResolveMediaUrl('uploads/some file.jpg'); + + // The %20 from S3 must NOT be escaped to %2520 + $this->assertSame('https://s3.example.com/uploads/some%20file.jpg', $result); + } + + // --------------------------------------------------------------- + // 2nd try: single-encoded name found — % escaped to %25 + // --------------------------------------------------------------- + + public function testSingleEncodedFallbackEscapesPercent(): void + { + // DB has "uploads/some file.jpg" but in storage the object is literally + // named "some%20file.jpg" (old code used rawurlencode before saving). + // 1st try ("uploads/some file.jpg") → not found (null). + // 2nd try ("uploads/some%20file.jpg") → found. + $this->mockStorageUrl([ + 'uploads/some%20file.jpg' => 'https://cdn.example.com/uploads/some%20file.jpg', + ]); + + $result = $this->callResolveMediaUrl('uploads/some file.jpg'); + + // The % must be escaped to %25 so browsers don't decode %20 as a space. + // "some%20file.jpg" → "some%2520file.jpg" + $this->assertSame('https://cdn.example.com/uploads/some%2520file.jpg', $result); + } + + // --------------------------------------------------------------- + // 3rd try: double-encoded name found — % escaped to %25 + // --------------------------------------------------------------- + + public function testDoubleEncodedFallbackEscapesPercent(): void + { + // DB has "uploads/some file.jpg" but in storage the object is literally + // named "some%2520file.jpg" (doubly encoded). + // 1st try ("uploads/some file.jpg") → not found. + // 2nd try ("uploads/some%20file.jpg") → not found. + // 3rd try ("uploads/some%2520file.jpg") → found. + $this->mockStorageUrl([ + 'uploads/some%2520file.jpg' => 'https://cdn.example.com/uploads/some%2520file.jpg', + ]); + + $result = $this->callResolveMediaUrl('uploads/some file.jpg'); + + // % escaped to %25 in the returned URL + $this->assertSame('https://cdn.example.com/uploads/some%252520file.jpg', $result); + } + + // --------------------------------------------------------------- + // Not found in any strategy — returns null + // --------------------------------------------------------------- + + public function testReturnsNullWhenFileNotFoundInAnyStrategy(): void + { + // Storage returns null for every path tried + $this->mockStorageUrl([]); + + $result = $this->callResolveMediaUrl('uploads/missing.jpg'); + + $this->assertNull($result); + } + + // --------------------------------------------------------------- + // Simple filenames (no spaces or special chars) + // --------------------------------------------------------------- + + public function testSimpleFilenameNoSpecialChars(): void + { + // A plain filename with no encoding concerns. + // rawurlencode("photo.jpg") === "photo.jpg", so 1st and 2nd try + // resolve to the same path. The 1st try should match. + $this->mockStorageUrl([ + 'uploads/photo.jpg' => 'https://s3.example.com/uploads/photo.jpg', + ]); + + $result = $this->callResolveMediaUrl('uploads/photo.jpg'); + + $this->assertSame('https://s3.example.com/uploads/photo.jpg', $result); + } + + // --------------------------------------------------------------- + // Deeply nested paths + // --------------------------------------------------------------- + + public function testNestedDirectoryPathPreserved(): void + { + $this->mockStorageUrl([ + 'a/b/c/photo.jpg' => 'https://s3.example.com/a/b/c/photo.jpg', + ]); + + $result = $this->callResolveMediaUrl('a/b/c/photo.jpg'); + + $this->assertSame('https://s3.example.com/a/b/c/photo.jpg', $result); + } + + // --------------------------------------------------------------- + // Priority: 1st try wins even if 2nd would also match + // --------------------------------------------------------------- + + public function testFirstTryTakesPriorityOverSecond(): void + { + // Both raw and encoded paths exist. 1st try should win, + // and the URL should NOT have % escaping applied. + $this->mockStorageUrl([ + 'uploads/some file.jpg' => 'https://s3.example.com/uploads/some%20file.jpg', + 'uploads/some%20file.jpg' => 'https://cdn.example.com/uploads/some%20file.jpg', + ]); + + $result = $this->callResolveMediaUrl('uploads/some file.jpg'); + + // 1st try URL returned as-is (no %25 escaping) + $this->assertSame('https://s3.example.com/uploads/some%20file.jpg', $result); + } +}