Merge pull request #1294 from jenkinsci/pull/1265/head

[FIXED JENKINS-23294] Interpret X-Forwarded-Port

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);
         }
     }
 

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/}.
      *
      * <p>
      * 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 <a href="https://wiki.jenkins-ci.org/display/JENKINS/Hyperlinks+in+HTML">Hyperlinks in HTML</a>
      */
-    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/}.
      *
      * <p>
      * Unlike {@link #getRootUrl()}, which uses the manually configured value,
@@ -1884,30 +1878,57 @@ 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
-     *
+     * <p>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.
+     * <a href="https://wiki.jenkins-ci.org/display/JENKINS/Running+Jenkins+behind+Apache">Running Jenkins behind Apache</a>
+     * 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");
-        if (forwardedScheme != null) {
-            scheme = forwardedScheme;
+        String scheme = getXForwardedHeader(req, "X-Forwarded-Proto", req.getScheme());
+        buf.append(scheme).append("://");
+        String host = getXForwardedHeader(req, "X-Forwarded-Host", req.getServerName());
+        buf.append(host);
+        int port = req.getServerPort();
+        String forwardedPort = getXForwardedHeader(req, "X-Forwarded-Port", null);
+        if (forwardedPort != null) {
+            try {
+                port = Integer.parseInt(forwardedPort);
+            } catch (NumberFormatException e) {
+                // ignore
+            }
+        }
+        if (port != ("https".equals(scheme) ? 443 : 80)) {
+            buf.append(':').append(port);
         }
-        buf.append(scheme+"://");
-        buf.append(req.getServerName());
-        if(req.getServerPort()!=80)
-            buf.append(':').append(req.getServerPort());
         buf.append(req.getContextPath()).append('/');
         return buf.toString();
     }
 
+    /**
+     * Gets the originating "X-Forwarded-..." header from the request. If there are multiple headers the originating
+     * header is the first header. If the originating header contains a comma separated list, the originating entry
+     * is the first one.
+     * @param req the request
+     * @param header the header name
+     * @param defaultValue the value to return if the header is absent.
+     * @return the originating entry of the header or the default value if the header was not present.
+     */
+    private static String getXForwardedHeader(StaplerRequest req, String header, String defaultValue) {
+        String value = req.getHeader(header);
+        if (value != null) {
+            int index = value.indexOf(',');
+            return index == -1 ? value.trim() : value.substring(0,index).trim();
+        }
+        return defaultValue;
+    }
+
     public File getRootDir() {
         return root;
     }

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<Integer>() {
+            @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);
     }

test/src/main/java/org/jvnet/hudson/test/HudsonTestCase.java

@@ -1815,6 +1815,7 @@ public abstract class HudsonTestCase extends TestCase implements RootAction {
         }
 
         public Page goTo(String relative, String expectedContentType) throws IOException, SAXException {
+            while (relative.startsWith("/")) relative = relative.substring(1);
             Page p;
             try {
                 p = super.getPage(getContextPath() + relative);

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);
     }
 
 }

test/src/test/java/jenkins/security/RekeySecretAdminMonitorTest.java

@@ -99,7 +99,7 @@ public class RekeySecretAdminMonitorTest extends HudsonTestCase {
         assertTrue(monitor.getLogFile().exists());
 
         // should be no warning/error now
-        HtmlPage manage = wc.goTo("/manage");
+        HtmlPage manage = wc.goTo("manage");
         assertEquals(0,manage.selectNodes("//*[class='error']").size());
         assertEquals(0,manage.selectNodes("//*[class='warning']").size());
 
@@ -121,7 +121,7 @@ public class RekeySecretAdminMonitorTest extends HudsonTestCase {
     }
 
     private HtmlForm getRekeyForm(WebClient wc) throws IOException, SAXException {
-        return wc.goTo("/manage").getFormByName("rekey");
+        return wc.goTo("manage").getFormByName("rekey");
     }
 
     private HtmlButton getButton(HtmlForm form, int index) {