From 45da024fc682850bd5b20257e91399d8d644d75f Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Tue, 16 Jun 2026 12:09:16 -0300 Subject: [PATCH] fix(automations): code-review hardening for feed fetch & URL validation - inspectFeed fetches via SafeHttpFetcher::get (SSRF guard + timeout + redirect cap + UA) instead of a raw, timeout-less Http::get; add a timeout to the node fetch. - ResolvableUrl now requires an http(s) scheme, so it's no weaker than the plain url rule it replaced (rejects file://, javascript://). - Apply ResolvableUrl to the HTTP Request and Webhook node URLs too, so templated {{ }} URLs validate consistently across every URL field. - Clear a fetch_rss node's discovered_fields when its feed_url changes, so stale fields from the previous feed stop showing up as autocomplete suggestions. --- .../Automation/Node/RunFetchRssNode.php | 2 +- .../Controllers/App/AutomationController.php | 12 ++--- .../Automations/UpdateAutomationRequest.php | 4 +- app/Rules/ResolvableUrl.php | 6 ++- .../automations/config/FetchRssNodeConfig.vue | 9 ++++ .../Feature/Automation/FeedInspectionTest.php | 54 +++++++++++++++++++ 6 files changed, 75 insertions(+), 12 deletions(-) diff --git a/app/Actions/Automation/Node/RunFetchRssNode.php b/app/Actions/Automation/Node/RunFetchRssNode.php index 3482d985..6bf2a388 100644 --- a/app/Actions/Automation/Node/RunFetchRssNode.php +++ b/app/Actions/Automation/Node/RunFetchRssNode.php @@ -60,7 +60,7 @@ public function __invoke(AutomationRun $run, array $config): NodeRunResult ]); } - $response = Http::get($feedUrl); + $response = Http::timeout(10)->get($feedUrl); if (! $response->successful()) { return NodeRunResult::failed(__('automations.errors.fetch_rss_request_failed'), ['status' => $response->status()]); diff --git a/app/Http/Controllers/App/AutomationController.php b/app/Http/Controllers/App/AutomationController.php index b31c74c4..e691eb8c 100644 --- a/app/Http/Controllers/App/AutomationController.php +++ b/app/Http/Controllers/App/AutomationController.php @@ -39,7 +39,6 @@ use Illuminate\Http\JsonResponse; use Illuminate\Http\RedirectResponse; use Illuminate\Http\Response as HttpResponse; -use Illuminate\Support\Facades\Http; use Inertia\Inertia; use Inertia\Response; use RuntimeException; @@ -225,15 +224,12 @@ public function inspectFeed( ['variables' => $automation->resolvedVariables()], ); + // SafeHttpFetcher::get() bundles the SSRF guard, a request timeout, a + // redirect cap and a branded user-agent — so a slow or hostile feed can't + // hang this synchronous request. It throws on SSRF, timeout or non-2xx. try { - $safeHttp->guardAgainstSsrf($feedUrl); + $response = $safeHttp->get($feedUrl); } catch (RuntimeException) { - return response()->json(['message' => __('automations.errors.url_not_allowed')], SymfonyResponse::HTTP_UNPROCESSABLE_ENTITY); - } - - $response = Http::get($feedUrl); - - if (! $response->successful()) { return response()->json(['message' => __('automations.errors.fetch_rss_request_failed')], SymfonyResponse::HTTP_UNPROCESSABLE_ENTITY); } diff --git a/app/Http/Requests/App/Automations/UpdateAutomationRequest.php b/app/Http/Requests/App/Automations/UpdateAutomationRequest.php index ed94fa4b..0b0a3677 100644 --- a/app/Http/Requests/App/Automations/UpdateAutomationRequest.php +++ b/app/Http/Requests/App/Automations/UpdateAutomationRequest.php @@ -136,7 +136,7 @@ private function dataRulesForNodeType(?string $type, int $i): array 'discovered_fields.*.sample' => ['nullable', 'string'], ], NodeType::HttpRequest->value => [ - 'url' => ['required', 'url'], + 'url' => ['required', new ResolvableUrl], 'method' => ['required', Rule::in(array_column(HttpMethod::cases(), 'value'))], 'auth_type' => ['required', Rule::in(array_column(AuthType::cases(), 'value'))], 'auth_token' => ['nullable', 'string'], @@ -171,7 +171,7 @@ private function dataRulesForNodeType(?string $type, int $i): array 'scheduled_offset' => ['required_if:nodes.'.$i.'.data.mode,'.PublishMode::Scheduled->value, 'integer', 'min:0'], ], NodeType::Webhook->value => [ - 'url' => ['required', 'url'], + 'url' => ['required', new ResolvableUrl], 'method' => ['required', Rule::in(array_column(HttpMethod::cases(), 'value'))], 'payload_template' => ['nullable', 'string'], 'headers' => ['nullable', 'array'], diff --git a/app/Rules/ResolvableUrl.php b/app/Rules/ResolvableUrl.php index af5db97d..45f899bf 100644 --- a/app/Rules/ResolvableUrl.php +++ b/app/Rules/ResolvableUrl.php @@ -28,8 +28,12 @@ public function validate(string $attribute, mixed $value, Closure $fail): void } $candidate = preg_replace('/\{\{\s*[\w.]+\s*\}\}/', 'placeholder', $value); + $scheme = strtolower((string) parse_url($candidate, PHP_URL_SCHEME)); - if (filter_var($candidate, FILTER_VALIDATE_URL) === false) { + // Require a real http(s) URL once expressions are substituted, so the rule + // is no weaker than the plain `url` rule it replaces (rejects file://, + // javascript://, etc.) while still allowing templated hosts/queries. + if (filter_var($candidate, FILTER_VALIDATE_URL) === false || ! in_array($scheme, ['http', 'https'], true)) { $fail('validation.url')->translate(); } } diff --git a/resources/js/components/automations/config/FetchRssNodeConfig.vue b/resources/js/components/automations/config/FetchRssNodeConfig.vue index 74249ae5..886ba05b 100644 --- a/resources/js/components/automations/config/FetchRssNodeConfig.vue +++ b/resources/js/components/automations/config/FetchRssNodeConfig.vue @@ -35,6 +35,15 @@ const local = ref({ watch(local, (val) => emit('update', val), { deep: true }); +// Changing the feed invalidates the discovered fields — they belong to the old +// feed and would otherwise keep being offered as stale autocomplete suggestions. +watch( + () => local.value.feed_url, + () => { + local.value.discovered_fields = []; + }, +); + const inspectHttp = useHttp<{ feed_url: string }, { fields: DiscoveredField[] }>({ feed_url: '' }); const isInspecting = ref(false); const inspectError = ref(null); diff --git a/tests/Feature/Automation/FeedInspectionTest.php b/tests/Feature/Automation/FeedInspectionTest.php index c5ab6d00..1889529a 100644 --- a/tests/Feature/Automation/FeedInspectionTest.php +++ b/tests/Feature/Automation/FeedInspectionTest.php @@ -125,6 +125,60 @@ ->assertSessionHasErrors('nodes.0.data.feed_url'); }); +it('rejects a non-http(s) scheme in feed_url', function () { + $automation = Automation::factory()->for($this->workspace)->create(); + + $this->actingAs($this->user) + ->put(route('app.automations.update', $automation->id), [ + 'nodes' => [[ + 'id' => 'fetch_1', + 'type' => 'fetch_rss', + 'position' => ['x' => 0, 'y' => 0], + 'data' => ['feed_url' => 'file:///etc/passwd'], + ]], + ]) + ->assertSessionHasErrors('nodes.0.data.feed_url'); +}); + +it('accepts a templated url on an http_request node', function () { + $automation = Automation::factory()->for($this->workspace)->create(); + + $this->actingAs($this->user) + ->put(route('app.automations.update', $automation->id), [ + 'nodes' => [[ + 'id' => 'http_1', + 'type' => 'http_request', + 'position' => ['x' => 0, 'y' => 0], + 'data' => [ + 'url' => 'https://api.example.com/{{ variables.PATH }}/feed', + 'method' => 'GET', + 'auth_type' => 'none', + ], + ]], + ]) + ->assertRedirect() + ->assertSessionHasNoErrors(); +}); + +it('accepts a templated url on a webhook node', function () { + $automation = Automation::factory()->for($this->workspace)->create(); + + $this->actingAs($this->user) + ->put(route('app.automations.update', $automation->id), [ + 'nodes' => [[ + 'id' => 'webhook_1', + 'type' => 'webhook', + 'position' => ['x' => 0, 'y' => 0], + 'data' => [ + 'url' => 'https://hooks.example.com/{{ variables.TOKEN }}', + 'method' => 'POST', + ], + ]], + ]) + ->assertRedirect() + ->assertSessionHasNoErrors(); +}); + it('forbids inspecting a feed for an automation in another workspace', function () { Http::fake(['1.1.1.1/*' => Http::response('', 200)]);