diff --git a/app/Actions/Automation/Node/RunFetchRssNode.php b/app/Actions/Automation/Node/RunFetchRssNode.php index 6bf2a388..36f04dd3 100644 --- a/app/Actions/Automation/Node/RunFetchRssNode.php +++ b/app/Actions/Automation/Node/RunFetchRssNode.php @@ -13,7 +13,6 @@ use App\Services\Automation\FeedParser; use App\Services\Brand\SafeHttpFetcher; use Carbon\CarbonImmutable; -use Illuminate\Support\Facades\Http; use RuntimeException; use Throwable; @@ -51,21 +50,19 @@ public function __invoke(AutomationRun $run, array $config): NodeRunResult return NodeRunResult::failed(__('automations.errors.fetch_rss_missing_url')); } + // SafeHttpFetcher::get() re-validates every redirect hop against the SSRF + // guard (not just the initial URL), so a public feed that 302s to an + // internal host is never followed. It throws on a blocked hop, connection + // failure, non-2xx status, or an excessive redirect chain — all of which + // are legitimate "this feed couldn't be fetched" failures for this node. try { - $this->safeHttp->guardAgainstSsrf($feedUrl); - } catch (RuntimeException) { - return NodeRunResult::failed(__('automations.errors.url_not_allowed'), [ - 'reason' => 'url_not_allowed', - 'url' => $feedUrl, + $response = $this->safeHttp->get($feedUrl); + } catch (RuntimeException $e) { + return NodeRunResult::failed(__('automations.errors.fetch_rss_request_failed'), [ + 'message' => $e->getMessage(), ]); } - $response = Http::timeout(10)->get($feedUrl); - - if (! $response->successful()) { - return NodeRunResult::failed(__('automations.errors.fetch_rss_request_failed'), ['status' => $response->status()]); - } - $items = $this->parser->parse($response->body()); if ($items === null) { diff --git a/app/Actions/Automation/Node/RunHttpRequestNode.php b/app/Actions/Automation/Node/RunHttpRequestNode.php index 0036cdc7..15063a78 100644 --- a/app/Actions/Automation/Node/RunHttpRequestNode.php +++ b/app/Actions/Automation/Node/RunHttpRequestNode.php @@ -364,7 +364,9 @@ private function buildRequest(array $config, array $context): PendingRequest $request = $request->withHeaders($headers); } - return $request->withUserAgent(config('trypost.user_agent')); + return $request + ->withUserAgent(config('trypost.user_agent')) + ->withOptions($this->safeHttp->redirectGuardOptions()); } /** diff --git a/app/Actions/Automation/Node/RunWebhookNode.php b/app/Actions/Automation/Node/RunWebhookNode.php index 0b5498c9..5bf25090 100644 --- a/app/Actions/Automation/Node/RunWebhookNode.php +++ b/app/Actions/Automation/Node/RunWebhookNode.php @@ -75,6 +75,7 @@ public function __invoke(AutomationRun $run, array $config): NodeRunResult try { $response = Http::withHeaders($headers) ->withUserAgent(config('trypost.user_agent')) + ->withOptions(['allow_redirects' => false]) ->send($method, $url, ['json' => $payload]); } catch (Throwable $e) { return NodeRunResult::failed(__('automations.errors.webhook_request_failed'), [ diff --git a/app/Services/Brand/SafeHttpFetcher.php b/app/Services/Brand/SafeHttpFetcher.php index 3369b03d..3961a76d 100644 --- a/app/Services/Brand/SafeHttpFetcher.php +++ b/app/Services/Brand/SafeHttpFetcher.php @@ -91,6 +91,27 @@ public function tryGet(string $url): ?Response } } + /** + * Guzzle allow_redirects options that re-run the SSRF guard on every hop. + * For callers that follow redirects on user-supplied URLs with methods/bodies + * that SafeHttpFetcher::get() cannot express. + * + * @return array + */ + public function redirectGuardOptions(int $max = self::MAX_REDIRECTS): array + { + return [ + 'allow_redirects' => [ + 'max' => $max, + 'strict' => true, + 'protocols' => ['http', 'https'], + 'on_redirect' => function ($request, $response, $uri): void { + $this->guardAgainstSsrf((string) $uri); + }, + ], + ]; + } + public function guardAgainstSsrf(string $url): void { $parts = parse_url($url); diff --git a/tests/Feature/Automation/Node/FetchRssNodeTest.php b/tests/Feature/Automation/Node/FetchRssNodeTest.php index 69189545..26f66daf 100644 --- a/tests/Feature/Automation/Node/FetchRssNodeTest.php +++ b/tests/Feature/Automation/Node/FetchRssNodeTest.php @@ -280,7 +280,7 @@ $result = app(RunFetchRssNode::class)($run, ['feed_url' => 'https://1.1.1.1/feed.xml']); expect($result->status)->toBe(NodeRunStatus::Failed); - expect($result->error['status'])->toBe(500); + expect($result->error['message'])->toContain('500'); }); it('fails on a malformed RSS feed', function () { @@ -313,6 +313,40 @@ expect($result->output['fetched']['key'])->toBe('d'); }); +it('never follows a feed redirect that targets a private or internal host', function () { + Http::fake([ + 'https://93.184.216.34/feed' => Http::response('', 302, ['Location' => 'http://127.0.0.1/internal']), + 'http://127.0.0.1/*' => Http::response(feedFixture('rss_old'), 200), + ]); + + $automation = Automation::factory()->active()->create(); + $run = AutomationRun::factory()->for($automation)->create(['current_node_id' => 'fetch_1']); + + $result = app(RunFetchRssNode::class)($run, ['feed_url' => 'https://93.184.216.34/feed']); + + expect($result->status)->toBe(NodeRunStatus::Failed); + Http::assertNotSent(fn ($request) => str_contains($request->url(), '127.0.0.1')); +}); + +it('still follows a legitimate public-to-public feed redirect', function () { + Carbon::setTestNow('2026-01-15 10:00:00'); + Http::fake([ + 'https://93.184.216.34/feed' => Http::response('', 301, ['Location' => 'https://1.1.1.1/feed']), + 'https://1.1.1.1/feed' => Http::response(feedFixture('rss_old'), 200), + ]); + + $automation = Automation::factory()->active()->create(); + $run = AutomationRun::factory()->for($automation)->create(['current_node_id' => 'fetch_1']); + + $result = app(RunFetchRssNode::class)($run, ['feed_url' => 'https://93.184.216.34/feed']); + + expect($result->status)->toBe(NodeRunStatus::Completed); + Http::assertSentInOrder([ + fn ($request) => str_contains($request->url(), '93.184.216.34'), + fn ($request) => str_contains($request->url(), '1.1.1.1'), + ]); +}); + it('falls back to the link as the dedup key when an item has no guid', function () { Carbon::setTestNow('2026-01-15 10:00:00'); Http::fake(['1.1.1.1/*' => Http::response(feedFixture('rss_no_guid'), 200)]); diff --git a/tests/Feature/Automation/Node/HttpRequestNodeTest.php b/tests/Feature/Automation/Node/HttpRequestNodeTest.php index 4f5a8188..0edc809f 100644 --- a/tests/Feature/Automation/Node/HttpRequestNodeTest.php +++ b/tests/Feature/Automation/Node/HttpRequestNodeTest.php @@ -53,6 +53,47 @@ Http::assertNothingSent(); }); +it('never follows a redirect that targets a private or internal host', function () { + Http::fake([ + 'https://93.184.216.34/*' => Http::response('', 302, ['Location' => 'http://127.0.0.1/internal']), + 'http://127.0.0.1/*' => Http::response('internal secret', 200), + ]); + + $automation = Automation::factory()->active()->create(); + $run = AutomationRun::factory()->for($automation)->create(['current_node_id' => 'http_1']); + + $result = app(RunHttpRequestNode::class)($run, [ + 'url' => 'https://93.184.216.34/start', + 'method' => 'GET', + 'auth_type' => 'none', + ]); + + expect($result->status)->toBe(NodeRunStatus::Failed); + Http::assertNotSent(fn ($request) => str_contains($request->url(), '127.0.0.1')); +}); + +it('still follows a legitimate public-to-public redirect', function () { + Http::fake([ + 'https://93.184.216.34/*' => Http::response('', 301, ['Location' => 'https://1.1.1.1/final']), + 'https://1.1.1.1/final' => Http::response(['ok' => true], 200), + ]); + + $automation = Automation::factory()->active()->create(); + $run = AutomationRun::factory()->for($automation)->create(['current_node_id' => 'http_1']); + + $result = app(RunHttpRequestNode::class)($run, [ + 'url' => 'https://93.184.216.34/start', + 'method' => 'GET', + 'auth_type' => 'none', + ]); + + expect($result->status)->toBe(NodeRunStatus::Completed); + Http::assertSentInOrder([ + fn ($request) => str_contains($request->url(), '93.184.216.34'), + fn ($request) => str_contains($request->url(), '1.1.1.1'), + ]); +}); + it('processes first new item and spawns siblings when items_path is set', function () { Carbon::setTestNow('2026-01-15 10:00:00'); Http::fake([ diff --git a/tests/Feature/Automation/Node/WebhookNodeTest.php b/tests/Feature/Automation/Node/WebhookNodeTest.php index 727de55b..6467a995 100644 --- a/tests/Feature/Automation/Node/WebhookNodeTest.php +++ b/tests/Feature/Automation/Node/WebhookNodeTest.php @@ -200,6 +200,27 @@ Http::assertSent(fn ($request) => $request->hasHeader('X-Token', 'tok-123')); }); +it('never follows a redirect to a private or internal host', function () { + Http::fake([ + 'https://93.184.216.34/*' => Http::response('', 302, ['Location' => 'http://127.0.0.1/internal']), + 'http://127.0.0.1/*' => Http::response('internal secret', 200), + ]); + + $run = AutomationRun::factory()->create(); + + $result = app(RunWebhookNode::class)($run, [ + 'url' => 'https://93.184.216.34/hook', + 'method' => 'POST', + 'payload_template' => '{}', + ]); + + // The 3xx is returned as-is (not followed), so the node completes with the + // redirect status rather than the internal host's response. + expect($result->status)->toBe(Status::Completed); + expect($result->output['webhook']['status'])->toBe(302); + Http::assertNotSent(fn ($request) => str_contains($request->url(), '127.0.0.1')); +}); + it('fails cleanly when the request throws a connection exception', function () { Http::fake(fn () => throw new ConnectionException('connection timed out'));