Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Merge pull request #1294 from /pull/1265/head
[FIXED JENKINS-23294] Interpret X-Forwarded-Port
  • Loading branch information
stephenc committed Jul 7, 2014
2 parents f277502 + 0ae4c2c commit a239197
Show file tree
Hide file tree
Showing 6 changed files with 109 additions and 39 deletions.
25 changes: 22 additions & 3 deletions core/src/main/java/hudson/diagnosis/ReverseProxySetupMonitor.java
Expand Up @@ -24,13 +24,18 @@
package hudson.diagnosis;

import hudson.Extension;
import hudson.Util;
import hudson.model.AdministrativeMonitor;
import org.kohsuke.stapler.HttpRedirect;
import org.kohsuke.stapler.HttpResponse;
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.
Expand All @@ -45,21 +50,35 @@
*/
@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
return true;
}

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

Expand Down
73 changes: 47 additions & 26 deletions core/src/main/java/jenkins/model/Jenkins.java
Expand Up @@ -1834,25 +1834,19 @@ public String getUrlChildPrefix() {
}

/**
* 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,"/");
Expand All @@ -1875,7 +1869,7 @@ public boolean isRootUrlSecure() {
}

/**
* 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,
Expand All @@ -1884,30 +1878,57 @@ public boolean isRootUrlSecure() {
* 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;
}
Expand Down
39 changes: 32 additions & 7 deletions core/src/test/java/jenkins/model/JenkinsGetRootUrlTest.java
Expand Up @@ -37,7 +37,10 @@
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;

Expand Down Expand Up @@ -107,19 +110,34 @@ public void doNotInheritProtocolWhenDispatchingRequest2() {
@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) {
Expand All @@ -137,7 +155,7 @@ private void configured(final String configuredHost) {
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);
}
Expand All @@ -149,8 +167,15 @@ private void accessing(final String realUrl) {
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);
}
Expand Down
Expand Up @@ -1815,6 +1815,7 @@ public HtmlPage goTo(String relative) throws IOException, SAXException {
}

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);
Expand Down
Expand Up @@ -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;
Expand All @@ -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);
}

}
Expand Up @@ -99,7 +99,7 @@ public void _testBasicWorkflow() throws Exception {
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());

Expand All @@ -121,7 +121,7 @@ public void _testBasicWorkflow() throws Exception {
}

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) {
Expand Down

0 comments on commit a239197

Please sign in to comment.