Skip to content

Commit ba24f7b

Browse files
fix(security): per-instance HttpTransport + tightened auth guard [APS-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
1 parent 9044b3a commit ba24f7b

2 files changed

Lines changed: 70 additions & 6 deletions

File tree

src/main/java/com/browserstack/client/BrowserStackClient.java

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ public Object parseAndClose(Reader reader, Type type) throws IOException {
6969
throw new IOException("Unsupported operation");
7070
}
7171
};
72-
private static HttpTransport HTTP_TRANSPORT = new ApacheHttpTransport();
72+
private HttpTransport httpTransport = new ApacheHttpTransport();
7373
protected final BrowserStackCache<String, Object> cacheMap;
7474

7575
private HttpRequestFactory requestFactory;
@@ -105,8 +105,8 @@ public BrowserStackClient(String baseUrl, String username, String accessKey) {
105105
this.accessKey = accessKey.trim();
106106
}
107107

108-
static HttpRequestFactory newRequestFactory() {
109-
return HTTP_TRANSPORT.createRequestFactory(httpRequest -> httpRequest.setParser(OBJECT_PARSER));
108+
HttpRequestFactory newRequestFactory() {
109+
return httpTransport.createRequestFactory(httpRequest -> httpRequest.setParser(OBJECT_PARSER));
110110
}
111111

112112
static HttpRequest newRequest(final HttpRequestFactory requestFactory, final Method method, final GenericUrl url) throws BrowserStackException {
@@ -174,7 +174,7 @@ public void setProxy(final String proxyHost, final int proxyPort, final String p
174174
}
175175

176176
final HttpClient client = clientBuilder.build();
177-
HTTP_TRANSPORT = new ApacheHttpTransport(client);
177+
this.httpTransport = new ApacheHttpTransport(client);
178178
this.requestFactory = newRequestFactory();
179179
}
180180

@@ -187,8 +187,8 @@ protected synchronized void setAccessKey(final String accessKey) {
187187
}
188188

189189
private void checkAuthState() {
190-
if (this.accessKey == null && this.username == null) {
191-
throw new IllegalStateException("Missing API credentials");
190+
if (this.accessKey == null || this.username == null) {
191+
throw new IllegalStateException("Missing API credentials (username or access key is null)");
192192
}
193193
}
194194

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
package com.browserstack.client;
2+
3+
import com.browserstack.automate.AutomateClient;
4+
import com.google.api.client.http.HttpTransport;
5+
import org.junit.Test;
6+
7+
import java.lang.reflect.Field;
8+
9+
import static org.junit.Assert.assertNotNull;
10+
import static org.junit.Assert.assertNotSame;
11+
import static org.junit.Assert.assertSame;
12+
13+
/**
14+
* APS-19019: setProxy must mutate per-instance state only.
15+
*
16+
* <p>Before this fix, {@code setProxy} reassigned a JVM-wide {@code static HTTP_TRANSPORT}
17+
* field — meaning a proxy configured on one client leaked to every other client in
18+
* the same process (and the only-process-wide reset persisted across requests on
19+
* unrelated tenants). The fix moves the field to an instance variable {@code httpTransport}.
20+
*
21+
* <p>This test verifies cross-instance isolation by snapshotting both clients'
22+
* {@code httpTransport} references before and after a proxy is configured on one of them.
23+
*/
24+
public class BrowserStackClientProxyIsolationTest {
25+
26+
private static HttpTransport getHttpTransport(BrowserStackClient client) throws Exception {
27+
Field f = BrowserStackClient.class.getDeclaredField("httpTransport");
28+
f.setAccessible(true);
29+
return (HttpTransport) f.get(client);
30+
}
31+
32+
@Test
33+
public void setProxy_isInstanceScoped_doesNotLeakAcrossClients() throws Exception {
34+
AutomateClient clientA = new AutomateClient("user_a", "key_a");
35+
AutomateClient clientB = new AutomateClient("user_b", "key_b");
36+
37+
HttpTransport beforeA = getHttpTransport(clientA);
38+
HttpTransport beforeB = getHttpTransport(clientB);
39+
assertNotNull("clientA must have a transport before setProxy", beforeA);
40+
assertNotNull("clientB must have a transport before setProxy", beforeB);
41+
42+
// Configure a proxy on clientA only.
43+
clientA.setProxy("proxy.example.invalid", 3128, null, null);
44+
45+
HttpTransport afterA = getHttpTransport(clientA);
46+
HttpTransport afterB = getHttpTransport(clientB);
47+
48+
// clientA's transport should have been replaced.
49+
assertNotSame(
50+
"clientA.httpTransport must be replaced after setProxy",
51+
beforeA, afterA);
52+
53+
// clientB's transport must NOT have changed.
54+
assertSame(
55+
"clientB.httpTransport must NOT change when setProxy is called on clientA "
56+
+ "(was the static-field cross-instance leak — see APS-19019)",
57+
beforeB, afterB);
58+
59+
// And clientA's new transport must not equal clientB's transport.
60+
assertNotSame(
61+
"clientA and clientB must have distinct transports after setProxy on A",
62+
afterA, afterB);
63+
}
64+
}

0 commit comments

Comments
 (0)