From 9bb1f266e942deb451aa969230f80d1f129b1efb Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Fri, 17 Jul 2026 14:32:40 -0300 Subject: [PATCH] fix(security): re-validate SSRF on every redirect hop in SafeHttpFetcher The shared fetcher guarded only the initial URL, then followed redirects without re-checking each hop, so a public page could 302 to an internal address. Follow redirects manually and run the SSRF guard on every hop; throw when the redirect cap is exceeded. Also hardens brand autofill and logo downloads. --- app/Services/Brand/SafeHttpFetcher.php | 40 +++++++++-- .../Services/Brand/SafeHttpFetcherTest.php | 70 +++++++++++++++++++ 2 files changed, 103 insertions(+), 7 deletions(-) create mode 100644 tests/Unit/Services/Brand/SafeHttpFetcherTest.php diff --git a/app/Services/Brand/SafeHttpFetcher.php b/app/Services/Brand/SafeHttpFetcher.php index 667146f6..3369b03d 100644 --- a/app/Services/Brand/SafeHttpFetcher.php +++ b/app/Services/Brand/SafeHttpFetcher.php @@ -8,6 +8,7 @@ use Illuminate\Http\Client\Response; use Illuminate\Support\Facades\Http; use RuntimeException; +use Symfony\Component\DomCrawler\UriResolver; /** * HTTP fetcher with SSRF protection, timeout, redirect cap and a branded user-agent. @@ -36,13 +37,38 @@ public function get(string $url): Response { $this->guardAgainstSsrf($url); - try { - $response = Http::timeout(self::TIMEOUT_SECONDS) - ->withUserAgent(self::USER_AGENT) - ->withOptions(['allow_redirects' => ['max' => self::MAX_REDIRECTS]]) - ->get($url); - } catch (ConnectionException $e) { - throw new RuntimeException(__('workspaces.create.autofill_errors.unreachable', ['reason' => $e->getMessage()])); + $currentUrl = $url; + + // Redirects are followed manually (allow_redirects disabled) so that every + // hop's Location target is re-validated against the SSRF guard before it is + // ever requested. A public page could otherwise 302 to an internal host and + // Guzzle's built-in redirect following would fetch it without re-checking. + for ($hop = 0; ; $hop++) { + try { + $response = Http::timeout(self::TIMEOUT_SECONDS) + ->withUserAgent(self::USER_AGENT) + ->withOptions(['allow_redirects' => false]) + ->get($currentUrl); + } catch (ConnectionException $e) { + throw new RuntimeException(__('workspaces.create.autofill_errors.unreachable', ['reason' => $e->getMessage()])); + } + + if (! $response->redirect() || $hop >= self::MAX_REDIRECTS) { + break; + } + + $location = $response->header('Location'); + + if ($location === '') { + break; + } + + $currentUrl = (string) UriResolver::resolve($location, $currentUrl); + $this->guardAgainstSsrf($currentUrl); + } + + if ($response->redirect()) { + throw new RuntimeException(__('workspaces.create.autofill_errors.unreachable', ['reason' => 'too many redirects'])); } if ($response->failed()) { diff --git a/tests/Unit/Services/Brand/SafeHttpFetcherTest.php b/tests/Unit/Services/Brand/SafeHttpFetcherTest.php new file mode 100644 index 00000000..d424d3c8 --- /dev/null +++ b/tests/Unit/Services/Brand/SafeHttpFetcherTest.php @@ -0,0 +1,70 @@ + Http::response('', 302, ['Location' => 'http://127.0.0.1/internal']), + 'http://127.0.0.1/internal' => Http::response('internal secret', 200), + ]); + + expect(app(SafeHttpFetcher::class)->tryGet('https://93.184.216.34/start'))->toBeNull(); + + Http::assertSent(fn ($request) => str_contains($request->url(), '93.184.216.34')); + Http::assertNotSent(fn ($request) => str_contains($request->url(), '127.0.0.1')); +}); + +test('follows a legitimate redirect from one public host to another', function () { + Http::fake([ + 'https://93.184.216.34/start' => Http::response('', 301, ['Location' => 'https://1.1.1.1/final']), + 'https://1.1.1.1/final' => Http::response('final body', 200), + ]); + + $response = app(SafeHttpFetcher::class)->get('https://93.184.216.34/start'); + + expect($response->status())->toBe(200) + ->and($response->body())->toBe('final body'); + + Http::assertSentInOrder([ + fn ($request) => str_contains($request->url(), '93.184.216.34'), + fn ($request) => str_contains($request->url(), '1.1.1.1'), + ]); +}); + +test('resolves a relative location header against the current url before guarding', function () { + Http::fake([ + 'https://93.184.216.34/start' => Http::response('', 302, ['Location' => '/final']), + 'https://93.184.216.34/final' => Http::response('final body', 200), + ]); + + $response = app(SafeHttpFetcher::class)->get('https://93.184.216.34/start'); + + expect($response->status())->toBe(200) + ->and($response->body())->toBe('final body'); +}); + +test('throws when a redirect chain exceeds the redirect cap', function () { + Http::fake([ + 'https://93.184.216.34/start' => Http::response('', 302, ['Location' => 'https://93.184.216.35/hop']), + 'https://93.184.216.35/hop' => Http::response('', 302, ['Location' => 'https://93.184.216.36/hop']), + 'https://93.184.216.36/hop' => Http::response('', 302, ['Location' => 'https://93.184.216.37/hop']), + 'https://93.184.216.37/hop' => Http::response('', 302, ['Location' => 'https://93.184.216.38/hop']), + ]); + + expect(fn () => app(SafeHttpFetcher::class)->get('https://93.184.216.34/start')) + ->toThrow(RuntimeException::class); + + expect(app(SafeHttpFetcher::class)->tryGet('https://93.184.216.34/start'))->toBeNull(); + + // The cap (3) is hit after following 3 redirects (4 requests); the next hop must never fire. + Http::assertNotSent(fn ($request) => str_contains($request->url(), '93.184.216.38')); +});