security: address APS-19018, APS-19019, APS-19020, APS-19024#82
security: address APS-19018, APS-19019, APS-19020, APS-19024#82Jimesh-browserstack wants to merge 5 commits intomasterfrom
Conversation
The session logs URL returned by the BrowserStack API was previously fetched without validation. A malicious or compromised API response could redirect the SDK to fetch from an attacker-controlled host. - Add validateBrowserStackUrl(String) helper in AutomateClient that parses the URL via java.net.URI and asserts: * scheme equals "https" (rejects http://) * URI.getHost() (lowercased) ends with ".browserstack.com" Host parsing via URI prevents the host-suffix-spoof footgun (https://automate.browserstack.com.attacker.example/...). - Call the guard as the first line inside getSessionLogs(Session) before any HTTP request is constructed. - Add hermetic AutomateClientSecurityTest covering: attacker host, http scheme, suffix-spoof, trusted host (also exercises the APS-19019 auth-guard fix). Resolves: APS-19018
…-19019] Two related leaks in BrowserStackClient: 1. setProxy() reassigned a static HTTP_TRANSPORT field, leaking proxy state across every BrowserStackClient/AutomateClient in the same JVM. Move HTTP_TRANSPORT to an instance field (httpTransport) and update newRequestFactory() (no longer static) and setProxy() to use it. No other readers of HTTP_TRANSPORT existed in the repo (verified via search_code). 2. checkAuthState() guarded with `accessKey == null && username == null` — only fired when both creds were missing, so a half-configured client would proceed and emit a malformed Basic auth header. Switch to `||` (either-null is an error) and tighten the exception message. Tests: - BrowserStackClientProxyIsolationTest verifies cross-instance isolation via reflection: setProxy on client A must not change client B's httpTransport. - AutomateClientSecurityTest (added in APS-19018 commit) covers checkAuthState: throws when username null, throws when accessKey null, passes when both present. BREAKING CHANGE for consumers relying on undocumented behavior: anyone whose code called setProxy on one client and expected it to apply to other clients in the same JVM must now call setProxy on each client individually. Resolves: APS-19019
…-19020] CI / release supply-chain hardening: - .github/workflows/ci.yml: * pin actions/checkout from @v2 to commit SHA 34e114876b0b11c390a56381ad16ebd13914f8d5 (v4.3.1) * pin actions/setup-java from @V3 to commit SHA c1e323688fd81a25caa38c78aa6df2d33d3e20d9 (v4.8.0) * add top-level `permissions: contents: read` block (mirrors the pattern already used in .github/workflows/Semgrep.yml in this repo). Token-scope minimization for the workflow. - pom.xml: nexus-staging-maven-plugin's <autoReleaseAfterClose> flipped true -> false. After mvn release:perform, the staged release on OSSRH must now be promoted manually. The release runbook will be updated separately. - CHANGELOG.md: new file with an Unreleased section summarizing all three security fixes (APS-19018, APS-19019, APS-19020) and calling out the setProxy behaviour change for consumers. Resolves: APS-19020
Test & regression evidence (per change)For each change, listing what the fix was tested against and what no-regression evidence we have. Anywhere coverage is incomplete is called out explicitly so reviewers can decide whether to gate on it. APS-19018 — SSRF guard on
|
| Branch | Tests run | Pass | Errors | Notes |
|---|---|---|---|---|
master |
25 | 24 | 1 | AppAutomateClientTest.testGetSession IOOBE — test BS account has no App Automate builds. Pre-existing. |
| this branch | 33 | 32 | 1 | Same single pre-existing error; +8 new tests, all green. |
No new failures; the 1 error is reproducible on master with identical signature. Verified by running mvn test on a clean checkout of master immediately after, comparing line-for-line.
Live smoke harness output (captured 2026-05-07)
[OK] getSession returned. logUrl=https://automate.browserstack.com/builds/.../sessions/.../logs
[OK] getSessionLogs accepted real logUrl. Body length=214392
preview=2026-5-7 13:37:14:126 REQUEST [...] POST /session/.../execute {"t...
[OK] SSRF guard threw on attacker host: AutomateException: Untrusted logs URL host: attacker.example
[OK] Cross-instance isolation: A changed, B untouched.
[OK] setProxy still routes traffic — request failed at proxy: AutomateException: Connect to 127.0.0.1:61783 [/127.0.0.1] failed: Connection refused
ALL SMOKE TESTS PASSED
Honest gaps (intentional, called out for reviewer discretion)
- PR's CI workflow run — still pending at time of this comment. This is the conclusive proof for the APS-19020 SHA-pinning + permissions-block fix. If you'd like to gate merge on CI green (which I'd recommend), please wait for it. If CI doesn't fire automatically, it may need a maintainer's "Approve and run" click.
autoReleaseAfterClose=falseend-to-end — only verifiable by a release dry-run, which is post-merge. The release runbook (Confluence page 538705969) will be updated separately to cover the new manualClose → Releasestep on https://oss.sonatype.org/.- No live BrowserStack session test gate — by design; this is a pure HTTP/JSON SDK, not a session-runner repo. The smoke harness above stands in for what the bs-session test gate would otherwise enforce.
APS-19020 verification — offline (without waiting on CI)Closing the "CI is the only proof" gap from the previous comment with four independent offline checks. Combined, these establish the SHA-pinning + permissions-block change is correct and the build still works, without depending on the PR's own workflow run. 1. Pinned SHAs are the canonical tag commitsResolved both pinned SHAs via the GitHub git-refs API: Both match the SHAs in 2. The exact workflow build command runs green locallyRan Exit 0. Build succeeded with the new 3. Workflow permissions audit —
|
| Step | What it does | Token scope needed |
|---|---|---|
actions/checkout@v4.3.1 |
Clones the repo | contents: read |
actions/setup-java@v4.8.0 (with cache: 'maven') |
Installs JDK; cache uses Actions Cache API (not GITHUB_TOKEN-scoped) | contents: read |
mvn clean install -DskipTests -Dgpg.skip |
Local build, writes only to runner FS | none |
No git push, no gh ..., no artifact upload, no comment posting, no API call that requires a write token. permissions: contents: read is exactly the minimum needed.
4. Cross-reference: existing Semgrep.yml already uses this pattern
The repo's existing .github/workflows/Semgrep.yml runs on every push/PR/cron and uses:
permissions:
contents: readat the top level, with job-level escalation only where it needs security-events: write for SARIF upload. Same pattern, in production, in this repo, today. Our ci.yml follows the same model — minus the SARIF escalation since the build doesn't upload anything.
What's left in the "actually run on GitHub-hosted Ubuntu" gap
The only thing the offline checks don't directly cover is "the pinned actions resolve and execute on a GitHub-hosted Ubuntu runner." Confidence is very high (these are the canonical official SHAs for the named versions, and the sister workflow uses the same Actions + cache mechanism in this repo today), but not 100% until the workflow run completes. The b89196e ci: trigger empty commit was pushed for that purpose; if Actions still hasn't fired by review time, an "Approve and run workflows" maintainer click on the PR will kick it.
<autoReleaseAfterClose>false</autoReleaseAfterClose> continues to be only verifiable by a release dry-run post-merge — that one's a process gate, not a code gate, and is captured in the post-merge runbook plan.
…App path [APS-19024] INJ-002 (CWE-918 SSRF): both AutomateClient and AppAutomateClient constructors read the API base URL from a JVM system property (browserstack.automate.api / browserstack.app-automate.api) without validation. An attacker who can set the property redirects every signed request to their host and harvests Basic Auth credentials. Each client now wraps the System.getProperty call with validateApiBaseUrl, which parses the URL via java.net.URI and rejects anything that is not https or whose host does not end in .browserstack.com (IllegalArgumentException on failure). INJ-003 (CWE-22 path traversal): AppAutomateClient.uploadApp validated the file extension with a suffix-only check on the supplied filePath. A symlink legit.apk -> /etc/passwd would pass that check and stream the target's bytes to the upload endpoint. uploadApp now resolves the file via File.getCanonicalFile() (IOException is wrapped as AppAutomateException), checks the extension on the canonical file name, and uses the canonical File for the upload. New unit tests in src/test/java/com/browserstack/automate/ApiBaseUrlAndUploadSecurityTest.java cover both fixes, including a real symlink case for INJ-003 (Assume-skipped on platforms that disallow symlink creation). Resolves: APS-19024
Jimesh-browserstack
changed the title
APS-19024 added to this PR — JVM-property base-URL SSRF + uploadApp path traversalBundled into PR #82 per direction (one release cycle covers all four tickets). Commit: Vulnerabilities (from APS-19024)
FixesINJ-002: New public AutomateClient(String username, String accessKey) {
super(validateApiBaseUrl(System.getProperty("browserstack.automate.api", BASE_URL)),
username, accessKey);
}Helper parses via INJ-003: Files touched
Test evidence
The new
Live smoke (real BrowserStack session)The local Breaking-change note (added to PR body)JVM properties |
APS-19024 INJ-002 — extra non-unit verification: no-network-leak smokeThe earlier smoke (case 5) only asserted the guard throws. To close the loop on "no traffic leaks even if the guard regressed in some unforeseen way," I extended
Run output (real BrowserStack creds, real session — full harness): What this strengthens beyond unit tests: unit tests assert the validator's throw + message. This asserts the network-IO contract — the rejection happens before any TCP connection is attempted. The local server is the witness; if a future regression introduced a code path that constructed the URL but only failed on response parsing (or some hypothetical retry that bypassed the guard), the counter would be non-zero and the test would fail. Note on scheme vs host: the local server uses Recipe is committed to the investigation doc at |
| <serverId>ossrh</serverId> | ||
| <nexusUrl>https://oss.sonatype.org/</nexusUrl> | ||
| <autoReleaseAfterClose>true</autoReleaseAfterClose> | ||
| <autoReleaseAfterClose>false</autoReleaseAfterClose> |
There was a problem hiding this comment.
Why is this needed?
|
|
||
| steps: | ||
| - uses: actions/checkout@v2 | ||
| - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1 |
There was a problem hiding this comment.
Why is this needed?
| @@ -0,0 +1,59 @@ | |||
| # Changelog | |||
| } | ||
|
|
||
| if (!filePath.endsWith(".apk") && !filePath.endsWith(".ipa")) { | ||
| // APS-19024 / INJ-003: canonicalize before validating extension. Without this, |
There was a problem hiding this comment.
Should we add detailed comments in this repo considering this is a customer facing client
3 participants
Summary
Single PR bundling four security fixes so they ship in one release cycle.
AutomateClient.getSessionLogshttpsURLs whose URI host ends in.browserstack.comare accepted.BrowserStackClient.setProxy,checkAuthStatesetProxyis now per-instance (was a JVM-wide static leak). Auth guard tightened from&&to||so a half-configured client errors out instead of producing a malformedAuthorization: Basicheader..github/workflows/ci.yml,pom.xmlpermissions: contents: readadded,nexus-staging-maven-pluginautoReleaseAfterCloseflipped tofalse.AutomateClient+AppAutomateClientconstructors,AppAutomateClient.uploadApp*.browserstack.comhost). INJ-003: canonicalize the upload file path before extension check so a symlinkedlegit.apkpointing at an arbitrary file is rejected.Per ticket details:
APS-19018 — SSRF guard on
getSessionLogsAdds a
validateBrowserStackUrl(String)helper inAutomateClient.javathat parses the URL viajava.net.URIand asserts:schemeequalshttps(case-insensitive)URI.getHost()lowercased ends with.browserstack.comCalling pattern:
validateBrowserStackUrl(session.getLogUrl());is the first line inside the existingtryblock ingetSessionLogs(Session), before any HTTP request is built. URI-based host parsing avoids the host-suffix-spoof footgun (https://automate.browserstack.com.attacker.example/...).APS-19019 —
setProxystatic leak + auth guardprivate static HttpTransport HTTP_TRANSPORT→private HttpTransport httpTransport(instance).static HttpRequestFactory newRequestFactory()→ instance method, body useshttpTransport.setProxy(...)assignsthis.httpTransport = new ApacheHttpTransport(client);before recreatingrequestFactory.checkAuthState()condition flipped fromaccessKey == null && username == nulltoaccessKey == null || username == nulland the message now says"Missing API credentials (username or access key is null)".search_codethat no other reader ofHTTP_TRANSPORTexists in the repo.APS-19020 — CI hardening + Maven auto-release off
actions/checkout@v2→actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1actions/setup-java@v3→actions/setup-java@c1e323688fd81a25caa38c78aa6df2d33d3e20d9 # v4.8.0permissions: contents: readadded (matches the pattern inSemgrep.yml).<autoReleaseAfterClose>true</autoReleaseAfterClose>→falsein thenexus-staging-maven-pluginconfig.APS-19024 — JVM-property base-URL SSRF + uploadApp path traversal
INJ-002 (CWE-918 SSRF) — both
AutomateClientandAppAutomateClientconstructors read the API base URL from a JVM system property (browserstack.automate.api/browserstack.app-automate.api) without validation. An attacker who can set the property redirects every signed request to their host and harvests Basic Auth creds for the lifetime of the JVM.A new
private static String validateApiBaseUrl(String url)helper in each client class (parallel to APS-19018'svalidateBrowserStackUrl— same URI-parsing approach) wraps theSystem.getPropertycall:Helper throws
IllegalArgumentException(matches the existing constructor'sthrow new IllegalArgumentException("Invalid baseUrl")pattern). No opt-in flag for localhost/non-prod despite the Jira description suggesting one — an opt-in flag would itself be a JVM property and reintroduce the same vector. The strict allowlist still permits any*.browserstack.comsubdomain (staging, dev, etc.).INJ-003 (CWE-22 path traversal) —
AppAutomateClient.uploadAppvalidated the file extension with a suffix-only check on the supplied path. A symlink/tmp/legit.apk -> /etc/passwdwould pass that check and stream the target's bytes.uploadAppnow resolves viaFile.getCanonicalFile()(wrapped to convertIOException→AppAutomateException), checks the extension oncanonical.getName().toLowerCase(), and uses the canonicalFilefor the upload.Breaking changes for consumers relying on undocumented behavior
setProxyis now per-instance. Previously, callingsetProxy(...)on anyBrowserStackClient/AutomateClientmutated a JVM-widestatic HTTP_TRANSPORTfield, so the proxy applied to every other client instance in the same process. After this change,setProxyonly affects the instance it's called on. Anyone whose code relied on cross-instance proxy state must now callsetProxyon each client they construct. (See CHANGELOG.md.)checkAuthStatenow throws when EITHER credential is missing. Previously the guard only fired whenusernameANDaccessKeywere bothnull. A client with one credential set and one missing now throwsIllegalStateExceptioninstead of silently producing a malformedAuthorizationheader.browserstack.automate.api/browserstack.app-automate.apiare now allowlist-validated. Anything outsidehttps://*.browserstack.commakes the constructor throwIllegalArgumentException. (Effectively no-op for prod consumers; teams pointing at private mocks orlocalhostfor tests will need to update those tests.)Manual release step now required
Because
<autoReleaseAfterClose>is nowfalse, aftermvn release:performthe staged release onhttps://oss.sonatype.org/must be manually promoted (close → release). The internal release runbook on Confluence will be updated separately to cover this manual step.Test plan
Local
mvn testwas run on bothmasterand this branch with validBROWSERSTACK_USER/BROWSERSTACK_ACCESSKEY. Comparative results:AutomateClientTest.testGetSessionLogsis environment-dependent (returns 404 "Text logs are not available while the session is running" when sessions[0] of the first build is in-progress). Reproduces on master in the same conditions — not introduced by this PR.New hermetic tests (no live API required):
src/test/java/com/browserstack/automate/AutomateClientSecurityTest.java(APS-19018 + APS-19019):getSessionLogs_rejectsAttackerHost_throwsBeforeAnyHttpRequestgetSessionLogs_rejectsHttpSchemegetSessionLogs_rejectsHostSuffixSpoof← validates URI parsing not stringendsWithgetSessionLogs_acceptsTrustedHostcheckAuthState_throwsWhenUsernameNullcheckAuthState_throwsWhenAccessKeyNullcheckAuthState_passesWhenBothCredentialsPresentsrc/test/java/com/browserstack/client/BrowserStackClientProxyIsolationTest.java(APS-19019):setProxy_isInstanceScoped_doesNotLeakAcrossClients— snapshotshttpTransporton two clients via reflection, callssetProxyon one, verifies the other's transport reference is unchanged.src/test/java/com/browserstack/automate/ApiBaseUrlAndUploadSecurityTest.java(APS-19024):automateClient_rejectsAttackerHostOverrideautomateClient_rejectsHttpSchemeautomateClient_rejectsHostSuffixSpoofautomateClient_acceptsTrustedDefaultautomateClient_acceptsTrustedOverride(e.g.https://api-staging.browserstack.com/automate)appAutomateClient_rejectsAttackerHostOverrideappAutomateClient_rejectsHttpSchemeappAutomateClient_acceptsTrustedDefaultuploadApp_rejectsSymlinkWhoseCanonicalNameLacksValidExtension— creates a reallegit.apk -> secret.txtsymlink and assertsInvalidFileExtensionException.Assume-skipped on platforms where symlink creation is not permitted.uploadApp_acceptsRealApkAfterCanonicalization— extension regression check against a plain.apkfile.Live smoke (real BrowserStack session —
3444a7b4414661cb4e1ca69bf9b2c816a00858c3)SsrfSmokeTest.java(sandbox-only, not committed) — 5 cases, all pass:https://automate.browserstack.com/.../logsaccepted, returns 214392 bytes.https://attacker.example/steal→AutomateException: Untrusted logs URL host: attacker.example.setProxyon A changes A's transport, leaves B's untouched.setProxyto an unused port fails the request at the proxy (proxy is honored, not bypassed).System.setProperty("browserstack.automate.api", "https://attacker.example/automate")causes the constructor to throwIllegalArgumentException: Untrusted API base URL host: attacker.example.Checklist
🤖 Generated with Claude Code