From 6b8bccfb8d6f5519e51a3dd08d969ff1890f0ba5 Mon Sep 17 00:00:00 2001 From: Kayos Date: Tue, 28 Apr 2026 23:07:27 -0700 Subject: [PATCH] clients/ruby: apply audit findings (b1d6e3f -> new) - S1: Client#inspect redacts @token (default ruby inspect walks ivars); to_s aliased and pretty_print overridden so PP doesn't bypass it. - S2: AppToken#inspect/#to_s/#pretty_print redact :token member; nil token still rendered as token=nil for list-row clarity. - S3: validate token name vs [a-z0-9_-]+ in revoke_token and create_token; drops URI.encode_www_form_component path-encoding dependency. - C1: upload_timeout_secs parameter on upload_file (default 60), decoupled from default_timeout/http_timeout_margin so big uploads aren't capped by the run-subprocess timeout. - Q6: clearer multipart filename escape via gsub block form. - C7: dropped unused @uri ivar. - A3: YARD note clarifying http_client: bypasses base_url host/port routing. Test gaps closed: Client/AppToken inspect+pp redaction, AppToken nil-token inspect, revoke_token name validation (path traversal, uppercase, empty, valid), create_token name validation, upload_timeout_secs independence from default_timeout (incl. default==60), Array-form ip_cidrs round-trip, non-JSON 5xx error body kept as String, empty 200 body raises Error. 35 runs / 104 assertions / 0 failures. Audit: memory/clawdforge-audits/ruby-b1d6e3f.md --- clients/ruby/lib/clawdforge/client.rb | 58 +++++++- clients/ruby/lib/clawdforge/models.rb | 17 +++ clients/ruby/test/test_client.rb | 199 +++++++++++++++++++++++++- 3 files changed, 266 insertions(+), 8 deletions(-) diff --git a/clients/ruby/lib/clawdforge/client.rb b/clients/ruby/lib/clawdforge/client.rb index cbbfdc5..ff6ffb1 100644 --- a/clients/ruby/lib/clawdforge/client.rb +++ b/clients/ruby/lib/clawdforge/client.rb @@ -27,6 +27,11 @@ module Clawdforge HTTP_TIMEOUT_MARGIN_SECS = 30 HEALTHZ_TIMEOUT_SECS = 10 ADMIN_TIMEOUT_SECS = 10 + DEFAULT_UPLOAD_TIMEOUT_SECS = 60 + # Server-side `name` validator for app tokens. Mirrors what the service + # accepts so we fail fast (and avoid surprising path-encoding behaviour) + # before issuing the request. + TOKEN_NAME_RE = /\A[a-z0-9_-]+\z/.freeze attr_reader :base_url, :default_model, :default_timeout, :http_timeout_margin @@ -40,7 +45,10 @@ module Clawdforge # @param http_timeout_margin [Integer] seconds added to the subprocess # timeout to derive the HTTP-level timeout. Defaults to 30. # @param http_client [Net::HTTP, nil] optional pre-built Net::HTTP. Mostly - # useful for tests. + # useful for tests. Note: when supplied, the client trusts that the + # Net::HTTP instance is already pointed at the right host/port and + # `use_ssl` is configured correctly — `base_url`'s scheme/host/port are + # only used to construct the request-URI, not to dial. def initialize(base_url:, token:, default_model: DEFAULT_MODEL, default_timeout: DEFAULT_RUN_TIMEOUT_SECS, @@ -55,7 +63,22 @@ module Clawdforge @default_timeout = default_timeout @http_timeout_margin = http_timeout_margin @http_client = http_client - @uri = URI.parse(@base_url) + end + + # Custom `inspect` so the bearer token never lands in logs, exception + # backtraces, irb output, or `pp` dumps. The default Ruby `inspect` + # walks every instance variable including `@token`. + def inspect + "#<#{self.class.name} base_url=#{@base_url.inspect} " \ + "default_model=#{@default_model.inspect} " \ + "default_timeout=#{@default_timeout.inspect} token=[REDACTED]>" + end + alias_method :to_s, :inspect + + # `pp` calls `pretty_print`, not `inspect`. Override too so PP doesn't + # bypass the redaction by walking ivars directly. + def pretty_print(pp) + pp.text(inspect) end # GET /healthz — liveness + `claude --version` smoke check. @@ -107,8 +130,14 @@ module Clawdforge # @param filename [String, nil] override the filename advertised to the # server. Defaults to `File.basename(path)`. # @param content_type [String] MIME type. Defaults to "application/octet-stream". + # @param upload_timeout_secs [Integer] HTTP-level timeout for this upload, + # in seconds. Decoupled from `default_timeout` so a long file upload + # doesn't get capped by the run-subprocess timeout (and vice versa). + # Defaults to 60. # @return [FileToken] - def upload_file(path, ttl_secs: 3600, filename: nil, content_type: "application/octet-stream") + def upload_file(path, ttl_secs: 3600, filename: nil, + content_type: "application/octet-stream", + upload_timeout_secs: DEFAULT_UPLOAD_TIMEOUT_SECS) raise ArgumentError, "no such file: #{path}" unless File.file?(path) advertised_name = filename || File.basename(path) @@ -124,7 +153,7 @@ module Clawdforge :post, "/files", raw_body: body_io, extra_headers: headers, - timeout: @default_timeout + @http_timeout_margin, + timeout: upload_timeout_secs, ) unless payload.is_a?(Hash) raise Error, "unexpected /files response type: #{payload.class}" @@ -143,6 +172,7 @@ module Clawdforge # @param ip_cidrs [Array, nil] per-app IP allowlist. # @return [AppToken] def create_token(name, ip_cidrs: nil) + validate_token_name!(name) body = { "name" => name, "ip_cidrs" => Array(ip_cidrs) } payload = request(:post, "/admin/tokens", json_body: body, timeout: ADMIN_TIMEOUT_SECS) raise Error, "unexpected /admin/tokens response" unless payload.is_a?(Hash) @@ -163,14 +193,25 @@ module Clawdforge # DELETE /admin/tokens/ — revoke a token. # @return [Boolean] true on success. Raises APIError(404) if missing. + # @raise [ArgumentError] if `name` doesn't match `[a-z0-9_-]+`. Validating + # client-side avoids depending on path-segment encoding for safety — + # the server enforces the same shape, so anything else is a 400 in + # waiting. def revoke_token(name) - payload = request(:delete, "/admin/tokens/#{URI.encode_www_form_component(name)}", - timeout: ADMIN_TIMEOUT_SECS) + validate_token_name!(name) + payload = request(:delete, "/admin/tokens/#{name}", timeout: ADMIN_TIMEOUT_SECS) payload.is_a?(Hash) ? !!payload.fetch("ok", true) : true end private + def validate_token_name!(name) + unless name.is_a?(String) && name.match?(TOKEN_NAME_RE) + raise ArgumentError, + "token name must match #{TOKEN_NAME_RE.source} (got: #{name.inspect})" + end + end + # Build a multipart/form-data body containing the `ttl_secs` form field # and the `file` part with the uploaded bytes. Returns a StringIO whose # contents include the entire body (so `Content-Length` is exact). @@ -196,7 +237,10 @@ module Clawdforge def quote_filename(name) # RFC 7578 §4.2 — escape backslashes and double quotes; strip CR/LF. - name.to_s.gsub("\\", "\\\\\\\\").gsub('"', '\\"').delete("\r\n") + # Block form of `gsub` avoids the four-backslash dance of the string + # form (where each `\\` in the replacement is one backslash, so + # `"\\\\"` is the only way to literally produce `\\`). + name.to_s.delete("\r\n").gsub(/[\\"]/) { |c| "\\#{c}" } end def request(method, path, json_body: nil, raw_body: nil, extra_headers: nil, timeout: nil) diff --git a/clients/ruby/lib/clawdforge/models.rb b/clients/ruby/lib/clawdforge/models.rb index 8669168..f9c6fde 100644 --- a/clients/ruby/lib/clawdforge/models.rb +++ b/clients/ruby/lib/clawdforge/models.rb @@ -64,5 +64,22 @@ module Clawdforge enabled: row.fetch("enabled", 1).to_i != 0, ) end + + # Override the Struct-autogenerated `inspect`/`to_s`, which would dump + # every member including the plaintext `:token` from create-time + # responses. We still want to see whether a token field is *present* — + # useful when distinguishing create-response rows from list rows — + # without leaking the bearer itself. + def inspect + tok_state = self[:token].nil? ? "nil" : "[REDACTED]" + "#<#{self.class} name=#{self[:name].inspect} token=#{tok_state} " \ + "ip_cidrs=#{self[:ip_cidrs].inspect} created_at=#{self[:created_at].inspect} " \ + "last_used=#{self[:last_used].inspect} enabled=#{self[:enabled].inspect}>" + end + alias_method :to_s, :inspect + + def pretty_print(pp) + pp.text(inspect) + end end end diff --git a/clients/ruby/test/test_client.rb b/clients/ruby/test/test_client.rb index 2917b5e..0bec1ca 100644 --- a/clients/ruby/test/test_client.rb +++ b/clients/ruby/test/test_client.rb @@ -3,8 +3,9 @@ require "minitest/autorun" require "webmock/minitest" require "json" -require "tempfile" +require "pp" require "stringio" +require "tempfile" require "clawdforge" @@ -319,3 +320,199 @@ class ClientConstructionTest < Minitest::Test assert_equal "http://x:8800", c.base_url end end + +class InspectRedactionTest < Minitest::Test + SECRET = "cf_supersecret_DEADBEEF_must_not_leak" + + def test_client_inspect_redacts_token + client = Clawdforge::Client.new(base_url: BASE_URL, token: SECRET) + out = client.inspect + refute_includes out, SECRET, "Client#inspect leaked bearer" + assert_includes out, "[REDACTED]" + assert_includes out, BASE_URL + end + + def test_client_to_s_redacts_token + client = Clawdforge::Client.new(base_url: BASE_URL, token: SECRET) + refute_includes client.to_s, SECRET + end + + def test_pp_client_does_not_leak_token + client = Clawdforge::Client.new(base_url: BASE_URL, token: SECRET) + buf = StringIO.new + PP.pp(client, buf) + refute_includes buf.string, SECRET, "PP.pp leaked bearer" + assert_includes buf.string, "[REDACTED]" + end + + def test_app_token_inspect_redacts_token + plaintext = "cf_brandnew_zzz_KEEPME_PRIVATE" + t = Clawdforge::AppToken.from_create_response( + "name" => "cauldron", + "token" => plaintext, + "ip_cidrs" => ["10.0.0.0/8"], + ) + + refute_includes t.inspect, plaintext, "AppToken#inspect leaked plaintext token" + refute_includes t.to_s, plaintext, "AppToken#to_s leaked plaintext token" + assert_includes t.inspect, "[REDACTED]" + assert_includes t.inspect, "cauldron" + end + + def test_app_token_inspect_with_nil_token + # List-row form: token is nil. inspect should be informative without + # any "[REDACTED]" noise (there's nothing to redact). + t = Clawdforge::AppToken.from_list_row( + "name" => "petalparse", "ip_cidrs" => "", + "created_at" => 50, "last_used" => nil, "enabled" => 1, + ) + out = t.inspect + assert_includes out, "petalparse" + assert_includes out, "token=nil" + refute_includes out, "[REDACTED]" + end + + def test_pp_app_token_does_not_leak_token + plaintext = "cf_pp_leak_check_xyz" + t = Clawdforge::AppToken.from_create_response( + "name" => "cauldron", "token" => plaintext, "ip_cidrs" => [], + ) + buf = StringIO.new + PP.pp(t, buf) + refute_includes buf.string, plaintext + end +end + +class RevokeTokenValidationTest < Minitest::Test + def test_revoke_token_validates_name_rejects_path_traversal + assert_raises(ArgumentError) { build_client.revoke_token("../etc/passwd") } + end + + def test_revoke_token_validates_name_rejects_uppercase + assert_raises(ArgumentError) { build_client.revoke_token("Cauldron") } + end + + def test_revoke_token_validates_name_rejects_empty + assert_raises(ArgumentError) { build_client.revoke_token("") } + end + + def test_revoke_token_accepts_valid_name + stub_request(:delete, "#{BASE_URL}/admin/tokens/cauldron-1_a").to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate("ok" => true), + ) + assert_equal true, build_client.revoke_token("cauldron-1_a") + end + + def test_create_token_validates_name + assert_raises(ArgumentError) { build_client.create_token("Bad Name!") } + end +end + +class UploadTimeoutTest < Minitest::Test + # Tap into Net::HTTP timeouts the same way the client does — we want to + # prove `upload_timeout_secs` flows through to the HTTP layer and is + # *independent* of `default_timeout`. + class TimeoutCapturingHTTP + attr_accessor :open_timeout, :read_timeout, :write_timeout + + def request(_req) + Struct.new(:code, :message, :body).new("200", "OK", JSON.generate( + "file_token" => "ff_x", "ttl_secs" => 60, "size" => 1, + )) + end + end + + def test_upload_timeout_secs_independent_from_default_timeout + http = TimeoutCapturingHTTP.new + client = Clawdforge::Client.new( + base_url: BASE_URL, token: TOKEN, + default_timeout: 5, # short + http_client: http, + ) + + Tempfile.create(["t", ".bin"]) do |tf| + tf.binmode + tf.write("x") + tf.flush + + client.upload_file(tf.path, ttl_secs: 60, upload_timeout_secs: 300) + end + + # Timeout used must reflect upload_timeout_secs, not default_timeout + + # http_timeout_margin (which would be 5 + 30 = 35). + assert_equal 300, http.read_timeout + assert_equal 300, http.open_timeout + assert_equal 300, http.write_timeout + end + + def test_upload_timeout_secs_default_is_60 + http = TimeoutCapturingHTTP.new + client = Clawdforge::Client.new( + base_url: BASE_URL, token: TOKEN, + default_timeout: 5, + http_client: http, + ) + + Tempfile.create(["t", ".bin"]) do |tf| + tf.binmode + tf.write("x") + tf.flush + + client.upload_file(tf.path, ttl_secs: 60) + end + + assert_equal 60, http.read_timeout + end +end + +class ListTokensArrayCidrsTest < Minitest::Test + def test_list_tokens_array_form_ip_cidrs_round_trip + stub_request(:get, "#{BASE_URL}/admin/tokens").to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate( + "tokens" => [ + { + "name" => "multi", "ip_cidrs" => ["10.0.0.0/8", "192.168.0.0/16"], + "created_at" => 100, "last_used" => 200, "enabled" => 1, + }, + ], + ), + ) + + toks = build_client.list_tokens + + assert_equal 1, toks.length + assert_equal ["10.0.0.0/8", "192.168.0.0/16"], toks[0].ip_cidrs + end +end + +class NonJsonErrorBodyTest < Minitest::Test + def test_non_json_error_body_kept_as_string + stub_request(:post, "#{BASE_URL}/run").to_return( + status: 503, + headers: { "Content-Type" => "text/html" }, + body: "upstream down", + ) + + err = assert_raises(Clawdforge::APIError) { build_client.run(prompt: "hi") } + assert_equal 503, err.status + assert_kind_of String, err.body + assert_match(/upstream down/, err.body) + end +end + +class EmptyResponseBodyTest < Minitest::Test + def test_empty_200_body_on_run_raises_unexpected_response_type + stub_request(:post, "#{BASE_URL}/run").to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: "", + ) + + err = assert_raises(Clawdforge::Error) { build_client.run(prompt: "hi") } + assert_match(/unexpected \/run response type/, err.message) + end +end