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.

Sign up for GitHub

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

Jump to bottom

feat: openapi add client ip #84

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: openapi add client ip #84

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions ...c/main/java/com/ctrip/framework/apollo/openapi/client/service/AbstractOpenApiService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,13 @@

import com.ctrip.framework.apollo.openapi.client.exception.ApolloOpenApiException;
import com.ctrip.framework.apollo.openapi.client.url.OpenApiPathBuilder;
import com.ctrip.framework.foundation.Foundation;
import com.google.common.base.Preconditions;
import com.google.common.base.Strings;
import com.google.common.net.HttpHeaders;
import com.google.gson.Gson;
import org.apache.http.HttpResponse;
import org.apache.http.ProtocolVersion;
import org.apache.http.StatusLine;
import org.apache.http.client.methods.*;
import org.apache.http.entity.ContentType;
Expand Down Expand Up @@ -74,6 +77,9 @@ private CloseableHttpResponse execute(HttpEntityEnclosingRequestBase requestBase
}

private CloseableHttpResponse execute(HttpUriRequest request) throws IOException {

addClientIP(request);

CloseableHttpResponse response = client.execute(request);

checkHttpResponseStatus(response);
Expand All @@ -97,6 +103,12 @@ private void checkHttpResponseStatus(HttpResponse response) {
throw new ApolloOpenApiException(status.getStatusCode(), status.getReasonPhrase(), message);
}

private void addClientIP(HttpUriRequest request) {
ProtocolVersion protocolVersion = request.getProtocolVersion();
request.addHeader(HttpHeaders.FORWARDED, String.format("for=%s; proto=%s", Foundation.net().getHostAddress(), protocolVersion.getProtocol()));
request.addHeader(HttpHeaders.X_FORWARDED_FOR, Foundation.net().getHostAddress());
}
Comment on lines +106 to +110
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid adding 'Forwarded' and 'X-Forwarded-For' headers from client code

Adding the Forwarded and X-Forwarded-For headers directly from the client side is not recommended. These headers are intended to be set by intermediary proxies to inform the server about the original client's IP address and protocol. When clients set these headers themselves, it can lead to security issues such as IP spoofing, as servers cannot trust the values provided.

Instead, servers typically derive the client's IP address from the network layer. If there's a need to pass client-specific information, consider using custom headers with appropriate validation on the server side.

Suggested change:

Remove the addClientIP method and its invocation:

80

-    addClientIP(request);

82

...

106

-  private void addClientIP(HttpUriRequest request) {
-    ProtocolVersion protocolVersion = request.getProtocolVersion();
-    request.addHeader(HttpHeaders.FORWARDED, String.format("for=%s; proto=%s", Foundation.net().getHostAddress(), protocolVersion.getProtocol()));
-    request.addHeader(HttpHeaders.X_FORWARDED_FOR, Foundation.net().getHostAddress());
-  }

111
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private void addClientIP(HttpUriRequest request) {
ProtocolVersion protocolVersion = request.getProtocolVersion();
request.addHeader(HttpHeaders.FORWARDED, String.format("for=%s; proto=%s", Foundation.net().getHostAddress(), protocolVersion.getProtocol()));
request.addHeader(HttpHeaders.X_FORWARDED_FOR, Foundation.net().getHostAddress());
}
private void addClientIP(HttpUriRequest request) {
// Method removed as per security recommendation
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also believe it's not a good practice to add these headers. BTW, why is this feature needed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nobodyiam, the purpose of this feature appears to be to include the client's IP address in the HTTP requests made by the OpenAPI client. This might be intended for the server to perform tasks such as logging, auditing, or applying IP-based access controls based on the client's real IP.

However, as you've mentioned, adding the Forwarded and X-Forwarded-For headers directly from the client side is not recommended due to security concerns like IP spoofing. These headers are traditionally set by proxies or load balancers, and servers typically cannot trust these headers when they originate from clients.

It might be beneficial to discuss with the contributor to understand the specific use case they are trying to address. If there is a legitimate need to pass client-specific information, perhaps implementing a secure method such as using custom headers with proper authentication or tokens would be a better approach.

Please let me know if there's anything else I can assist with.


protected void checkNotNull(Object value, String name) {
Preconditions.checkArgument(null != value, name + " should not be null");
}
Expand Down
Loading