Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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'
)

Expand All @@ -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
)

Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
Loading