[JENKINS-76335] Allow redirect on GET and HEAD only (#229)

* [JENKINS-76335] Allow redirect on GET and HEAD only

In Apache http client 5, the behavior of the DefaultRedirectStrategy
has changed. With httpclient4 a redirect was only done on GET and HEAD
requests, using the same method. In httpclient 5 a redirect is done for
all methods and all redirect status codes. But a POST request is
converted to a GET for 301 and 302 codes. Also a redirect is only done
when the url is on the same server:port or when no cookies and
authorization headers are included.
This change implements a new RedirectStrategy, that limits redirects to
GET and HEAD as in client 4 but also keeps the enhanced security of not
following redirects then the server/port changes and
cookies/authorization is included.

* Remove unused import

* add test

---------

Co-authored-by: Mark Waite <mark.earl.waite@gmail.com>

Author: mawinter69

src/main/java/jenkins/plugins/http_request/HttpRequestExecution.java

@@ -76,6 +76,7 @@
 import jenkins.plugins.http_request.auth.CertificateAuthentication;
 import jenkins.plugins.http_request.auth.CredentialBasicAuthentication;
 import jenkins.plugins.http_request.auth.CredentialNtlmAuthentication;
+import jenkins.plugins.http_request.util.BackWardCompatibleRedirectStrategy;
 import jenkins.plugins.http_request.util.HttpClientUtil;
 import jenkins.plugins.http_request.util.HttpRequestFormDataPart;
 import jenkins.plugins.http_request.util.HttpRequestNameValuePair;
@@ -310,7 +311,7 @@ private ResponseContentSupplier authAndRequest()
         try {
             HttpClientBuilder clientBuilder = HttpClientBuilder.create();
 			clientBuilder.disableAutomaticRetries();
-			clientBuilder.disableRedirectHandling();
+			clientBuilder.setRedirectStrategy(new BackWardCompatibleRedirectStrategy());
 
             if (useSystemProperties) {
                 clientBuilder.useSystemProperties();

src/main/java/jenkins/plugins/http_request/util/BackWardCompatibleRedirectStrategy.java

@@ -0,0 +1,44 @@
+package jenkins.plugins.http_request.util;
+
+import org.apache.hc.client5.http.classic.methods.HttpGet;
+import org.apache.hc.client5.http.classic.methods.HttpHead;
+import org.apache.hc.client5.http.impl.DefaultRedirectStrategy;
+import org.apache.hc.core5.http.Header;
+import org.apache.hc.core5.http.HttpHeaders;
+import org.apache.hc.core5.http.HttpRequest;
+import org.apache.hc.core5.http.HttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.protocol.HttpContext;
+
+public class BackWardCompatibleRedirectStrategy extends DefaultRedirectStrategy {
+
+	private static final String[] REDIRECT_METHODS = new String[]{HttpGet.METHOD_NAME, HttpHead.METHOD_NAME};
+
+	@Override
+	public boolean isRedirected(final HttpRequest request, final HttpResponse response, final HttpContext context) {
+		if (!response.containsHeader(HttpHeaders.LOCATION)) {
+			return false;
+		}
+
+		final int statusCode = response.getCode();
+		final String method = request.getMethod();
+		final Header locationHeader = response.getFirstHeader("location");
+		return switch (statusCode) {
+			case HttpStatus.SC_MOVED_TEMPORARILY ->
+					isRedirectable(method) && locationHeader != null;
+			case HttpStatus.SC_MOVED_PERMANENTLY, HttpStatus.SC_TEMPORARY_REDIRECT,
+				 HttpStatus.SC_PERMANENT_REDIRECT -> isRedirectable(method);
+			case HttpStatus.SC_SEE_OTHER -> true;
+			default -> false;
+		};
+	}
+
+	protected boolean isRedirectable(final String method) {
+		for (final String m : REDIRECT_METHODS) {
+			if (m.equalsIgnoreCase(method)) {
+				return true;
+			}
+		}
+		return false;
+	}
+}

src/test/java/jenkins/plugins/http_request/HttpRequestTest.java

@@ -17,6 +17,7 @@
 import static jenkins.plugins.http_request.Registers.registerTimeout;
 import static jenkins.plugins.http_request.Registers.registerUnwrappedPutFileUpload;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.io.File;
@@ -31,16 +32,17 @@
 
 import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
 import org.apache.hc.client5.http.impl.classic.HttpResponseAdapter;
-import org.apache.hc.core5.http.HttpResponse;
 import org.apache.hc.core5.http.io.entity.StringEntity;
 import org.apache.hc.core5.http.message.BasicClassicHttpResponse;
-import org.apache.hc.core5.http.message.BasicHttpResponse;
 import org.eclipse.jetty.http.HttpCookie;
 import org.eclipse.jetty.http.HttpStatus;
 import org.eclipse.jetty.server.Request;
 import org.eclipse.jetty.server.Response;
 import org.eclipse.jetty.util.Callback;
 import org.eclipse.jetty.util.Fields;
+import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
+import org.jenkinsci.plugins.workflow.job.WorkflowJob;
+import org.jenkinsci.plugins.workflow.job.WorkflowRun;
 import org.junit.jupiter.api.Assumptions;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
@@ -1089,4 +1091,82 @@ private static File newFolder(File root, String... subDirs) throws IOException {
         return result;
     }
 
+	private static class RedirectStatus {
+		boolean called = false;
+		boolean redirected = false;
+	};
+
+	@Test
+	void noRedirectOnPOSTOn302() throws Exception {
+		final RedirectStatus redirectStatus = new RedirectStatus();
+
+		registerHandler("/redirected", HttpMode.GET, new SimpleHandler() {
+			@Override
+			boolean doHandle(Request request, Response response, Callback callback) {
+				redirectStatus.called = true;
+				return okAllIsWell(response, callback);
+			}
+		});
+		registerHandler("/redirectPOST", HttpMode.POST, new SimpleHandler() {
+			@Override
+			boolean doHandle(Request request, Response response, Callback callback) {
+				redirectStatus.redirected = true;
+				Response.sendRedirect(request, response, callback, 302, "/redirected", false);
+				return true;
+			}
+		});
+		String body = "send-body-workflow";
+		WorkflowJob proj = j.jenkins.createProject(WorkflowJob.class, "postBody");
+		proj.setDefinition(new CpsFlowDefinition(
+				"def response = httpRequest" +
+						" httpMode: 'POST'," +
+						" requestBody: '" + body + "'," +
+						" url: '" + baseURL() + "/redirectPOST'\n" +
+						"println('Response: ' + response.content)\n",
+				true));
+
+		WorkflowRun run = proj.scheduleBuild2(0).get();
+
+		// Check expectations
+		j.assertBuildStatusSuccess(run);
+		assertTrue(redirectStatus.redirected);
+		assertFalse(redirectStatus.called);
+	}
+
+	@Test
+	void redirectOnGETOn302() throws Exception {
+		final RedirectStatus redirectStatus = new RedirectStatus();
+
+		registerHandler("/redirected", HttpMode.GET, new SimpleHandler() {
+			@Override
+			boolean doHandle(Request request, Response response, Callback callback) {
+				redirectStatus.called = true;
+				return okAllIsWell(response, callback);
+			}
+		});
+		registerHandler("/redirectGET", HttpMode.GET, new SimpleHandler() {
+			@Override
+			boolean doHandle(Request request, Response response, Callback callback) {
+				redirectStatus.redirected = true;
+				Response.sendRedirect(request, response, callback, 302, "/redirected", false);
+				return true;
+			}
+		});
+		String body = "send-body-workflow";
+		WorkflowJob proj = j.jenkins.createProject(WorkflowJob.class, "postBody");
+		proj.setDefinition(new CpsFlowDefinition(
+				"def response = httpRequest" +
+						" httpMode: 'GET'," +
+						" requestBody: '" + body + "'," +
+						" url: '" + baseURL() + "/redirectGET'\n" +
+						"println('Response: ' + response.content)\n",
+				true));
+
+		WorkflowRun run = proj.scheduleBuild2(0).get();
+
+		// Check expectations
+		j.assertBuildStatusSuccess(run);
+		assertTrue(redirectStatus.redirected);
+		assertTrue(redirectStatus.called);
+	}
 }