Skip to content

Commit 9044b3a

Browse files
Jimesh-browserstackJimesh-browserstack
committed
fix(security): add SSRF guard on getSessionLogs [APS-19018]
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
1 parent 301163d commit 9044b3a

2 files changed

Lines changed: 201 additions & 0 deletions

File tree

‎src/main/java/com/browserstack/automate/AutomateClient.java‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import javax.annotation.Nonnull;
1616
import java.util.ArrayList;
1717
import java.util.Arrays;
18+
import java.net.URI;
1819
import java.util.HashMap;
1920
import java.util.List;
2021
import java.util.Map;
@@ -458,6 +459,7 @@ public String getSessionLogs(final Session session) throws AutomateException {
458459
}
459460

460461
try {
462+
validateBrowserStackUrl(session.getLogUrl());
461463
BrowserStackRequest request = newRequest(Method.GET, session.getLogUrl(), false);
462464
request.getHttpRequest().getHeaders().setAccept("*/*");
463465
return request.asString();
@@ -529,4 +531,40 @@ public String recycleKey() throws AutomateException {
529531
setAccessKey(newAccessKey);
530532
return newAccessKey;
531533
}
534+
535+
/**
536+
* Validates that a URL is safe to fetch as a BrowserStack-issued logs URL.
537+
* <p>
538+
* Defends against SSRF (APS-19018): only allow {@code https} scheme and a host
539+
* whose lowercased value ends with {@code .browserstack.com}. Host comparison
540+
* uses {@link java.net.URI#getHost()} (NOT a string suffix on the raw URL) so that
541+
* spoofed hosts such as {@code https://automate.browserstack.com.attacker.example/}
542+
* are rejected.
543+
*
544+
* @param url URL to validate. Must be a syntactically-valid absolute URL.
545+
* @throws AutomateException if the URL is malformed, uses a non-https scheme,
546+
* or has a host outside {@code *.browserstack.com}.
547+
*/
548+
private static void validateBrowserStackUrl(String url) throws AutomateException {
549+
if (url == null) {
550+
throw new AutomateException("Logs URL is null", 400);
551+
}
552+
553+
final URI uri;
554+
try {
555+
uri = URI.create(url);
556+
} catch (IllegalArgumentException e) {
557+
throw new AutomateException("Malformed logs URL", 400);
558+
}
559+
560+
final String scheme = uri.getScheme();
561+
if (!"https".equalsIgnoreCase(scheme)) {
562+
throw new AutomateException("Insecure logs URL scheme: " + scheme, 400);
563+
}
564+
565+
final String host = uri.getHost();
566+
if (host == null || !host.toLowerCase().endsWith(".browserstack.com")) {
567+
throw new AutomateException("Untrusted logs URL host: " + host, 400);
568+
}
569+
}
532570
}

