Security hardening: job_id validation, error redaction, parse guard (closes 9 LIVE tickets)

[NewSense21]

Summary

Consolidated security hardening in lib/screenshot/client.rb that addresses 9 findings from the APPSEC-409 scan of ruby-screenshots. All changes are scoped to one file (plus a small exception-class refactor) and preserve the existing rescue Screenshot::*Error interface.

Tickets closed

Component (6):

• LIVE-20563 F-002 — Unsanitized job_id horizontal IDOR
• LIVE-20564 F-003 — Unsanitized job_id remote API path manipulation
• LIVE-20567 F-006 — Basic Auth credential leakage via inspect
• LIVE-20568 F-007 — Unguarded nil dereference on non-JSON 200 body
• LIVE-20571 F-010 — CRLF injection via job_id (Ruby 1.8.7 Net::HTTP)
• LIVE-20572 F-011 — Raw response body embedded in exception messages

Chains (3 — auto-close when their chain-breakers above ship):

• LIVE-20574 C-001 — error-body leakage → credential exfil → IDOR (breaker: F-011)
• LIVE-20575 C-002 — CRLF → credential exfil → IDOR (breaker: F-010)
• LIVE-20576 C-003 — TOCTOU → nil-parse → process crash (breaker: F-007)

Changes

1. job_id allowlist (closes F-002, F-003, F-010, chain C-002)

JOB_ID_FORMAT = /\A[\w\-]{1,64}\z/

def screenshots_status job_id
  validate_job_id! job_id   # NEW
  ...
end

def screenshots job_id
  validate_job_id! job_id   # NEW
  ...
end

A single allowlist enforced before the value is interpolated into the URL path. Blocks IDOR enumeration via unexpected formats, path traversal (../), and CRLF (\r\n) injection in one rule.

2. parse rejects non-Hash results (closes F-007, chain C-003)

def parse(response)
  parser = Yajl::Parser.new(:symbolize_keys => true)
  result = parser.parse(response.body)
  unless result.is_a?(Hash)
    raise ParseError, "Expected a JSON object from BrowserStack API, got #{result.class}"
  end
  result
end

Empty/non-JSON 200 bodies now raise a typed Screenshot::ParseError instead of letting NoMethodError: undefined method [] for nil:NilClass escape past consumer rescue Screenshot::* blocks.

3. Exception body redaction (closes F-011, chain C-001)

class APIError < StandardError
  attr_reader :body
  def initialize(message = nil, body = nil)
    super(message)
    @body = body
  end
end

class AuthenticationError < APIError; end
# ... InvalidRequestError, ScreenshotNotAllowedError, UnexpectedError

http_response_code_check now raises with a fixed "BrowserStack API responded NNN" message; the response body is opt-in via e.body. APM/log capture no longer auto-ingests the body alongside the receiver's instance variables.

4. Client#inspect / #to_s redaction (closes F-006)

def inspect
  "#<#{self.class.name}:0x... @authentication=[REDACTED]>"
end
alias_method :to_s, :inspect

APM error trackers (Sentry/Bugsnag/Datadog) that serialise the exception receiver can no longer recover the reversible Base64-encoded Basic Auth credential.

Backwards compatibility

• AuthenticationError/InvalidRequestError/ScreenshotNotAllowedError/UnexpectedError now subclass APIError (which is a StandardError), so existing rescue Screenshot::*Error blocks still catch them.
• Breaking: the exception .message format has changed — callers parsing the previously JSON-encoded message string will break. Migration: use the new #body reader.
• Breaking: job_id values outside [\w\-]{1,64} now raise ArgumentError instead of being silently sent to the API. This is the intended security gain.

Out of scope (separate tickets)

• LIVE-20561/F-001 + LIVE-20565/F-004 — yajl-ruby bump (separate PR LIVE-20561 + LIVE-20566: Bump yajl-ruby to >= 1.4.3 (CVE-2022-24795) and commit Gemfile.lock #15)
• LIVE-20566/F-005 — commit Gemfile.lock (small follow-up)
• LIVE-20569/F-008 — pin Docker image in CI workflow (small follow-up)
• LIVE-20570/F-009 + LIVE-20573/F-012 — public-API refactors (return full response, collapse poll/result calls) — needs product sign-off; not bundled here

Test plan

• ruby -c lib/screenshot/client.rb — syntax (passes locally).
• Manual smoke: instantiate Client, call screenshots_status("abc-123") against staging — succeeds.
• screenshots_status("../foo") and screenshots_status("abc\r\nHost: x") — both raise ArgumentError before any HTTP request is made.
• Trigger 401/422 from API — exception .message no longer contains body; e.body returns the raw body.
• Screenshot::Client.new(...).inspect returns @authentication=[REDACTED].
• Simulate a 200 response with empty body — raises Screenshot::ParseError (catchable by rescue Screenshot::ParseError or StandardError), not NoMethodError.

🤖 Generated with claude-flow