fix: getRemoteHost and getRemotePort for ALB #827
Conversation
| @@ -441,6 +443,13 @@ public String getRemoteAddr() { | |||
|
|
|||
| @Override | |||
| public String getRemoteHost() { | |||
| if (Objects.nonNull(request.getRequestContext().getElb())) { | |||
| String host_header = request.getHeaders().get(HttpHeaders.HOST); | |||
There was a problem hiding this comment.
please avoid underscores in variable names, use hostHeader instead.
| @@ -471,6 +480,12 @@ public RequestDispatcher getRequestDispatcher(String s) { | |||
|
|
|||
| @Override | |||
| public int getRemotePort() { | |||
| if (Objects.nonNull(request.getRequestContext().getElb())) { | |||
| String hostHeader = request.getHeaders().get(HttpHeaders.HOST); | |||
| String port = Arrays.asList(hostHeader.split(":")).get(1); | |||
There was a problem hiding this comment.
I'm currently seeing java.lang.ArrayIndexOutOfBoundsException: Index 1 out of bounds for length 1 when calling getRemotePort().
For a http Request on 80 the payload looks like this:
{
"requestContext": {
"elb": {
"targetGroupArn": "arn:aws:elasticloadbalancing:eu-central-1:5952169XXXXX:targetgroup/sam-ap-MyTar-ZEEXXXXX/77425e11d8XXXX"
}
},
"httpMethod": "GET",
"path": "/",
"queryStringParameters": {},
"headers": {
"accept": "*/*",
"host": "sam-ap-MyLoa-jNFZXXXXX-000000000.eu-central-1.elb.amazonaws.com",
"user-agent": "curl/8.4.0",
"x-amzn-trace-id": "Root=1-661944f2-348171d959c3a5cb69d00000",
"x-forwarded-for": "1.2.3.4",
"x-forwarded-port": "80",
"x-forwarded-proto": "http"
},
"body": "",
"isBase64Encoded": false
}
There was a problem hiding this comment.
Looking at the Javadoc for jakarta.servlet.ServletRequest.getRemoteHost()
Returns the fully qualified name of the address returned by
getRemoteAddr(). If the engine cannot or chooses not to resolve the hostname (to improve performance), this method returns the IP address.
it doesn't seem consistent. In my view we likely need to change getRemoteAddr() as well, wdyt?
getRemotePort() now looks similar but not identical to getServerPort(). Is that intended?
| @@ -471,6 +480,11 @@ public RequestDispatcher getRequestDispatcher(String s) { | |||
|
|
|||
| @Override | |||
| public int getRemotePort() { | |||
| if (Objects.nonNull(request.getRequestContext().getElb())) { | |||
| String hostHeader = request.getHeaders().get(PORT_HEADER_NAME); | |||
There was a problem hiding this comment.
Variable naming is misleading as it is no longer the host header.
There was a problem hiding this comment.
I think you're right. We need to update getRemoteAddr() to take into account the x-forwarded-for header. For getServerPort(), I'm a bit confused. Looking at the javadoc for both getServerPort() and getRemotePort(), I'm wondering if Returns the Internet Protocol (IP) source port of the client or last proxy that sent the request. and Returns the port number to which the request was sent don't mean the same thing. Is ALB considered as a client in this case?
There was a problem hiding this comment.
To make it even more interesting there is also getLocalPort().
ALB is the last proxy so that should be fine.
For comparison Tomcat sets serverPort based on scheme https://github.com/apache/tomcat/blob/main/java/org/apache/catalina/connector/CoyoteAdapter.java#L579.
| @@ -471,6 +480,11 @@ public RequestDispatcher getRequestDispatcher(String s) { | |||
|
|
|||
| @Override | |||
| public int getRemotePort() { | |||
| if (Objects.nonNull(request.getRequestContext().getElb())) { | |||
| String hostHeader = request.getHeaders().get(PORT_HEADER_NAME); | |||
| if (Objects.nonNull(hostHeader)) | |||
There was a problem hiding this comment.
please use brackets for the if statement
|
Merged this for now as it fixes #816 and should be included in next release. Additional improvements for ALB can be added as separate PR. |
Issue #, if available: #816
Description of changes:
ALB host header information comes in a singleValueHeader object, instead of the multivalueHeaders that was checked.
By submitting this pull request