Harden URL handling, credentials, and CI workflow

[Jimesh-browserstack]

Summary

This PR hardens URL handling, credential validation, and CI/release configuration in the automate-client-java client.

Area	Change
AutomateClient.getSessionLogs	URL allowlist — only https URLs whose host ends with .browserstack.com (parsed via java.net.URI, not a raw-string suffix) are accepted before fetching the logs URL returned by the API.
AutomateClient and AppAutomateClient constructors	The base URL read from the JVM system properties browserstack.automate.api / browserstack.app-automate.api is now validated against the same allowlist before being passed to the parent constructor.
BrowserStackClient.setProxy	The HTTP transport is now per-instance (was a JVM-wide static field). Configuring a proxy on one client no longer affects other client instances.
BrowserStackClient.checkAuthState	Now throws when EITHER the username OR access key is missing (was: only when both were null).
AppAutomateClient.uploadApp	File path is canonicalized via File.getCanonicalFile() before the .apk / .ipa extension check; the canonical file is used for the upload.
.github/workflows/ci.yml	Actions pinned to commit SHAs (actions/checkout@v4.3.1, actions/setup-java@v4.8.0); top-level permissions: contents: read added.
pom.xml	nexus-staging-maven-plugin autoReleaseAfterClose is now false — staged releases must be manually promoted after mvn release:perform.

Breaking changes for consumers relying on undocumented behaviour

• setProxy is now per-instance. Previously, calling setProxy(...) on any BrowserStackClient / AutomateClient mutated a JVM-wide static HTTP_TRANSPORT field, so the proxy applied to every other client instance in the same process. After this change, setProxy only affects the instance it's called on. Anyone whose code relied on cross-instance proxy state must now call setProxy on each client they construct.
• checkAuthState now throws when EITHER credential is missing. Previously the guard only fired when username AND accessKey were both null. A client with one credential set and one missing now throws IllegalStateException instead of silently producing a malformed Authorization header.
• JVM properties browserstack.automate.api / browserstack.app-automate.api are now allowlist-validated. Anything outside https://*.browserstack.com makes the constructor throw IllegalArgumentException. Effectively a no-op for production consumers; teams pointing at private mocks or localhost for tests will need to update those tests.

New manual release step

Because <autoReleaseAfterClose> is now false, after mvn release:perform the staged release on https://oss.sonatype.org/ must be manually promoted (close → release in the OSSRH UI, or mvn nexus-staging:release -DstagingRepositoryId=combrowserstack-NNNN). The internal release runbook will be updated separately.

Tests

• 18 new hermetic unit tests added (no live API required):
  • AutomateClientSecurityTest — 7 tests for the logs URL allowlist and the credential-presence check.
  • ApiBaseUrlAndUploadSecurityTest — 10 tests for the JVM-property base URL allowlist and the canonical-path extension check.
  • BrowserStackClientProxyIsolationTest — 1 test for per-instance HTTP transport isolation.
• All 18 hermetic tests pass.
• Existing live-API tests pass under the same conditions as master.

[Jimesh-browserstack]

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 getSessionLogs

Fix tested

• Hermetic JUnit (AutomateClientSecurityTest):
  • getSessionLogs_rejectsAttackerHost_throwsBeforeAnyHttpRequest
  • getSessionLogs_rejectsHttpScheme
  • getSessionLogs_rejectsHostSuffixSpoof ← proves URI parsing, not raw endsWith (the automate.browserstack.com.attacker.example footgun)
• Live smoke against session 3444a7b4... after mvn package -DskipTests -Dgpg.skip + mvn dependency:copy-dependencies:
  • Mutated Session.logUrl to https://attacker.example/steal via reflection → AutomateException: Untrusted logs URL host: attacker.example. Confirmed the exception fires before any HTTP request.

No-regression tested

• getSessionLogs_acceptsTrustedHost unit test passes.
• Live smoke happy path: real BS-issued https://automate.browserstack.com/.../logs URL accepted; SDK returned a 214,392-byte body containing actual selenium request log content. The allowlist does not break the production happy path.

---

APS-19019 — setProxy static→instance + checkAuthState &&→||

Fix tested

• Hermetic JUnit:
  • BrowserStackClientProxyIsolationTest.setProxy_isInstanceScoped_doesNotLeakAcrossClients — reflection on httpTransport field; A's ref changes after setProxy, B's stays.
  • AutomateClientSecurityTest.checkAuthState_throwsWhenUsernameNull
  • AutomateClientSecurityTest.checkAuthState_throwsWhenAccessKeyNull
• Live smoke (same harness):
  • Cross-instance isolation reproduced end-to-end against the built jar.

No-regression tested

