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

@@ -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);

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);

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)) {

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:

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')
+		);
+	}
 }