diff --git a/src/Integrations/Media/MediaImporter.php b/src/Integrations/Media/MediaImporter.php index 5df1678..44c0c3e 100644 --- a/src/Integrations/Media/MediaImporter.php +++ b/src/Integrations/Media/MediaImporter.php @@ -11,18 +11,15 @@ use LIT\MCP\Limits; use LIT\MCP\Log; use Throwable; use WP_Error; -use WpOrg\Requests\Exception as RequestsException; defined('ABSPATH') || exit; /** * Imports files into the media library (spec §6.3, §10). * - * - URL: only http(s), checked with wp_http_validate_url() and the plugin's own range check - * (PublicAddress, independent of the core version) before any request and on every redirect, - * fetched with wp_safe_remote_get() (core re-validates redirects too), streamed into a temp file - * and cut off one byte above the upload limit. Network failures reach the client only as one - * generic message (no refused/timeout/status oracle, security audit M3); details go to the log. + * - URL: downloaded by RemoteFileFetcher (public http(s) only, redirects checked, streamed into a + * temp file and cut off one byte above the upload limit). Network failures reach the client only + * as one generic message (no refused/timeout/status oracle, security audit M3); details go to the log. * - Base64: decoded strictly; the size is checked against Limits::inlineBytes() (upload limit, * at most 10 MB – the whole file sits in memory) before anything is decoded or written. * - Type: whitelist (filter lit_mcp_upload_mimes) intersected with what WordPress allows the @@ -48,28 +45,20 @@ class MediaImporter 'mp3' => 'audio/mpeg', ]; - private const TIMEOUT = 30; private const ALT_KEY = '_wp_attachment_image_alt'; - private const REDIRECT_HOOK = 'requests-requests.before_redirect'; - /** @var Closure(string): list Host name => IPv4 addresses. */ - private Closure $resolve; + private RemoteFileFetcher $fetcher; /** @param (Closure(string): list)|null $resolve DNS lookup; null = the real one. */ public function __construct(private Limits $limits, ?Closure $resolve = null) { - $this->resolve = $resolve ?? PublicAddress::dnsResolver(); + $this->fetcher = new RemoteFileFetcher($resolve); } /** @param array{title?: string, alt?: string, post_id?: int} $meta */ public function fromUrl(string $url, ?string $filename, array $meta): int|WP_Error { - $scheme = strtolower((string) wp_parse_url($url, PHP_URL_SCHEME)); - if ( - ! in_array($scheme, ['http', 'https'], true) - || ! wp_http_validate_url($url) - || ! $this->isPublicUrl($url) - ) { + if (! $this->fetcher->allows($url)) { return Errors::invalidInput( 'Die URL ist nicht erlaubt.', ['url' => 'nur öffentlich erreichbare http(s)-Adressen'], @@ -134,42 +123,9 @@ class MediaImporter /** @return array|WP_Error The response (body streamed into $tmp). */ private function download(string $url, string $tmp): array|WP_Error { - // IPv4 only, for this request only: wp_http_validate_url() checks the IPv4 address - // (gethostbyname), so an AAAA record must not lead cURL to an unchecked IPv6 target. - $ipv4 = static function ($handle): void { - if (defined('CURLOPT_IPRESOLVE') && defined('CURL_IPRESOLVE_V4')) { - // phpcs:ignore WordPress.WP.AlternativeFunctions.curl_curl_setopt -- http_api_curl hands us the handle. - curl_setopt($handle, CURLOPT_IPRESOLVE, CURL_IPRESOLVE_V4); - } - }; - // Every redirect target gets the plugin's own range check too; Requests turns the - // exception into a failed request (WP_Http returns a WP_Error). - $redirect = function ($location): void { - if (! $this->isPublicUrl((string) $location)) { - throw new RequestsException('Redirect to a non-public address refused', 'lit_mcp_redirect'); - } - }; - add_action('http_api_curl', $ipv4); - add_action(self::REDIRECT_HOOK, $redirect); - try { - $response = wp_safe_remote_get($url, [ - 'timeout' => self::TIMEOUT, - 'stream' => true, - 'filename' => $tmp, - 'limit_response_size' => $this->limits->uploadBytes() + 1, - ]); - } finally { - remove_action('http_api_curl', $ipv4); - remove_action(self::REDIRECT_HOOK, $redirect); - } - - if ($response instanceof WP_Error) { - return $this->downloadFailed($url, $response->get_error_message()); - } - - $status = (int) wp_remote_retrieve_response_code($response); - if ($status !== 200) { - return $this->downloadFailed($url, sprintf('HTTP status %d', $status)); + $response = $this->fetcher->fetch($url, $tmp, $this->limits->uploadBytes(), 'media import'); + if ($response === null) { + return $this->downloadFailed(); } $length = (string) wp_remote_retrieve_header($response, 'content-length'); @@ -361,30 +317,9 @@ class MediaImporter } } - /** Host (name or IP literal) of $url resolves to public addresses only (PublicAddress). */ - private function isPublicUrl(string $url): bool + /** One message for every network failure (security audit M3); RemoteFileFetcher logged the reason. */ + private function downloadFailed(): WP_Error { - return PublicAddress::hostIsPublic((string) wp_parse_url($url, PHP_URL_HOST), $this->resolve); - } - - /** Scheme, host, port and path of $url – no credentials, query string or fragment in the log. */ - private static function loggableUrl(string $url): string - { - $parts = wp_parse_url($url); - if (! is_array($parts)) { - return '(ungültige URL)'; - } - - $port = isset($parts['port']) ? ':' . (int) $parts['port'] : ''; - - return sprintf('%s://%s%s%s', $parts['scheme'] ?? '', $parts['host'] ?? '', $port, $parts['path'] ?? ''); - } - - /** One message for every network failure (security audit M3); the reason is logged only. */ - private function downloadFailed(string $url, string $reason): WP_Error - { - Log::failure('media import from ' . self::loggableUrl($url), $reason); - return Errors::invalidInput( 'Die Datei konnte nicht geladen werden.', ['url' => 'nicht abrufbar'], diff --git a/src/Integrations/Media/RemoteFileFetcher.php b/src/Integrations/Media/RemoteFileFetcher.php new file mode 100644 index 0000000..00e76d2 --- /dev/null +++ b/src/Integrations/Media/RemoteFileFetcher.php @@ -0,0 +1,120 @@ + Host name => IPv4 addresses. */ + private Closure $resolve; + + /** @param (Closure(string): list)|null $resolve DNS lookup; null = the real one. */ + public function __construct(?Closure $resolve = null) + { + $this->resolve = $resolve ?? PublicAddress::dnsResolver(); + } + + /** Whether $url may be fetched at all: http(s), accepted by core, host with public addresses only. */ + public function allows(string $url): bool + { + $scheme = strtolower((string) wp_parse_url($url, PHP_URL_SCHEME)); + + return in_array($scheme, ['http', 'https'], true) && wp_http_validate_url($url) && $this->isPublicUrl($url); + } + + /** + * Streams $url into $tmp; the caller checks allows() first and the size of $tmp afterwards (a body above + * $maxBytes is cut off at $maxBytes + 1 bytes). + * + * @param string $context Names the download in the failure log, e.g. "media import". + * @return array|null The response (headers, status), null when the download failed. + */ + public function fetch(string $url, string $tmp, int $maxBytes, string $context): ?array + { + // IPv4 only, for this request only: wp_http_validate_url() checks the IPv4 address + // (gethostbyname), so an AAAA record must not lead cURL to an unchecked IPv6 target. + $ipv4 = static function ($handle): void { + if (defined('CURLOPT_IPRESOLVE') && defined('CURL_IPRESOLVE_V4')) { + // phpcs:ignore WordPress.WP.AlternativeFunctions.curl_curl_setopt -- http_api_curl hands us the handle. + curl_setopt($handle, CURLOPT_IPRESOLVE, CURL_IPRESOLVE_V4); + } + }; + // Every redirect target gets the plugin's own range check too; Requests turns the + // exception into a failed request (WP_Http returns a WP_Error). + $redirect = function ($location): void { + if (! $this->isPublicUrl((string) $location)) { + throw new RequestsException('Redirect to a non-public address refused', 'lit_mcp_redirect'); + } + }; + add_action('http_api_curl', $ipv4); + add_action(self::REDIRECT_HOOK, $redirect); + try { + $response = wp_safe_remote_get($url, [ + 'timeout' => self::TIMEOUT, + 'stream' => true, + 'filename' => $tmp, + 'limit_response_size' => $maxBytes + 1, + ]); + } finally { + remove_action('http_api_curl', $ipv4); + remove_action(self::REDIRECT_HOOK, $redirect); + } + + if ($response instanceof WP_Error) { + return $this->failed($context, $url, $response->get_error_message()); + } + + $status = (int) wp_remote_retrieve_response_code($response); + if ($status !== 200) { + return $this->failed($context, $url, sprintf('HTTP status %d', $status)); + } + + return $response; + } + + /** Host (name or IP literal) of $url resolves to public addresses only (PublicAddress). */ + private function isPublicUrl(string $url): bool + { + return PublicAddress::hostIsPublic((string) wp_parse_url($url, PHP_URL_HOST), $this->resolve); + } + + /** The reason goes to the log only (security audit M3). */ + private function failed(string $context, string $url, string $reason): ?array + { + Log::failure($context . ' from ' . self::loggableUrl($url), $reason); + + return null; + } + + /** Scheme, host, port and path of $url – no credentials, query string or fragment in the log. */ + private static function loggableUrl(string $url): string + { + $parts = wp_parse_url($url); + if (! is_array($parts)) { + return '(ungültige URL)'; + } + + $port = isset($parts['port']) ? ':' . (int) $parts['port'] : ''; + + return sprintf('%s://%s%s%s', $parts['scheme'] ?? '', $parts['host'] ?? '', $port, $parts['path'] ?? ''); + } +} diff --git a/tests/Unit/Integrations/Media/RemoteFileFetcherTest.php b/tests/Unit/Integrations/Media/RemoteFileFetcherTest.php new file mode 100644 index 0000000..5b0d595 --- /dev/null +++ b/tests/Unit/Integrations/Media/RemoteFileFetcherTest.php @@ -0,0 +1,133 @@ +tmp = (string) tempnam(sys_get_temp_dir(), 'lit-fetch'); + $this->errorLog = (string) tempnam(sys_get_temp_dir(), 'lit-mcp-log'); + $this->previousErrorLog = ini_set('error_log', $this->errorLog); + Functions\when('wp_parse_url')->alias('parse_url'); + Functions\when('wp_http_validate_url')->returnArg(); + Functions\when('wp_remote_retrieve_response_code')->alias(static fn (array $r): int => $r['response']['code']); + } + + protected function tearDown(): void + { + ini_set('error_log', (string) $this->previousErrorLog); + foreach ([$this->tmp, $this->errorLog] as $file) { + if (is_file($file)) { + unlink($file); + } + } + parent::tearDown(); + } + + /** @param list $addresses What every host name resolves to (no DNS in unit tests). */ + private function fetcher(array $addresses = ['93.184.216.34']): RemoteFileFetcher + { + return new RemoteFileFetcher(static fn (string $host): array => $addresses); + } + + public function testOnlyPublicHttpUrlsAreAllowed(): void + { + self::assertTrue($this->fetcher()->allows('https://fonts.example.com/inter.woff2')); + self::assertTrue($this->fetcher()->allows('HTTP://fonts.example.com/inter.woff2')); + self::assertFalse($this->fetcher()->allows('ftp://fonts.example.com/inter.woff2')); + self::assertFalse($this->fetcher()->allows('file:///etc/passwd')); + self::assertFalse($this->fetcher()->allows('http://169.254.169.254/latest/meta-data/')); + self::assertFalse($this->fetcher(['10.0.0.5'])->allows('https://intern.example.com/x.woff2')); + self::assertFalse($this->fetcher([])->allows('https://nirgends.example.com/x.woff2')); + + Functions\when('wp_http_validate_url')->justReturn(false); + self::assertFalse($this->fetcher()->allows('https://fonts.example.com/inter.woff2')); + } + + public function testTheBodyIsStreamedIntoTheTempFileAndCutOffOneByteAboveTheLimit(): void + { + Functions\expect('wp_safe_remote_get')->once()->with( + 'https://fonts.example.com/inter.woff2', + Mockery::on(function (array $args): bool { + file_put_contents($args['filename'], 'wOF2DATA'); + + return $args['filename'] === $this->tmp && $args['stream'] === true && $args['timeout'] === 30 + && $args['limit_response_size'] === 2048 + 1; + }) + )->andReturn(['response' => ['code' => 200], 'headers' => ['content-type' => 'font/woff2']]); + + $response = $this->fetcher()->fetch('https://fonts.example.com/inter.woff2', $this->tmp, 2048, 'font upload'); + + self::assertSame(['code' => 200], $response['response'] ?? null); + self::assertSame('wOF2DATA', file_get_contents($this->tmp)); + } + + public function testAFailedRequestIsNullAndOnlyTheLogNamesTheReasonAndTheContext(): void + { + Functions\when('wp_safe_remote_get')->justReturn(new WP_Error('http_request_failed', 'Connection refused')); + + $response = $this->fetcher()->fetch( + 'https://nutzer:geheim@fonts.example.com:8443/inter.woff2?token=abc', + $this->tmp, + 2048, + 'font upload' + ); + + $log = (string) file_get_contents($this->errorLog); + self::assertNull($response); + self::assertStringContainsString('font upload from https://fonts.example.com:8443/inter.woff2', $log); + self::assertStringContainsString('Connection refused', $log); + self::assertStringNotContainsString('geheim', $log); + self::assertStringNotContainsString('token=abc', $log); + } + + public function testAnHttpErrorStatusIsNull(): void + { + Functions\when('wp_safe_remote_get')->justReturn(['response' => ['code' => 404], 'headers' => []]); + + self::assertNull($this->fetcher()->fetch('https://fonts.example.com/fehlt.woff2', $this->tmp, 2048, 'x')); + self::assertStringContainsString('HTTP status 404', (string) file_get_contents($this->errorLog)); + } + + public function testTheIpv4AndRedirectHooksLiveOnlyDuringTheRequest(): void + { + $listener = null; + Actions\expectAdded('http_api_curl')->once(); + Actions\expectAdded('requests-requests.before_redirect')->once() + ->whenHappen(static function (callable $callback) use (&$listener): void { + $listener = $callback; + }); + Functions\when('wp_safe_remote_get')->justReturn(['response' => ['code' => 200], 'headers' => []]); + + $this->fetcher()->fetch('https://fonts.example.com/inter.woff2', $this->tmp, 2048, 'x'); + + self::assertFalse(has_action('http_api_curl')); + self::assertFalse(has_action('requests-requests.before_redirect')); + self::assertIsCallable($listener); + $listener('https://cdn.example.com/inter.woff2'); // public: no exception + $this->expectException(\WpOrg\Requests\Exception::class); + $listener('http://127.0.0.1/inter.woff2'); + } +}