refactor: split MediaAttacher into orchestrator + downloader + safety guard

The previous MediaAttacher had ~5 responsibilities crammed in one class
(URL validation, HTTP fetch, streaming, MIME check, Storage write,
post.media merge) plus an `if ($app->runningUnitTests()) return true`
inside the SSRF guard — production code that knew about test mode.

Split into three:

- UrlSafetyGuard (interface) + DnsUrlSafetyGuard (default impl). The
  full SSRF check is now isolated and unit-testable in
  tests/Unit/Services/Post/DnsUrlSafetyGuardTest. Tests bind a permissive
  guard in tests/TestCase::setUp so feature tests using Http::fake() with
  synthetic hosts (cdn.example.com) still work — the runningUnitTests()
  check inside production code is gone.

- MediaDownloader: takes a URL, returns a temp file + MIME or null. Uses
  Http::sink with a Guzzle progress callback that throws once maxBytes
  is exceeded, so we abort mid-stream without buffering the body in PHP
  memory (the previous chunk loop was append-to-string which defeated
  the whole point of streaming).

- MediaAttacher: pure orchestrator. Calls MediaDownloader, validates the
  MIME against the post's enabled platforms, persists via Storage::putFileAs,
  appends the Media record under a row lock. Drops from 275 to ~190 lines
  with zero responsibility overlap.

The IO contract (`null` on failure, `['path','mime','bytes']` on success)
keeps MediaAttacher free of the temp-file lifecycle on the happy path —
ownership is documented in MediaDownloader's PHPDoc and the orchestrator
unlinks via try/finally in processOne().

Bind the interface in AppServiceProvider so production resolves
DnsUrlSafetyGuard automatically; tests override it.

7 new unit tests cover the SSRF cases (loopback, RFC1918, link-local,
zero/broadcast, IPv6 loopback/ULA/link-local, plus a public-IP positive
test). The 14 existing AttachMedia feature tests stay green.
This commit is contained in:
Paulo Castellano 2026-05-04 13:45:43 -03:00
parent a2f98d551c
commit c871edf144
7 changed files with 335 additions and 156 deletions

View file

@ -37,6 +37,8 @@
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;
@ -81,6 +83,7 @@ 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);

View file

@ -0,0 +1,60 @@
<?php
declare(strict_types=1);
namespace App\Services\Post;
/**
* Default UrlSafetyGuard. Rejects:
* - non-http(s) schemes
* - missing or empty hosts
* - literal IPv4/IPv6 hosts in restricted ranges
* - DNS hostnames whose A/AAAA records resolve into restricted ranges
* (covers DNS-rebinding attempts where the first lookup is public and
* subsequent lookups resolve internally)
*/
class DnsUrlSafetyGuard implements UrlSafetyGuard
{
public function isSafe(string $url): bool
{
$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 $this->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;
}
}

View file

