Re-check the SSRF guard on every redirect hop and cover icon URLs - #1599
Merged
Conversation
The website-lookup endpoint validated only the initial URL, then let Guzzle follow redirects unchecked. A public URL that answered with a Location pointing at an internal address was fetched and its body returned. The add/edit item icon fetch used file_get_contents with no address check at all. Both now go through a shared App\Helpers\SafeUrlFetcher that: - allows only http(s) URLs - resolves the host (A and AAAA) and refuses any private or reserved address, including IPv4-mapped IPv6 forms - pins the checked address and the URL's actual port via CURLOPT_RESOLVE - follows redirects itself, up to five hops, running the same guard on every Location before requesting it ALLOW_INTERNAL_REQUESTS keeps its meaning and is now read through config so config caching works. Tests use Guzzle's MockHandler so nothing touches the network. Reported by Kashish Topiwala. Icon URL guard based on PR #1590 by Bunlong Heng.
KodeStar
force-pushed
the
fix/ssrf-redirect-and-icon
branch
from
September 5, 2026 15:09
2883aeb to
16a43f8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Two server-side fetches of user-supplied URLs could reach internal services on a default install (no password on the seeded user):
GET /items/websitelookup/{url}checked the initial URL against the private/reserved IP list, but the Guzzle client then followed redirects with no further check. A public URL that 302s tohttp://127.0.0.1/orhttp://169.254.169.254/was fetched and its body returned. Reported privately by Kashish Topiwala, verified against the official image.file_get_contentsand had no address check at all. Reported in fix: validate icon URL to prevent server-side request forgery (SSRF) #1590 by @bunlongheng.Fix
Both paths now use a shared
App\Helpers\SafeUrlFetcher:httpandhttpsURLs are fetched::ffff:127.0.0.1) is unwrapped and checked as IPv4CURLOPT_RESOLVEon the URL's actual port (previously only 80 and 443 were pinned)Locationgoes through the full guard before it is requestedLegitimate redirects (http to https, apex to www) still work, so website lookup behaviour for real sites is unchanged.
ALLOW_INTERNAL_REQUESTS=truestill disables the address check, and is now read viaconfig('app.allow_internal_requests')so it survivesconfig:cache.Why not merge #1590 as-is
The approach was right, but the branch's tests all fail in CI: the "public IP" test does a real
file_get_contentsto 8.8.8.8 and times out, and the rejection tests expect a 422 from a non-JSON POST, which Laravel answers with a 302. It also disabled redirects for icons entirely, which would break icons served behind an http to https redirect. This PR keeps the guard idea, extends it to the website lookup, and replaces the tests with mocked-transport ones. Bunlong is credited in the commit message.Tests
tests/Unit/SafeUrlFetcherTest.php: address classification, scheme rejection, port pinning, redirect following, redirect-to-private blocked with the second request never sent, relative Location resolution, redirect loop cap, opt-out env.tests/Feature/WebsiteLookupSsrfTest.php: direct private 403, redirect-to-private 403 with body not leaked, public and redirected-public 200.tests/Feature/IconUrlSsrfTest.php: private/loopback/metadata/IPv6 icon URLs rejected before any request, redirect-to-private rejected, public icon stored, non-image and failed download rejected, opt-out env.All tests use Guzzle's
MockHandler; nothing hits the network. Full suite: 153 passed, 1 skipped (pre-existing).Also verified with a real client through
artisan tinker: httpbin redirect-to loopback and to the metadata address both throwBlockedUrlExceptionbefore the second request;http://github.com/follows its redirect and returns 200.Closes #1590