Skip to content

Commit 7442d01

Browse files
bric3bric3
authored
Jetty leaking out of :dd-java-agent:testing (#10218)
* fix: Workaround for :dd-java-agent:testing coming with a more recent version of jetty * fix: Proper shadowing of jetty in agent-testing * chore: lockfiles * fix: jsp instrumentation needlessly requiring jetty http status * fix: Exports servlet request api This was lost when jetty server got shadowed. * fix: Use config lazy API * chore: Make spotless happy * fix: Replaces testing http client by raw jetty in Apache Http Client instrumentation Now that :dd-java-agent:testing shadows jetty, the jetty instrumentation was not applied. * chore: Make spotless happy
1 parent ff6bd9d commit 7442d01

File tree

16 files changed

+131
-83
lines changed

16 files changed

+131
-83
lines changed

‎dd-java-agent/instrumentation/apache-httpclient/apache-httpclient-4.0/build.gradle‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,12 @@ dependencies {
3939
// to instrument the integration test
4040
iastIntegrationTestImplementation project(':dd-java-agent:agent-iast:iast-test-fixtures')
4141
iastIntegrationTestImplementation group: 'org.apache.httpcomponents', name: 'httpclient', version: '4.0'
42-
iastIntegrationTestRuntimeOnly(project(':dd-java-agent:instrumentation:jetty:jetty-server:jetty-server-9.0'))
42+
// Provide real (non-shadowed) jetty for the test server bootstrap
43+
iastIntegrationTestImplementation group: 'org.eclipse.jetty', name: 'jetty-server', version: '9.4.56.v20240826'
44+
iastIntegrationTestImplementation group: 'org.eclipse.jetty', name: 'jetty-servlet', version: '9.4.56.v20240826'
4345
iastIntegrationTestRuntimeOnly(project(':dd-java-agent:instrumentation:apache-httpcore:apache-httpcore-4.0'))
4446
iastIntegrationTestRuntimeOnly(project(':dd-java-agent:instrumentation:servlet:javax-servlet:javax-servlet-common'))
47+
iastIntegrationTestRuntimeOnly(project(':dd-java-agent:instrumentation:jetty:jetty-server:jetty-server-9.0'))
4548
iastIntegrationTestRuntimeOnly(project(':dd-java-agent:instrumentation:java:java-lang:java-lang-1.8'))
4649
iastIntegrationTestRuntimeOnly(project(':dd-java-agent:instrumentation:java:java-net:java-net-1.8'))
4750
iastIntegrationTestRuntimeOnly project(':dd-java-agent:instrumentation:iast-instrumenter')

‎dd-java-agent/instrumentation/apache-httpclient/apache-httpclient-4.0/src/iastIntegrationTest/groovy/IastHttpClientIntegrationTest.groovy‎

Lines changed: 60 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,16 @@ import datadog.trace.agent.test.base.HttpServer
55
import foo.bar.VulnerableUrlBuilder
66
import okhttp3.HttpUrl
77
import okhttp3.Request
8+
import org.eclipse.jetty.server.Server
9+
import org.eclipse.jetty.server.ServerConnector
10+
import org.eclipse.jetty.servlet.ServletContextHandler
11+
import org.eclipse.jetty.servlet.ServletHolder
812

9-
import static datadog.trace.agent.test.server.http.TestHttpServer.httpServer
13+
import javax.servlet.http.HttpServlet
14+
import javax.servlet.http.HttpServletRequest
15+
import javax.servlet.http.HttpServletResponse
1016

11-
class IastHttpClientIntegrationTest extends IastHttpServerTest<HttpServer> {
17+
class IastHttpClientIntegrationTest extends IastHttpServerTest<Server> {
1218

1319
static final CLIENTS = [
1420
'org.apache.http.impl.client.AutoRetryHttpClient',
@@ -20,28 +26,58 @@ class IastHttpClientIntegrationTest extends IastHttpServerTest<HttpServer> {
2026
@Override
2127
HttpServer server() {
2228
final controller = new SsrfController()
23-
return httpServer {
24-
handlers {
25-
prefix('/ssrf/execute') {
26-
final msg = controller.apacheSsrf(
27-
VulnerableUrlBuilder.url(request),
28-
(String) request.getParameter('clientClassName'),
29-
(String) request.getParameter('method'),
30-
(String) request.getParameter('requestType'),
31-
(String) request.getParameter('scheme')
32-
)
33-
response.status(200).send(msg)
34-
}
29+
30+
// Use _raw_ jetty API, so the jetty instrumentation is applied
31+
// (the :dd-java-agent:testing has a shadowed jetty server.)
32+
return new HttpServer() {
33+
Server jettyServer
34+
int port
35+
36+
@Override
37+
void start() {
38+
jettyServer = new Server(0) // 0 = random port
39+
40+
ServletContextHandler context = new ServletContextHandler(ServletContextHandler.SESSIONS)
41+
context.setContextPath('/')
42+
jettyServer.setHandler(context)
43+
44+
// Add servlet for the /ssrf/execute endpoint
45+
context.addServlet(new ServletHolder(new HttpServlet() {
46+
@Override
47+
protected void doGet(HttpServletRequest req, HttpServletResponse resp) {
48+
final msg = controller.apacheSsrf(
49+
VulnerableUrlBuilder.url(req),
50+
req.getParameter('clientClassName'),
51+
req.getParameter('method'),
52+
req.getParameter('requestType'),
53+
req.getParameter('scheme')
54+
)
55+
resp.setStatus(200)
56+
resp.writer.write(msg)
57+
}
58+
}), '/ssrf/execute')
59+
60+
jettyServer.start()
61+
port = ((ServerConnector) jettyServer.connectors[0]).localPort
3562
}
36-
}.asHttpServer()
63+
64+
@Override
65+
void stop() {
66+
jettyServer?.stop()
67+
}
68+
69+
@Override
70+
URI address() {
71+
return new URI("http://localhost:${port}/")
72+
}
73+
}
3774
}
3875

3976
void 'ssrf is present: #suite'() {
4077
setup:
4178
final url = suite.url(address)
4279
final request = new Request.Builder().url(url).get().build()
4380

44-
4581
when:
4682
def response = client.newCall(request).execute()
4783

@@ -52,6 +88,9 @@ class IastHttpClientIntegrationTest extends IastHttpServerTest<HttpServer> {
5288
assert to != null
5389
hasVulnerability(vul -> vul.type == VulnerabilityType.SSRF && suite.evidenceMatches(vul.evidence))
5490

91+
cleanup:
92+
response?.close()
93+
5594
where:
5695
suite << createTestSuite()
5796
}
@@ -129,7 +168,7 @@ class IastHttpClientIntegrationTest extends IastHttpServerTest<HttpServer> {
129168
}
130169

131170
private static enum TaintedTarget {
132-
URL{
171+
URL {
133172
void addTainted(HttpUrl.Builder builder, TestSuite suite) {
134173
builder.addQueryParameter('url', suite.scheme + '://inexistent/test?1=1')
135174
}
@@ -138,7 +177,7 @@ class IastHttpClientIntegrationTest extends IastHttpServerTest<HttpServer> {
138177
return assertTainted(evidence, 'url', suite.scheme + '://inexistent/test?1=1')
139178
}
140179
},
141-
SCHEME{
180+
SCHEME {
142181
void addTainted(HttpUrl.Builder builder, TestSuite suite) {
143182
// already added by the test
144183
}
@@ -147,7 +186,7 @@ class IastHttpClientIntegrationTest extends IastHttpServerTest<HttpServer> {
147186
return assertTainted(evidence, 'scheme', suite.scheme)
148187
}
149188
},
150-
HOST{
189+
HOST {
151190
void addTainted(HttpUrl.Builder builder, TestSuite suite) {
152191
builder.addQueryParameter('host', 'inexistent')
153192
}
@@ -156,7 +195,7 @@ class IastHttpClientIntegrationTest extends IastHttpServerTest<HttpServer> {
156195
return assertTainted(evidence, 'host', 'inexistent')
157196
}
158197
},
159-
PATH{
198+
PATH {
160199
void addTainted(HttpUrl.Builder builder, TestSuite suite) {
161200
builder.addQueryParameter('path', '/test')
162201
}
@@ -165,7 +204,7 @@ class IastHttpClientIntegrationTest extends IastHttpServerTest<HttpServer> {
165204
return assertTainted(evidence, 'path', '/test')
166205
}
167206
},
168-
QUERY{
207+
QUERY {
169208
void addTainted(HttpUrl.Builder builder, TestSuite suite) {
170209
builder.addQueryParameter('query', '?1=1')
171210
}

‎dd-java-agent/instrumentation/apache-httpclient/apache-httpclient-4.0/src/iastIntegrationTest/java/foo/bar/VulnerableUrlBuilder.java‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,28 @@
11
package foo.bar;
22

3-
import datadog.trace.agent.test.server.http.TestHttpServer.HandlerApi.RequestApi;
3+
import javax.servlet.http.HttpServletRequest;
44

55
/** Class to be instrumented by IAST call sites containing methods to work with urls */
66
public abstract class VulnerableUrlBuilder {
77

88
private VulnerableUrlBuilder() {}
99

10-
public static String url(RequestApi request) {
11-
final String url = (String) request.getParameter("url");
10+
public static String url(HttpServletRequest request) {
11+
final String url = request.getParameter("url");
1212
if (url != null) {
1313
return url;
1414
}
15-
final String scheme = (String) request.getParameter("scheme");
15+
final String scheme = request.getParameter("scheme");
1616
final boolean https = "https".equals(scheme);
17-
final String host = (String) request.getParameter("host");
17+
final String host = request.getParameter("host");
1818
if (host != null) {
1919
return (https ? "https://" : "http://") + host + "/test?1=1";
2020
}
21-
final String path = (String) request.getParameter("path");
21+
final String path = request.getParameter("path");
2222
if (path != null) {
2323
return (https ? "https://inexistent/" : "http://inexistent/") + path + "?1=1";
2424
}
25-
final String query = (String) request.getParameter("query");
25+
final String query = request.getParameter("query");
2626
if (query != null) {
2727
return (https ? "https://inexistent/test" : "http://inexistent/test") + query;
2828
}

‎dd-java-agent/instrumentation/dropwizard/build.gradle‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,6 @@ dependencies {
66
testImplementation project(':dd-java-agent:instrumentation:rs:jax-rs:jax-rs-annotations:jax-rs-annotations-2')
77
testImplementation project(':dd-java-agent:instrumentation:servlet:javax-servlet:javax-servlet-3.0')
88

9-
// Don't want to conflict with jetty from the test server.
10-
testImplementation(project(':dd-java-agent:instrumentation-testing')) {
11-
exclude group: 'org.eclipse.jetty', module: 'jetty-server'
12-
}
13-
149
// First version with DropwizardTestSupport:
1510
testImplementation group: 'io.dropwizard', name: 'dropwizard-testing', version: '0.8.0'
1611
testImplementation group: 'javax.xml.bind', name: 'jaxb-api', version: '2.2.3'

‎dd-java-agent/instrumentation/jetty/jetty-client/jetty-client-9.1/build.gradle‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -51,13 +51,10 @@ dependencies {
5151
implementation(project(':dd-java-agent:instrumentation:jetty:jetty-client:jetty-client-common')) {
5252
transitive = false
5353
}
54-
testImplementation(project(':dd-java-agent:instrumentation-testing')) {
55-
// explicitly declared below.
56-
exclude group: 'org.eclipse.jetty'
57-
}
5854
testImplementation project(':dd-java-agent:instrumentation:jetty:jetty-util-9.4.31')
5955
testImplementation group: 'org.eclipse.jetty', name: 'jetty-client', version: '9.1.0.v20131115'
6056
testImplementation group: 'org.eclipse.jetty', name: 'jetty-server', version: '9.1.0.v20131115'
57+
6158
latestDepTestImplementation group: 'org.eclipse.jetty', name: 'jetty-client', version: '9.+'
6259
latestDepTestImplementation group: 'org.eclipse.jetty', name: 'jetty-server', version: '9.+'
6360
}

‎dd-java-agent/instrumentation/jetty/jetty-client/jetty-client-9.1/gradle.lockfile‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,8 +82,7 @@ org.eclipse.jetty:jetty-http:9.1.0.v20131115=compileClasspath,testCompileClasspa
8282
org.eclipse.jetty:jetty-http:9.4.58.v20250814=latestDepTestCompileClasspath,latestDepTestRuntimeClasspath
8383
org.eclipse.jetty:jetty-io:9.1.0.v20131115=compileClasspath,testCompileClasspath,testRuntimeClasspath
8484
org.eclipse.jetty:jetty-io:9.4.58.v20250814=latestDepTestCompileClasspath,latestDepTestRuntimeClasspath
85-
org.eclipse.jetty:jetty-server:9.1.0.v20131115=testCompileClasspath,testRuntimeClasspath
86-
org.eclipse.jetty:jetty-server:9.4.58.v20250814=latestDepTestCompileClasspath,latestDepTestRuntimeClasspath
85+
org.eclipse.jetty:jetty-server:9.1.0.v20131115=latestDepTestCompileClasspath,latestDepTestRuntimeClasspath,testCompileClasspath,testRuntimeClasspath
8786
org.eclipse.jetty:jetty-util:9.1.0.v20131115=compileClasspath,testCompileClasspath,testRuntimeClasspath
8887
org.eclipse.jetty:jetty-util:9.4.58.v20250814=latestDepTestCompileClasspath,latestDepTestRuntimeClasspath
8988
org.gmetrics:GMetrics:2.1.0=codenarc

‎dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.0.4/build.gradle‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,6 @@ dependencies {
1717
implementation project(":dd-java-agent:instrumentation:jetty:jetty-server:jetty-server-9.0")
1818
compileOnly group: 'org.eclipse.jetty', name: 'jetty-server', version: '9.0.4.v20130625'
1919

20-
// Don't want to conflict with jetty from the test server.
21-
testImplementation(project(':dd-java-agent:instrumentation-testing')) {
22-
exclude group: 'org.eclipse.jetty', module: 'jetty-server'
23-
}
2420
testImplementation project(':dd-java-agent:instrumentation:jetty:jetty-util-9.4.31')
2521

2622
testImplementation group: 'org.eclipse.jetty', name: 'jetty-server', version: '9.0.4.v20130625'

‎dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.0/build.gradle‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,7 @@ dependencies {
2626
testFixturesImplementation(project(':dd-java-agent:instrumentation-testing')) {
2727
exclude group: 'org.eclipse.jetty', module: 'jetty-server'
2828
}
29-
// Don't want to conflict with jetty from the test server.
30-
testImplementation(project(':dd-java-agent:instrumentation-testing')) {
31-
exclude group: 'org.eclipse.jetty', module: 'jetty-server'
32-
}
29+
3330
testImplementation project(':dd-java-agent:instrumentation:jetty:jetty-util-9.4.31')
3431

3532
String jetty9Version = '9.0.0.v20130308'

‎dd-java-agent/instrumentation/jsp-2.3/src/test/groovy/JSPInstrumentationBasicTests.groovy‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import okhttp3.Request
66
import okhttp3.RequestBody
77
import okhttp3.Response
88
import org.apache.jasper.JasperException
9-
import org.eclipse.jetty.http.HttpStatus
109

1110
class JSPInstrumentationBasicTests extends JSPTestBase {
1211
def "non-erroneous GET #test test"() {
@@ -72,7 +71,7 @@ class JSPInstrumentationBasicTests extends JSPTestBase {
7271
}
7372
}
7473
}
75-
res.code() == HttpStatus.OK_200
74+
res.code() == HTTP_OK
7675

7776
cleanup:
7877
res.close()
@@ -149,7 +148,7 @@ class JSPInstrumentationBasicTests extends JSPTestBase {
149148
}
150149
}
151150
}
152-
res.code() == HttpStatus.OK_200
151+
res.code() == HTTP_OK
153152

154153
cleanup:
155154
res.close()
@@ -222,7 +221,7 @@ class JSPInstrumentationBasicTests extends JSPTestBase {
222221
}
223222
}
224223
}
225-
res.code() == HttpStatus.OK_200
224+
res.code() == HTTP_OK
226225

227226
cleanup:
228227
res.close()
@@ -305,7 +304,7 @@ class JSPInstrumentationBasicTests extends JSPTestBase {
305304
}
306305
}
307306
}
308-
res.code() == HttpStatus.INTERNAL_SERVER_ERROR_500
307+
res.code() == HTTP_INTERNAL_ERROR
309308

310309
cleanup:
311310
res.close()
@@ -380,7 +379,7 @@ class JSPInstrumentationBasicTests extends JSPTestBase {
380379
}
381380
}
382381
}
383-
res.code() == HttpStatus.OK_200
382+
res.code() == HTTP_OK
384383

385384
cleanup:
386385
res.close()
@@ -503,7 +502,7 @@ class JSPInstrumentationBasicTests extends JSPTestBase {
503502
}
504503
}
505504
}
506-
res.code() == HttpStatus.OK_200
505+
res.code() == HTTP_OK
507506

508507
cleanup:
509508
res.close()
@@ -561,7 +560,7 @@ class JSPInstrumentationBasicTests extends JSPTestBase {
561560
}
562561
}
563562
}
564-
res.code() == HttpStatus.INTERNAL_SERVER_ERROR_500
563+
res.code() == HTTP_INTERNAL_ERROR
565564

566565
cleanup:
567566
res.close()
@@ -581,7 +580,7 @@ class JSPInstrumentationBasicTests extends JSPTestBase {
581580
Response res = client.newCall(req).execute()
582581

583582
then:
584-
res.code() == HttpStatus.OK_200
583+
res.code() == HTTP_OK
585584
assertTraces(1) {
586585
trace(1) {
587586
span {

‎dd-java-agent/instrumentation/jsp-2.3/src/test/groovy/JSPInstrumentationForwardTests.groovy‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ import datadog.trace.bootstrap.instrumentation.api.Tags
33
import okhttp3.Request
44
import okhttp3.Response
55
import org.apache.jasper.JasperException
6-
import org.eclipse.jetty.http.HttpStatus
76

87
class JSPInstrumentationForwardTests extends JSPTestBase {
98
def "non-erroneous GET forward to #forwardTo"() {
@@ -97,7 +96,7 @@ class JSPInstrumentationForwardTests extends JSPTestBase {
9796
}
9897
}
9998
}
100-
res.code() == HttpStatus.OK_200
99+
res.code() == HTTP_OK
101100

102101
cleanup:
103102
res.close()
@@ -171,7 +170,7 @@ class JSPInstrumentationForwardTests extends JSPTestBase {
171170
}
172171
}
173172
}
174-
res.code() == HttpStatus.OK_200
173+
res.code() == HTTP_OK
175174

176175
cleanup:
177176
res.close()
@@ -324,7 +323,7 @@ class JSPInstrumentationForwardTests extends JSPTestBase {
324323
}
325324
}
326325
}
327-
res.code() == HttpStatus.OK_200
326+
res.code() == HTTP_OK
328327

329328
cleanup:
330329
res.close()
@@ -449,7 +448,7 @@ class JSPInstrumentationForwardTests extends JSPTestBase {
449448
}
450449
}
451450
}
452-
res.code() == HttpStatus.OK_200
451+
res.code() == HTTP_OK
453452

454453
cleanup:
455454
res.close()
@@ -535,7 +534,7 @@ class JSPInstrumentationForwardTests extends JSPTestBase {
535534
}
536535
}
537536
}
538-
res.code() == HttpStatus.INTERNAL_SERVER_ERROR_500
537+
res.code() == HTTP_INTERNAL_ERROR
539538

540539
cleanup:
541540
res.close()
@@ -604,7 +603,7 @@ class JSPInstrumentationForwardTests extends JSPTestBase {
604603
}
605604
}
606605
}
607-
res.code() == HttpStatus.NOT_FOUND_404
606+
res.code() == HTTP_NOT_FOUND
608607

609608
cleanup:
610609
res.close()

0 commit comments

Comments
 (0)