From 0dc93e5bb627465a1f1619783b5b4d87d26db9e7 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 16 Sep 2026 21:44:39 +0000 Subject: [PATCH] Coerce container-valued query params and scrub handoff state on read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three hardenings the hosted platform made to its own copy of the dashboard controllers in September, ported to the engine so a host that mounts it gets the same behaviour. A query value can arrive as a container (`minutes[]=1&minutes[]=2`, or `page[x]=1`), and neither Array nor ActionController::Parameters responds to `to_i`. Every list the dashboard paginates or windows read those params directly and turned a malformed query into a 500. Api::BaseController gains integer_param and clamped_param: a multi-valued param means its first value, a nested object floors to the default. Applied to agent runs, agent analytics, the account analytics, interactions and session recordings. Sandboxes#compare read `providers` as a list. A bare string or a nested object raised NoMethodError; it is now a 400. A recording's show response returned the handoff state — a copy of the visitor's cookies and web storage — unscrubbed, both as its own key and nested inside metadata, while every other read path redacted. Both are now stripped of cookies, session_storage and local_storage; only #handoff returns them, to the owner, when they continue the session. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01DkXcH6XVKyD5wYnpLuH9iE --- .../action_agent/api/agent_runs_controller.rb | 4 +- .../action_agent/api/agents_controller.rb | 8 +-- .../action_agent/api/analytics_controller.rb | 2 +- .../action_agent/api/base_controller.rb | 20 ++++++ .../api/interactions_controller.rb | 6 +- .../action_agent/api/sandboxes_controller.rb | 7 +- .../api/session_recordings_controller.rb | 31 +++++++-- actionagent/test/param_coercion_test.rb | 66 +++++++++++++++++++ .../test/session_recording_privacy_test.rb | 23 +++++++ 9 files changed, 149 insertions(+), 18 deletions(-) create mode 100644 actionagent/test/param_coercion_test.rb diff --git a/actionagent/app/controllers/action_agent/api/agent_runs_controller.rb b/actionagent/app/controllers/action_agent/api/agent_runs_controller.rb index 805fb1d4..d30bc27e 100644 --- a/actionagent/app/controllers/action_agent/api/agent_runs_controller.rb +++ b/actionagent/app/controllers/action_agent/api/agent_runs_controller.rb @@ -33,8 +33,8 @@ def index scope = scope.where(agent_id: params[:agent_id]) if params[:agent_id].present? scope = scope.where(status: params[:status]) if params[:status].present? - page = (params[:page] || 1).to_i - per_page = (params[:per_page] || 20).to_i + page = integer_param(:page, default: 1) + per_page = integer_param(:per_page, default: 20) total = scope.count runs = scope.offset((page - 1) * per_page).limit(per_page) diff --git a/actionagent/app/controllers/action_agent/api/agents_controller.rb b/actionagent/app/controllers/action_agent/api/agents_controller.rb index 3c4a4bed..3a7d25dd 100644 --- a/actionagent/app/controllers/action_agent/api/agents_controller.rb +++ b/actionagent/app/controllers/action_agent/api/agents_controller.rb @@ -132,9 +132,9 @@ def restore # observed from telemetry have no AgentRun rows at all, so a runs-only # list showed them as empty while their scorecard reported real traffic. def runs - minutes = params[:minutes].presence&.then { |m| m.to_i.clamp(1, 60 * 24 * 90) } - page = (params[:page] || 1).to_i - per_page = (params[:per_page] || 20).to_i + minutes = integer_param(:minutes)&.clamp(1, 60 * 24 * 90) + page = integer_param(:page, default: 1) + per_page = integer_param(:per_page, default: 20) executions = AgentExecutions.new( agents: [ @agent ], @@ -282,7 +282,7 @@ def tool_roster # with all-zero metrics beside a card and a runs list reporting real # traffic. def analytics - days = (params[:days] || 30).to_i + days = integer_param(:days, default: 30) start_date = days.days.ago.beginning_of_day runs = @agent.agent_runs.where("created_at >= ?", start_date) diff --git a/actionagent/app/controllers/action_agent/api/analytics_controller.rb b/actionagent/app/controllers/action_agent/api/analytics_controller.rb index 1bf51b36..2f799cb5 100644 --- a/actionagent/app/controllers/action_agent/api/analytics_controller.rb +++ b/actionagent/app/controllers/action_agent/api/analytics_controller.rb @@ -5,7 +5,7 @@ module Api class AnalyticsController < BaseController # GET /api/analytics def index - days = (params[:days] || 30).to_i + days = integer_param(:days, default: 30) start_date = days.days.ago.beginning_of_day # Table names are interpolated rather than written literally: the diff --git a/actionagent/app/controllers/action_agent/api/base_controller.rb b/actionagent/app/controllers/action_agent/api/base_controller.rb index 960d6e7a..07ebd9df 100644 --- a/actionagent/app/controllers/action_agent/api/base_controller.rb +++ b/actionagent/app/controllers/action_agent/api/base_controller.rb @@ -129,6 +129,26 @@ def require_execution_enabled! render json: { error: "Agent execution is disabled on this dashboard" }, status: :forbidden end + # An integer query param. A value can arrive as a container + # (`minutes[]=1&minutes[]=2`, or `page[x]=1`), and neither Array nor + # ActionController::Parameters responds to `to_i`: reading them + # directly raised NoMethodError and turned a malformed query into a + # 500. A multi-valued param means its first value; anything else that + # is not a scalar falls back to the default. + def integer_param(name, default: nil) + raw = params[name] + raw = raw.first if raw.is_a?(Array) + return default if raw.blank? || !(raw.is_a?(String) || raw.is_a?(Numeric)) + + raw.to_s.to_i + end + + # integer_param, then clamped into [min, max]. Non-numeric input becomes + # 0 and is then clamped up to `min`. + def clamped_param(name, default:, min:, max:) + integer_param(name, default: default).clamp(min, max) + end + def not_found render json: { error: "Record not found" }, status: :not_found end diff --git a/actionagent/app/controllers/action_agent/api/interactions_controller.rb b/actionagent/app/controllers/action_agent/api/interactions_controller.rb index 17c1fec0..1bb7a479 100644 --- a/actionagent/app/controllers/action_agent/api/interactions_controller.rb +++ b/actionagent/app/controllers/action_agent/api/interactions_controller.rb @@ -16,7 +16,7 @@ class InteractionsController < BaseController # GET /api/interactions def index - limit = params.fetch(:limit, DEFAULT_LIMIT).to_i.clamp(1, 200) + limit = clamped_param(:limit, default: DEFAULT_LIMIT, min: 1, max: 200) contexts = interactions_scope .includes(:contextable) @@ -140,8 +140,8 @@ def traces_for_agent(agent_id) def window_minutes return @window_minutes if defined?(@window_minutes) - raw = params[:minutes].presence - @window_minutes = raw ? raw.to_i.clamp(1, MAX_WINDOW_MINUTES) : nil + raw = integer_param(:minutes) + @window_minutes = raw ? raw.clamp(1, MAX_WINDOW_MINUTES) : nil end def interactions_scope diff --git a/actionagent/app/controllers/action_agent/api/sandboxes_controller.rb b/actionagent/app/controllers/action_agent/api/sandboxes_controller.rb index 5600ac7f..42f4084d 100644 --- a/actionagent/app/controllers/action_agent/api/sandboxes_controller.rb +++ b/actionagent/app/controllers/action_agent/api/sandboxes_controller.rb @@ -18,11 +18,16 @@ class SandboxesController < BaseController # POST /api/sandboxes/compare # Run multiple providers in a single sandbox using parallel generation jobs def compare - providers = params[:providers] || %w[anthropic openai ollama] + providers = params[:providers].nil? ? %w[anthropic openai ollama] : params[:providers] task = params[:task] sandbox_id = params[:sandbox_id] return render json: { error: "Task required" }, status: :bad_request unless task.present? + # A bare string or a nested object is a malformed request, not a list + # of one provider: reading it as a list raised NoMethodError. + unless providers.is_a?(Array) && providers.all? { |name| name.is_a?(String) } + return render json: { error: "providers must be a list of provider names" }, status: :bad_request + end return render json: { error: "At least 2 providers required" }, status: :bad_request if providers.size < 2 # Validate providers diff --git a/actionagent/app/controllers/action_agent/api/session_recordings_controller.rb b/actionagent/app/controllers/action_agent/api/session_recordings_controller.rb index e54dcab7..c9fbf78b 100644 --- a/actionagent/app/controllers/action_agent/api/session_recordings_controller.rb +++ b/actionagent/app/controllers/action_agent/api/session_recordings_controller.rb @@ -13,6 +13,12 @@ class SessionRecordingsController < BaseController before_action :set_recording, only: [ :show, :actions, :snapshot, :export, :handoff ] + # Browser state that must never leave the server in a read response: + # the handoff state a recording carries is a copy of the visitor's + # cookies and web storage. Only #handoff returns it, to the owner, when + # they continue the session. + SENSITIVE_STATE_KEYS = %w[cookies session_storage local_storage].freeze + # GET /api/session_recordings # List recordings with optional filters def index @@ -36,8 +42,8 @@ def index end # Pagination - page = (params[:page] || 1).to_i - per_page = [ (params[:per_page] || 20).to_i, 100 ].min + page = integer_param(:page, default: 1) + per_page = [ integer_param(:per_page, default: 20), 100 ].min offset = (page - 1) * per_page total = recordings.count @@ -79,10 +85,10 @@ def actions # Support pagination for large recordings if params[:after_sequence].present? - actions = actions.where("sequence > ?", params[:after_sequence].to_i) + actions = actions.where("sequence > ?", integer_param(:after_sequence, default: 0)) end - limit = [ params[:limit]&.to_i || 100, 500 ].min + limit = [ integer_param(:limit, default: 100), 500 ].min actions = actions.limit(limit) render json: { @@ -332,7 +338,7 @@ def recording_detail(recording) created_at: recording.created_at.iso8601, updated_at: recording.updated_at.iso8601, timeline: recording.timeline, - handoff_state: recording.metadata["handoff_state"], + handoff_state: safe_handoff_state(recording.metadata["handoff_state"]), agent: recording.agent_run&.agent&.slice(:id, :name), sandbox_session: recording.sandbox_session&.summary } @@ -343,9 +349,20 @@ def first_screenshot_url(recording) action&.screenshot_url(expires_in: 1.hour) end + # Strips the browser state at the top level and inside handoff_state, + # which the model stores nested (a recording's metadata carries the + # handoff as one key), so a show response never ships a session cookie. def safe_metadata(metadata) - # Remove sensitive data from metadata - metadata.except("cookies", "session_storage", "local_storage") + safe = (metadata || {}).except(*SENSITIVE_STATE_KEYS) + return safe unless safe["handoff_state"].is_a?(Hash) + + safe.merge("handoff_state" => safe_handoff_state(safe["handoff_state"])) + end + + def safe_handoff_state(handoff_state) + return handoff_state unless handoff_state.is_a?(Hash) + + handoff_state.except(*SENSITIVE_STATE_KEYS) end def generate_visitor_id diff --git a/actionagent/test/param_coercion_test.rb b/actionagent/test/param_coercion_test.rb new file mode 100644 index 00000000..e139fdd5 --- /dev/null +++ b/actionagent/test/param_coercion_test.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +require "test_helper" + +# A query value can arrive as a container (`minutes[]=1&minutes[]=2`, or +# `page[x]=1`), and neither Array nor ActionController::Parameters responds +# to `to_i`. Reading them directly raised NoMethodError and turned a +# malformed query into a 500 on every list the dashboard paginates or +# windows. A multi-valued param means its first value; a nested object is +# malformed and floors to the default. +class ParamCoercionTest < ActionDispatch::IntegrationTest + def setup + ActionAgent::Agent.delete_all + @agent = ActionAgent::Agent.create!(name: "Support", provider: "openai", model: "gpt-4o-mini") + @agent.agent_runs.create!(input_prompt: "hi", output: "hello", status: :complete) + end + + test "agent runs coerce container-valued minutes, page and per_page" do + get "/activeagents/api/agents/#{@agent.id}/runs", params: { minutes: [ 1, 2 ], page: { x: 1 }, per_page: [ 5 ] } + + assert_response :success, response.body + body = JSON.parse(response.body) + assert_equal 1, body["runs"].length + assert_equal 5, body["meta"]["per_page"] + assert_equal 1, body["meta"]["page"] + end + + test "agent analytics coerces a container-valued days param" do + get "/activeagents/api/agents/#{@agent.id}/analytics", params: { days: [ 7, 30 ] } + + assert_response :success, response.body + end + + test "interactions coerce container-valued minutes and limit" do + context = ActionAgent::AgentContext.create!(contextable: @agent, agent_name: "SupportAgent", action_name: "respond") + context.add_user_message("Where is order 88213?") + + get "/activeagents/api/interactions", params: { minutes: [ 60, 120 ], limit: { n: 10 } } + + assert_response :success, response.body + assert_equal 1, JSON.parse(response.body)["interactions"].length + end + + test "session recordings coerce container-valued page, per_page, after_sequence and limit" do + recording = ActionAgent::SessionRecording.start_user_session!(page_url: "https://example.com/") + recording.record_action!(action_type: "click", selector: "button") + + get "/activeagents/api/session_recordings", params: { page: [ 1 ], per_page: { n: 20 } } + assert_response :success, response.body + assert_equal 1, JSON.parse(response.body).dig("pagination", "page") + + get "/activeagents/api/session_recordings/#{recording.id}/actions", params: { after_sequence: [ 0 ], limit: { n: 5 } } + assert_response :success, response.body + assert_equal 1, JSON.parse(response.body)["actions"].size + end + + test "sandbox compare rejects a providers value that is not a list of names" do + sandbox = ActionAgent::SandboxSession.create!(session_id: SecureRandom.uuid, status: :ready, expires_at: 1.hour.from_now) + + post "/activeagents/api/sandboxes/compare", params: { task: "Take a screenshot", providers: "anthropic", sandbox_id: sandbox.session_id } + assert_response :bad_request, response.body + + post "/activeagents/api/sandboxes/compare", params: { task: "Take a screenshot", providers: { a: "anthropic" }, sandbox_id: sandbox.session_id } + assert_response :bad_request, response.body + end +end diff --git a/actionagent/test/session_recording_privacy_test.rb b/actionagent/test/session_recording_privacy_test.rb index cfa96127..2e953b94 100644 --- a/actionagent/test/session_recording_privacy_test.rb +++ b/actionagent/test/session_recording_privacy_test.rb @@ -48,6 +48,29 @@ def setup assert_nil exported["metadata"]["password"] assert_not_includes response.body, "hunter2secret" end + + # The handoff state is a copy of the visitor's browser: cookies and web + # storage. Stripping only the top level of the metadata left the same + # secrets readable one key down, and as the top-level handoff_state key. + test "show strips cookies and web storage from the nested handoff state" do + @recording.update!(metadata: @recording.metadata.merge( + "handoff_state" => { + "url" => "https://example.com/checkout", + "cookies" => [ { "name" => "_session", "value" => "sekrit-cookie" } ], + "local_storage" => { "auth_token" => "lst-secret" }, + "session_storage" => { "csrf" => "sst-secret" } + } + )) + + get "/activeagents/api/session_recordings/#{@recording.id}" + + assert_response :success + body = JSON.parse(response.body)["recording"] + assert_equal "https://example.com/checkout", body.dig("handoff_state", "url") + assert_nil body.dig("handoff_state", "cookies") + assert_nil body.dig("metadata", "handoff_state", "cookies") + %w[sekrit-cookie lst-secret sst-secret].each { |secret| assert_not_includes response.body, secret } + end end # In a per-user install the list has to show the recordings the caller can