-
Notifications
You must be signed in to change notification settings - Fork 291
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Merge pull request #3682 from DataDog/okhttpVuln
Added exception catches for the OkHTTPClient header vulnerability
- Loading branch information
Showing
6 changed files
with
291 additions
and
13 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
159 changes: 159 additions & 0 deletions
159
communication/src/main/java/datadog/communication/http/SafeRequestBuilder.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,159 @@ | ||
| package datadog.communication.http; | ||
|
|
||
| import java.net.URL; | ||
| import javax.annotation.Nullable; | ||
| import okhttp3.CacheControl; | ||
| import okhttp3.Headers; | ||
| import okhttp3.HttpUrl; | ||
| import okhttp3.Request; | ||
| import okhttp3.RequestBody; | ||
|
|
||
| /* | ||
| The purpose of this class is to wrap okhttp3.Request.Builder. | ||
| This adapter is used to resolve a vulnerability in Request.builder where | ||
| the .header() and .addHeader() methods can cause header secrets to be | ||
| printed/logged when invalid characters are parsed. | ||
| */ | ||
| public final class SafeRequestBuilder { | ||
|
|
||
| private final Request.Builder requestBuilder; | ||
|
|
||
| public SafeRequestBuilder() { | ||
| this.requestBuilder = new Request.Builder(); | ||
| } | ||
|
|
||
| // Constructs a SafeRequestBuilder from an existing request | ||
| public SafeRequestBuilder(Request.Builder request) { | ||
| this.requestBuilder = request; | ||
| } | ||
|
|
||
| public SafeRequestBuilder url(HttpUrl url) { | ||
| this.requestBuilder.url(url); | ||
| return this; | ||
| } | ||
|
|
||
| public SafeRequestBuilder url(String url) { | ||
| this.requestBuilder.url(url); | ||
| return this; | ||
| } | ||
|
|
||
| public SafeRequestBuilder url(URL url) { | ||
| this.requestBuilder.url(url); | ||
| return this; | ||
| } | ||
|
|
||
| public SafeRequestBuilder header(String name, String value) { | ||
| // This try/catch block prevents header secrets from being outputted to | ||
| // console or logs. | ||
| try { | ||
| this.requestBuilder.header(name, value); | ||
| return this; | ||
| } catch (IllegalArgumentException e) { | ||
| throw new IllegalArgumentException( | ||
| new StringBuilder() | ||
| .append("InvalidArgumentException at header() for header: ") | ||
| .append(name) | ||
| .toString()); | ||
| } | ||
| } | ||
|
|
||
| public SafeRequestBuilder addHeader(String name, String value) { | ||
| // This try/catch block prevents header secrets from being outputted to | ||
| // console or logs. | ||
| try { | ||
| this.requestBuilder.addHeader(name, value); | ||
| return this; | ||
| } catch (IllegalArgumentException e) { | ||
| throw new IllegalArgumentException( | ||
| new StringBuilder() | ||
| .append("InvalidArgumentException at addHeader() for header: ") | ||
| .append(name) | ||
| .toString()); | ||
| } | ||
| } | ||
|
|
||
| public static Request.Builder addHeader(Request.Builder builder, String name, String value) { | ||
| try { | ||
| return builder.addHeader(name, value); | ||
| } catch (IllegalArgumentException e) { | ||
| throw new IllegalArgumentException( | ||
| new StringBuilder() | ||
| .append("InvalidArgumentException at addHeader() for header: ") | ||
| .append(name) | ||
| .toString()); | ||
| } | ||
| } | ||
|
|
||
| public SafeRequestBuilder removeHeader(String name) { | ||
| this.requestBuilder.removeHeader(name); | ||
| return this; | ||
| } | ||
|
|
||
| public SafeRequestBuilder headers(Headers headers) { | ||
| this.requestBuilder.headers(headers); | ||
| return this; | ||
| } | ||
|
|
||
| public SafeRequestBuilder cacheControl(CacheControl cacheControl) { | ||
| this.requestBuilder.cacheControl(cacheControl); | ||
| return this; | ||
| } | ||
|
|
||
| public SafeRequestBuilder get() { | ||
| this.requestBuilder.get(); | ||
| return this; | ||
| } | ||
|
|
||
| public SafeRequestBuilder head() { | ||
| this.requestBuilder.head(); | ||
| return this; | ||
| } | ||
|
|
||
| public SafeRequestBuilder post(RequestBody body) { | ||
| this.requestBuilder.post(body); | ||
| return this; | ||
| } | ||
|
|
||
| public SafeRequestBuilder delete(@Nullable RequestBody body) { | ||
| this.requestBuilder.delete(body); | ||
| return this; | ||
| } | ||
|
|
||
| public SafeRequestBuilder delete() { | ||
| this.requestBuilder.delete(); | ||
| return this; | ||
| } | ||
|
|
||
| public SafeRequestBuilder put(RequestBody body) { | ||
| this.requestBuilder.put(body); | ||
| return this; | ||
| } | ||
|
|
||
| public SafeRequestBuilder patch(RequestBody body) { | ||
| this.requestBuilder.patch(body); | ||
| return this; | ||
| } | ||
|
|
||
| public SafeRequestBuilder method(String method, @Nullable RequestBody body) { | ||
| this.requestBuilder.method(method, body); | ||
| return this; | ||
| } | ||
|
|
||
| public SafeRequestBuilder tag(@Nullable Object tag) { | ||
| this.requestBuilder.tag(tag); | ||
| return this; | ||
| } | ||
|
|
||
| public <T> SafeRequestBuilder tag(Class<? super T> type, @Nullable T tag) { | ||
| this.requestBuilder.tag(type, tag); | ||
| return this; | ||
| } | ||
|
|
||
| public Request build() { | ||
| return this.requestBuilder.build(); | ||
| } | ||
|
|
||
| public Request.Builder getBuilder() { | ||
| return this.requestBuilder; | ||
| } | ||
| } |
115 changes: 115 additions & 0 deletions
115
communication/src/test/groovy/datadog/communication/http/SafeRequestBuilderTest.groovy
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,115 @@ | ||
| package datadog.communication.http | ||
|
|
||
|
|
||
| import okhttp3.Headers | ||
| import okhttp3.MediaType | ||
| import okhttp3.Request | ||
| import okhttp3.RequestBody | ||
| import org.junit.Test | ||
| import org.junit.Assert | ||
|
|
||
| class SafeRequestBuilderTest { | ||
| SafeRequestBuilder testBuilder = new SafeRequestBuilder() | ||
|
|
||
| @Test | ||
| void "test add header"(){ | ||
| testBuilder.url("http:localhost").addHeader("test","test") | ||
| Assert.assertEquals(testBuilder.build().headers().get("test"),"test") | ||
| } | ||
| @Test | ||
| void "test remove header"(){ | ||
| testBuilder.url("http:localhost").removeHeader("test") | ||
| Assert.assertEquals(testBuilder.build().headers().get("test"),null) | ||
| } | ||
| @Test | ||
| void "test static add header"(){ | ||
| Request.Builder builder = new Request.Builder().url("http://localhost") | ||
| builder = SafeRequestBuilder.addHeader(builder,"test","test") | ||
| Assert.assertEquals(builder.build().headers().get("test"),"test") | ||
| } | ||
| @Test (expected = IllegalArgumentException) | ||
| void "test bad static add header"(){ | ||
| Request.Builder builder = new Request.Builder().url("http://localhost") | ||
| builder = SafeRequestBuilder.addHeader(builder,"\n\n","\n\n") | ||
| } | ||
| @Test(expected = IllegalArgumentException) | ||
| void "test adding bad header"(){ | ||
| testBuilder.url("http:localhost").addHeader("\n\n","\n\n") | ||
| } | ||
| @Test (expected = IllegalArgumentException) | ||
| void "test adding bad header2"(){ | ||
| testBuilder.url("localhost").header("\u0019","\u0080") | ||
| } | ||
| @Test | ||
| void "test building result"(){ | ||
| Request testRequest = new SafeRequestBuilder().url("http://localhost") | ||
| .header("key","value").build() | ||
| Assert.assertEquals(Request,testRequest.getClass()) | ||
| } | ||
| @Test | ||
| void "test getBuilder"(){ | ||
| Request.Builder originalBuilder = new Request.Builder().url("http://localhost") | ||
| .addHeader("test","test") | ||
| testBuilder = new SafeRequestBuilder(originalBuilder) | ||
| Assert.assertEquals(originalBuilder,testBuilder.getBuilder()) | ||
| } | ||
| @Test | ||
| void "test get method"(){ | ||
| testBuilder = new SafeRequestBuilder().url("http://localhost") | ||
| testBuilder.get() | ||
| testBuilder.headers(new Headers()) | ||
| Assert.assertEquals(testBuilder.build().method(),"GET") | ||
| } | ||
| @Test | ||
| void "test post method"(){ | ||
| testBuilder = new SafeRequestBuilder().url("http://localhost") | ||
| RequestBody body = RequestBody.create(MediaType.parse("application/json"), "{}") | ||
| testBuilder.post(body) | ||
| Assert.assertEquals(testBuilder.build().method(),"POST") | ||
| } | ||
| @Test | ||
| void "test put method"(){ | ||
| testBuilder = new SafeRequestBuilder().url("http://localhost") | ||
| RequestBody body = RequestBody.create(MediaType.parse("application/json"), "{}") | ||
| testBuilder.put(body) | ||
| Assert.assertEquals(testBuilder.build().method(),"PUT") | ||
| } | ||
| @Test | ||
| void "test patch method"(){ | ||
| testBuilder = new SafeRequestBuilder().url("http://localhost") | ||
| RequestBody body = RequestBody.create(MediaType.parse("application/json"), "{}") | ||
| testBuilder.patch(body) | ||
| Assert.assertEquals(testBuilder.build().method(),"PATCH") | ||
| } | ||
| @Test | ||
| void "test delete method"(){ | ||
| testBuilder = new SafeRequestBuilder().url("http://localhost") | ||
| testBuilder.delete() | ||
| Assert.assertEquals(testBuilder.build().method(),"DELETE") | ||
| } | ||
| @Test | ||
| void "test method adder"(){ | ||
| testBuilder = new SafeRequestBuilder().url("http://localhost") | ||
| testBuilder.method("GET",null) | ||
| } | ||
| @Test | ||
| void "test tag"(){ | ||
| testBuilder = new SafeRequestBuilder() | ||
| URL url = new URL("http://localhost") | ||
| testBuilder.url(url).tag("test") | ||
| Assert.assertEquals(testBuilder.build().tag().toString(),"test") | ||
| } | ||
| @Test | ||
| void "test head"(){ | ||
| testBuilder = new SafeRequestBuilder().url("http://localhost") | ||
| testBuilder.head() | ||
| Assert.assertEquals(testBuilder.build().method(),"HEAD") | ||
| } | ||
| @Test | ||
| void "test delete with parameter method"(){ | ||
| testBuilder = new SafeRequestBuilder().url("http://localhost") | ||
| RequestBody body = RequestBody.create(MediaType.parse("application/json"), "{}") | ||
| testBuilder.delete(body) | ||
| Assert.assertEquals(testBuilder.build().method(),"DELETE") | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.