diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/workflow/workflow_executor_proxy.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/workflow/workflow_executor_proxy.rb index 1ca7ed127..738d90088 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/workflow/workflow_executor_proxy.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/workflow/workflow_executor_proxy.rb @@ -1,13 +1,13 @@ require 'faraday' +require 'uri' module ForestAdminAgent module Routes module Workflow - # Generic proxy: forwards any sub-path/verb under AGENT_PREFIX to EXECUTOR_PREFIX, so a - # new executor route needs no change here (PRD-567). Mounted only when `workflow_executor_url` is set. + # Generic proxy: forwards any sub-path/verb under AGENT_PREFIX to the executor verbatim, so a + # new executor route needs no change here. Mounted only when `workflow_executor_url` is set. class WorkflowExecutorProxy < AbstractAuthenticatedRoute - AGENT_PREFIX = '/_internal/workflow-executions'.freeze - EXECUTOR_PREFIX = '/runs'.freeze + AGENT_PREFIX = '/_internal/executor'.freeze # Never forwarded (request or response): hop-by-hop, Host, and body-framing headers. # Faraday and `render json:` set their own length and de/recompress the body, so relaying # the upstream length/encoding would mismatch the bytes we actually send — and forwarding @@ -17,7 +17,7 @@ class WorkflowExecutorProxy < AbstractAuthenticatedRoute proxy-authenticate proxy-authorization host content-length content-encoding accept-encoding ].freeze - # Fragments that could escape EXECUTOR_PREFIX once decoded. + # Fragments that could let the wildcard leave the executor origin once decoded. UNSAFE_PATH_FRAGMENTS = ['..', '%2e', '%2E', '\\', "\0"].freeze OPEN_TIMEOUT = 2 REQUEST_TIMEOUT = 120 @@ -68,19 +68,32 @@ def configured_executor_url end def build_target_url(args) + base = configured_executor_url path = build_executor_path(args.dig(:params, 'path') || args.dig(:params, :path)) + reject_off_origin!(base, path) query = args[:query_string].to_s - url = "#{configured_executor_url}#{path}" + url = "#{base}#{path}" query.empty? ? url : "#{url}?#{query}" end - # Security boundary: reject anything that could escape EXECUTOR_PREFIX. + # First-pass rejection of escape attempts; the authoritative origin check is reject_off_origin!. def build_executor_path(raw_path) path = raw_path.to_s raise Http::Exceptions::NotFoundError, 'Invalid workflow executor path' if unsafe_path?(path) - "#{EXECUTOR_PREFIX}/#{path}" + "/#{path}" + end + + # Authoritative SSRF check: forwarding must never leave the executor origin. + def reject_off_origin!(base, path) + base_uri = URI.parse(base) + target = URI.parse("#{base}#{path}") + same = target.scheme == base_uri.scheme && target.host == base_uri.host && + target.port == base_uri.port + raise Http::Exceptions::NotFoundError, 'Invalid workflow executor path' unless same + rescue URI::InvalidURIError + raise Http::Exceptions::NotFoundError, 'Invalid workflow executor path' end def unsafe_path?(path) diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/workflow/workflow_executor_proxy_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/workflow/workflow_executor_proxy_spec.rb index c6089b0a6..a89c80ef4 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/workflow/workflow_executor_proxy_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/workflow/workflow_executor_proxy_spec.rb @@ -82,7 +82,7 @@ def override_config(extra) route = proxy.routes['forest_workflow_executor_proxy'] expect(route).to include( method: :all, - uri: '/_internal/workflow-executions/*path' + uri: '/_internal/executor/*path' ) end @@ -109,11 +109,11 @@ def override_config(extra) end describe '#handle_request — generic forwarding' do - it 'forwards a GET under /runs, preserving the sub-path and query verbatim' do + it 'forwards a run GET (caller includes runs/), preserving the sub-path and query verbatim' do proxy.handle_request( method: 'GET', headers: headers, - params: { 'path' => run_id }, + params: { 'path' => "runs/#{run_id}" }, query_string: 'page=2&q=foo' ) @@ -128,7 +128,7 @@ def override_config(extra) proxy.handle_request( method: 'POST', headers: headers, - params: { 'path' => "#{run_id}/trigger" }, + params: { 'path' => "runs/#{run_id}/trigger" }, body: raw ) @@ -137,15 +137,15 @@ def override_config(extra) expect(captured[:body]).to eq(raw) end - it 'forwards any verb and any future sub-path without a dedicated route' do + it 'forwards a non-runs route verbatim (no /runs prefix injected)' do proxy.handle_request( method: 'DELETE', headers: headers, - params: { 'path' => "#{run_id}/cancel" } + params: { 'path' => 'mcp-oauth-credentials' } ) expect(captured[:method]).to eq(:delete) - expect(captured[:url]).to eq("#{executor_url}/runs/#{run_id}/cancel") + expect(captured[:url]).to eq("#{executor_url}/mcp-oauth-credentials") end it 'forwards all client headers except hop-by-hop / host / length / encoding' do @@ -175,14 +175,15 @@ def override_config(extra) end end - describe 'path traversal protection (the namespace security boundary)' do + describe 'SSRF guard (cannot escape the executor origin)' do [ '..', '../mcp-oauth-credentials', 'run_abc123/../../mcp-oauth-credentials', '%2e%2e/mcp-oauth-credentials', 'run_abc123%2E%2E', - '/runs/run_abc123', + '/mcp-oauth-credentials', + "\t//evil.com", "run\u0000id" ].each do |evil_path| it "rejects #{evil_path.inspect} with NotFoundError and never forwards" do