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

Fix HttpServletRequest.getRemoteAddr for the HttpConnector mode #311

Merged
merged 2 commits into from
Nov 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,8 @@ public final class AppEngineConstants {
// (<internal20>)
public static final String WARMUP_IP = "0.1.0.3";

public static final String UNSPECIFIED_IP = "0.0.0.0";

public static final String DEFAULT_SECRET_KEY = "secretkey";

public static final String ENVIRONMENT_ATTR = "appengine.environment";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import static com.google.apphosting.runtime.AppEngineConstants.IS_TRUSTED;
import static com.google.apphosting.runtime.AppEngineConstants.PRIVATE_APPENGINE_HEADERS;
import static com.google.apphosting.runtime.AppEngineConstants.SKIP_ADMIN_CHECK_ATTR;
import static com.google.apphosting.runtime.AppEngineConstants.UNSPECIFIED_IP;
import static com.google.apphosting.runtime.AppEngineConstants.WARMUP_IP;
import static com.google.apphosting.runtime.AppEngineConstants.WARMUP_REQUEST_URL;
import static com.google.apphosting.runtime.AppEngineConstants.X_APPENGINE_API_TICKET;
Expand Down Expand Up @@ -59,14 +60,18 @@
import com.google.apphosting.runtime.jetty.AppInfoFactory;
import com.google.common.base.Strings;
import com.google.common.flogger.GoogleLogger;
import java.net.InetSocketAddress;
import java.net.SocketAddress;
import java.time.Duration;
import java.util.Objects;
import java.util.stream.Stream;
import org.eclipse.jetty.http.HttpField;
import org.eclipse.jetty.http.HttpFields;
import org.eclipse.jetty.http.HttpScheme;
import org.eclipse.jetty.http.HttpURI;
import org.eclipse.jetty.server.ConnectionMetaData;
import org.eclipse.jetty.server.Request;
import org.eclipse.jetty.util.HostPort;

/**
* Implementation for the {@link RequestAPIData} to allow for the Jetty {@link Request} to be used
Expand Down Expand Up @@ -274,6 +279,7 @@ public JettyRequestAPIData(
traceContext =
com.google.apphosting.base.protos.TracePb.TraceContextProto.getDefaultInstance();

String finalUserIp = userIp;
this.originalRequest = request;
this.request =
new Request.Wrapper(request) {
Expand All @@ -291,6 +297,26 @@ public boolean isSecure() {
public HttpFields getHeaders() {
return fields;
}

@Override
public ConnectionMetaData getConnectionMetaData() {
return new ConnectionMetaData.Wrapper(super.getConnectionMetaData()) {
@Override
public SocketAddress getRemoteSocketAddress() {
return InetSocketAddress.createUnresolved(finalUserIp, 0);
}

@Override
public HostPort getServerAuthority() {
return new HostPort(UNSPECIFIED_IP, 0);
}

@Override
public SocketAddress getLocalSocketAddress() {
return InetSocketAddress.createUnresolved(UNSPECIFIED_IP, 0);
}
};
}
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import static com.google.apphosting.runtime.AppEngineConstants.IS_TRUSTED;
import static com.google.apphosting.runtime.AppEngineConstants.PRIVATE_APPENGINE_HEADERS;
import static com.google.apphosting.runtime.AppEngineConstants.SKIP_ADMIN_CHECK_ATTR;
import static com.google.apphosting.runtime.AppEngineConstants.UNSPECIFIED_IP;
import static com.google.apphosting.runtime.AppEngineConstants.WARMUP_IP;
import static com.google.apphosting.runtime.AppEngineConstants.WARMUP_REQUEST_URL;
import static com.google.apphosting.runtime.AppEngineConstants.X_APPENGINE_API_TICKET;
Expand Down Expand Up @@ -60,15 +61,12 @@
import com.google.common.base.Strings;
import com.google.common.flogger.GoogleLogger;
import java.time.Duration;
import java.util.Collections;
import java.util.Enumeration;
import java.util.List;
import java.util.Locale;
import java.util.Objects;
import java.util.stream.Stream;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletRequestWrapper;

import org.eclipse.jetty.http.HttpField;
import org.eclipse.jetty.http.HttpFields;
import org.eclipse.jetty.http.HttpScheme;
Expand Down Expand Up @@ -287,6 +285,7 @@ public JettyRequestAPIData(
traceContext = TraceContextProto.getDefaultInstance();
}

String finalUserIp = userIp;
this.httpServletRequest =
new HttpServletRequestWrapper(httpServletRequest) {

Expand Down Expand Up @@ -329,6 +328,41 @@ public String getScheme() {
public boolean isSecure() {
return isSecure;
}

@Override
public String getRemoteAddr() {
return finalUserIp;
Copy link
Collaborator

Choose a reason for hiding this comment

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

[nit] Why not just use userIp directly. It doesn't mutate between these lines right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not possible because it is reassigned above and the variable has to be "effectively final", the compiler will give the error:

Variable 'userIp' is accessed from within inner class, needs to be final or effectively final

}

@Override
public String getServerName() {
return UNSPECIFIED_IP;
}

@Override
public String getRemoteHost() {
return finalUserIp;
}

@Override
public int getRemotePort() {
return 0;
}

@Override
public String getLocalName() {
return UNSPECIFIED_IP;
}

@Override
public String getLocalAddr() {
return UNSPECIFIED_IP;
}

@Override
public int getLocalPort() {
return 0;
}
};

this.baseRequest = request;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package com.google.apphosting.runtime.jetty9;

import static com.google.apphosting.runtime.AppEngineConstants.UNSPECIFIED_IP;

import com.google.apphosting.base.protos.RuntimePb.UPRequest;
import com.google.apphosting.base.protos.RuntimePb.UPResponse;
import com.google.apphosting.runtime.MutableUpResponse;
Expand Down Expand Up @@ -55,7 +57,7 @@ public MutableUpResponse getUpResponse() {

@Override
public InetSocketAddress getLocalAddress() {
return InetSocketAddress.createUnresolved("0.0.0.0", 0);
return InetSocketAddress.createUnresolved(UNSPECIFIED_IP, 0);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
/*
* Copyright 2021 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.google.apphosting.runtime.jetty9;

import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.MatcherAssert.assertThat;

import java.util.Arrays;
import java.util.Collection;
import org.eclipse.jetty.client.HttpClient;
import org.eclipse.jetty.client.api.ContentResponse;
import org.eclipse.jetty.http.HttpStatus;
import org.junit.After;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;

@RunWith(Parameterized.class)
public class RemoteAddressTest extends JavaRuntimeViaHttpBase {

@Parameterized.Parameters
public static Collection<Object[]> data() {
return Arrays.asList(
new Object[][] {
{"jetty94", false},
{"jetty94", true},
{"ee8", false},
{"ee8", true},
{"ee10", false},
{"ee10", true},
});
}

@Rule public TemporaryFolder temp = new TemporaryFolder();
private final HttpClient httpClient = new HttpClient();
private final boolean httpMode;
private final String environment;
private RuntimeContext<?> runtime;

public RemoteAddressTest(String environment, boolean httpMode) {
this.environment = environment;
this.httpMode = httpMode;
System.setProperty("appengine.use.HttpConnector", Boolean.toString(httpMode));
}

@Before
public void before() throws Exception {
String app = "com/google/apphosting/runtime/jetty9/remoteaddrapp/" + environment;
copyAppToDir(app, temp.getRoot().toPath());
httpClient.start();
runtime = runtimeContext();
System.err.println("==== Using Environment: " + environment + " " + httpMode + " ====");
}

@After
public void after() throws Exception {
httpClient.stop();
runtime.close();
}

@Test
public void test() throws Exception {
String url = runtime.jettyUrl("/");
ContentResponse response = httpClient.newRequest(url)
.header("X-AppEngine-User-IP", "203.0.113.1")
.send();

assertThat(response.getStatus(), equalTo(HttpStatus.OK_200));
String contentReceived = response.getContentAsString();
assertThat(contentReceived, containsString("getRemoteAddr: 203.0.113.1"));
assertThat(contentReceived, containsString("getLocalAddr: 0.0.0.0"));
assertThat(contentReceived, containsString("getServerPort: " + runtime.getPort()));
assertThat(contentReceived, containsString("getRemotePort: 0"));
assertThat(contentReceived, containsString("getLocalPort: 0"));
assertThat(contentReceived, containsString("getServerName: 0.0.0.0"));
}

private RuntimeContext<?> runtimeContext() throws Exception {
RuntimeContext.Config<?> config =
RuntimeContext.Config.builder().setApplicationPath(temp.getRoot().toString()).build();
return RuntimeContext.create(config);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import java.util.Enumeration;
import org.eclipse.jetty.util.IO;

/** Servlet that prints all the system properties. */
public class EE10EchoServlet extends HttpServlet {
@Override
protected void service(HttpServletRequest req, HttpServletResponse resp) throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import javax.servlet.http.HttpServletResponse;
import org.eclipse.jetty.util.IO;

/** Servlet that prints all the system properties. */
public class EE8EchoServlet extends HttpServlet {
@Override
protected void service(HttpServletRequest req, HttpServletResponse resp) throws IOException {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* Copyright 2021 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.google.apphosting.runtime.jetty9.remoteaddrapp;

import jakarta.servlet.http.HttpServlet;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import java.io.IOException;
import java.io.PrintWriter;

public class EE10RemoteAddrServlet extends HttpServlet {
@Override
protected void service(HttpServletRequest req, HttpServletResponse resp) throws IOException {
resp.setContentType("text/plain");
PrintWriter writer = resp.getWriter();
writer.println("getRemoteAddr: " + req.getRemoteAddr());
writer.println("getLocalAddr: " + req.getLocalAddr());
writer.println("getServerPort: " + req.getServerPort());
writer.println("getRemotePort: " + req.getRemotePort());
writer.println("getLocalPort: " + req.getLocalPort());
writer.println("getServerName: " + req.getServerName());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* Copyright 2021 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.google.apphosting.runtime.jetty9.remoteaddrapp;

import java.io.IOException;
import java.io.PrintWriter;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

public class EE8RemoteAddrServlet extends HttpServlet {
@Override
protected void service(HttpServletRequest req, HttpServletResponse resp) throws IOException {
resp.setContentType("text/plain");
PrintWriter writer = resp.getWriter();
writer.println("getRemoteAddr: " + req.getRemoteAddr());
writer.println("getLocalAddr: " + req.getLocalAddr());
writer.println("getServerPort: " + req.getServerPort());
writer.println("getRemotePort: " + req.getRemotePort());
writer.println("getLocalPort: " + req.getLocalPort());
writer.println("getServerName: " + req.getServerName());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,6 @@
<application>gzip</application>
<threadsafe>true</threadsafe>
<system-properties>
<property name="appengine.use.EE8" value="true"/>
<property name="appengine.use.EE8" value="false"/>
</system-properties>
</appengine-web-app>
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?xml version="1.0" encoding="utf-8"?>
<!--
Copyright 2021 Google LLC

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

https://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
-->

<appengine-web-app xmlns="http://appengine.google.com/ns/1.0">
<runtime>java21</runtime>
<application>remoteaddr</application>
<system-properties>
<property name="appengine.use.EE10" value="false"/>
<property name="appengine.use.EE10" value="true"/>
</system-properties>
</appengine-web-app>
Loading
Loading