Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Http Client implementation #1741

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Refactor Http Client implementation #1741

wants to merge 5 commits into from

Conversation

tylerwowen
Copy link
Contributor

@tylerwowen tylerwowen commented Nov 6, 2024

Refactor the implementation using java.net with OkHttp.

Improvements

  1. Observability for outgoing requests
  2. 1 shared connection pool managed by OkHttp instead of many ad-hoc connections

@github-actions github-actions bot added the deploy-service Includes changes to deploy-service label Nov 6, 2024
throws IOException {
Request request =
new Request.Builder()
.url(buildUrl(url, params))

Check failure

Code scanning / CodeQL

Server-side request forgery

Potential server-side request forgery due to a [user-provided value](1). Potential server-side request forgery due to a [user-provided value](2). Potential server-side request forgery due to a [user-provided value](3).

Copilot Autofix AI about 13 hours ago

To fix the SSRF vulnerability, we need to validate the user-provided input before incorporating it into the URL. One approach is to maintain a list of authorized URLs or URL prefixes and ensure that the constructed URL matches one of these authorized patterns. Alternatively, we can restrict the URL construction to a specific host or URL prefix.

In this case, we will implement a validation method that checks if the constructed URL starts with a predefined URL prefix. This ensures that the URL is within the expected domain and prevents SSRF attacks.

Suggested changeset 1
deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java
--- a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java
+++ b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java
@@ -116,2 +116,9 @@
 
+    private void validateUrlPrefix(String url) {
+        String allowedPrefix = "https://api.github.com"; // Example allowed prefix
+        if (!url.startsWith(allowedPrefix)) {
+            throw new IllegalArgumentException("URL is not allowed: " + url);
+        }
+    }
+
     private static OkHttpObservationInterceptor.Builder observationInterceptorBuilder() {
@@ -169,2 +176,3 @@
     public HttpUrl buildUrl(String url, Map<String, String> params) {
+        validateUrlPrefix(url);
         HttpUrl.Builder urlBuilder = HttpUrl.parse(url).newBuilder();
EOF
@@ -116,2 +116,9 @@

private void validateUrlPrefix(String url) {
String allowedPrefix = "https://api.github.com"; // Example allowed prefix
if (!url.startsWith(allowedPrefix)) {
throw new IllegalArgumentException("URL is not allowed: " + url);
}
}

private static OkHttpObservationInterceptor.Builder observationInterceptorBuilder() {
@@ -169,2 +176,3 @@
public HttpUrl buildUrl(String url, Map<String, String> params) {
validateUrlPrefix(url);
HttpUrl.Builder urlBuilder = HttpUrl.parse(url).newBuilder();
Copilot is powered by AI and may make mistakes. Always verify output.
Unable to commit as this autofix suggestion is now outdated
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy-service Includes changes to deploy-service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant