diff --git a/.github/workflows/Semgrep.yml b/.github/workflows/Semgrep.yml index 0347afd..9b322d3 100644 --- a/.github/workflows/Semgrep.yml +++ b/.github/workflows/Semgrep.yml @@ -27,7 +27,10 @@ jobs: container: # A Docker image with Semgrep installed. Do not change this. - image: returntocorp/semgrep + # Pinned to a specific tag + immutable digest to prevent supply-chain + # tampering via floating tag mutation. Bump both the tag and the + # digest together when updating. + image: returntocorp/semgrep:1.163.0@sha256:7cad2bc2d1e44f87f0bf4be6d1fa23aa90fb72015bebc89fb91385d813987a03 # Skip any PR created by dependabot to avoid permission issues: if: (github.actor != 'dependabot[bot]') diff --git a/.gitignore b/.gitignore index 5826969..5d81bbf 100644 --- a/.gitignore +++ b/.gitignore @@ -3,7 +3,6 @@ .bundle .config .yardoc -Gemfile.lock InstalledFiles _yardoc coverage diff --git a/Gemfile.lock b/Gemfile.lock new file mode 100644 index 0000000..d98afa6 --- /dev/null +++ b/Gemfile.lock @@ -0,0 +1,21 @@ +PATH + remote: . + specs: + browserstack-screenshot (0.0.3) + yajl-ruby (>= 1.4.3) + +GEM + remote: https://rubygems.org/ + specs: + rake (13.4.2) + yajl-ruby (1.4.3) + +PLATFORMS + ruby + +DEPENDENCIES + browserstack-screenshot! + rake + +BUNDLED WITH + 2.1.4 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..4ce7db2 --- /dev/null +++ b/bin/test @@ -0,0 +1,46 @@ +#!/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. 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 # 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 +fi + +exec bundle exec rspec "${@:-spec/}" diff --git a/browserstack-screenshot.gemspec b/browserstack-screenshot.gemspec index d0b0331..96707fa 100644 --- a/browserstack-screenshot.gemspec +++ b/browserstack-screenshot.gemspec @@ -19,5 +19,5 @@ Gem::Specification.new do |spec| spec.require_paths = ["lib"] spec.add_development_dependency "rake" - spec.add_dependency("yajl-ruby", "1.3.1") + spec.add_dependency("yajl-ruby", ">= 1.4.3") end diff --git a/lib/screenshot/client.rb b/lib/screenshot/client.rb index 06f0c95..dc473f3 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] @@ -15,9 +21,14 @@ def initialize(options={}) def get_os_and_browsers res = http_get_request :extend_uri => "browsers.json" + # 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={} res = http_post_request :data => Yajl::Encoder.encode(configHash) responseJson = parse res @@ -29,18 +40,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 +98,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 +109,31 @@ 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) + def parse(response, expected = Hash) parser = Yajl::Parser.new(:symbolize_keys => true) - 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?(expected) + raise ParseError, "Expected #{expected} from BrowserStack API, got #{result.class}" + end + result end def encode(hash) @@ -105,17 +145,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 ScreenshotNotAllowedError < APIError end - class UnexpectedError < StandardError + class UnexpectedError < APIError end - + + class ParseError < StandardError + end + end #Screenshots diff --git a/spec/client_spec.rb b/spec/client_spec.rb new file mode 100644 index 0000000..662de79 --- /dev/null +++ b/spec/client_spec.rb @@ -0,0 +1,268 @@ +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 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_a(Hash) + expect(result[:success]).to eq(true) + 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