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
This commit is contained in:
parent
1b097a21be
commit
6b8bccfb8d
3 changed files with 266 additions and 8 deletions
|
|
@ -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<String>, 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/<name> — 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)
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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: "<html><body>upstream down</body></html>",
|
||||
)
|
||||
|
||||
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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue