From 5375e20423d79873b935adb8239eaae3ee3255f0 Mon Sep 17 00:00:00 2001 From: Matt Date: Tue, 16 Jun 2026 16:15:17 +0200 Subject: [PATCH 1/6] refactor(agent): forward workflow executor routes generically [PRD-567] The agent proxied the workflow executor with one explicitly declared route per executor route, so every new executor route required a matching agent route and release. Replace the two hand-written routes with a single catch-all under /_internal/workflow-executions/* that forwards any sub-path and any verb to the executor's /runs/* namespace. - request forwarded verbatim: all client headers (minus hop-by-hop / host / content-length), raw body (no reshaping), query string and method as-is - security boundary: the wildcard can only map into /runs (path-traversal, encoded-dot and absolute-path inputs are rejected), so executor routes outside that namespace stay unreachable through the proxy - ForestController passes the raw request context (method/query/body) to the route closure; other routes ignore the extra keys fixes PRD-567 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../workflow/workflow_executor_proxy.rb | 126 +++++------ .../workflow/workflow_executor_proxy_spec.rb | 198 +++++++++--------- .../forest_admin_rails/forest_controller.rb | 12 +- 3 files changed, 176 insertions(+), 160 deletions(-) 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 da72810fc..b314efb99 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 @@ -3,43 +3,42 @@ module ForestAdminAgent module Routes module Workflow - # Forwards workflow-execution traffic from the agent to the workflow executor. - # Mounted only when the integrator sets `workflow_executor_url` + # 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. class WorkflowExecutorProxy < AbstractAuthenticatedRoute AGENT_PREFIX = '/_internal/workflow-executions'.freeze EXECUTOR_PREFIX = '/runs'.freeze - FORWARDED_HEADERS = %w[Authorization Cookie].freeze - ROUTING_KEYS = %w[run_id route_alias controller action format].freeze + # Hop-by-hop headers + those the HTTP client must recompute — never forwarded. + SKIPPED_HEADERS = %w[ + connection keep-alive transfer-encoding upgrade te trailer + proxy-authenticate proxy-authorization host content-length + ].freeze + # Substrings that could let the wildcard escape EXECUTOR_PREFIX (traversal, encoded + # dots, backslash, null byte). + UNSAFE_PATH_FRAGMENTS = ['..', '%2e', '%2E', '\\', "\0"].freeze OPEN_TIMEOUT = 2 - GET_TIMEOUT = 10 - TRIGGER_TIMEOUT = 120 + REQUEST_TIMEOUT = 120 def setup_routes return self unless executor_configured? + # The glob can only ever map into EXECUTOR_PREFIX (build_executor_path rejects + # traversal), so executor routes outside it stay unreachable through the proxy. add_route( - 'forest_workflow_run_show', - 'get', - "#{AGENT_PREFIX}/:run_id", - ->(args) { handle_request(:get, args) } - ) - add_route( - 'forest_workflow_run_trigger', - 'post', - "#{AGENT_PREFIX}/:run_id/trigger", - ->(args) { handle_request(:post, args) } + 'forest_workflow_executor_proxy', + :all, + "#{AGENT_PREFIX}/*path", + ->(args) { handle_request(args) } ) self end - def handle_request(method, args = {}) + def handle_request(args = {}) build(args) - base_url = configured_executor_url - run_id = args.dig(:params, 'run_id') || args.dig(:params, :run_id) - path = build_path(run_id, method) - response = forward(method, base_url, path, args) + method = (args[:method] || 'get').to_s.downcase.to_sym + response = forward(method, build_target_url(args), args) { content: response.body, @@ -67,71 +66,72 @@ def configured_executor_url url.to_s.sub(%r{/+\z}, '') end - def build_path(run_id, method) - suffix = method == :post ? '/trigger' : '' - "#{EXECUTOR_PREFIX}/#{run_id}#{suffix}" + def build_target_url(args) + path = build_executor_path(args.dig(:params, 'path') || args.dig(:params, :path)) + query = args[:query_string].to_s + url = "#{configured_executor_url}#{path}" + + query.empty? ? url : "#{url}?#{query}" + end + + # Security boundary: reject anything that could escape EXECUTOR_PREFIX. + 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}" end - def forward(method, base_url, path, args) - query = forwarded_query_params(args[:params]) + def unsafe_path?(path) + return true if path.empty? || path.start_with?('/') + + UNSAFE_PATH_FRAGMENTS.any? { |fragment| path.include?(fragment) } + end + + def forward(method, target_url, args) headers = forwarded_request_headers(args[:headers]) - body = forwarded_body(method, args[:params]) - target_url = "#{base_url}#{path}" + body = method == :get ? nil : args[:body] - client = build_client(timeout_for(method)) - client.run_request(method, target_url, body, headers) do |req| - req.params.update(query) unless query.empty? - end + build_client.run_request(method, target_url, body, headers) rescue Faraday::TimeoutError => e raise Http::Exceptions::ServiceUnavailableError.new('Workflow executor timed out', cause: e) rescue Faraday::ConnectionFailed => e raise Http::Exceptions::ServiceUnavailableError.new('Workflow executor unreachable', cause: e) end - def timeout_for(method) - method == :get ? GET_TIMEOUT : TRIGGER_TIMEOUT - end - - def build_client(request_timeout) - Faraday.new(request: { open_timeout: OPEN_TIMEOUT, timeout: request_timeout }) do |f| - f.request :json + def build_client + Faraday.new(request: { open_timeout: OPEN_TIMEOUT, timeout: REQUEST_TIMEOUT }) do |f| + # No request :json middleware: body is forwarded raw (not reshaped). f.response :json, content_type: /\bjson$/ f.adapter Faraday.default_adapter end end - # Strip Rails-injected routing keys; keep only true client query params. - def forwarded_query_params(params) - return {} unless params.is_a?(Hash) + # `env` is the Rack env: real HTTP headers are the HTTP_* keys (+ CONTENT_TYPE); + # rack.*/action_dispatch.*/server vars are not headers and are dropped. + def forwarded_request_headers(env) + return {} unless env.is_a?(Hash) - params.each_with_object({}) do |(key, value), acc| - next if ROUTING_KEYS.include?(key.to_s) - next if value.is_a?(Hash) || value.is_a?(Array) # 'data' body, etc. + env.each_with_object({}) do |(key, value), acc| + name = http_header_name(key.to_s) + next unless name + next if SKIPPED_HEADERS.include?(name.downcase) + next if value.nil? || value.to_s.empty? - acc[key.to_s] = value + acc[name] = value.to_s end end - def forwarded_request_headers(headers) - return {} unless headers.is_a?(Hash) - - FORWARDED_HEADERS.each_with_object({}) do |name, acc| - value = headers[name] || headers[name.downcase] || headers["HTTP_#{name.upcase}"] - acc[name] = value if value && !value.to_s.empty? + def http_header_name(env_key) + if env_key.start_with?('HTTP_') + titleize_header(env_key.delete_prefix('HTTP_')) + elsif %w[CONTENT_TYPE CONTENT_LENGTH].include?(env_key) + titleize_header(env_key) end end - def forwarded_body(method, params) - return nil if method == :get - return nil unless params.is_a?(Hash) - - # JSON request bodies arrive parsed under :data when sent as JSON:API, - # or as the raw top-level params hash otherwise. Prefer :data when - # present; fall back to a sanitized copy of params. - body = params['data'] || params[:data] - return body if body - - params.reject { |key, _| ROUTING_KEYS.include?(key.to_s) } + def titleize_header(rack_name) + rack_name.split('_').map(&:capitalize).join('-') end def forwarded_response_headers(response) 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 67c0b933c..2d29fce30 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 @@ -11,17 +11,23 @@ module Workflow let(:executor_url) { 'http://workflow-executor.test:4001' } let(:run_id) { 'run_abc123' } + # Rack env shape: HTTP_* (+ CONTENT_*) are headers; the rest is rack/server internals. let(:headers) do { 'HTTP_AUTHORIZATION' => bearer, - 'Authorization' => bearer, - 'Cookie' => 'forest_session_token=abc', - 'forest-secret-key' => 'should-not-be-forwarded' + 'HTTP_COOKIE' => 'forest_session_token=abc', + 'HTTP_X_CUSTOM_HEADER' => 'custom-value', + 'CONTENT_TYPE' => 'application/json', + 'CONTENT_LENGTH' => '123', + 'HTTP_CONNECTION' => 'keep-alive', + 'HTTP_HOST' => 'agent.test', + 'action_dispatch.remote_ip' => '127.0.0.1', + 'rack.input' => 'raw-body' } end # Capture-based Faraday stubbing: store the request that hit `run_request` - # so each test can assert exact url/method/body/headers/params. + # so each test can assert exact url/method/body/headers. let(:captured) { {} } let(:fake_response) do instance_double( @@ -46,23 +52,14 @@ def override_config(extra) # the auth chain itself (covered by AbstractAuthenticatedRoute specs). allow(proxy).to receive(:build).and_return(nil) - faraday_request_class = Struct.new(:params) captured_state = captured response_for_request = fake_response allow(Faraday).to receive(:new).and_wrap_original do |original, *args, &block| captured_state[:faraday_options] = args.first connection = original.call(*args, &block) - allow(connection).to receive(:run_request) do |method, url, body, hdrs, &req_block| - captured_params = {} - req_block&.call(faraday_request_class.new(captured_params)) - captured_state.merge!( - method: method, - url: url, - body: body, - headers: hdrs, - query: captured_params.dup - ) + allow(connection).to receive(:run_request) do |method, url, body, hdrs| + captured_state.merge!(method: method, url: url, body: body, headers: hdrs) response_for_request end connection @@ -70,28 +67,24 @@ def override_config(extra) end describe '#setup_routes' do - it 'registers the GET /_internal/workflow-executions/:run_id route' do + it 'registers a single catch-all route forwarding every verb' do proxy.setup_routes - route = proxy.routes['forest_workflow_run_show'] + route = proxy.routes['forest_workflow_executor_proxy'] expect(route).to include( - method: 'get', - uri: '/_internal/workflow-executions/:run_id' + method: :all, + uri: '/_internal/workflow-executions/*path' ) end - it 'registers the POST /_internal/workflow-executions/:run_id/trigger route' do + it 'registers exactly one route (generic — no per-executor-route entry)' do proxy.setup_routes - route = proxy.routes['forest_workflow_run_trigger'] - expect(route).to include( - method: 'post', - uri: '/_internal/workflow-executions/:run_id/trigger' - ) + expect(proxy.routes.keys).to eq(['forest_workflow_executor_proxy']) end context 'when workflow_executor_url is nil' do before { override_config(workflow_executor_url: nil) } - it 'does not register any route (so they are absent from rails routes)' do + it 'does not register any route' do expect(described_class.new.routes).to be_empty end end @@ -105,35 +98,60 @@ def override_config(extra) end end - describe '#handle_request (GET)' do - let(:args) do - { + describe '#handle_request — generic forwarding' do + it 'forwards a GET under /runs, preserving the sub-path and query verbatim' do + proxy.handle_request( + method: 'GET', headers: headers, - params: { 'run_id' => run_id, 'page' => '2', 'route_alias' => 'forest_workflow_run_show' } - } - end + params: { 'path' => run_id }, + query_string: 'page=2&q=foo' + ) - it 'forwards GET to the executor /runs/:run_id path' do - proxy.handle_request(:get, args) expect(captured[:method]).to eq(:get) - expect(captured[:url]).to eq("#{executor_url}/runs/#{run_id}") + expect(captured[:url]).to eq("#{executor_url}/runs/#{run_id}?page=2&q=foo") + expect(captured[:body]).to be_nil end - it 'forwards client query params and drops Rails routing keys' do - proxy.handle_request(:get, args) - expect(captured[:query]).to eq('page' => '2') + it 'forwards a POST with the raw body untouched (no reshaping)' do + raw = '{"pendingData":{"step":"approve","value":42}}' + + proxy.handle_request( + method: 'POST', + headers: headers, + params: { 'path' => "#{run_id}/trigger" }, + body: raw + ) + + expect(captured[:method]).to eq(:post) + expect(captured[:url]).to eq("#{executor_url}/runs/#{run_id}/trigger") + expect(captured[:body]).to eq(raw) end - it 'forwards only Authorization and Cookie headers' do - proxy.handle_request(:get, args) + it 'forwards any verb and any future sub-path without a dedicated route' do + proxy.handle_request( + method: 'DELETE', + headers: headers, + params: { 'path' => "#{run_id}/cancel" } + ) + + expect(captured[:method]).to eq(:delete) + expect(captured[:url]).to eq("#{executor_url}/runs/#{run_id}/cancel") + end + + it 'forwards all client headers except hop-by-hop / host / content-length' do + proxy.handle_request(method: 'GET', headers: headers, params: { 'path' => run_id }) + expect(captured[:headers]).to eq( 'Authorization' => bearer, - 'Cookie' => 'forest_session_token=abc' + 'Cookie' => 'forest_session_token=abc', + 'X-Custom-Header' => 'custom-value', + 'Content-Type' => 'application/json' ) end it 'returns the executor status, body and Content-Type to the controller' do - result = proxy.handle_request(:get, args) + result = proxy.handle_request(method: 'GET', headers: headers, params: { 'path' => run_id }) + expect(result).to eq( content: { 'id' => run_id, 'state' => 'pending' }, status: 200, @@ -142,33 +160,50 @@ def override_config(extra) end end - describe '#handle_request (POST trigger)' do - let(:args) do - { - headers: headers, - params: { - 'run_id' => run_id, - 'data' => { 'step' => 'approve', 'value' => 42 } - } - } + describe 'path traversal protection (the namespace security boundary)' do + [ + '..', + '../mcp-oauth-credentials', + 'run_abc123/../../mcp-oauth-credentials', + '%2e%2e/mcp-oauth-credentials', + 'run_abc123%2E%2E', + '/runs/run_abc123', + "run\u0000id" + ].each do |evil_path| + it "rejects #{evil_path.inspect} with NotFoundError and never forwards" do + expect do + proxy.handle_request(method: 'GET', headers: headers, params: { 'path' => evil_path }) + end.to raise_error(Http::Exceptions::NotFoundError, /Invalid workflow executor path/) + + expect(captured).to be_empty + end end + end - it 'forwards POST to the executor /runs/:run_id/trigger path with the JSON body' do - proxy.handle_request(:post, args) - expect(captured[:method]).to eq(:post) - expect(captured[:url]).to eq("#{executor_url}/runs/#{run_id}/trigger") - expect(captured[:body]).to eq('step' => 'approve', 'value' => 42) + describe 'when the executor returns a non-2xx response' do + let(:fake_response) do + instance_double( + Faraday::Response, + status: 422, + body: { 'error' => 'invalid step' }, + headers: { 'content-type' => 'application/json' } + ) + end + + it 'forwards the status and body verbatim' do + result = proxy.handle_request(method: 'GET', headers: headers, params: { 'path' => run_id }) + + expect(result[:status]).to eq(422) + expect(result[:content]).to eq('error' => 'invalid step') end end describe 'when workflow_executor_url is not configured' do - before do - override_config(workflow_executor_url: nil) - end + before { override_config(workflow_executor_url: nil) } it 'raises NotFoundError so the controller renders 404' do expect do - proxy.handle_request(:get, headers: headers, params: { 'run_id' => run_id }) + proxy.handle_request(method: 'GET', headers: headers, params: { 'path' => run_id }) end.to raise_error(Http::Exceptions::NotFoundError, /not configured/) end end @@ -177,16 +212,14 @@ def override_config(extra) before do allow(Faraday).to receive(:new).and_wrap_original do |original, *args, &block| connection = original.call(*args, &block) - allow(connection).to receive(:run_request).and_raise( - Faraday::ConnectionFailed.new('boom') - ) + allow(connection).to receive(:run_request).and_raise(Faraday::ConnectionFailed.new('boom')) connection end end it 'raises ServiceUnavailableError (translated to 503 by ErrorTranslator)' do expect do - proxy.handle_request(:get, headers: headers, params: { 'run_id' => run_id }) + proxy.handle_request(method: 'GET', headers: headers, params: { 'path' => run_id }) end.to raise_error(Http::Exceptions::ServiceUnavailableError, /unreachable/) end end @@ -195,49 +228,22 @@ def override_config(extra) before do allow(Faraday).to receive(:new).and_wrap_original do |original, *args, &block| connection = original.call(*args, &block) - allow(connection).to receive(:run_request).and_raise( - Faraday::TimeoutError.new('slow') - ) + allow(connection).to receive(:run_request).and_raise(Faraday::TimeoutError.new('slow')) connection end end it 'raises ServiceUnavailableError' do expect do - proxy.handle_request(:get, headers: headers, params: { 'run_id' => run_id }) + proxy.handle_request(method: 'GET', headers: headers, params: { 'path' => run_id }) end.to raise_error(Http::Exceptions::ServiceUnavailableError, /timed out/) end end - describe 'when the executor returns a non-2xx response' do - let(:fake_response) do - instance_double( - Faraday::Response, - status: 422, - body: { 'error' => 'invalid step' }, - headers: { 'content-type' => 'application/json' } - ) - end + describe 'Faraday timeout' do + it 'applies a single large timeout regardless of the verb' do + proxy.handle_request(method: 'POST', headers: headers, params: { 'path' => "#{run_id}/trigger" }, body: '{}') - it 'forwards the status and body verbatim' do - result = proxy.handle_request(:get, headers: headers, params: { 'run_id' => run_id }) - expect(result[:status]).to eq(422) - expect(result[:content]).to eq('error' => 'invalid step') - end - end - - describe 'Faraday timeouts' do - it 'applies a short timeout on GET requests' do - proxy.handle_request(:get, headers: headers, params: { 'run_id' => run_id }) - expect(captured[:faraday_options]).to eq(request: { open_timeout: 2, timeout: 10 }) - end - - it 'applies a longer timeout on POST trigger requests' do - proxy.handle_request( - :post, - headers: headers, - params: { 'run_id' => run_id, 'data' => { 'step' => 'approve' } } - ) expect(captured[:faraday_options]).to eq(request: { open_timeout: 2, timeout: 120 }) end end diff --git a/packages/forest_admin_rails/app/controllers/forest_admin_rails/forest_controller.rb b/packages/forest_admin_rails/app/controllers/forest_admin_rails/forest_controller.rb index 765e76aba..2f11afe77 100644 --- a/packages/forest_admin_rails/app/controllers/forest_admin_rails/forest_controller.rb +++ b/packages/forest_admin_rails/app/controllers/forest_admin_rails/forest_controller.rb @@ -11,7 +11,17 @@ def index route = ForestAdminAgent::Http::Router.cached_routes[params['route_alias']] begin - forest_response route[:closure].call({ params: params.to_unsafe_h, headers: request.headers.to_h }) + forest_response route[:closure].call( + { + params: params.to_unsafe_h, + headers: request.headers.to_h, + # Raw request context for verbatim-forwarding routes (workflow executor proxy); + # other routes ignore these keys. + method: request.request_method, + query_string: request.query_string, + body: request.raw_post + } + ) rescue StandardError => e exception_handler e end From d935d76055105e69fe953c5bd2f1b45ea3496b2d Mon Sep 17 00:00:00 2001 From: Matt Date: Mon, 22 Jun 2026 10:56:17 +0200 Subject: [PATCH 2/6] fix(agent): forward workflow executor response headers through the proxy [PRD-567] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The generic proxy only relayed Content-Type, so response headers set by the executor (e.g. X-Forest-Executor-Version) were dropped — breaking the version gate. Forward every executor response header except hop-by-hop ones, symmetric to the request-header policy. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../routes/workflow/workflow_executor_proxy.rb | 10 ++++++++-- .../workflow/workflow_executor_proxy_spec.rb | 14 +++++++++++--- 2 files changed, 19 insertions(+), 5 deletions(-) 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 b314efb99..d10061585 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 @@ -134,9 +134,15 @@ def titleize_header(rack_name) rack_name.split('_').map(&:capitalize).join('-') end + # Forward every executor response header except hop-by-hop ones, so the version gate + # (X-Forest-Executor-Version) survives the proxy. def forwarded_response_headers(response) - content_type = response.headers['content-type'] || response.headers['Content-Type'] - content_type ? { 'Content-Type' => content_type } : {} + response.headers.each_with_object({}) do |(name, value), acc| + next if name.nil? || SKIPPED_HEADERS.include?(name.to_s.downcase) + next if value.nil? || value.to_s.empty? + + acc[name.to_s] = value.to_s + end end end end 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 2d29fce30..694fc741a 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 @@ -34,7 +34,12 @@ module Workflow Faraday::Response, status: 200, body: { 'id' => run_id, 'state' => 'pending' }, - headers: { 'content-type' => 'application/json' } + headers: { + 'content-type' => 'application/json', + 'x-forest-executor-version' => '1.2.3', + # hop-by-hop response header — must be dropped, not forwarded. + 'transfer-encoding' => 'chunked' + } ) end @@ -149,13 +154,16 @@ def override_config(extra) ) end - it 'returns the executor status, body and Content-Type to the controller' do + it 'returns the executor status, body and response headers (version gate) except hop-by-hop' do result = proxy.handle_request(method: 'GET', headers: headers, params: { 'path' => run_id }) expect(result).to eq( content: { 'id' => run_id, 'state' => 'pending' }, status: 200, - headers: { 'Content-Type' => 'application/json' } + headers: { + 'content-type' => 'application/json', + 'x-forest-executor-version' => '1.2.3' + } ) end end From 1dbe02da92a18fb4d7f2ca2ff04b592435a0212a Mon Sep 17 00:00:00 2001 From: Matt Date: Mon, 22 Jun 2026 11:28:13 +0200 Subject: [PATCH 3/6] chore(agent): drop speculative header name from proxy comments/tests [PRD-567] Generalize the response-header forwarding rationale: the agent forwards every executor header transparently so new executor headers never need an agent change. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../routes/workflow/workflow_executor_proxy.rb | 4 ++-- .../routes/workflow/workflow_executor_proxy_spec.rb | 7 ++++--- 2 files changed, 6 insertions(+), 5 deletions(-) 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 d10061585..5d4037b72 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 @@ -134,8 +134,8 @@ def titleize_header(rack_name) rack_name.split('_').map(&:capitalize).join('-') end - # Forward every executor response header except hop-by-hop ones, so the version gate - # (X-Forest-Executor-Version) survives the proxy. + # Forward every executor response header except hop-by-hop ones, so new executor headers + # never require an agent change (PRD-567: zero breaking, the agent stays a transparent proxy). def forwarded_response_headers(response) response.headers.each_with_object({}) do |(name, value), acc| next if name.nil? || SKIPPED_HEADERS.include?(name.to_s.downcase) 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 694fc741a..2a631b690 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 @@ -36,7 +36,8 @@ module Workflow body: { 'id' => run_id, 'state' => 'pending' }, headers: { 'content-type' => 'application/json', - 'x-forest-executor-version' => '1.2.3', + # arbitrary executor response header — must be forwarded untouched. + 'x-executor-custom' => 'passthrough-value', # hop-by-hop response header — must be dropped, not forwarded. 'transfer-encoding' => 'chunked' } @@ -154,7 +155,7 @@ def override_config(extra) ) end - it 'returns the executor status, body and response headers (version gate) except hop-by-hop' do + it 'returns the executor status, body and response headers except hop-by-hop' do result = proxy.handle_request(method: 'GET', headers: headers, params: { 'path' => run_id }) expect(result).to eq( @@ -162,7 +163,7 @@ def override_config(extra) status: 200, headers: { 'content-type' => 'application/json', - 'x-forest-executor-version' => '1.2.3' + 'x-executor-custom' => 'passthrough-value' } ) end From 877c60733cceb2e68869826f7c44c91a0b013faa Mon Sep 17 00:00:00 2001 From: Matt Date: Tue, 23 Jun 2026 10:09:02 +0200 Subject: [PATCH 4/6] fix(agent): actually apply proxy response headers and drop encoding headers [PRD-567] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on the workflow executor proxy: - ForestController now honors a root data[:headers] so executor response headers (e.g. X-Forest-Executor-Version) reach the HTTP response instead of being silently dropped — the proxy returns headers at the root, but the controller only read content[:headers] (always nil for the proxy). Additive; other routes are unaffected. Covered by a new controller-level spec. - skip accept-encoding (request) and content-encoding (response): the body is re-serialized, so upstream encoding/length no longer match, and forwarding accept-encoding disabled Faraday's transparent gzip decompression. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../workflow/workflow_executor_proxy.rb | 7 +++++-- .../workflow/workflow_executor_proxy_spec.rb | 12 ++++++++--- .../forest_admin_rails/forest_controller.rb | 4 ++++ .../forest_controller_spec.rb | 21 +++++++++++++++++++ 4 files changed, 39 insertions(+), 5 deletions(-) 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 5d4037b72..2341a445f 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 @@ -8,10 +8,13 @@ module Workflow class WorkflowExecutorProxy < AbstractAuthenticatedRoute AGENT_PREFIX = '/_internal/workflow-executions'.freeze EXECUTOR_PREFIX = '/runs'.freeze - # Hop-by-hop headers + those the HTTP client must recompute — never forwarded. + # Never forwarded (request or response): hop-by-hop, Host, and body-framing headers. + # The body is re-serialized, so upstream length/encoding no longer match — and forwarding + # accept-encoding would disable Faraday's transparent gzip decompression. SKIPPED_HEADERS = %w[ connection keep-alive transfer-encoding upgrade te trailer - proxy-authenticate proxy-authorization host content-length + proxy-authenticate proxy-authorization host + content-length content-encoding accept-encoding ].freeze # Substrings that could let the wildcard escape EXECUTOR_PREFIX (traversal, encoded # dots, backslash, null byte). 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 2a631b690..b40fef12c 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 @@ -21,6 +21,8 @@ module Workflow 'CONTENT_LENGTH' => '123', 'HTTP_CONNECTION' => 'keep-alive', 'HTTP_HOST' => 'agent.test', + # Forwarding this would disable Faraday's transparent gzip decompression. + 'HTTP_ACCEPT_ENCODING' => 'gzip, deflate', 'action_dispatch.remote_ip' => '127.0.0.1', 'rack.input' => 'raw-body' } @@ -39,7 +41,9 @@ module Workflow # arbitrary executor response header — must be forwarded untouched. 'x-executor-custom' => 'passthrough-value', # hop-by-hop response header — must be dropped, not forwarded. - 'transfer-encoding' => 'chunked' + 'transfer-encoding' => 'chunked', + # body-framing header — meaningless once the body is re-serialized; must be dropped. + 'content-encoding' => 'gzip' } ) end @@ -144,9 +148,10 @@ def override_config(extra) expect(captured[:url]).to eq("#{executor_url}/runs/#{run_id}/cancel") end - it 'forwards all client headers except hop-by-hop / host / content-length' do + it 'forwards all client headers except hop-by-hop / host / length / encoding' do proxy.handle_request(method: 'GET', headers: headers, params: { 'path' => run_id }) + # Exact match: accept-encoding (in the fixture) must not leak through. expect(captured[:headers]).to eq( 'Authorization' => bearer, 'Cookie' => 'forest_session_token=abc', @@ -155,9 +160,10 @@ def override_config(extra) ) end - it 'returns the executor status, body and response headers except hop-by-hop' do + it 'returns the executor status, body and response headers except hop-by-hop / encoding' do result = proxy.handle_request(method: 'GET', headers: headers, params: { 'path' => run_id }) + # Exact match: transfer-encoding and content-encoding (in fake_response) must be dropped. expect(result).to eq( content: { 'id' => run_id, 'state' => 'pending' }, status: 200, diff --git a/packages/forest_admin_rails/app/controllers/forest_admin_rails/forest_controller.rb b/packages/forest_admin_rails/app/controllers/forest_admin_rails/forest_controller.rb index 2f11afe77..c5cb0d661 100644 --- a/packages/forest_admin_rails/app/controllers/forest_admin_rails/forest_controller.rb +++ b/packages/forest_admin_rails/app/controllers/forest_admin_rails/forest_controller.rb @@ -47,6 +47,10 @@ def forest_response(data = {}) end end + # Proxy routes expose response headers at the root (their `content` is the rendered body, + # so headers can't nest under it like Stream/File/CSV do). + response.headers.merge!(data[:headers]) if data[:headers].is_a?(Hash) + respond_to do |format| format.json { render json: data[:content], status: data[:status] || data[:content][:status] || 200 } format.csv { render plain: data[:content][:export], status: 200, filename: data[:filename] } diff --git a/packages/forest_admin_rails/spec/controllers/forest_admin_rails/forest_controller_spec.rb b/packages/forest_admin_rails/spec/controllers/forest_admin_rails/forest_controller_spec.rb index 0be7ac4ec..9c385dbb7 100644 --- a/packages/forest_admin_rails/spec/controllers/forest_admin_rails/forest_controller_spec.rb +++ b/packages/forest_admin_rails/spec/controllers/forest_admin_rails/forest_controller_spec.rb @@ -316,6 +316,27 @@ module ForestAdminRails expect(response_mock.headers['Access-Control-Expose-Headers']).to eq('Content-Disposition') end end + + context 'when a verbatim-forwarding route returns response headers at the root' do + # The workflow executor proxy returns { content: , status:, headers: }; its `content` + # is the rendered body, so response headers live at the root rather than nested under + # content. Without honoring data[:headers] they would be silently dropped (PRD-567). + let(:proxy_data) do + { + content: { 'id' => 'run-1', 'state' => 'pending' }, + status: 200, + headers: { 'X-Forest-Executor-Version' => '1.2.3' } + } + end + + before { allow(controller).to receive(:respond_to) } + + it 'merges the root headers into the HTTP response' do + controller.send(:forest_response, proxy_data) + + expect(response_mock.headers['X-Forest-Executor-Version']).to eq('1.2.3') + end + end end end end From 6e2ea32586baf9fccc8c9db264cc741ef56051b0 Mon Sep 17 00:00:00 2001 From: Matt Date: Tue, 23 Jun 2026 10:30:41 +0200 Subject: [PATCH 5/6] docs(agent): trim redundant proxy comments to the non-obvious rationale [PRD-567] Co-Authored-By: Claude Opus 4.8 (1M context) --- .../routes/workflow/workflow_executor_proxy.rb | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) 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 2341a445f..3853b90c2 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 @@ -16,8 +16,7 @@ class WorkflowExecutorProxy < AbstractAuthenticatedRoute proxy-authenticate proxy-authorization host content-length content-encoding accept-encoding ].freeze - # Substrings that could let the wildcard escape EXECUTOR_PREFIX (traversal, encoded - # dots, backslash, null byte). + # Fragments that could escape EXECUTOR_PREFIX once decoded. UNSAFE_PATH_FRAGMENTS = ['..', '%2e', '%2E', '\\', "\0"].freeze OPEN_TIMEOUT = 2 REQUEST_TIMEOUT = 120 @@ -25,8 +24,6 @@ class WorkflowExecutorProxy < AbstractAuthenticatedRoute def setup_routes return self unless executor_configured? - # The glob can only ever map into EXECUTOR_PREFIX (build_executor_path rejects - # traversal), so executor routes outside it stay unreachable through the proxy. add_route( 'forest_workflow_executor_proxy', :all, @@ -137,8 +134,6 @@ def titleize_header(rack_name) rack_name.split('_').map(&:capitalize).join('-') end - # Forward every executor response header except hop-by-hop ones, so new executor headers - # never require an agent change (PRD-567: zero breaking, the agent stays a transparent proxy). def forwarded_response_headers(response) response.headers.each_with_object({}) do |(name, value), acc| next if name.nil? || SKIPPED_HEADERS.include?(name.to_s.downcase) From 42f1bc990ab482c41d77bd4c87c233b33816d059 Mon Sep 17 00:00:00 2001 From: Matt Date: Tue, 23 Jun 2026 11:12:28 +0200 Subject: [PATCH 6/6] fix(agent): map all transport-level Faraday errors to 503; correct proxy comments [PRD-567] Review follow-up: - forward now rescues Faraday::Error (SSL, etc.) as ServiceUnavailableError, so a transport failure surfaces as 503 like ConnectionFailed instead of a generic 500. Faraday never raises on executor 4xx/5xx (no raise_error middleware), so this only catches reachability failures. - fix a self-contradictory SKIPPED_HEADERS comment ("re-serialized" vs raw body) and two minor comment imprecisions. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../routes/workflow/workflow_executor_proxy.rb | 9 +++++++-- .../workflow/workflow_executor_proxy_spec.rb | 16 ++++++++++++++++ .../forest_admin_rails/forest_controller.rb | 2 +- 3 files changed, 24 insertions(+), 3 deletions(-) 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 3853b90c2..1ca7ed127 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 @@ -9,7 +9,8 @@ class WorkflowExecutorProxy < AbstractAuthenticatedRoute AGENT_PREFIX = '/_internal/workflow-executions'.freeze EXECUTOR_PREFIX = '/runs'.freeze # Never forwarded (request or response): hop-by-hop, Host, and body-framing headers. - # The body is re-serialized, so upstream length/encoding no longer match — and forwarding + # 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 # accept-encoding would disable Faraday's transparent gzip decompression. SKIPPED_HEADERS = %w[ connection keep-alive transfer-encoding upgrade te trailer @@ -97,6 +98,10 @@ def forward(method, target_url, args) raise Http::Exceptions::ServiceUnavailableError.new('Workflow executor timed out', cause: e) rescue Faraday::ConnectionFailed => e raise Http::Exceptions::ServiceUnavailableError.new('Workflow executor unreachable', cause: e) + # Any other transport-level Faraday failure (SSL, etc.) is an executor-reachability problem, + # not a 500 — Faraday never raises on executor 4xx/5xx (no raise_error middleware). + rescue Faraday::Error => e + raise Http::Exceptions::ServiceUnavailableError.new('Workflow executor request failed', cause: e) end def build_client @@ -107,7 +112,7 @@ def build_client end end - # `env` is the Rack env: real HTTP headers are the HTTP_* keys (+ CONTENT_TYPE); + # `env` is the Rack env: real HTTP headers are the HTTP_* keys (+ CONTENT_TYPE/CONTENT_LENGTH); # rack.*/action_dispatch.*/server vars are not headers and are dropped. def forwarded_request_headers(env) return {} unless env.is_a?(Hash) 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 b40fef12c..c6089b0a6 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 @@ -255,6 +255,22 @@ def override_config(extra) end end + describe 'when the executor connection fails at the transport level (e.g. SSL)' do + before do + allow(Faraday).to receive(:new).and_wrap_original do |original, *args, &block| + connection = original.call(*args, &block) + allow(connection).to receive(:run_request).and_raise(Faraday::SSLError.new('cert')) + connection + end + end + + it 'raises ServiceUnavailableError rather than a generic 500' do + expect do + proxy.handle_request(method: 'GET', headers: headers, params: { 'path' => run_id }) + end.to raise_error(Http::Exceptions::ServiceUnavailableError, /request failed/) + end + end + describe 'Faraday timeout' do it 'applies a single large timeout regardless of the verb' do proxy.handle_request(method: 'POST', headers: headers, params: { 'path' => "#{run_id}/trigger" }, body: '{}') diff --git a/packages/forest_admin_rails/app/controllers/forest_admin_rails/forest_controller.rb b/packages/forest_admin_rails/app/controllers/forest_admin_rails/forest_controller.rb index c5cb0d661..12c4bf93b 100644 --- a/packages/forest_admin_rails/app/controllers/forest_admin_rails/forest_controller.rb +++ b/packages/forest_admin_rails/app/controllers/forest_admin_rails/forest_controller.rb @@ -48,7 +48,7 @@ def forest_response(data = {}) end # Proxy routes expose response headers at the root (their `content` is the rendered body, - # so headers can't nest under it like Stream/File/CSV do). + # so headers can't nest under it like Stream/File do). response.headers.merge!(data[:headers]) if data[:headers].is_a?(Hash) respond_to do |format|