• checkAuthState_passesWhenBothCredentialsPresent unit test passes (regression for the documented happy path).
• setProxy still functionally proxies — live regression test added: created a client, called setProxy("127.0.0.1", <unused-port>, "", ""), then attempted getSession(...). Request failed with Connect to 127.0.0.1:<port> ... Connection refused, which proves the proxy is being honored on the new per-instance transport. If the proxy had been bypassed, the call would have succeeded against the real API. (Code-shape evidence backs this too: setProxy body lines 158-176 are mechanically unchanged; only line 177's assignment target moved from static to instance.)

---

APS-19020 — CI hardening (ci.yml) + autoReleaseAfterClose=false

Fix tested

• SHA pins resolve and YAML is valid: mvn package build succeeded against the branch's worktree (the workflow's mvn clean install -DskipTests -Dgpg.skip step is the same command).
• permissions: contents: read mirrors the pre-existing pattern in Semgrep.yml in this same repo.
• autoReleaseAfterClose=false is a single config flag in pom.xml — there is no code path to exercise locally; it's read by nexus-staging-maven-plugin only during mvn nexus-staging:deploy.

No-regression tested

• Build step runs in the PR's own CI workflow. Status as of this comment: still queued/pending — pushed an empty ci: trigger commit (b89196e) to kick the run, awaiting status. Once CI lands green, that's the conclusive proof the SHA-pinned actions resolve and permissions: contents: read doesn't break the build (it shouldn't — the build doesn't write anything via GITHUB_TOKEN).
• autoReleaseAfterClose=false regression is not testable without a release dry-run. Mitigation: the post-merge plan calls for a mvn ... release:prepare -DdryRun=true against a sandbox before the next real release, with a coordinated runbook update on Confluence.

---

Comparative mvn test

Same JDK, same shell, same env vars on both branches:

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=false end-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 manual Close → Release step 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.

[Jimesh-browserstack]

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 commits

Resolved both pinned SHAs via the GitHub git-refs API:

$ curl -sS https://api.github.com/repos/actions/checkout/git/refs/tags/v4.3.1
  → object.sha = 34e114876b0b11c390a56381ad16ebd13914f8d5  (type: commit)

$ curl -sS https://api.github.com/repos/actions/setup-java/git/refs/tags/v4.8.0
  → object.sha = c1e323688fd81a25caa38c78aa6df2d33d3e20d9  (type: commit)

Both match the SHAs in ci.yml byte-for-byte. The pins point to the legitimate published v4.3.1 / v4.8.0 commits, not arbitrary refs.

2. The exact workflow build command runs green locally

Ran mvn clean install -DskipTests -Dgpg.skip on the branch — same command the workflow's Build step runs:

$ mvn -q clean install -DskipTests -Dgpg.skip
$ echo $?
0

Exit 0. Build succeeded with the new pom.xml (including autoReleaseAfterClose=false) and the rest of the branch's changes applied.

3. Workflow permissions audit — contents: read is sufficient

Walked through every step in ci.yml and confirmed none require write scope on the GITHUB_TOKEN:

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: read

at 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.

[Jimesh-browserstack]

APS-19024 added to this PR — JVM-property base-URL SSRF + uploadApp path traversal

Bundled into PR #82 per direction (one release cycle covers all four tickets).

Commit: 159c132 — fix(security): validate JVM-property base URL and canonicalize uploadApp path [APS-19024]

Vulnerabilities (from APS-19024)

• INJ-002 / CWE-918 SSRF — AutomateClient constructor (line 39) and AppAutomateClient constructor (line 35) call System.getProperty("browserstack.(app-)automate.api", BASE_URL) and pass the result straight to super(). An attacker who can set the JVM property redirects every signed API request — and the Basic Auth credentials those requests carry — to their host.
• INJ-003 / CWE-22 path traversal — AppAutomateClient.uploadApp (line 74) validated the file extension with filePath.endsWith(".apk") / ".ipa" directly on the supplied path. A symlink /tmp/legit.apk -> /etc/passwd passes the suffix check and would stream the target's bytes to the upload endpoint.

Fixes

INJ-002: New private static String validateApiBaseUrl(String url) helper in each of AutomateClient.java and AppAutomateClient.java, parallel to APS-19018's validateBrowserStackUrl:

public AutomateClient(String username, String accessKey) {
  super(validateApiBaseUrl(System.getProperty("browserstack.automate.api", BASE_URL)),
      username, accessKey);
}

