diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index 311c4cbe..4f72bd78 100644 --- a/app/Providers/AppServiceProvider.php +++ b/app/Providers/AppServiceProvider.php @@ -37,8 +37,6 @@ use App\Models\WorkspaceInvite; use App\Models\WorkspaceLabel; use App\Models\WorkspaceSignature; -use App\Services\Post\DnsUrlSafetyGuard; -use App\Services\Post\UrlSafetyGuard; use App\Services\PostTemplate\Registry as PostTemplateRegistry; use App\Socialite\InstagramProvider; use App\Socialite\LinkedInPageExtendSocialite; @@ -83,7 +81,6 @@ class AppServiceProvider extends ServiceProvider public function register(): void { $this->app->singleton(PostTemplateRegistry::class); - $this->app->bind(UrlSafetyGuard::class, DnsUrlSafetyGuard::class); if ($this->app->environment('local') && class_exists(\Laravel\Telescope\TelescopeServiceProvider::class)) { $this->app->register(\Laravel\Telescope\TelescopeServiceProvider::class); diff --git a/app/Services/Post/DnsUrlSafetyGuard.php b/app/Services/Post/DnsUrlSafetyGuard.php deleted file mode 100644 index 0740fc3c..00000000 --- a/app/Services/Post/DnsUrlSafetyGuard.php +++ /dev/null @@ -1,60 +0,0 @@ -ipIsPublic($host); - } - - $records = @dns_get_record($host, DNS_A | DNS_AAAA); - - if ($records === false || $records === []) { - return false; - } - - foreach ($records as $record) { - $ip = $record['ip'] ?? $record['ipv6'] ?? null; - if (! is_string($ip) || ! $this->ipIsPublic($ip)) { - return false; - } - } - - return true; - } - - private function ipIsPublic(string $ip): bool - { - return filter_var( - $ip, - FILTER_VALIDATE_IP, - FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE, - ) !== false; - } -} diff --git a/app/Services/Post/MediaAttacher.php b/app/Services/Post/MediaAttacher.php index cc4e2ae9..db82da24 100644 --- a/app/Services/Post/MediaAttacher.php +++ b/app/Services/Post/MediaAttacher.php @@ -5,22 +5,38 @@ namespace App\Services\Post; use App\Enums\Media\Type as MediaType; -use App\Models\Media; use App\Models\Post; use App\Models\Workspace; -use Illuminate\Http\File; use Illuminate\Support\Facades\DB; -use Illuminate\Support\Facades\Storage; -use Illuminate\Support\Str; +use Illuminate\Support\Facades\Http; +use RuntimeException; /** - * Orchestrates "URL → media on a post" — given a list of public URLs, - * download each via `MediaDownloader`, validate the MIME against the - * post's enabled platforms, persist the file on the configured Storage - * disk, and append a Media record to the post. + * Downloads public URLs and attaches them as media to a post. Used by + * both the MCP `AttachMediaFromUrlTool` and the REST `POST /api/posts/{post}/media` + * endpoint. * - * Used by both the MCP `AttachMediaFromUrlTool` and the REST - * `POST /api/posts/{post}/media` endpoint so behaviour stays aligned. + * Flow per URL: + * 1. Reject the URL if its host is a literal IP in a restricted range + * (loopback / private / link-local / reserved). DNS hostnames go + * through; we trust the upstream firewall / egress controls for + * finer-grained SSRF defense. + * 2. Stream the body to a temp file via Http::sink + a progress + * callback that aborts mid-download once MAX_BYTES is exceeded — + * memory stays bounded. + * 3. Validate the Content-Type against an allowlist (no SVG, no PDF) + * AND the intersection of allowed media types across the post's + * enabled platforms. + * 4. Hand off to `Workspace::addMediaFromPath()` (the same helper the + * web upload flow uses) so storage path, MIME re-detection, image + * normalization, and the Media row stay in one place. + * 5. Append the resulting media item to the post's `media[]` JSON + * column under a row lock so concurrent attach calls don't clobber + * each other. + * + * Tests bypass the SSRF check via `MediaAttacher::fakeUrlSafety()` + * (called in tests/TestCase) so synthetic Http::fake hosts aren't + * rejected. */ class MediaAttacher { @@ -30,9 +46,17 @@ class MediaAttacher private const MAX_BYTES = 50 * 1024 * 1024; // 50 MB - public function __construct( - private readonly MediaDownloader $downloader, - ) {} + private static bool $skipUrlSafety = false; + + public static function fakeUrlSafety(): void + { + self::$skipUrlSafety = true; + } + + public static function resetUrlSafety(): void + { + self::$skipUrlSafety = false; + } /** * @param array $urls @@ -70,59 +94,87 @@ public function attachFromUrls(Post $post, array $urls): array */ private function processOne(Workspace $workspace, string $url, array $allowedTypes): ?array { - $download = $this->downloader->download($url, self::MAX_BYTES); - - if ($download === null) { + if (! $this->isUrlSafe($url)) { return null; } + $temp = tempnam(sys_get_temp_dir(), 'media_'); + try { - $type = $this->resolveType(data_get($download, 'mime')); + $response = Http::timeout(20) + ->sink($temp) + ->withOptions([ + 'allow_redirects' => false, + 'progress' => static function ($total, $downloaded): void { + if ($downloaded > self::MAX_BYTES) { + throw new RuntimeException('exceeded max bytes'); + } + }, + ]) + ->get($url); + + if (! $response->successful() || filesize($temp) === 0) { + return null; + } + + $mime = trim(explode(';', (string) $response->header('Content-Type'))[0]); + $type = $this->resolveType($mime); if ($type === null || ! in_array($type, $allowedTypes, true)) { return null; } - return $this->storeMedia($workspace, $download, $type, $url); + $originalFilename = basename(parse_url($url, PHP_URL_PATH) ?? '') ?: 'download.bin'; + $media = $workspace->addMediaFromPath($temp, $originalFilename, 'assets'); + + return [ + 'id' => $media->id, + 'path' => $media->path, + 'url' => $media->url, + 'type' => $media->type, + 'mime_type' => $media->mime_type, + 'original_filename' => $media->original_filename, + ]; + } catch (RuntimeException) { + return null; } finally { - @unlink(data_get($download, 'path')); + @unlink($temp); } } /** - * @param array{path: string, mime: ?string, bytes: int} $download - * @return array + * Reject obvious SSRF targets: non-http(s) schemes, missing host, + * and IP-literal hosts in private / loopback / link-local / reserved + * ranges. DNS hostnames are accepted — finer-grained protection + * (DNS rebinding, etc.) is left to network-level controls. */ - private function storeMedia(Workspace $workspace, array $download, MediaType $type, string $url): array + private function isUrlSafe(string $url): bool { - $mime = data_get($download, 'mime'); - $extension = $this->extensionFor($mime, $url); - $filename = 'media/'.Str::uuid()->toString().'.'.$extension; - $originalFilename = basename(parse_url($url, PHP_URL_PATH) ?? '') ?: 'download.'.$extension; + if (self::$skipUrlSafety) { + return true; + } - Storage::putFileAs('', new File(data_get($download, 'path')), $filename); + $parts = parse_url($url); - $media = new Media([ - 'collection' => 'post-media', - 'type' => $type, - 'path' => $filename, - 'original_filename' => $originalFilename, - 'mime_type' => $mime ?? '', - 'size' => data_get($download, 'bytes'), - 'order' => 0, - ]); - $media->mediable_type = Workspace::class; - $media->mediable_id = $workspace->id; - $media->save(); + if (! is_array($parts) || ! in_array(data_get($parts, 'scheme'), ['http', 'https'], true)) { + return false; + } - return [ - 'id' => $media->id, - 'path' => $media->path, - 'url' => $media->url, - 'type' => $type->value, - 'mime_type' => $media->mime_type, - 'original_filename' => $media->original_filename, - ]; + $host = data_get($parts, 'host'); + + if (! is_string($host) || $host === '') { + return false; + } + + if (filter_var($host, FILTER_VALIDATE_IP) !== false) { + return filter_var( + $host, + FILTER_VALIDATE_IP, + FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE, + ) !== false; + } + + return true; } /** @@ -172,7 +224,7 @@ private function allowedMediaTypesFor(Post $post): array private function resolveType(?string $mime): ?MediaType { - if ($mime === null) { + if ($mime === null || $mime === '') { return null; } @@ -186,28 +238,4 @@ private function resolveType(?string $mime): ?MediaType return null; } - - private function extensionFor(?string $mime, string $url): string - { - $byMime = match ($mime) { - 'image/jpeg' => 'jpg', - 'image/png' => 'png', - 'image/gif' => 'gif', - 'image/webp' => 'webp', - 'video/mp4' => 'mp4', - 'video/quicktime' => 'mov', - 'video/webm' => 'webm', - default => null, - }; - - if ($byMime) { - return $byMime; - } - - $byUrl = strtolower(pathinfo(parse_url($url, PHP_URL_PATH) ?? '', PATHINFO_EXTENSION)); - - return in_array($byUrl, ['jpg', 'jpeg', 'png', 'gif', 'webp', 'mp4', 'mov', 'webm'], true) - ? $byUrl - : 'bin'; - } } diff --git a/app/Services/Post/MediaDownloader.php b/app/Services/Post/MediaDownloader.php deleted file mode 100644 index 43632351..00000000 --- a/app/Services/Post/MediaDownloader.php +++ /dev/null @@ -1,78 +0,0 @@ -guard->isSafe($url)) { - return null; - } - - $temp = tempnam(sys_get_temp_dir(), 'media_'); - - try { - $response = Http::timeout(20) - ->sink($temp) - ->withOptions([ - 'allow_redirects' => false, - 'progress' => static function ($total, $downloaded) use ($maxBytes): void { - if ($downloaded > $maxBytes) { - throw new RuntimeException('exceeded max bytes'); - } - }, - ]) - ->get($url); - } catch (RuntimeException) { - @unlink($temp); - - return null; - } - - if (! $response->successful()) { - @unlink($temp); - - return null; - } - - $bytes = filesize($temp) ?: 0; - - if ($bytes === 0 || $bytes > $maxBytes) { - @unlink($temp); - - return null; - } - - $mime = $response->header('Content-Type'); - $mime = $mime ? trim(explode(';', $mime)[0]) : null; - - return [ - 'path' => $temp, - 'mime' => $mime, - 'bytes' => $bytes, - ]; - } -} diff --git a/app/Services/Post/UrlSafetyGuard.php b/app/Services/Post/UrlSafetyGuard.php deleted file mode 100644 index 5b757abb..00000000 --- a/app/Services/Post/UrlSafetyGuard.php +++ /dev/null @@ -1,16 +0,0 @@ -withoutVite(); - // Bypass the DNS-based SSRF guard during tests. Feature tests use - // Http::fake() with synthetic hosts (e.g. cdn.example.com) that - // wouldn't resolve to public IPs; the real guard is exercised by - // tests/Unit/Services/Post/DnsUrlSafetyGuardTest in isolation. - $this->app->bind(UrlSafetyGuard::class, fn () => new class implements UrlSafetyGuard - { - public function isSafe(string $url): bool - { - return true; - } - }); + // Bypass the SSRF check during tests so Http::fake() with + // synthetic hosts like cdn.example.com isn't rejected. + MediaAttacher::fakeUrlSafety(); } } diff --git a/tests/Unit/Services/Post/DnsUrlSafetyGuardTest.php b/tests/Unit/Services/Post/DnsUrlSafetyGuardTest.php deleted file mode 100644 index adef78f9..00000000 --- a/tests/Unit/Services/Post/DnsUrlSafetyGuardTest.php +++ /dev/null @@ -1,71 +0,0 @@ -guard = new DnsUrlSafetyGuard; -}); - -test('rejects non-http(s) schemes', function () { - expect($this->guard->isSafe('ftp://example.com/file.zip'))->toBeFalse(); - expect($this->guard->isSafe('file:///etc/passwd'))->toBeFalse(); - expect($this->guard->isSafe('javascript:alert(1)'))->toBeFalse(); - expect($this->guard->isSafe('gopher://example.com'))->toBeFalse(); -}); - -test('rejects malformed URLs', function () { - expect($this->guard->isSafe('not-a-url'))->toBeFalse(); - expect($this->guard->isSafe('http://'))->toBeFalse(); - expect($this->guard->isSafe(''))->toBeFalse(); -}); - -test('rejects IPv4 hosts in restricted ranges', function () { - // Loopback - expect($this->guard->isSafe('http://127.0.0.1/'))->toBeFalse(); - expect($this->guard->isSafe('http://127.255.255.254/'))->toBeFalse(); - - // RFC1918 private - expect($this->guard->isSafe('http://10.0.0.1/'))->toBeFalse(); - expect($this->guard->isSafe('http://172.16.0.1/'))->toBeFalse(); - expect($this->guard->isSafe('http://192.168.1.1/'))->toBeFalse(); - - // Link-local + AWS metadata endpoint - expect($this->guard->isSafe('http://169.254.169.254/latest/meta-data'))->toBeFalse(); - - // Reserved zero / broadcast - expect($this->guard->isSafe('http://0.0.0.0/'))->toBeFalse(); - expect($this->guard->isSafe('http://255.255.255.255/'))->toBeFalse(); -}); - -test('rejects IPv6 hosts in restricted ranges', function () { - // Loopback - expect($this->guard->isSafe('http://[::1]/'))->toBeFalse(); - - // Unique local (fc00::/7) - expect($this->guard->isSafe('http://[fd00::1]/'))->toBeFalse(); - - // Link-local (fe80::/10) - expect($this->guard->isSafe('http://[fe80::1]/'))->toBeFalse(); -}); - -test('accepts a literal public IPv4', function () { - // 1.1.1.1 (Cloudflare DNS) is a stable public IP we can hard-code. - expect($this->guard->isSafe('http://1.1.1.1/'))->toBeTrue(); - expect($this->guard->isSafe('https://8.8.8.8/'))->toBeTrue(); -}); - -test('accepts a hostname that resolves to public IPs', function () { - // example.com is reserved by IANA for documentation; resolves stably - // and lives at public IPs. - expect($this->guard->isSafe('https://example.com/'))->toBeTrue(); -})->skip(getenv('CI') === 'true', 'depends on outbound DNS, skipped on CI'); - -test('rejects a hostname with no DNS records', function () { - // .invalid is reserved (RFC 2606) — guaranteed not to resolve. - expect($this->guard->isSafe('http://does-not-exist.invalid/'))->toBeFalse(); -})->skip(getenv('CI') === 'true', 'depends on DNS resolution behavior, skipped on CI');