From 7e8ef628d93f6f7c8a5d1b24b44359674cf4e8f5 Mon Sep 17 00:00:00 2001 From: Alexandre Alapetite Date: Tue, 13 Oct 2026 17:50:38 +0000 Subject: [PATCH] Better doc and log proxy cases (#9436) Fix https://github.com/FreshRSS/FreshRSS/issues/9423 And strip redundant proxy scheme. And add a few more unit tests. --- app/Controllers/feedController.php | 1 + app/Controllers/subscriptionController.php | 1 + app/Utils/httpUtil.php | 5 ++- docs/en/admins/09_AccessControl.md | 3 ++ tests/app/Utils/httpUtilTest.php | 46 ++++++++++++++++++++++ 5 files changed, 55 insertions(+), 1 deletion(-) diff --git a/app/Controllers/feedController.php b/app/Controllers/feedController.php index 51fe49458..97f90388f 100644 --- a/app/Controllers/feedController.php +++ b/app/Controllers/feedController.php @@ -187,6 +187,7 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController { $max_redirs = Minz_Request::paramInt('curl_params_redirects'); $useragent = Minz_Request::paramString('curl_params_useragent', plaintext: true); $proxy_address = Minz_Request::paramString('curl_params', plaintext: true); + $proxy_address = preg_replace('#^.*://#', '', $proxy_address) ?? $proxy_address; $proxy_type = Minz_Request::paramIntNull('proxy_type'); $request_method = Minz_Request::paramString('curl_method', plaintext: true); $request_fields = Minz_Request::paramString('curl_fields', plaintext: true); diff --git a/app/Controllers/subscriptionController.php b/app/Controllers/subscriptionController.php index 4c15ee19e..fb49c9b9f 100644 --- a/app/Controllers/subscriptionController.php +++ b/app/Controllers/subscriptionController.php @@ -174,6 +174,7 @@ class FreshRSS_subscription_Controller extends FreshRSS_ActionController { $max_redirs = Minz_Request::paramInt('curl_params_redirects'); $useragent = Minz_Request::paramString('curl_params_useragent', plaintext: true); $proxy_address = Minz_Request::paramString('curl_params', plaintext: true); + $proxy_address = preg_replace('#^.*://#', '', $proxy_address) ?? $proxy_address; $proxy_type = Minz_Request::paramIntNull('proxy_type'); $request_method = Minz_Request::paramString('curl_method', plaintext: true); $request_fields = Minz_Request::paramString('curl_fields', plaintext: true); diff --git a/app/Utils/httpUtil.php b/app/Utils/httpUtil.php index cb88b1a5a..72eb54013 100644 --- a/app/Utils/httpUtil.php +++ b/app/Utils/httpUtil.php @@ -525,8 +525,9 @@ final class FreshRSS_http_Util { } } } - $proxy = is_string($curl_options[CURLOPT_PROXY] ?? null) ? $curl_options[CURLOPT_PROXY] : $proxy; $proxy_type = is_int($curl_options[CURLOPT_PROXYTYPE] ?? null) ? $curl_options[CURLOPT_PROXYTYPE] : $proxy_type; + $proxy = is_string($curl_options[CURLOPT_PROXY] ?? null) ? $curl_options[CURLOPT_PROXY] : $proxy; + $proxy = preg_replace('#^.*://#', '', $proxy) ?? $proxy; // Strip scheme, as it is provided by CURLOPT_PROXYTYPE if (($retryAfter = FreshRSS_http_Util::getRetryAfter($url, $proxy)) > 0) { Minz_Log::warning('For that domain, will first retry after ' . date('c', $retryAfter) . '. ' . \SimplePie\Misc::url_remove_credentials($url)); @@ -574,6 +575,7 @@ final class FreshRSS_http_Util { \SimplePie\Misc::url_remove_credentials($url) . ']'); return ['body' => '', 'effective_url' => '', 'redirect_count' => 0, 'fail' => true, 'status' => -500, 'error' => '', 'method' => $c_method]; } elseif ($resolve === false) { + Minz_Log::warning('Error during HTTP fetch: unable to resolve hostname! ' . \SimplePie\Misc::url_remove_credentials($url)); return ['body' => '', 'effective_url' => '', 'redirect_count' => 0, 'fail' => true, 'status' => -500, 'error' => '', 'method' => $c_method]; } if (!empty($resolve)) { @@ -602,6 +604,7 @@ final class FreshRSS_http_Util { \SimplePie\Misc::url_remove_credentials($proxy_url) . ']'); return ['body' => '', 'effective_url' => '', 'redirect_count' => 0, 'fail' => true, 'status' => -500, 'error' => '', 'method' => $c_method]; } elseif ($resolve === false) { + Minz_Log::warning('Error during HTTP fetch: unable to resolve the proxy’s hostname! ' . \SimplePie\Misc::url_remove_credentials($proxy_url)); return ['body' => '', 'effective_url' => '', 'redirect_count' => 0, 'fail' => true, 'status' => -500, 'error' => '', 'method' => $c_method]; } if (!empty($resolve)) { diff --git a/docs/en/admins/09_AccessControl.md b/docs/en/admins/09_AccessControl.md index e2e966b5e..036d38035 100644 --- a/docs/en/admins/09_AccessControl.md +++ b/docs/en/admins/09_AccessControl.md @@ -22,6 +22,9 @@ Entries are separated by newlines, and must be a `host:port` combination, for ex Another option is to set an `INTERNAL_HOST_ALLOWLIST` environment variable (e.g. in your docker-compose file). The entries there are separated by whitespace instead. Adding `*` disables the SSRF check completely (unsafe). +The same allowlist is needed for HTTP and SOCKS proxies: +for instance, a proxy `socks5h://127.0.0.1:1080` is blocked unless its location is allowed. + ### Recommended additional mitigations for shared/public setups Although there are already existing mitigations for SSRF, some extra steps can be taken for more safety: diff --git a/tests/app/Utils/httpUtilTest.php b/tests/app/Utils/httpUtilTest.php index 063939734..5284a75e5 100644 --- a/tests/app/Utils/httpUtilTest.php +++ b/tests/app/Utils/httpUtilTest.php @@ -15,6 +15,7 @@ final class httpUtilTest extends \PHPUnit\Framework\TestCase { #[\Override] protected function tearDown(): void { + FreshRSS_Context::initSystem(reload: true); // Restore the default internal host allowlist $resolveOk = new ReflectionProperty(FreshRSS_http_Util::class, 'resolve_ok'); $resolveOk->setValue(null, []); // Restore the default empty cache } @@ -141,4 +142,49 @@ final class httpUtilTest extends \PHPUnit\Framework\TestCase { FreshRSS_http_Util::getCurlResolveInfo('https://example.test/feed') ); } + + /** Socks schemes default to port 1080, and an IP address proxy produces no resolve entry */ + public function test_getCurlResolveInfoForProxyAddress(): void { + FreshRSS_Context::initSystem(); + // A proxy on a private address is rejected by default + self::assertNull(FreshRSS_http_Util::getCurlResolveInfo('socks5h://127.0.0.1:1080')); + + // Once the proxy is allowlisted, no resolve override is needed for an IP address + FreshRSS_Context::systemConf()->internal_host_allowlist = ['127.0.0.1:1080']; + self::assertSame([], FreshRSS_http_Util::getCurlResolveInfo('socks5h://127.0.0.1:1080')); + + // Other proxy ports need their own entry + FreshRSS_Context::systemConf()->internal_host_allowlist = ['127.0.0.1:8080']; + self::assertNull(FreshRSS_http_Util::getCurlResolveInfo('socks5h://127.0.0.1:1080')); + self::assertSame([], FreshRSS_http_Util::getCurlResolveInfo('https://127.0.0.1:8080')); + } + + /** For a hostname proxy, the resolved IP of the proxy is pinned with a resolve entry, to prevent DNS rebinding */ + public function test_getCurlResolveInfoForProxyHostname(): void { + FreshRSS_Context::initSystem(); + $resolveOk = new ReflectionProperty(FreshRSS_http_Util::class, 'resolve_ok'); + $resolveOk->setValue(null, ['proxy.example.test' => ['198.51.100.7']]); + FreshRSS_Context::systemConf()->internal_host_allowlist = ['198.51.100.7:1080']; + + self::assertSame( + ['proxy.example.test:1080:198.51.100.7'], + FreshRSS_http_Util::getCurlResolveInfo('socks5h://proxy.example.test:1080') + ); + + // An entry is still produced without allowlisting, because the address is public + FreshRSS_Context::systemConf()->internal_host_allowlist = []; + self::assertSame( + ['proxy.example.test:1080:198.51.100.7'], + FreshRSS_http_Util::getCurlResolveInfo('socks5h://proxy.example.test:1080') + ); + + // A proxy resolving to a private address is rejected unless allowlisted + $resolveOk->setValue(null, ['proxy.example.test' => ['192.168.1.1']]); + self::assertNull(FreshRSS_http_Util::getCurlResolveInfo('socks5h://proxy.example.test:1080')); + FreshRSS_Context::systemConf()->internal_host_allowlist = ['192.168.1.1:1080']; + self::assertSame( + ['proxy.example.test:1080:192.168.1.1'], + FreshRSS_http_Util::getCurlResolveInfo('socks5h://proxy.example.test:1080') + ); + } }