Skip to content

Harden URL handling, credentials, and CI workflow#83

Open
Jimesh-browserstack wants to merge 1 commit intomasterfrom
harden/url-and-credential-validation
Open

Harden URL handling, credentials, and CI workflow#83
Jimesh-browserstack wants to merge 1 commit intomasterfrom
harden/url-and-credential-validation

Conversation

@Jimesh-browserstack
Copy link
Copy Markdown
Collaborator

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.

Rationale on the CI / release changes

  • Pinning GitHub Actions to commit SHAs (rather than mutable tags like @v2 / @v3) makes our CI builds reproducible and forces every action upgrade to be an explicit, reviewed change in this repo. The # v4.3.1 / # v4.8.0 trailing comments preserve human readability of the version that the SHA corresponds to. The top-level permissions: contents: read block sets the default GITHUB_TOKEN to read-only so the workflow only gets escalated permissions if a job explicitly asks for them.
  • autoReleaseAfterClose: false prevents mvn release:perform from immediately publishing the staged artifact to Maven Central. With auto-promote on, a bad upload (missing signatures, wrong groupId, accidental SNAPSHOT, etc.) is irrevocably published the moment the Maven build finishes — Central is append-only, so the only fix is to ship a follow-up version. With this flag off, the staged repo on https://oss.sonatype.org/ waits for a human to inspect and click Release (or Drop) before it becomes permanent.

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.

Replaces #82 (closed when its branch was renamed; review comments from that PR have been addressed in this diff — CHANGELOG.md removed and inline source/test comments rewritten in customer-facing language).

- Validate API base URL read from JVM system properties
  (browserstack.automate.api / browserstack.app-automate.api): require
  https scheme and a host under .browserstack.com.
- Validate the BrowserStack-issued logs URL in
  AutomateClient.getSessionLogs before fetching, with the same
  scheme + host allowlist.
- Make HttpTransport per-instance (was a JVM-wide static field) so
  setProxy on one client does not affect other client instances in
  the same process.
- Tighten BrowserStackClient.checkAuthState so it throws when EITHER
  the username OR access key is missing (was: only when both were
  null).
- AppAutomateClient.uploadApp now canonicalizes the file path via
  File.getCanonicalFile() before applying the .apk/.ipa extension
  check, and uses the canonical File for the upload.
- Pin GitHub Actions in .github/workflows/ci.yml to commit SHAs and
  add a top-level permissions: contents: read block.
- Set nexus-staging-maven-plugin autoReleaseAfterClose to false so
  staged releases must be promoted manually after mvn release:perform.

Adds hermetic unit tests for the URL allowlist, credential check,
per-instance HttpTransport, and canonical-path extension check.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant