refactor: RemoteFileFetcher aus MediaImporter herausgelöst (URL-Prüfung, Weiterleitungen, Download)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

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
+ * - URL: downloaded by RemoteFileFetcher (public http(s) only, redirects checked, streamed into a
- *   (PublicAddress, independent of the core version) before any request and on every redirect,
+ *   temp file and cut off one byte above the upload limit). Network failures reach the client only
- *   fetched with wp_safe_remote_get() (core re-validates redirects too), streamed into a temp file
+ *   as one generic message (no refused/timeout/status oracle, security audit M3); details go to the log.
- *   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<string> Host name => IPv4 addresses. */
+    private RemoteFileFetcher $fetcher;
-    private Closure $resolve;
 
     /** @param (Closure(string): list<string>)|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 (! $this->fetcher->allows($url)) {
-        if (
-            ! in_array($scheme, ['http', 'https'], true)
-            || ! wp_http_validate_url($url)
-            || ! $this->isPublicUrl($url)
-        ) {
             return Errors::invalidInput(
                 'Die URL ist nicht erlaubt.',
                 ['url' => 'nur öffentlich erreichbare http(s)-Adressen'],
@@ -134,42 +123,9 @@ class MediaImporter
     /** @return array<string, mixed>|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
+        $response = $this->fetcher->fetch($url, $tmp, $this->limits->uploadBytes(), 'media import');
-        // (gethostbyname), so an AAAA record must not lead cURL to an unchecked IPv6 target.
+        if ($response === null) {
-        $ipv4 = static function ($handle): void {
+            return $this->downloadFailed();
-            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));
         }
 
         $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). */
+    /** One message for every network failure (security audit M3); RemoteFileFetcher logged the reason. */
-    private function isPublicUrl(string $url): bool
+    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'],

src/Integrations/Media/RemoteFileFetcher.php

@@ -0,0 +1,120 @@
+<?php
+
+declare(strict_types=1);
+
+namespace LIT\MCP\Integrations\Media;
+
+use Closure;
+use LIT\MCP\Log;
+use WP_Error;
+use WpOrg\Requests\Exception as RequestsException;
+
+defined('ABSPATH') || exit;
+
+/**
+ * Downloads a file from a public http(s) URL into a temp file (spec §6.3, §10) – shared by the media import and
+ * the font upload. 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() over IPv4 only (core re-validates redirects too), streamed into the temp file and cut off
+ * one byte above $maxBytes. A failure reaches the caller only as null (no refused/timeout/status oracle, security
+ * audit M3); the reason goes to the log. Not final so abilities can be tested with a fake.
+ */
+class RemoteFileFetcher
+{
+    private const TIMEOUT = 30;
+    private const REDIRECT_HOOK = 'requests-requests.before_redirect';
+
+    /** @var Closure(string): list<string> Host name => IPv4 addresses. */
+    private Closure $resolve;
+
+    /** @param (Closure(string): list<string>)|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<string, mixed>|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'] ?? '');
+    }
+}

tests/Unit/Integrations/Media/RemoteFileFetcherTest.php

@@ -0,0 +1,133 @@
+<?php
+
+declare(strict_types=1);
+
+namespace LIT\MCP\Tests\Unit\Integrations\Media;
+
+use Brain\Monkey\Actions;
+use Brain\Monkey\Functions;
+use LIT\MCP\Integrations\Media\RemoteFileFetcher;
+use LIT\MCP\Tests\Unit\TestCase;
+use Mockery;
+use WP_Error;
+
+/**
+ * The URL guard and the download shared by the media import and the font upload. MediaImporterTest keeps covering
+ * the same guard through fromUrl(); this test pins the parts a second caller relies on: the size limit as
+ * parameter, null instead of an error message, the log context.
+ */
+final class RemoteFileFetcherTest extends TestCase
+{
+    private string $tmp = '';
+
+    /** error_log() is a PHP internal and cannot be stubbed; it is redirected to this file. */
+    private string $errorLog = '';
+    private string|false $previousErrorLog = false;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+        $this->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<string> $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');
+    }
+}