diff --git a/core/src/main/java/hudson/diagnosis/ReverseProxySetupMonitor.java b/core/src/main/java/hudson/diagnosis/ReverseProxySetupMonitor.java index 46851d7b0a..be3a6f385d 100644 --- a/core/src/main/java/hudson/diagnosis/ReverseProxySetupMonitor.java +++ b/core/src/main/java/hudson/diagnosis/ReverseProxySetupMonitor.java @@ -24,6 +24,7 @@ package hudson.diagnosis; import hudson.Extension; +import hudson.Util; import hudson.model.AdministrativeMonitor; import org.kohsuke.stapler.HttpRedirect; import org.kohsuke.stapler.HttpResponse; @@ -31,6 +32,10 @@ import org.kohsuke.stapler.HttpResponses; import org.kohsuke.stapler.QueryParameter; import java.io.IOException; +import java.util.logging.Level; +import java.util.logging.Logger; +import jenkins.model.Jenkins; +import org.kohsuke.stapler.Stapler; /** * Looks out for a broken reverse proxy setup that doesn't rewrite the location header correctly. @@ -45,6 +50,9 @@ import java.io.IOException; */ @Extension public class ReverseProxySetupMonitor extends AdministrativeMonitor { + + private static final Logger LOGGER = Logger.getLogger(ReverseProxySetupMonitor.class.getName()); + @Override public boolean isActivated() { // return true to always inject an HTML fragment to perform a test @@ -52,14 +60,25 @@ public class ReverseProxySetupMonitor extends AdministrativeMonitor { } public HttpResponse doTest() { - return new HttpRedirect("testForReverseProxySetup/a%2Fb/"); + String referer = Stapler.getCurrentRequest().getReferer(); + Jenkins j = Jenkins.getInstance(); + assert j != null; + // May need to send an absolute URL, since handling of HttpRedirect with a relative URL does not currently honor X-Forwarded-Proto/Port at all. + String redirect = j.getRootUrl() + "administrativeMonitor/" + id + "/testForReverseProxySetup/" + Util.rawEncode(referer) + "/"; + LOGGER.log(Level.FINE, "coming from {0} and redirecting to {1}", new Object[] {referer, redirect}); + return new HttpRedirect(redirect); } public void getTestForReverseProxySetup(String rest) { - if (rest.equals("a/b")) { + Jenkins j = Jenkins.getInstance(); + assert j != null; + String inferred = j.getRootUrlFromRequest() + "manage"; + // TODO this could also verify that j.getRootUrl() has been properly configured, and send a different message if not + if (rest.equals(inferred)) { throw HttpResponses.ok(); } else { - throw HttpResponses.errorWithoutStack(404, "expected a/b but got " + rest); + LOGGER.log(Level.WARNING, "{0} vs. {1}", new Object[] {inferred, rest}); + throw HttpResponses.errorWithoutStack(404, inferred + " vs. " + rest); } } diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java index 6f11cb8f14..6066a49795 100644 --- a/core/src/main/java/jenkins/model/Jenkins.java +++ b/core/src/main/java/jenkins/model/Jenkins.java @@ -1834,25 +1834,19 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve } /** - * Gets the absolute URL of Jenkins, - * such as "http://localhost/jenkins/". + * Gets the absolute URL of Jenkins, such as {@code http://localhost/jenkins/}. * *

* This method first tries to use the manually configured value, then - * fall back to {@link StaplerRequest#getRootPath()}. + * fall back to {@link #getRootUrlFromRequest}. * It is done in this order so that it can work correctly even in the face * of a reverse proxy. * - * @return - * This method returns null if this parameter is not configured by the user. - * The caller must gracefully deal with this situation. - * The returned URL will always have the trailing '/'. + * @return null if this parameter is not configured by the user and the calling thread is not in an HTTP request; otherwise the returned URL will always have the trailing {@code /} * @since 1.66 - * @see Descriptor#getCheckUrl(String) - * @see #getRootUrlFromRequest() * @see Hyperlinks in HTML */ - public String getRootUrl() { + public @Nullable String getRootUrl() { String url = JenkinsLocationConfiguration.get().getUrl(); if(url!=null) { return Util.ensureEndsWith(url,"/"); @@ -1875,7 +1869,7 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve } /** - * Gets the absolute URL of Hudson top page, such as "http://localhost/hudson/". + * Gets the absolute URL of Hudson top page, such as {@code http://localhost/hudson/}. * *

* Unlike {@link #getRootUrl()}, which uses the manually configured value, @@ -1884,16 +1878,18 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve * correctly, especially when a migration is involved), but the downside * is that unless you are processing a request, this method doesn't work. * - * Please note that this will not work in all cases if Jenkins is running behind a - * reverse proxy (e.g. when user has switched off ProxyPreserveHost, which is - * default setup or the actual url uses https) and you should use getRootUrl if - * you want to be sure you reflect user setup. - * See https://wiki.jenkins-ci.org/display/JENKINS/Running+Jenkins+behind+Apache - * + *

Please note that this will not work in all cases if Jenkins is running behind a + * reverse proxy which has not been fully configured. + * Specifically the {@code Host} and {@code X-Forwarded-Proto} headers must be set. + * Running Jenkins behind Apache + * shows some examples of configuration. * @since 1.263 */ - public String getRootUrlFromRequest() { + public @Nonnull String getRootUrlFromRequest() { StaplerRequest req = Stapler.getCurrentRequest(); + if (req == null) { + throw new IllegalStateException("cannot call getRootUrlFromRequest from outside a request handling thread"); + } StringBuilder buf = new StringBuilder(); String scheme = req.getScheme(); String forwardedScheme = req.getHeader("X-Forwarded-Proto"); @@ -1902,8 +1898,11 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve } buf.append(scheme+"://"); buf.append(req.getServerName()); - if(req.getServerPort()!=80) - buf.append(':').append(req.getServerPort()); + int forwardedPort = req.getIntHeader("X-Forwarded-Port"); + int port = forwardedPort == -1 ? req.getServerPort() : forwardedPort; + if (port != ("https".equals(scheme) ? 443 : 80)) { + buf.append(':').append(port); + } buf.append(req.getContextPath()).append('/'); return buf.toString(); } diff --git a/core/src/test/java/jenkins/model/JenkinsGetRootUrlTest.java b/core/src/test/java/jenkins/model/JenkinsGetRootUrlTest.java index 67095ede32..0c255e4d0f 100644 --- a/core/src/test/java/jenkins/model/JenkinsGetRootUrlTest.java +++ b/core/src/test/java/jenkins/model/JenkinsGetRootUrlTest.java @@ -37,7 +37,10 @@ import org.junit.runner.RunWith; import org.jvnet.hudson.test.Bug; import org.kohsuke.stapler.Stapler; import org.kohsuke.stapler.StaplerRequest; +import static org.mockito.Matchers.anyString; import org.mockito.Mockito; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; import org.powermock.core.classloader.annotations.PrepareForTest; import org.powermock.modules.junit4.PowerMockRunner; @@ -107,19 +110,34 @@ public class JenkinsGetRootUrlTest { @Test public void useForwardedProtoWhenPresent() { configured("https://ci/jenkins/"); - + // Without a forwarded protocol, it should use the request protocol accessing("http://ci/jenkins/"); rootUrlFromRequestIs("http://ci/jenkins/"); - + + accessing("http://ci:8080/jenkins/"); + rootUrlFromRequestIs("http://ci:8080/jenkins/"); + // With a forwarded protocol, it should use the forwarded protocol - accessing("http://ci/jenkins/"); + accessing("https://ci/jenkins/"); withHeader("X-Forwarded-Proto", "https"); rootUrlFromRequestIs("https://ci/jenkins/"); - - accessing("https://ci/jenkins/"); + + accessing("http://ci/jenkins/"); withHeader("X-Forwarded-Proto", "http"); rootUrlFromRequestIs("http://ci/jenkins/"); + + // ServletRequest.getServerPort is not always meaningful. + // http://tomcat.apache.org/tomcat-5.5-doc/config/http.html#Proxy_Support or + // http://wiki.eclipse.org/Jetty/Howto/Configure_mod_proxy#Configuring_mod_proxy_as_a_Reverse_Proxy.5D + // can be used to ensure that it is hardcoded or that X-Forwarded-Port is interpreted. + // But this is not something that can be configured purely from the reverse proxy; the container must be modified too. + // And the standard bundled Jetty in Jenkins does not work that way; + // it will return 80 even when Jenkins is fronted by Apache with SSL. + accessing("http://ci/jenkins/"); // as if the container is not aware of the forwarded port + withHeader("X-Forwarded-Port", "443"); // but we tell it + withHeader("X-Forwarded-Proto", "https"); + rootUrlFromRequestIs("https://ci/jenkins/"); } private void rootUrlFromRequestIs(final String expectedRootUrl) { @@ -137,7 +155,7 @@ public class JenkinsGetRootUrlTest { when(config.getUrl()).thenReturn(configuredHost); } - private void withHeader(String name, String value) { + private void withHeader(String name, final String value) { final StaplerRequest req = Stapler.getCurrentRequest(); when(req.getHeader(name)).thenReturn(value); } @@ -149,8 +167,15 @@ public class JenkinsGetRootUrlTest { final StaplerRequest req = mock(StaplerRequest.class); when(req.getScheme()).thenReturn(url.getProtocol()); when(req.getServerName()).thenReturn(url.getHost()); - when(req.getServerPort()).thenReturn(url.getPort() == -1 ? 80 : url.getPort()); + when(req.getServerPort()).thenReturn(url.getPort() == -1 ? ("https".equals(url.getProtocol()) ? 443 : 80) : url.getPort()); when(req.getContextPath()).thenReturn(url.getPath().replaceAll("/$", "")); + when(req.getIntHeader(anyString())).thenAnswer(new Answer() { + @Override public Integer answer(InvocationOnMock invocation) throws Throwable { + String name = (String) invocation.getArguments()[0]; + String value = ((StaplerRequest) invocation.getMock()).getHeader(name); + return value != null ? Integer.parseInt(value) : -1; + } + }); when(Stapler.getCurrentRequest()).thenReturn(req); } diff --git a/test/src/test/java/hudson/diagnosis/ReverseProxySetupMonitorTest.java b/test/src/test/java/hudson/diagnosis/ReverseProxySetupMonitorTest.java index afd1454d19..2e2d8dd6f1 100644 --- a/test/src/test/java/hudson/diagnosis/ReverseProxySetupMonitorTest.java +++ b/test/src/test/java/hudson/diagnosis/ReverseProxySetupMonitorTest.java @@ -24,6 +24,8 @@ package hudson.diagnosis; +import com.gargoylesoftware.htmlunit.WebRequestSettings; +import java.net.URL; import org.junit.Test; import org.junit.Rule; import org.jvnet.hudson.test.JenkinsRule; @@ -33,7 +35,9 @@ public class ReverseProxySetupMonitorTest { @Rule public JenkinsRule r = new JenkinsRule(); @Test public void normal() throws Exception { - r.createWebClient().goTo(r.jenkins.getAdministrativeMonitor(ReverseProxySetupMonitor.class.getName()).getUrl() + "/test", null); + WebRequestSettings wrs = new WebRequestSettings(new URL(r.getURL(), r.jenkins.getAdministrativeMonitor(ReverseProxySetupMonitor.class.getName()).getUrl() + "/test")); + wrs.setAdditionalHeader("Referer", r.getURL() + "manage"); + r.createWebClient().getPage(wrs); } }