diff --git a/app/Http/Controllers/Api/PostController.php b/app/Http/Controllers/Api/PostController.php index 2ef5eee8..c9df1a24 100644 --- a/app/Http/Controllers/Api/PostController.php +++ b/app/Http/Controllers/Api/PostController.php @@ -88,7 +88,7 @@ public function attachMedia(Request $request, Post $post): PostMediaAttachResour $validated = $request->validate([ 'urls' => ['required', 'array', 'min:1', 'max:10'], - 'urls.*' => ['url:http,https'], + 'urls.*' => ['url:http,https', 'active_url'], ]); $result = app(MediaAttacher::class)->attachFromUrls($post, $validated['urls']); diff --git a/app/Mcp/Tools/Post/AttachMediaFromUrlTool.php b/app/Mcp/Tools/Post/AttachMediaFromUrlTool.php index efd65842..97a00463 100644 --- a/app/Mcp/Tools/Post/AttachMediaFromUrlTool.php +++ b/app/Mcp/Tools/Post/AttachMediaFromUrlTool.php @@ -22,7 +22,7 @@ public function handle(Request $request): Response|ResponseFactory $validated = $request->validate([ 'post_id' => ['required', 'uuid'], 'urls' => ['required', 'array', 'min:1', 'max:10'], - 'urls.*' => ['url:http,https'], + 'urls.*' => ['url:http,https', 'active_url'], ]); $post = Post::where('workspace_id', $request->user()->current_workspace_id) diff --git a/app/Services/Post/MediaAttacher.php b/app/Services/Post/MediaAttacher.php index a332dc4d..15d3eba8 100644 --- a/app/Services/Post/MediaAttacher.php +++ b/app/Services/Post/MediaAttacher.php @@ -16,51 +16,33 @@ * both the MCP `AttachMediaFromUrlTool` and the REST `POST /api/posts/{post}/media` * endpoint. * + * URL syntax (`url:http,https`) and DNS resolvability (`active_url`) are + * enforced at the request validation layer. SSRF defense beyond that is + * the responsibility of network-level egress controls in production. + * * 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) + * 1. Stream the body to a temp file via Http::sink + a progress + * callback that aborts mid-download once MAX_BYTES is exceeded. + * 2. Validate the Content-Type against the MediaType enum's allow-list * 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 + * 3. Hand off to `Workspace::addMediaFromPath()` so storage path, + * MIME re-detection, image normalization, and the Media row stay + * in one place (same path as the web upload flow). + * 4. 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 { /** - * Cap on URL-fetched payloads. Smaller than the web upload cap (which - * can be 1 GB for direct uploads) because URL fetches have stricter - * server-side concerns: bandwidth, timeout, and unbounded user input. - * 50 MB covers a long photo or a short video; bigger files should be - * uploaded directly. + * Cap on URL-fetched payloads. Smaller than the web upload cap (1 GB) + * because URL fetches have different operational constraints: + * bandwidth, timeout, and unbounded user input. 50 MB covers a long + * photo or a short video; bigger files should be uploaded directly. */ private const MAX_BYTES = 50 * 1024 * 1024; - private static bool $skipUrlSafety = false; - - public static function fakeUrlSafety(): void - { - self::$skipUrlSafety = true; - } - - public static function resetUrlSafety(): void - { - self::$skipUrlSafety = false; - } - /** * @param array $urls * @return array{attached: array>, failed: array} @@ -97,10 +79,6 @@ public function attachFromUrls(Post $post, array $urls): array */ private function processOne(Workspace $workspace, string $url, array $allowedTypes): ?array { - if (! $this->isUrlSafe($url)) { - return null; - } - $temp = tempnam(sys_get_temp_dir(), 'media_'); try { @@ -145,41 +123,6 @@ private function processOne(Workspace $workspace, string $url, array $allowedTyp } } - /** - * 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 isUrlSafe(string $url): bool - { - if (self::$skipUrlSafety) { - return true; - } - - $parts = parse_url($url); - - if (! is_array($parts) || ! in_array(data_get($parts, 'scheme'), ['http', 'https'], true)) { - return false; - } - - $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; - } - /** * Lock-then-merge so concurrent attach calls don't overwrite each * other's appended items in the JSON `media` column. diff --git a/tests/Feature/Api/PostMediaApiTest.php b/tests/Feature/Api/PostMediaApiTest.php index 32ec6491..df19d322 100644 --- a/tests/Feature/Api/PostMediaApiTest.php +++ b/tests/Feature/Api/PostMediaApiTest.php @@ -38,7 +38,7 @@ it('attaches media from url', function () { Http::fake([ - 'cdn.example.com/photo.png' => Http::response( + 'example.com/photo.png' => Http::response( file_get_contents(__DIR__.'/../../fixtures/1x1.png'), 200, ['Content-Type' => 'image/png'], @@ -47,7 +47,7 @@ $this->withHeaders(['Authorization' => 'Bearer '.$this->plainToken]) ->postJson(route('api.posts.attach-media', $this->post), [ - 'urls' => ['https://cdn.example.com/photo.png'], + 'urls' => ['https://example.com/photo.png'], ]) ->assertOk() ->assertJsonPath('attached_count', 1) @@ -59,16 +59,16 @@ it('reports failures for unreachable urls', function () { Http::fake([ - 'cdn.example.com/missing.png' => Http::response(null, 404), + 'example.com/missing.png' => Http::response(null, 404), ]); $this->withHeaders(['Authorization' => 'Bearer '.$this->plainToken]) ->postJson(route('api.posts.attach-media', $this->post), [ - 'urls' => ['https://cdn.example.com/missing.png'], + 'urls' => ['https://example.com/missing.png'], ]) ->assertOk() ->assertJsonPath('attached_count', 0) - ->assertJsonPath('failed_urls.0', 'https://cdn.example.com/missing.png'); + ->assertJsonPath('failed_urls.0', 'https://example.com/missing.png'); }); it('cannot attach media to a post from another workspace', function () { @@ -77,7 +77,7 @@ $this->withHeaders(['Authorization' => 'Bearer '.$this->plainToken]) ->postJson(route('api.posts.attach-media', $post), [ - 'urls' => ['https://cdn.example.com/photo.png'], + 'urls' => ['https://example.com/photo.png'], ]) ->assertNotFound(); }); diff --git a/tests/Feature/Mcp/AttachMediaFromUrlToolTest.php b/tests/Feature/Mcp/AttachMediaFromUrlToolTest.php index ddfe710e..6b2aa69a 100644 --- a/tests/Feature/Mcp/AttachMediaFromUrlToolTest.php +++ b/tests/Feature/Mcp/AttachMediaFromUrlToolTest.php @@ -28,7 +28,7 @@ test('attaches an image from url and creates a media row', function () { Http::fake([ - 'cdn.example.com/photo.jpg' => Http::response( + 'example.com/photo.jpg' => Http::response( file_get_contents(__DIR__.'/../../fixtures/1x1.png'), 200, ['Content-Type' => 'image/png'], @@ -38,7 +38,7 @@ $response = TryPostServer::actingAs($this->user) ->tool(AttachMediaFromUrlTool::class, [ 'post_id' => $this->post->id, - 'urls' => ['https://cdn.example.com/photo.jpg'], + 'urls' => ['https://example.com/photo.jpg'], ]); $response->assertOk(); @@ -49,13 +49,13 @@ test('rejects url that returns non-image content type', function () { Http::fake([ - 'evil.example.com/payload' => Http::response('not an image', 200, ['Content-Type' => 'text/html']), + 'example.org/payload' => Http::response('not an image', 200, ['Content-Type' => 'text/html']), ]); $response = TryPostServer::actingAs($this->user) ->tool(AttachMediaFromUrlTool::class, [ 'post_id' => $this->post->id, - 'urls' => ['https://evil.example.com/payload'], + 'urls' => ['https://example.org/payload'], ]); $response->assertOk(); @@ -66,25 +66,25 @@ test('reports failures and successes separately', function () { Http::fake([ - 'cdn.example.com/ok.png' => Http::response( + 'example.com/ok.png' => Http::response( file_get_contents(__DIR__.'/../../fixtures/1x1.png'), 200, ['Content-Type' => 'image/png'], ), - 'cdn.example.com/missing.png' => Http::response(null, 404), + 'example.com/missing.png' => Http::response(null, 404), ]); $response = TryPostServer::actingAs($this->user) ->tool(AttachMediaFromUrlTool::class, [ 'post_id' => $this->post->id, 'urls' => [ - 'https://cdn.example.com/ok.png', - 'https://cdn.example.com/missing.png', + 'https://example.com/ok.png', + 'https://example.com/missing.png', ], ]); $response->assertOk() - ->assertSee(['cdn.example.com/missing.png']); + ->assertSee(['example.com/missing.png']); expect(Media::where('mediable_id', $this->workspace->id)->count())->toBe(1); expect($this->post->fresh()->media)->toHaveCount(1); @@ -97,7 +97,7 @@ $response = TryPostServer::actingAs($this->user) ->tool(AttachMediaFromUrlTool::class, [ 'post_id' => $post->id, - 'urls' => ['https://cdn.example.com/photo.jpg'], + 'urls' => ['https://example.com/photo.jpg'], ]); $response->assertHasErrors(['Post not found.']); @@ -107,7 +107,7 @@ $response = TryPostServer::actingAs($this->user) ->tool(AttachMediaFromUrlTool::class, [ 'post_id' => $this->post->id, - 'urls' => ['ftp://cdn.example.com/photo.jpg'], + 'urls' => ['ftp://example.com/photo.jpg'], ]); $response->assertHasErrors(); @@ -126,7 +126,7 @@ }); test('rejects more than 10 urls per call', function () { - $urls = collect(range(1, 11))->map(fn ($i) => "https://cdn.example.com/photo-{$i}.jpg")->all(); + $urls = collect(range(1, 11))->map(fn ($i) => "https://example.com/photo-{$i}.jpg")->all(); $response = TryPostServer::actingAs($this->user) ->tool(AttachMediaFromUrlTool::class, [ diff --git a/tests/TestCase.php b/tests/TestCase.php index ef25a290..3312b582 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -4,7 +4,6 @@ namespace Tests; -use App\Services\Post\MediaAttacher; use Illuminate\Foundation\Testing\TestCase as BaseTestCase; abstract class TestCase extends BaseTestCase @@ -23,9 +22,5 @@ protected function setUp(): void parent::setUp(); $this->withoutVite(); - - // Bypass the SSRF check during tests so Http::fake() with - // synthetic hosts like cdn.example.com isn't rejected. - MediaAttacher::fakeUrlSafety(); } }