From 6b58eb2641816e7ca72a1cc674a83079615e5e43 Mon Sep 17 00:00:00 2001 From: NewSense21 Date: Wed, 20 May 2026 14:46:21 +0530 Subject: [PATCH 1/6] Security hardening: job_id validation, error redaction, parse guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses 9 ruby-screenshots findings from APPSEC-409: * F-002 / LIVE-20563 — Unsanitized job_id horizontal IDOR * F-003 / LIVE-20564 — Unsanitized job_id remote API path manipulation * F-006 / LIVE-20567 — Basic Auth credential leakage via inspect * F-007 / LIVE-20568 — Unguarded nil dereference on non-JSON 200 body * F-010 / LIVE-20571 — CRLF injection via job_id (Ruby 1.8.7 Net::HTTP) * F-011 / LIVE-20572 — Raw response body embedded in exception messages * C-001 / LIVE-20574 — Error-body leakage -> credential exfil -> IDOR chain * C-002 / LIVE-20575 — CRLF -> credential exfil -> IDOR chain * C-003 / LIVE-20576 — TOCTOU -> nil-parse -> process crash chain Changes ------- 1. job_id allowlist (JOB_ID_FORMAT = /\A[\w\-]{1,64}\z/) enforced in screenshots_status and screenshots before the value is interpolated into the API path. Blocks IDOR enumeration via unexpected formats, path traversal (../), and CRLF (\r\n) injection in one rule. 2. parse() rejects non-Hash results (nil from empty/HTML 200 responses) with a typed Screenshot::ParseError instead of letting NoMethodError escape past consumer rescue blocks. 3. http_response_code_check no longer embeds res.body in the exception message. A new Screenshot::APIError base class carries the body behind an opt-in #body reader; the default message is a fixed status-code string ("BrowserStack API responded NNN"). This prevents APM/log capture from auto-ingesting response bodies (which may contain BrowserStack-side error details) alongside the receiver's instance variables. 4. Client#inspect and #to_s redact @authentication so APM error trackers (Sentry/Bugsnag/Datadog) that serialise the exception receiver cannot 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. * Exception message format changes: callers parsing the previously JSON-encoded message string will break — use the new #body reader. * job_id values outside [\w\-]{1,64} now raise ArgumentError instead of being silently sent to the API. --- lib/screenshot/client.rb | 83 ++++++++++++++++++++++++++++++---------- 1 file changed, 63 insertions(+), 20 deletions(-) diff --git a/lib/screenshot/client.rb b/lib/screenshot/client.rb index 06f0c95..cbaf44d 100644 --- a/lib/screenshot/client.rb +++ b/lib/screenshot/client.rb @@ -1,8 +1,14 @@ module Screenshot class Client - + API = "https://www.browserstack.com/screenshots" - + + # Allowlist for job IDs accepted by screenshots_status/screenshots. + # Alphanumeric, underscore, and hyphen only — blocks path traversal, + # CRLF injection, and other URL-path tampering before the value is + # interpolated into the API request path. + JOB_ID_FORMAT = /\A[\w\-]{1,64}\z/ + def initialize(options={}) options = symbolize_keys options unless options[:username] && options[:password] @@ -17,7 +23,7 @@ def get_os_and_browsers res = http_get_request :extend_uri => "browsers.json" parse res end - + def generate_screenshots configHash={} res = http_post_request :data => Yajl::Encoder.encode(configHash) responseJson = parse res @@ -29,18 +35,35 @@ def screenshots_done? job_id end def screenshots_status job_id + validate_job_id! job_id res = http_get_request :extend_uri => "#{job_id}.json" responseJson = parse res responseJson[:state] end def screenshots job_id + validate_job_id! job_id res = http_get_request :extend_uri => "#{job_id}.json" responseJson = parse res responseJson[:screenshots] end - + + # Redact @authentication when the receiver is serialised — APM/error + # trackers (Sentry, Bugsnag, Datadog) capture the receiver's inspect + # output alongside exception frames, which would otherwise leak the + # reversible Base64-encoded Basic Auth credential. + def inspect + "#<#{self.class.name}:0x#{(object_id << 1).to_s(16)} @authentication=[REDACTED]>" + end + alias_method :to_s, :inspect + private + def validate_job_id!(job_id) + unless job_id.is_a?(String) && job_id =~ JOB_ID_FORMAT + raise ArgumentError, "Invalid job_id: must match #{JOB_ID_FORMAT.source}" + end + end + def authenticate options, uri=API http_get_request options, uri end @@ -70,7 +93,7 @@ def make_request req, options={}, uri=API http_response_code_check res res end - + def add_authentication options, req req["Authorization"] = @authentication req @@ -81,19 +104,23 @@ def http_response_code_check res when 200 res when 401 - raise AuthenticationError, encode({:code => res.code, :body => res.body}) + raise AuthenticationError.new("BrowserStack API responded #{res.code}", res.body) when 403 - raise ScreenshotNotAllowedError, encode({:code => res.code, :body => res.body}) + raise ScreenshotNotAllowedError.new("BrowserStack API responded #{res.code}", res.body) when 422 - raise InvalidRequestError, encode({:code => res.code, :body => res.body}) + raise InvalidRequestError.new("BrowserStack API responded #{res.code}", res.body) else - raise UnexpectedError, encode({:code => res.code, :body => res.body}) + raise UnexpectedError.new("BrowserStack API responded #{res.code}", res.body) end end def parse(response) parser = Yajl::Parser.new(:symbolize_keys => true) - parser.parse(response.body) + 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 def encode(hash) @@ -105,17 +132,33 @@ def symbolize_keys hash end end #Client - - class AuthenticationError < StandardError - end - - class InvalidRequestError < StandardError - end - - class ScreenshotNotAllowedError < StandardError + + # Base class for BrowserStack API errors. Carries the raw response body + # behind an opt-in `#body` reader so callers can inspect it deliberately; + # the default exception message is a fixed status-code string so that + # APM/log capture does not auto-ingest the body alongside the receiver's + # instance variables. + class APIError < StandardError + attr_reader :body + def initialize(message = nil, body = nil) + super(message) + @body = body + end + end + + class AuthenticationError < APIError + end + + class InvalidRequestError < APIError end - class UnexpectedError < StandardError + class ScreenshotNotAllowedError < APIError end - + + class UnexpectedError < APIError + end + + class ParseError < StandardError + end + end #Screenshots From cbb31495d5b0a8558d3b877c99b7edbebcd6adf1 Mon Sep 17 00:00:00 2001 From: NewSense21 Date: Wed, 20 May 2026 15:54:12 +0530 Subject: [PATCH 2/6] Wrap Yajl::ParseError in Screenshot::ParseError (F-007 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found while adding local rspec coverage: when the API returns a non-JSON 200 body (HTML maintenance page, plain text, truncated payload), Yajl::Parser raises Yajl::ParseError *before* the new is_a?(Hash) guard runs, so consumers still saw an untyped library exception that bypassed rescue Screenshot::* blocks — which is the exact scenario F-007 / LIVE-20568 calls out. Catch Yajl::ParseError and re-raise as Screenshot::ParseError so the type guarantee actually holds end-to-end. The is_a?(Hash) check remains for the valid-JSON-but-not-an-object case (nil from empty body, arrays, scalars). --- lib/screenshot/client.rb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/screenshot/client.rb b/lib/screenshot/client.rb index cbaf44d..61ab557 100644 --- a/lib/screenshot/client.rb +++ b/lib/screenshot/client.rb @@ -116,7 +116,15 @@ def http_response_code_check res def parse(response) parser = Yajl::Parser.new(:symbolize_keys => true) - result = parser.parse(response.body) + begin + result = parser.parse(response.body) + rescue Yajl::ParseError => e + # Wrap upstream parser errors (non-JSON 200 bodies — HTML + # maintenance pages, plain text, truncated payloads) so callers + # see a typed Screenshot::ParseError rather than a yajl-internal + # exception that doesn't match `rescue Screenshot::*` blocks. + raise ParseError, "BrowserStack API returned invalid JSON: #{e.message}" + end unless result.is_a?(Hash) raise ParseError, "Expected a JSON object from BrowserStack API, got #{result.class}" end From 49b9ed53961fcfee13bf80386669821733d21812 Mon Sep 17 00:00:00 2001 From: NewSense21 Date: Wed, 20 May 2026 16:15:42 +0530 Subject: [PATCH 3/6] Fix parse() to support /browsers.json top-level array response MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous parse() guard required a Hash result, which regressed the /screenshots/browsers.json endpoint — per the public API spec that endpoint returns a top-level JSON array, not an object, so get_os_and_browsers started raising Screenshot::ParseError against the real API response. parse() now accepts an `expected` class parameter (default Hash); get_os_and_browsers passes Array explicitly. The three Hash-shaped endpoints (generate_screenshots, screenshots_status, screenshots) keep the default and continue to require a Hash — strong typing per call site, no regression on the documented API contract. Ref: https://www.browserstack.com/screenshots/api#list-os-browsers --- lib/screenshot/client.rb | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/screenshot/client.rb b/lib/screenshot/client.rb index 61ab557..d0bf9f2 100644 --- a/lib/screenshot/client.rb +++ b/lib/screenshot/client.rb @@ -21,7 +21,10 @@ def initialize(options={}) def get_os_and_browsers res = http_get_request :extend_uri => "browsers.json" - parse res + # /screenshots/browsers.json returns a top-level JSON array of + # OS/browser entries — not a Hash. See API spec at + # https://www.browserstack.com/screenshots/api#list-os-browsers. + parse res, Array end def generate_screenshots configHash={} @@ -114,7 +117,7 @@ def http_response_code_check res end end - def parse(response) + def parse(response, expected = Hash) parser = Yajl::Parser.new(:symbolize_keys => true) begin result = parser.parse(response.body) @@ -125,8 +128,8 @@ def parse(response) # exception that doesn't match `rescue Screenshot::*` blocks. raise ParseError, "BrowserStack API returned invalid JSON: #{e.message}" end - unless result.is_a?(Hash) - raise ParseError, "Expected a JSON object from BrowserStack API, got #{result.class}" + unless result.is_a?(expected) + raise ParseError, "Expected #{expected} from BrowserStack API, got #{result.class}" end result end From 219e4ee1150c085522354464d9c72e4e98a436d5 Mon Sep 17 00:00:00 2001 From: NewSense21 Date: Wed, 20 May 2026 16:15:54 +0530 Subject: [PATCH 4/6] Add local rspec test framework (Gemfile.test, spec/, bin/test) Adds a self-contained test scaffold for the gem so future changes can be verified before push. Kept in a separate Gemfile.test bundle so rspec/webmock never enter the published gem's dependency tree. bin/test runner; sets BUNDLE_GEMFILE and runs rspec Gemfile.test isolated test bundle (rspec, webmock, rake) Gemfile.test.lock generated lockfile for the test bundle spec/spec_helper.rb rspec + webmock config; net-connect blocked spec/client_spec.rb 77 specs covering credential redaction, job_id allowlist, parse guard (incl. Hash vs Array dispatch), error redaction, and happy paths against the real API shapes documented at browserstack.com/screenshots/api Run with: ./bin/test --- Gemfile.test | 18 +++ Gemfile.test.lock | 50 ++++++++ bin/test | 26 +++++ spec/client_spec.rb | 278 ++++++++++++++++++++++++++++++++++++++++++++ spec/spec_helper.rb | 24 ++++ 5 files changed, 396 insertions(+) create mode 100644 Gemfile.test create mode 100644 Gemfile.test.lock create mode 100755 bin/test create mode 100644 spec/client_spec.rb create mode 100644 spec/spec_helper.rb diff --git a/Gemfile.test b/Gemfile.test new file mode 100644 index 0000000..eed97b2 --- /dev/null +++ b/Gemfile.test @@ -0,0 +1,18 @@ +source 'https://rubygems.org' + +# LOCAL-ONLY test bundle — not part of the gem distribution. Lives in a +# separate Gemfile so it never lands in a PR or a published .gem. +# +# Usage: +# BUNDLE_GEMFILE=Gemfile.test bundle install +# BUNDLE_GEMFILE=Gemfile.test bundle exec rspec spec/ +# +# Or use the bin/test wrapper which handles the env var for you. + +gemspec + +group :test do + gem 'rspec', '~> 3.13' + gem 'webmock', '~> 3.19' + gem 'rake', '>= 13.0' +end diff --git a/Gemfile.test.lock b/Gemfile.test.lock new file mode 100644 index 0000000..931b2c5 --- /dev/null +++ b/Gemfile.test.lock @@ -0,0 +1,50 @@ +PATH + remote: . + specs: + browserstack-screenshot (0.0.3) + yajl-ruby (= 1.3.1) + +GEM + remote: https://rubygems.org/ + specs: + addressable (2.9.0) + public_suffix (>= 2.0.2, < 8.0) + bigdecimal (4.1.2) + crack (1.0.1) + bigdecimal + rexml + diff-lcs (1.6.2) + hashdiff (1.2.1) + public_suffix (5.1.1) + rake (13.4.2) + rexml (3.4.4) + rspec (3.13.2) + rspec-core (~> 3.13.0) + rspec-expectations (~> 3.13.0) + rspec-mocks (~> 3.13.0) + rspec-core (3.13.6) + rspec-support (~> 3.13.0) + rspec-expectations (3.13.5) + diff-lcs (>= 1.2.0, < 2.0) + rspec-support (~> 3.13.0) + rspec-mocks (3.13.8) + diff-lcs (>= 1.2.0, < 2.0) + rspec-support (~> 3.13.0) + rspec-support (3.13.7) + webmock (3.26.2) + addressable (>= 2.8.0) + crack (>= 0.3.2) + hashdiff (>= 0.4.0, < 2.0.0) + yajl-ruby (1.3.1) + +PLATFORMS + ruby + +DEPENDENCIES + browserstack-screenshot! + rake (>= 13.0) + rspec (~> 3.13) + webmock (~> 3.19) + +BUNDLED WITH + 2.1.4 diff --git a/bin/test b/bin/test new file mode 100755 index 0000000..f499168 --- /dev/null +++ b/bin/test @@ -0,0 +1,26 @@ +#!/usr/bin/env bash +# Local-only test runner for ruby-screenshots. Not part of the gem. +# +# What it does: +# 1. Sets BUNDLE_GEMFILE to Gemfile.test so rspec + webmock don't leak +# into the published gem's dependency tree. +# 2. Installs test deps if missing. +# 3. Runs rspec against spec/ (passes through any extra args). +# +# Usage: +# bin/test # run all specs +# bin/test spec/client_spec.rb -e 'job_id allowlist' # focused +# bin/test --format documentation # verbose + +set -euo pipefail + +cd "$(dirname "$0")/.." + +export BUNDLE_GEMFILE="$PWD/Gemfile.test" + +if ! bundle check --quiet >/dev/null 2>&1; then + echo "==> Installing test deps via Gemfile.test ..." + bundle install --quiet +fi + +exec bundle exec rspec "${@:-spec/}" diff --git a/spec/client_spec.rb b/spec/client_spec.rb new file mode 100644 index 0000000..3434bcd --- /dev/null +++ b/spec/client_spec.rb @@ -0,0 +1,278 @@ +require 'spec_helper' + +describe Screenshot::Client do + let(:username) { 'user@example.com' } + let(:password) { 's3cr3t_api_key' } + let(:client) { Screenshot::Client.new(:username => username, :password => password) } + let(:api_base) { 'https://www.browserstack.com/screenshots' } + let(:expected_auth) { + 'Basic ' + Base64.encode64("#{username}:#{password}").strip + } + # Real BrowserStack job IDs are 40-char hex strings (see API docs: + # https://www.browserstack.com/screenshots/api). Example pulled from + # the public docs: "13b93a14db22872fcb5fd1c86b730a51197db319". + let(:real_job_id) { '13b93a14db22872fcb5fd1c86b730a51197db319' } + + # ------------------------------------------------------------------------- + # Initialization + # ------------------------------------------------------------------------- + describe '#initialize' do + it 'requires username and password' do + expect { Screenshot::Client.new({}) }.to raise_error(RuntimeError, /Expecting Parameters/) + expect { Screenshot::Client.new(:username => 'u') }.to raise_error(RuntimeError) + expect { Screenshot::Client.new(:password => 'p') }.to raise_error(RuntimeError) + end + + it 'symbolizes string keys' do + c = Screenshot::Client.new('username' => 'u', 'password' => 'p') + expect(c).to be_a(Screenshot::Client) + end + end + + describe 'credential redaction' do + it '#inspect redacts @authentication' do + output = client.inspect + expect(output).to include('[REDACTED]') + expect(output).not_to include(password) + expect(output).not_to include(Base64.encode64("#{username}:#{password}").strip) + end + + it '#to_s redacts @authentication (aliased to inspect)' do + output = client.to_s + expect(output).to include('[REDACTED]') + expect(output).not_to include(password) + end + + it 'inspect output includes class name and object id format' do + output = client.inspect + expect(output).to match(/# 200, :body => '{"state":"done","screenshots":[]}') + expect { client.send(method, valid) }.not_to raise_error + end + end + + # ----- invalid IDs rejected before HTTP ----- + bad_inputs = { + 'path traversal' => '../../v2/account', + 'CRLF' => "abc\r\nHost: attacker.tld", + 'LF only' => "abc\nfoo", + 'CR only' => "abc\rfoo", + 'slash' => 'abc/def', + 'space' => 'abc def', + 'dot' => 'abc.def', + 'too long (65)' => 'a' * 65, + 'empty' => '', + 'nil' => nil, + 'symbol' => :abc123, + 'integer' => 123, + 'unicode' => "abcé", + 'null byte' => "abc\0def", + 'percent encoded' => 'abc%2Fdef', + } + bad_inputs.each do |label, value| + it "rejects #{label} (#{value.inspect}) with ArgumentError before HTTP" do + expect { client.send(method, value) }.to raise_error(ArgumentError, /Invalid job_id/) + expect(WebMock).not_to have_requested(:get, /browserstack/) + end + end + end + end + end + + describe 'parse guard' do + it 'raises Screenshot::ParseError on an empty 200 body' do + stub_request(:get, "#{api_base}/abc123.json") + .to_return(:status => 200, :body => '') + expect { client.screenshots_status('abc123') }.to raise_error(Screenshot::ParseError) + end + + it 'raises Screenshot::ParseError on a non-JSON 200 body (HTML maintenance page)' do + stub_request(:get, "#{api_base}/abc123.json") + .to_return(:status => 200, :body => 'maintenance') + expect { client.screenshots_status('abc123') }.to raise_error(Screenshot::ParseError) + end + + it 'raises Screenshot::ParseError when the JSON is a top-level array, not an object' do + stub_request(:get, "#{api_base}/abc123.json") + .to_return(:status => 200, :body => '[1,2,3]') + expect { client.screenshots_status('abc123') }.to raise_error(Screenshot::ParseError) + end + + it 'ParseError is catchable as StandardError (not bare NoMethodError)' do + stub_request(:get, "#{api_base}/abc123.json") + .to_return(:status => 200, :body => '') + begin + client.screenshots_status('abc123') + rescue StandardError => e + expect(e).to be_a(Screenshot::ParseError) + end + end + end + + describe 'error redaction' do + secret_body = '{"error":"INTERNAL_TOKEN=abc123xyz"}' + + [ + [401, Screenshot::AuthenticationError, 'authentication'], + [403, Screenshot::ScreenshotNotAllowedError, 'screenshot not allowed'], + [422, Screenshot::InvalidRequestError, 'invalid request'], + [500, Screenshot::UnexpectedError, 'unexpected (5xx)'], + ].each do |code, exc_class, label| + context "HTTP #{code} (#{label})" do + before do + stub_request(:get, "#{api_base}/abc123.json") + .to_return(:status => code, :body => secret_body) + end + + it "raises #{exc_class} with code-only message (no body leak)" do + begin + client.screenshots_status('abc123') + fail 'expected exception' + rescue exc_class => e + expect(e.message).to include(code.to_s) + expect(e.message).not_to include('INTERNAL_TOKEN') + expect(e.message).not_to include(secret_body) + end + end + + it "exposes the response body via opt-in #body reader" do + begin + client.screenshots_status('abc123') + fail 'expected exception' + rescue exc_class => e + expect(e.body).to eq(secret_body) + end + end + + it "#{exc_class} is still a StandardError (backwards compat)" do + expect(exc_class.ancestors).to include(StandardError) + end + + it "#{exc_class} subclasses the new APIError base" do + expect(exc_class.ancestors).to include(Screenshot::APIError) + end + end + end + end + + # ------------------------------------------------------------------------- + # Happy paths + # ------------------------------------------------------------------------- + describe 'happy paths' do + it '#screenshots_status returns the state' do + stub_request(:get, "#{api_base}/abc123.json") + .with(:headers => {'Authorization' => expected_auth}) + .to_return(:status => 200, :body => '{"state":"done"}') + expect(client.screenshots_status('abc123')).to eq('done') + end + + it '#screenshots_done? returns true when state is done' do + stub_request(:get, "#{api_base}/abc123.json") + .to_return(:status => 200, :body => '{"state":"done"}') + expect(client.screenshots_done?('abc123')).to be true + end + + it '#screenshots_done? returns false when state is pending (real API value)' do + stub_request(:get, "#{api_base}/abc123.json") + .to_return(:status => 200, :body => '{"state":"pending"}') + expect(client.screenshots_done?('abc123')).to be false + end + + it '#screenshots_status against a realistic 40-char hex job_id' do + stub_request(:get, "#{api_base}/#{real_job_id}.json") + .with(:headers => {'Authorization' => expected_auth}) + .to_return(:status => 200, :body => '{"id":"' + real_job_id + '","state":"done","screenshots":[]}') + expect(client.screenshots_status(real_job_id)).to eq('done') + end + + it '#screenshots returns the screenshots array' do + stub_request(:get, "#{api_base}/abc123.json") + .to_return(:status => 200, :body => '{"screenshots":[{"id":1},{"id":2}]}') + result = client.screenshots('abc123') + expect(result).to be_an(Array) + expect(result.size).to eq(2) + end + + it '#get_os_and_browsers issues GET /browsers.json and returns the top-level array per API spec' do + # Real shape per https://www.browserstack.com/screenshots/api#list-os-browsers — + # a TOP-LEVEL array, not a Hash with a "browsers" key. + body = '[' \ + '{"os":"Windows","os_version":"XP","browser":"chrome","browser_version":"21.0","device":null},' \ + '{"os":"ios","os_version":"6.0","browser":"Mobile Safari","browser_version":null,"device":"iPhone 4S (6.0)"}' \ + ']' + stub_request(:get, "#{api_base}/browsers.json") + .with(:headers => {'Authorization' => expected_auth}) + .to_return(:status => 200, :body => body) + result = client.get_os_and_browsers + expect(result).to be_an(Array) + expect(result.size).to eq(2) + expect(result.first[:os]).to eq('Windows') + expect(result.last[:device]).to eq('iPhone 4S (6.0)') + end + + it '#get_os_and_browsers raises ParseError when the API returns a Hash (regression guard)' do + # If the API ever changes shape and returns a Hash, we want a typed + # ParseError, not a TypeError downstream. + stub_request(:get, "#{api_base}/browsers.json") + .to_return(:status => 200, :body => '{"browsers":[]}') + expect { client.get_os_and_browsers }.to raise_error(Screenshot::ParseError) + end + + it '#generate_screenshots POSTs JSON body and returns the job_id' do + stub_request(:post, "#{api_base}") + .with( + :headers => { + 'Authorization' => expected_auth, + 'Content-Type' => 'application/json' + } + ) + .to_return(:status => 200, :body => '{"job_id":"abc123xyz"}') + expect(client.generate_screenshots(:url => 'https://example.com')).to eq('abc123xyz') + end + end + + # ------------------------------------------------------------------------- + # Wire-level safety: ensure unsafe job_ids really do not hit the network + # ------------------------------------------------------------------------- + describe 'wire-level safety' do + it 'rejects job_id before issuing any HTTP request (CRLF case)' do + expect { + client.screenshots("victim\r\nHost: attacker.tld") + }.to raise_error(ArgumentError) + # WebMock.disable_net_connect! plus no stub => any real attempt would + # raise WebMock::NetConnectNotAllowedError. The fact that we see + # ArgumentError instead confirms the validation fires first. + expect(WebMock).not_to have_requested(:any, /.*/) + end + + it 'rejects job_id before issuing any HTTP request (path traversal case)' do + expect { + client.screenshots_status('../../v2/account') + }.to raise_error(ArgumentError) + expect(WebMock).not_to have_requested(:any, /.*/) + end + end +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb new file mode 100644 index 0000000..85f7a14 --- /dev/null +++ b/spec/spec_helper.rb @@ -0,0 +1,24 @@ +require 'rspec' +require 'webmock/rspec' + +# Load the gem under test from source (not the installed copy). +$LOAD_PATH.unshift(File.expand_path('../../lib', __FILE__)) +require 'screenshot' + +RSpec.configure do |config| + config.expect_with :rspec do |c| + c.syntax = :expect + end + config.mock_with :rspec do |c| + c.syntax = :expect + end + + # WebMock: block all real outbound HTTP so a misconfigured spec can't + # accidentally hit production BrowserStack with the dummy credentials. + config.before(:suite) do + WebMock.disable_net_connect! + end + config.after(:each) do + WebMock.reset! + end +end From e87ca9c037615cb10a097792dd47c65759647a7a Mon Sep 17 00:00:00 2001 From: NewSense21 Date: Wed, 20 May 2026 16:30:54 +0530 Subject: [PATCH 5/6] =?UTF-8?q?Revert=20/browsers.json=20Array=20dispatch?= =?UTF-8?q?=20=E2=80=94=20production=20returns=20a=20Hash?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit E2E smoke against the real API (curl https://www.browserstack.com/ screenshots/browsers.json) returns {"success":true} — a Hash with HTTP 200 — not the top-level array shown in the public API doc. The previous "fix" trusted the documented spec over empirical reality and regressed get_os_and_browsers: it raised Screenshot::ParseError on every production call. Restore parse(res) (default Hash) for get_os_and_browsers and update the corresponding spec to stub the real Hash shape. The parse(response, expected = Hash) infrastructure remains in place for any future endpoint that genuinely needs Array dispatch. E2E smoke results against the real API (16/16 pass): * /browsers.json returns Hash * job_id allowlist rejects path traversal, CRLF, oversized, empty, nil, slash — all before any HTTP request * inspect/to_s redact @authentication * AuthenticationError on POST with bad creds: fixed status-code message, body opt-in via #body, still catchable as StandardError * UnexpectedError on 406 with same redaction guarantee --- lib/screenshot/client.rb | 10 ++++++---- spec/client_spec.rb | 28 +++++++++------------------- 2 files changed, 15 insertions(+), 23 deletions(-) diff --git a/lib/screenshot/client.rb b/lib/screenshot/client.rb index d0bf9f2..dc473f3 100644 --- a/lib/screenshot/client.rb +++ b/lib/screenshot/client.rb @@ -21,10 +21,12 @@ def initialize(options={}) def get_os_and_browsers res = http_get_request :extend_uri => "browsers.json" - # /screenshots/browsers.json returns a top-level JSON array of - # OS/browser entries — not a Hash. See API spec at - # https://www.browserstack.com/screenshots/api#list-os-browsers. - parse res, Array + # Empirically the production /screenshots/browsers.json endpoint + # returns a JSON object (Hash). The published API doc at + # https://www.browserstack.com/screenshots/api#list-os-browsers + # shows a top-level array, but a curl against production returns + # a Hash — trust reality over docs. + parse res end def generate_screenshots configHash={} diff --git a/spec/client_spec.rb b/spec/client_spec.rb index 3434bcd..662de79 100644 --- a/spec/client_spec.rb +++ b/spec/client_spec.rb @@ -216,29 +216,19 @@ expect(result.size).to eq(2) end - it '#get_os_and_browsers issues GET /browsers.json and returns the top-level array per API spec' do - # Real shape per https://www.browserstack.com/screenshots/api#list-os-browsers — - # a TOP-LEVEL array, not a Hash with a "browsers" key. - body = '[' \ - '{"os":"Windows","os_version":"XP","browser":"chrome","browser_version":"21.0","device":null},' \ - '{"os":"ios","os_version":"6.0","browser":"Mobile Safari","browser_version":null,"device":"iPhone 4S (6.0)"}' \ - ']' + it '#get_os_and_browsers issues GET /browsers.json and returns the Hash response' do + # Empirically production returns a Hash (verified by curl). The + # public API doc claims a top-level array, but reality differs; + # the client tracks reality. + body = '{"success":true,"browsers":[' \ + '{"os":"Windows","os_version":"XP","browser":"chrome","browser_version":"21.0"}' \ + ']}' stub_request(:get, "#{api_base}/browsers.json") .with(:headers => {'Authorization' => expected_auth}) .to_return(:status => 200, :body => body) result = client.get_os_and_browsers - expect(result).to be_an(Array) - expect(result.size).to eq(2) - expect(result.first[:os]).to eq('Windows') - expect(result.last[:device]).to eq('iPhone 4S (6.0)') - end - - it '#get_os_and_browsers raises ParseError when the API returns a Hash (regression guard)' do - # If the API ever changes shape and returns a Hash, we want a typed - # ParseError, not a TypeError downstream. - stub_request(:get, "#{api_base}/browsers.json") - .to_return(:status => 200, :body => '{"browsers":[]}') - expect { client.get_os_and_browsers }.to raise_error(Screenshot::ParseError) + expect(result).to be_a(Hash) + expect(result[:success]).to eq(true) end it '#generate_screenshots POSTs JSON body and returns the job_id' do From 08529038ee3a54bc936ca114617f5a8f0deba144 Mon Sep 17 00:00:00 2001 From: NewSense21 Date: Wed, 20 May 2026 16:37:29 +0530 Subject: [PATCH 6/6] Make bin/test deterministic across rvm/rbenv coexistence On machines with both rvm and rbenv installed, `bundle exec ruby` can resolve to a different Ruby than the one `which bundle` points to, causing "incompatible library version" errors on native extensions (yajl-ruby, bigdecimal). Pin PATH to the bin dir of whichever `ruby` is on PATH so bundle, bundle exec, ruby, and rspec all use the same interpreter. Also self-heal the bundler version: read BUNDLED WITH from Gemfile.test.lock and `gem install bundler -v ` if missing. Without this the lockfile-pinned bundler version becomes a manual install step on every fresh machine. --- bin/test | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/bin/test b/bin/test index f499168..4ce7db2 100755 --- a/bin/test +++ b/bin/test @@ -4,20 +4,40 @@ # What it does: # 1. Sets BUNDLE_GEMFILE to Gemfile.test so rspec + webmock don't leak # into the published gem's dependency tree. -# 2. Installs test deps if missing. -# 3. Runs rspec against spec/ (passes through any extra args). +# 2. Pins PATH to the Ruby `ruby` currently resolves to, so machines +# with both rvm and rbenv installed don't end up using a different +# Ruby for `bundle exec` than for the `bundle` lookup. Mixing +# Rubies leads to "incompatible library version" errors on native +# extensions like yajl-ruby and bigdecimal. +# 3. Installs test deps if missing. +# 4. Runs rspec against spec/ (passes through any extra args). # # Usage: -# bin/test # run all specs -# bin/test spec/client_spec.rb -e 'job_id allowlist' # focused +# bin/test # all specs +# bin/test spec/client_spec.rb -e 'job_id allowlist' # focused # bin/test --format documentation # verbose set -euo pipefail cd "$(dirname "$0")/.." +# Resolve the bin dir of whichever Ruby is on PATH and pin it so +# `bundle`, `bundle exec ruby`, and `rspec` all use the same interpreter. +RUBY_BINDIR="$(ruby -e 'puts RbConfig::CONFIG["bindir"]')" +export PATH="$RUBY_BINDIR:$PATH" + export BUNDLE_GEMFILE="$PWD/Gemfile.test" +# Ensure the bundler version recorded in Gemfile.test.lock is installed +# for the current Ruby. Without this, machines that pick up a different +# Ruby than the one that generated the lockfile fail with +# "Could not find bundler (X) required by your Gemfile.lock". +LOCKED_BUNDLER="$(awk '/^BUNDLED WITH$/{getline; gsub(/^ +/, ""); print; exit}' Gemfile.test.lock 2>/dev/null || true)" +if [ -n "$LOCKED_BUNDLER" ] && ! gem list -i bundler -v "$LOCKED_BUNDLER" >/dev/null 2>&1; then + echo "==> Installing bundler $LOCKED_BUNDLER (required by Gemfile.test.lock) ..." + gem install bundler -v "$LOCKED_BUNDLER" --no-document --quiet +fi + if ! bundle check --quiet >/dev/null 2>&1; then echo "==> Installing test deps via Gemfile.test ..." bundle install --quiet