‎src/test/java/com/browserstack/automate/AutomateClientSecurityTest.java‎

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,163 @@
1+
package com.browserstack.automate;
2+
3+
import com.browserstack.automate.exception.AutomateException;
4+
import com.browserstack.automate.model.Session;
5+
import com.browserstack.client.BrowserStackClient;
6+
import com.fasterxml.jackson.databind.ObjectMapper;
7+
import org.junit.Test;
8+
9+
import java.lang.reflect.Field;
10+
import java.lang.reflect.InvocationTargetException;
11+
import java.lang.reflect.Method;
12+
13+
import static org.junit.Assert.assertEquals;
14+
import static org.junit.Assert.assertNotNull;
15+
import static org.junit.Assert.assertTrue;
16+
import static org.junit.Assert.fail;
17+
18+
/**
19+
* Hermetic unit tests for security fixes — no live BrowserStack API calls.
20+
*
21+
* <ul>
22+
* <li>APS-19018: SSRF guard on getSessionLogs (validateBrowserStackUrl)
23+
* <li>APS-19019: checkAuthState now throws when EITHER credential is null (was: only both)
24+
* </ul>
25+
*/
26+
public class AutomateClientSecurityTest {
27+
28+
private static final String DUMMY_USER = "dummy_user";
29+
private static final String DUMMY_KEY = "dummy_key";
30+
31+
// Build a Session with a controlled logUrl by JSON-deserializing through Jackson —
32+
// setLogUrl() is private; the @JsonProperty("logs") setter accepts the field by name.
33+
private static Session sessionWithLogUrl(String logUrl) throws Exception {
34+
String json = "{\"logs\": " + (logUrl == null ? "null" : "\"" + logUrl + "\"") + "}";
35+
return new ObjectMapper().readValue(json, Session.class);
36+
}
37+
38+
// ---------- APS-19018: SSRF guard ----------
39+
40+
@Test
41+
public void getSessionLogs_rejectsAttackerHost_throwsBeforeAnyHttpRequest() throws Exception {
42+
AutomateClient client = new AutomateClient(DUMMY_USER, DUMMY_KEY);
43+
Session session = sessionWithLogUrl("https://attacker.example/steal");
44+
try {
45+
client.getSessionLogs(session);
46+
fail("Expected AutomateException for untrusted host");
47+
} catch (AutomateException e) {
48+
assertTrue("message mentions Untrusted host: " + e.getMessage(),
49+
e.getMessage().toLowerCase().contains("untrusted"));
50+
assertEquals(400, e.getStatusCode());
51+
}
52+
}
53+
54+
@Test
55+
public void getSessionLogs_rejectsHttpScheme() throws Exception {
56+
AutomateClient client = new AutomateClient(DUMMY_USER, DUMMY_KEY);
57+
Session session = sessionWithLogUrl("http://automate.browserstack.com/foo");
58+
try {
59+
client.getSessionLogs(session);
60+
fail("Expected AutomateException for non-https scheme");
61+
} catch (AutomateException e) {
62+
assertTrue("message mentions Insecure scheme: " + e.getMessage(),
63+
e.getMessage().toLowerCase().contains("insecure"));
64+
assertEquals(400, e.getStatusCode());
65+
}
66+
}
67+
68+
/**
69+
* URI parsing must extract the actual host, not match by string suffix.
70+
* <p>
71+
* "https://automate.browserstack.com.attacker.example/foo" — naive
72+
* {@code url.endsWith(".browserstack.com")} would FAIL to reject this because
73+
* the URL doesn't end with that suffix; but an even more dangerous variant
74+
* {@code https://attacker.example/path?x=automate.browserstack.com} could be
75+
* mistaken for trusted by string contains-checks. URI.getHost() returns
76+
* {@code automate.browserstack.com.attacker.example}, which our suffix
77+
* check on the host (not the URL) correctly rejects.
78+
*/
79+
@Test
80+
public void getSessionLogs_rejectsHostSuffixSpoof() throws Exception {
81+
AutomateClient client = new AutomateClient(DUMMY_USER, DUMMY_KEY);
82+
Session session = sessionWithLogUrl("https://automate.browserstack.com.attacker.example/foo");
83+
try {
84+
client.getSessionLogs(session);
85+
fail("Expected AutomateException for spoofed host");
86+
} catch (AutomateException e) {
87+
assertTrue("message mentions Untrusted host: " + e.getMessage(),
88+
e.getMessage().toLowerCase().contains("untrusted"));
89+
assertEquals(400, e.getStatusCode());
90+
}
91+
}
92+
93+
/**
94+
* A trusted host should pass the validation guard. The HTTP request will
95+
* still fail (DNS / 401 / etc.) but specifically NOT with our 400
96+
* "Untrusted/Insecure/Malformed" guard message — that proves the URL
97+
* cleared the SSRF check.
98+
*/
99+
@Test
100+
public void getSessionLogs_acceptsTrustedHost() throws Exception {
101+
AutomateClient client = new AutomateClient(DUMMY_USER, DUMMY_KEY);
102+
Session session = sessionWithLogUrl("https://api.browserstack.com/automate/path");
103+
try {
104+
client.getSessionLogs(session);
105+
// If somehow it succeeds (unlikely with dummy creds), that also passes the guard.
106+
} catch (AutomateException e) {
107+
String msg = e.getMessage() == null ? "" : e.getMessage().toLowerCase();
108+
assertTrue("Trusted host should not be rejected by SSRF guard. Got: " + e.getMessage(),
109+
!msg.contains("untrusted") && !msg.contains("insecure")
110+
&& !msg.contains("malformed"));
111+
}
112+
}
113+
114+
// ---------- APS-19019: checkAuthState && -> || ----------
115+
116+
private static void invokeCheckAuthState(BrowserStackClient client) throws Throwable {
117+
Method m = BrowserStackClient.class.getDeclaredMethod("checkAuthState");
118+
m.setAccessible(true);
119+
try {
120+
m.invoke(client);
121+
} catch (InvocationTargetException e) {
122+
throw e.getTargetException();
123+
}
124+
}
125+
126+
private static void setField(BrowserStackClient client, String fieldName, Object value)
127+
throws ReflectiveOperationException {
128+
Field f = BrowserStackClient.class.getDeclaredField(fieldName);
129+
f.setAccessible(true);
130+
f.set(client, value);
131+
}
132+
133+
@Test
134+
public void checkAuthState_throwsWhenUsernameNull() throws Throwable {
135+
AutomateClient client = new AutomateClient(DUMMY_USER, DUMMY_KEY);
136+
setField(client, "username", null);
137+
try {
138+
invokeCheckAuthState(client);
139+
fail("Expected IllegalStateException when username is null");
140+
} catch (IllegalStateException e) {
141+
assertNotNull(e.getMessage());
142+
}
143+
}
144+
145+
@Test
146+
public void checkAuthState_throwsWhenAccessKeyNull() throws Throwable {
147+
AutomateClient client = new AutomateClient(DUMMY_USER, DUMMY_KEY);
148+
setField(client, "accessKey", null);
149+
try {
150+
invokeCheckAuthState(client);
151+
fail("Expected IllegalStateException when accessKey is null");
152+
} catch (IllegalStateException e) {
153+
assertNotNull(e.getMessage());
154+
}
155+
}
156+
157+
@Test
158+
public void checkAuthState_passesWhenBothCredentialsPresent() throws Throwable {
159+
AutomateClient client = new AutomateClient(DUMMY_USER, DUMMY_KEY);
160+
// Should not throw
161+
invokeCheckAuthState(client);
162+
}
163+
}

0 commit comments

Comments
 (0)