Helper parses via java.net.URI, requires https scheme, requires URI.getHost().toLowerCase().endsWith(".browserstack.com"). Throws IllegalArgumentException on failure (matches the existing constructor's 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.com subdomain (staging, dev, etc.).

INJ-003: uploadApp now resolves the path via File.getCanonicalFile() (wrapped in a try/catch that converts IOException → AppAutomateException), checks the extension on canonical.getName().toLowerCase(), and uses the canonical File for the upload. A symlink whose target is a non-.apk/.ipa file is rejected with InvalidFileExtensionException.

Files touched

File	What
src/main/java/com/browserstack/automate/AutomateClient.java	Added validateApiBaseUrl helper; wrapped System.getProperty in ctor
src/main/java/com/browserstack/appautomate/AppAutomateClient.java	Added imports (URI, IOException); added validateApiBaseUrl helper; wrapped System.getProperty in ctor; canonicalize path in uploadApp and check extension on canonical name
src/test/java/com/browserstack/automate/ApiBaseUrlAndUploadSecurityTest.java	NEW — 10 tests (8 INJ-002 + 2 INJ-003 incl. real symlink case Assume-skipped on platforms without symlink permission)
CHANGELOG.md	Added APS-19024 entries to the Unreleased Security section

Test evidence

mvn test -Dgpg.skip with valid BROWSERSTACK_USER / BROWSERSTACK_ACCESSKEY:

Branch	Tests run	Pass	Failures	Errors
master	18	18	0	0
security/aps-19018-19019-19020-multifix (post APS-19024)	36	36	0	0

The new ApiBaseUrlAndUploadSecurityTest runs 10/10 green:

[INFO] Tests run: 10, Failures: 0, Errors: 0, Skipped: 0
       in com.browserstack.automate.ApiBaseUrlAndUploadSecurityTest

AutomateClientTest.testGetSessionLogs is environment-dependent (returns 404 "Text logs are not available while the session is running" when sessions[0] of the first build is still in-progress). Reproduces on master under the same conditions — not introduced by this PR.

Live smoke (real BrowserStack session)

The local SsrfSmokeTest.java (sandbox-only) was extended with case 5 covering APS-19024. All 5 cases pass against session 3444a7b4414661cb4e1ca69bf9b2c816a00858c3:

[OK] getSession returned. logUrl=https://automate.browserstack.com/.../logs
[OK] getSessionLogs accepted real logUrl. Body length=214392
[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: Connection refused
[OK] base-URL guard threw on attacker host override: Untrusted API base URL host: attacker.example
ALL SMOKE TESTS PASSED

Breaking-change note (added to PR body)

JVM properties browserstack.automate.api and browserstack.app-automate.api are now allowlist-validated. Anything outside https://*.browserstack.com makes the constructor throw IllegalArgumentException. Effectively no-op for prod consumers; teams pointing at private mocks or localhost for tests will need to update those tests.

[Jimesh-browserstack]

APS-19024 INJ-002 — extra non-unit verification: no-network-leak smoke

The 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 SsrfSmokeTest.java (sandbox harness, not committed to the repo) with a 6th case that:

1. Spins up a real HttpServer on 127.0.0.1:0 with an AtomicInteger request counter wired into the handler.
2. Sets the JVM property to point the SDK at that local server (http://127.0.0.1:<random>/automate).
3. Constructs the client and asserts both:
   • IllegalArgumentException is thrown synchronously, and
   • The local server's hit counter is 0 after a 250 ms settle.
4. Repeats the whole flow for browserstack.app-automate.api + AppAutomateClient.

Run output (real BrowserStack creds, real session — full harness):

[OK] getSession returned. logUrl=https://automate.browserstack.com/builds/.../logs
[OK] getSessionLogs accepted real logUrl. Body length=214392
[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: Connect to 127.0.0.1:63186 [/127.0.0.1] failed: Connection refused
[OK] base-URL guard threw on attacker host override: Untrusted API base URL host: attacker.example
[OK] browserstack.automate.api no-leak: ctor rejected and 0 requests reached 127.0.0.1:63188
[OK] browserstack.app-automate.api no-leak: ctor rejected and 0 requests reached 127.0.0.1:63191
ALL SMOKE TESTS PASSED

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 http:// (no TLS), so the scheme check actually fires first ("Insecure API base URL scheme: http"). The host-check path is independently exercised by case 5 with https://attacker.example. Together they cover both validator branches at runtime, and both branches throw before any network IO.

Recipe is committed to the investigation doc at aps-investigations/tickets/APS-19018-19019-19020-debugging.md for repeatability.

[karanshah-browserstack]

Why is this needed?

[karanshah-browserstack]

Why is this needed?

[Baaryan]

Do we need to commit this?

[Baaryan]

Should we add detailed comments in this repo considering this is a customer facing client

[Jimesh-browserstack]

Closing this PR was unintentional — it happened automatically when the branch was renamed and GitHub deleted the old ref before retargeting.

Replacement PR: #83 — same diff, with the feedback on this thread addressed:

• @Baaryan — CHANGELOG.md removed, and the inline source/test comments are rewritten in customer-facing language (no internal references).
• @karanshah-browserstack — added a "Rationale on the CI / release changes" section in the new PR description explaining why we pin GitHub Actions to commit SHAs and why autoReleaseAfterClose is now false.

Please continue the review on #83.