@ -8,15 +8,19 @@
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\Http;
use Illuminate\Support\Facades\Storage;
use Illuminate\Support\Str;
/**
* Downloads media from public URLs and attaches them to a post. Used by both
* the MCP `AttachMediaFromUrlTool` and the REST `POST /api/posts/{post}/media`
* endpoint so behaviour and validation stay aligned.
* 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.
*
* Used by both the MCP `AttachMediaFromUrlTool` and the REST
* `POST /api/posts/{post}/media` endpoint so behaviour stays aligned.
*/
class MediaAttacher
{
@ -26,6 +30,10 @@ class MediaAttacher
private const MAX_BYTES = 50 * 1024 * 1024; // 50 MB
public function __construct(
private readonly MediaDownloader $downloader,
) {}
/**
* @param array<int, string> $urls
* @return array{attached: array<int, array<string, mixed>>, failed: array<int, string>}
@ -38,7 +46,7 @@ public function attachFromUrls(Post $post, array $urls): array
$failed = [];
foreach ($urls as $url) {
$item = $this->downloadAndStore($post->workspace, $url, $allowedTypes);
$item = $this->processOne($post->workspace, $url, $allowedTypes);
if ($item === null) {
$failed[] = $url;
@ -50,23 +58,93 @@ public function attachFromUrls(Post $post, array $urls): array
}
if ($attached !== []) {
// Lock + reload before merging so concurrent attach calls don't
// overwrite each other's appended items (lost-update race).
DB::transaction(function () use ($post, $attached) {
$fresh = Post::whereKey($post->id)->lockForUpdate()->first();
$fresh->update([
'media' => collect($fresh->media ?? [])->concat($attached)->all(),
]);
$post->setRawAttributes($fresh->getAttributes(), true);
});
$this->mergeIntoPostMedia($post, $attached);
}
return ['attached' => $attached, 'failed' => $failed];
}
/**
* Intersection of allowed media types across platforms enabled on the
* post. If no platform is enabled, accept anything supported.
* @param array<MediaType> $allowedTypes
* @return array<string, mixed>|null
*/
private function processOne(Workspace $workspace, string $url, array $allowedTypes): ?array
{
$download = $this->downloader->download($url, self::MAX_BYTES);
if ($download === null) {
return null;
}
try {
$type = $this->resolveType(data_get($download, 'mime'));
if ($type === null || ! in_array($type, $allowedTypes, true)) {
return null;
}
return $this->storeMedia($workspace, $download, $type, $url);
} finally {
@unlink(data_get($download, 'path'));
}
}
/**
* @param array{path: string, mime: ?string, bytes: int} $download
* @return array<string, mixed>
*/
private function storeMedia(Workspace $workspace, array $download, MediaType $type, string $url): array
{
$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;
Storage::putFileAs('', new File(data_get($download, 'path')), $filename);
$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();
return [
'id' => $media->id,
'path' => $media->path,
'url' => $media->url,
'type' => $type->value,
'mime_type' => $media->mime_type,
'original_filename' => $media->original_filename,
];
}
/**
* Lock-then-merge so concurrent attach calls don't overwrite each
* other's appended items in the JSON `media` column.
*
* @param array<int, array<string, mixed>> $attached
*/
private function mergeIntoPostMedia(Post $post, array $attached): void
{
DB::transaction(function () use ($post, $attached): void {
$fresh = Post::whereKey($post->id)->lockForUpdate()->first();
$fresh->update([
'media' => collect($fresh->media ?? [])->concat($attached)->all(),
]);
$post->setRawAttributes($fresh->getAttributes(), true);
});
}
/**
* Intersection of allowed media types across platforms enabled on
* the post. With no enabled platform, accept anything we support.
*
* @return array<MediaType>
*/
@ -92,146 +170,6 @@ private function allowedMediaTypesFor(Post $post): array
return array_map(fn ($value) => MediaType::from($value), $intersection);
}
/**
* @param array<MediaType> $allowedTypes
* @return array<string, mixed>|null
*/
private function downloadAndStore(Workspace $workspace, string $url, array $allowedTypes): ?array
{
if (! $this->isPublicHttpUrl($url)) {
return null;
}
// Disable redirects (a public URL could 302 to an internal target),
// stream the body, and abort once we exceed MAX_BYTES so a malicious
// host can't exhaust memory or our process timeout.
$response = Http::timeout(20)
->withOptions([
'allow_redirects' => false,
'stream' => true,
])
->get($url);
if (! $response->successful()) {
return null;
}
$body = '';
$bytes = 0;
$stream = $response->toPsrResponse()->getBody();
while (! $stream->eof()) {
$chunk = $stream->read(8192);
$bytes += strlen($chunk);
if ($bytes > self::MAX_BYTES) {
return null;
}
$body .= $chunk;
}
if ($bytes === 0) {
return null;
}
$mime = $response->header('Content-Type');
$mime = $mime ? trim(explode(';', $mime)[0]) : null;
$type = $this->resolveType($mime);
if ($type === null || ! in_array($type, $allowedTypes, true)) {
return null;
}
$extension = $this->extensionFor($mime, $url);
$filename = 'media/'.Str::uuid()->toString().'.'.$extension;
$originalFilename = basename(parse_url($url, PHP_URL_PATH) ?? '') ?: 'download.'.$extension;
Storage::put($filename, $body);
$media = new Media([
'collection' => 'post-media',
'type' => $type,
'path' => $filename,
'original_filename' => $originalFilename,
'mime_type' => $mime ?? '',
'size' => $bytes,
'order' => 0,
]);
$media->mediable_type = Workspace::class;
$media->mediable_id = $workspace->id;
$media->save();
return [
'id' => $media->id,
'path' => $media->path,
'url' => $media->url,
'type' => $type->value,
'mime_type' => $media->mime_type,
'original_filename' => $media->original_filename,
];
}
/**
* Reject anything that isn't a plain http(s) URL targeting a public host.
* Blocks loopback, link-local, private, and reserved ranges so a caller
* can't pivot from us into the internal network (SSRF).
*/
private function isPublicHttpUrl(string $url): bool
{
$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;
}
// Under `Http::fake()` the HTTP facade short-circuits real network
// calls; skip DNS resolution so tests can stub responses for synthetic
// hosts without our SSRF guard rejecting them.
if (app()->runningUnitTests()) {
return true;
}
// Reject literal IPv4/IPv6 host inputs that fall in restricted ranges.
if (filter_var($host, FILTER_VALIDATE_IP) !== false) {
return $this->ipIsPublic($host);
}
// For DNS hostnames, resolve and check every record. Fail closed
// (no records / unresolvable / private) to prevent DNS-rebinding tricks
// where the first lookup is public and the second resolves internally.
$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;
}
private function resolveType(?string $mime): ?MediaType
{
if ($mime === null) {

View file

@ -0,0 +1,78 @@
<?php
declare(strict_types=1);
namespace App\Services\Post;
use Illuminate\Support\Facades\Http;
use RuntimeException;
/**
* Downloads a single media file from a public URL into a local temporary
* file, streaming the body to disk so we don't buffer it in PHP memory.
* Aborts mid-stream once `maxBytes` is exceeded.
*
* The caller owns the temp file lifecycle receive `path`, do whatever
* (validate / upload to Storage), then delete it.
*/
class MediaDownloader
{
public function __construct(
private readonly UrlSafetyGuard $guard,
) {}
/**
* @return array{path: string, mime: ?string, bytes: int}|null
* null when the URL is unsafe, the response failed, or the
* download exceeded `maxBytes`.
*/
public function download(string $url, int $maxBytes): ?array
{
if (! $this->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,
];
}
}

View file

@ -0,0 +1,16 @@
<?php
declare(strict_types=1);
namespace App\Services\Post;
/**
* Decides whether a user-supplied URL is safe to fetch from the server side.
* The default implementation (DnsUrlSafetyGuard) blocks loopback / private /
* link-local / reserved IP ranges to prevent SSRF; tests bind a permissive
* implementation so synthetic hosts under Http::fake() aren't rejected.
*/
interface UrlSafetyGuard
{
public function isSafe(string $url): bool;
}

View file

@ -4,6 +4,7 @@
namespace Tests;
use App\Services\Post\UrlSafetyGuard;
use Illuminate\Foundation\Testing\TestCase as BaseTestCase;
abstract class TestCase extends BaseTestCase
@ -22,5 +23,17 @@ protected function setUp(): void
parent::setUp();
$this->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;
}
});
}
}

View file

@ -0,0 +1,71 @@
<?php
declare(strict_types=1);
use App\Services\Post\DnsUrlSafetyGuard;
beforeEach(function () {
// The TestCase setUp binds a permissive UrlSafetyGuard for feature
// tests; this unit test exercises the real DnsUrlSafetyGuard directly,
// so we instantiate it instead of resolving from the container.
$this->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');