LIVE-20561 + LIVE-20566: Bump yajl-ruby to >= 1.4.3 (CVE-2022-24795) and commit Gemfile.lock#15
Merged
Merged
Conversation
yajl-ruby was pinned to exactly 1.3.1, which has a heap corruption in yajl_buf_append (GHSA-jj47-x69x-mxrm / CVE-2022-24795, CVSS 9.8). All four API methods in lib/screenshot/client.rb invoke parse() on the HTTP response body, making the vulnerable C parser reachable on every call path. Fixed by allowing >= 1.4.3, which contains the upstream integer- overflow fix.
Abk-P
previously approved these changes
May 20, 2026
Merged
6 tasks
Locks the transitive dependency tree so this repo's CI and developer machines install identical versions: yajl-ruby 1.4.3 (the CVE-2022-24795 fix from F-001), rake 13.4.2. Removes Gemfile.lock from .gitignore. This is a deviation from the standard Bundler convention of not committing the lock for a gem (since consumers resolve from gemspec ranges, not the gem's lockfile), but F-005 explicitly requires it for supply-chain reproducibility of the gem's own CI and development environment.
NewSense21
changed the title
Abk-P
previously approved these changes
May 20, 2026
…closes 9 LIVE tickets) (#16) * Security hardening: job_id validation, error redaction, parse guard 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. * Wrap Yajl::ParseError in Screenshot::ParseError (F-007 follow-up) 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). * Fix parse() to support /browsers.json top-level array response 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 * 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 * Revert /browsers.json Array dispatch — production returns a Hash 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 * 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 <that>` if missing. Without this the lockfile-pinned bundler version becomes a manual install step on every fresh machine.
The Semgrep workflow used the unpinned floating tag `returntocorp/semgrep` (resolving to `:latest`), allowing a future tag mutation to introduce attacker-controlled code into CI. Pinning to `1.163.0@sha256:7cad2bc2...` makes the image immutable; this matches the SHA-pinning pattern already used for the actions/checkout and codeql-action steps in the same workflow.
Abk-P
approved these changes
May 20, 2026
ParthPrajapati43
approved these changes
May 20, 2026
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two related supply-chain fixes for
ruby-screenshots:yajl-rubydependency inbrowserstack-screenshot.gemspecfrom the exact pin"1.3.1"to">= 1.4.3", which contains the upstream integer-overflow fix for GHSA-jj47-x69x-mxrm (CVSS 9.8).Gemfile.lockpinning the transitive tree (yajl-ruby 1.4.3, rake 13.4.2) and removesGemfile.lockfrom.gitignore.Why bump yajl
All four API methods in
lib/screenshot/client.rb(get_os_and_browsers,generate_screenshots,screenshots_status,screenshots) callYajl::Parser#parseon the HTTP response body, so the vulnerable C parser is reachable on every call path.Why commit Gemfile.lock
This deviates from the standard Bundler convention of not committing a lockfile for a gem (since consumers resolve from gemspec ranges, not the gem's lockfile). But F-005 explicitly requires it for supply-chain reproducibility of the gem's own CI and development environment, and there's no downside for consumers.
Risk
Very low. Pin loosens from exact to lower-bound; 1.4.3 is API-compatible (same
Yajl::Parser#parseinterface). Lockfile only affects this repo's development/CI environment, not gem consumers.Closes
Test plan
bundle installresolves to yajl-ruby 1.4.3 (locked).🤖 Generated with claude-flow