From da2b34e5284f3cd8c2499f1544bd511a7dc55414 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Mon, 23 Aug 2021 20:02:12 +0200 Subject: [PATCH] Preserve request method on force base url (#1778) The redirect which is used to force base url uses now 307 instead of 302 in order to preserve the request method. Closes #1771 --- gradle/changelog/force_baseurl.yaml | 2 + .../java/sonia/scm/filter/BaseUrlFilter.java | 107 +++---------- .../sonia/scm/filter/BaseUrlFilterTest.java | 147 +++++++++++++++--- 3 files changed, 146 insertions(+), 110 deletions(-) create mode 100644 gradle/changelog/force_baseurl.yaml diff --git a/gradle/changelog/force_baseurl.yaml b/gradle/changelog/force_baseurl.yaml new file mode 100644 index 0000000000..5b9297372b --- /dev/null +++ b/gradle/changelog/force_baseurl.yaml @@ -0,0 +1,2 @@ +- type: fixed + description: Preserve request method on force base url ([#1771](https://github.com/scm-manager/scm-manager/issues/1771) and [#1778](https://github.com/scm-manager/scm-manager/pull/1778)) diff --git a/scm-webapp/src/main/java/sonia/scm/filter/BaseUrlFilter.java b/scm-webapp/src/main/java/sonia/scm/filter/BaseUrlFilter.java index 80dce05dc4..43d24b97fb 100644 --- a/scm-webapp/src/main/java/sonia/scm/filter/BaseUrlFilter.java +++ b/scm-webapp/src/main/java/sonia/scm/filter/BaseUrlFilter.java @@ -21,10 +21,8 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - -package sonia.scm.filter; -//~--- non-JDK imports -------------------------------------------------------- +package sonia.scm.filter; import com.google.common.annotations.VisibleForTesting; import com.google.inject.Inject; @@ -34,8 +32,6 @@ import sonia.scm.util.HttpUtil; import sonia.scm.util.Util; import sonia.scm.web.filter.HttpFilter; -//~--- JDK imports ------------------------------------------------------------ - import java.io.IOException; import javax.servlet.FilterChain; @@ -50,110 +46,49 @@ import sonia.scm.Priority; */ @Priority(Filters.PRIORITY_BASEURL) @WebElement(Filters.PATTERN_ALL) -public class BaseUrlFilter extends HttpFilter -{ +public class BaseUrlFilter extends HttpFilter { + + /** scm configuration */ + private final ScmConfiguration configuration; - /** - * Constructs ... - * - * - * @param configuration - */ @Inject - public BaseUrlFilter(ScmConfiguration configuration) - { + public BaseUrlFilter(ScmConfiguration configuration) { this.configuration = configuration; } - //~--- methods -------------------------------------------------------------- - - /** - * Method description - * - * - * @param requestUrl - * @param baseUrl - * - * @return - */ @VisibleForTesting - boolean startsWith(String requestUrl, String baseUrl) - { - return HttpUtil.normalizeUrl(requestUrl).startsWith( - HttpUtil.normalizeUrl(baseUrl)); + boolean startsWith(String requestUrl, String baseUrl) { + return HttpUtil.normalizeUrl(requestUrl).startsWith(HttpUtil.normalizeUrl(baseUrl)); } - /** - * Method description - * - * - * @param request - * @param response - * @param chain - * - * @throws IOException - * @throws ServletException - */ @Override - protected void doFilter(HttpServletRequest request, - HttpServletResponse response, FilterChain chain) - throws IOException, ServletException - { - if (Util.isEmpty(configuration.getBaseUrl())) - { + protected void doFilter( + HttpServletRequest request, HttpServletResponse response, FilterChain chain + ) throws IOException, ServletException { + if (Util.isEmpty(configuration.getBaseUrl())) { configuration.setBaseUrl(createDefaultBaseUrl(request)); } - if (!configuration.isForceBaseUrl() || isBaseUrl(request)) - { + if (!configuration.isForceBaseUrl() || isBaseUrl(request)) { chain.doFilter(request, response); - } - else - { - String url = HttpUtil.getCompleteUrl(configuration, - HttpUtil.getStrippedURI(request)); - - response.sendRedirect(url); + } else { + String url = HttpUtil.getCompleteUrl(configuration, HttpUtil.getStrippedURI(request)); + response.setStatus(HttpServletResponse.SC_TEMPORARY_REDIRECT); + response.setHeader(HttpUtil.HEADER_LOCATION, url); } } - /** - * Method description - * - * - * @param request - * - * @return - */ - private String createDefaultBaseUrl(HttpServletRequest request) - { + private String createDefaultBaseUrl(HttpServletRequest request) { StringBuilder sb = new StringBuilder(request.getScheme()); sb.append("://").append(request.getServerName()).append(":"); - sb.append(String.valueOf(request.getServerPort())); + sb.append(request.getServerPort()); sb.append(request.getContextPath()); return sb.toString(); } - //~--- get methods ---------------------------------------------------------- - - /** - * Method description - * - * - * @param request - * - * @return - */ - private boolean isBaseUrl(HttpServletRequest request) - { - return startsWith(request.getRequestURL().toString(), - configuration.getBaseUrl()); + private boolean isBaseUrl(HttpServletRequest request) { + return startsWith(request.getRequestURL().toString(), configuration.getBaseUrl()); } - - //~--- fields --------------------------------------------------------------- - - /** scm configuration */ - private final ScmConfiguration configuration; } diff --git a/scm-webapp/src/test/java/sonia/scm/filter/BaseUrlFilterTest.java b/scm-webapp/src/test/java/sonia/scm/filter/BaseUrlFilterTest.java index 7fdc8f4dcf..5ba3348d53 100644 --- a/scm-webapp/src/test/java/sonia/scm/filter/BaseUrlFilterTest.java +++ b/scm-webapp/src/test/java/sonia/scm/filter/BaseUrlFilterTest.java @@ -21,40 +21,139 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.filter; -//~--- non-JDK imports -------------------------------------------------------- - -import org.junit.Test; - +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; import sonia.scm.config.ScmConfiguration; -import static org.junit.Assert.*; +import javax.servlet.FilterChain; +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import java.io.IOException; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + /** - * * @author Sebastian Sdorra */ -public class BaseUrlFilterTest -{ +@ExtendWith(MockitoExtension.class) +class BaseUrlFilterTest { + + + @Mock + private HttpServletResponse response; + + @Mock + private FilterChain chain; + + private ScmConfiguration configuration; + + private BaseUrlFilter filter; + + @BeforeEach + void setUpFilter() { + configuration = new ScmConfiguration(); + filter = new BaseUrlFilter(configuration); + } - /** - * Method description - * - */ @Test - public void startsWithTest() - { - BaseUrlFilter filter = new BaseUrlFilter(new ScmConfiguration()); + void shouldSetBaseUrl() throws ServletException, IOException { + HttpServletRequest request = mockRequest("https", "hitchhiker.com", 443, "/scm"); + + filter.doFilter(request, response, chain); + + verify(chain).doFilter(request, response); + assertThat(configuration.getBaseUrl()).isEqualTo("https://hitchhiker.com:443/scm"); + } + + @Test + void shouldSendRedirect() throws ServletException, IOException { + configuration.setBaseUrl("https://hitchhiker.com:443/scm"); + configuration.setForceBaseUrl(true); + HttpServletRequest request = mockRequest("http://192.168.1.42:8081", "/scm", "/api/v2"); + + filter.doFilter(request, response, chain); + + verifyNoInteractions(chain); + verify(response).setStatus(307); + verify(response).setHeader("Location", "https://hitchhiker.com:443/scm/api/v2"); + } + + @Test + void shouldNotSendRedirectIfDisabled() throws ServletException, IOException { + configuration.setBaseUrl("https://hitchhiker.com:443/scm"); + configuration.setForceBaseUrl(false); + HttpServletRequest request = mockRequest("http://192.168.1.42:8081", "/scm", "/api/v2"); + + filter.doFilter(request, response, chain); + + verify(chain).doFilter(request, response); + } + + @Test + void shouldNotSendRedirect() throws ServletException, IOException { + configuration.setBaseUrl("https://hitchhiker.com:443/scm"); + configuration.setForceBaseUrl(true); + HttpServletRequest request = mockRequest("https://hitchhiker.com", "/scm", "/api/v2/users"); + + filter.doFilter(request, response, chain); + + verify(chain).doFilter(request, response); + } + + private HttpServletRequest mockRequest(String baseUrl, String contextPath, String path) { + HttpServletRequest request = mock(HttpServletRequest.class); + lenient().when(request.getRequestURL()).thenReturn(new StringBuffer(baseUrl).append(contextPath).append(path)); + lenient().when(request.getContextPath()).thenReturn(contextPath); + lenient().when(request.getRequestURI()).thenReturn(contextPath + path); + return request; + } + + private HttpServletRequest mockRequest(String scheme, String serverName, int port, String contextPath) { + HttpServletRequest request = mock(HttpServletRequest.class); + when(request.getScheme()).thenReturn(scheme); + when(request.getServerName()).thenReturn(serverName); + when(request.getServerPort()).thenReturn(port); + when(request.getContextPath()).thenReturn(contextPath); + return request; + } + + @Nested + class StartsWithTests { + + @Test + void shouldReturnTrue() { + assertThat( + filter.startsWith("http://www.scm-manager.org/scm", "http://www.scm-manager.org/scm") + ).isTrue(); + assertThat( + filter.startsWith("http://www.scm-manager.org:80/scm", "http://www.scm-manager.org/scm") + ).isTrue(); + assertThat( + filter.startsWith("https://www.scm-manager.org/scm", "https://www.scm-manager.org:443/scm") + ).isTrue(); + } + + @Test + void shouldReturnFalse() { + assertThat( + filter.startsWith("http://www.scm-manager.org/acb", "http://www.scm-manager.org/scm") + ).isFalse(); + } - assertTrue(filter.startsWith("http://www.scm-manager.org/scm", - "http://www.scm-manager.org/scm")); - assertTrue(filter.startsWith("http://www.scm-manager.org:80/scm", - "http://www.scm-manager.org/scm")); - assertTrue(filter.startsWith("https://www.scm-manager.org/scm", - "https://www.scm-manager.org:443/scm")); - assertFalse(filter.startsWith("http://www.scm-manager.org/acb", - "http://www.scm-manager.org/scm")); } }