Skip to content

Commit ac495d7

Browse files
committed
Polish ForwardedHeaderFilter and related code
Issue: SPR-16506
1 parent c9d08bf commit ac495d7

File tree

4 files changed

+58
-76
lines changed

4 files changed

+58
-76
lines changed

spring-web/src/main/java/org/springframework/web/filter/ForwardedHeaderFilter.java

+34-47
Original file line numberDiff line numberDiff line change
@@ -101,13 +101,13 @@ public void setRemoveOnly(boolean removeOnly) {
101101
}
102102

103103
/**
104-
* Use this property to enable relative redirects as explained in and also
105-
* using the same response wrapper as {@link RelativeRedirectFilter} does.
106-
* Or if both filters are used, only one will wrap the response.
104+
* Use this property to enable relative redirects as explained in
105+
* {@link RelativeRedirectFilter}, and also using the same response wrapper
106+
* as that filter does, or if both are configured, only one will wrap.
107107
* <p>By default, if this property is set to false, in which case calls to
108108
* {@link HttpServletResponse#sendRedirect(String)} are overridden in order
109-
* to turn relative into absolute URLs since (which Servlet containers are
110-
* also required to do) also taking forwarded headers into consideration.
109+
* to turn relative into absolute URLs, also taking into account forwarded
110+
* headers.
111111
* @param relativeRedirects whether to use relative redirects
112112
* @since 4.3.10
113113
*/
@@ -307,54 +307,41 @@ public ForwardedHeaderExtractingResponse(HttpServletResponse response, HttpServl
307307
this.request = request;
308308
}
309309

310-
@Override
311-
public void sendRedirect(String location) throws IOException {
312-
UriComponentsBuilder builder = UriComponentsBuilder.fromUriString(location);
313-
314-
// Absolute location
315-
if (builder.build().getScheme() != null) {
316-
super.sendRedirect(location);
317-
return;
318-
}
310+
@Override
311+
public void sendRedirect(String location) throws IOException {
319312

320-
// Network-path reference
321-
if (location.startsWith("//")) {
322-
String scheme = this.request.getScheme();
323-
super.sendRedirect(builder.scheme(scheme).toUriString());
324-
return;
325-
}
313+
UriComponentsBuilder builder = UriComponentsBuilder.fromUriString(location);
314+
UriComponents uriComponents = builder.build();
326315

327-
String fragment = null;
328-
int fragmentIndex = location.indexOf('#');
329-
if (fragmentIndex != -1) {
330-
if (location.length() > fragmentIndex) {
331-
fragment = location.substring(fragmentIndex + 1);
332-
}
333-
location = location.substring(0, fragmentIndex);
334-
}
316+
// Absolute location
317+
if (uriComponents.getScheme() != null) {
318+
super.sendRedirect(location);
319+
return;
320+
}
335321

336-
String query = null;
337-
int queryIndex = location.indexOf('?');
338-
if (queryIndex != -1) {
339-
if (location.length() > queryIndex) {
340-
query = location.substring(queryIndex + 1);
341-
}
342-
location = location.substring(0, queryIndex);
343-
}
322+
// Network-path reference
323+
if (location.startsWith("//")) {
324+
String scheme = this.request.getScheme();
325+
super.sendRedirect(builder.scheme(scheme).toUriString());
326+
return;
327+
}
344328

345-
// Relative to Servlet container root or to current request
346-
String path = (location.startsWith(FOLDER_SEPARATOR) ? location :
347-
StringUtils.applyRelativePath(this.request.getRequestURI(), location));
329+
String path = uriComponents.getPath();
330+
if (path != null) {
331+
// Relative to Servlet container root or to current request
332+
path = (path.startsWith(FOLDER_SEPARATOR) ? path :
333+
StringUtils.applyRelativePath(this.request.getRequestURI(), path));
334+
}
348335

349-
String result = UriComponentsBuilder
350-
.fromHttpRequest(new ServletServerHttpRequest(this.request))
351-
.replacePath(path)
352-
.replaceQuery(query)
353-
.fragment(fragment)
354-
.build().normalize().toUriString();
336+
String result = UriComponentsBuilder
337+
.fromHttpRequest(new ServletServerHttpRequest(this.request))
338+
.replacePath(path)
339+
.replaceQuery(uriComponents.getQuery())
340+
.fragment(uriComponents.getFragment())
341+
.build().normalize().toUriString();
355342

356-
super.sendRedirect(result);
357-
}
343+
super.sendRedirect(result);
344+
}
358345
}
359346

360347
}

spring-web/src/main/java/org/springframework/web/filter/RelativeRedirectFilter.java

+8-7
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2017 the original author or authors.
2+
* Copyright 2002-2018 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -27,13 +27,14 @@
2727

2828
/**
2929
* Overrides {@link HttpServletResponse#sendRedirect(String)} and handles it by
30-
* setting the HTTP status and "Location" headers. This keeps the Servlet
31-
* container from re-writing relative redirect URLs and instead follows the
32-
* recommendation in <a href="https://tools.ietf.org/html/rfc7231#section-7.1.2">
33-
* RFC 7231 Section 7.1.2</a>.
30+
* setting the HTTP status and "Location" headers, which keeps the Servlet
31+
* container from re-writing relative redirect URLs into absolute ones.
32+
* Servlet containers are required to do that but against the recommendation of
33+
* <a href="https://tools.ietf.org/html/rfc7231#section-7.1.2"> RFC 7231 Section 7.1.2</a>,
34+
* and furthermore not necessarily taking into account "X-Forwarded" headers.
3435
*
35-
* <p><strong>Note:</strong> While relative redirects are more efficient they
36-
* may not work with reverse proxies under some configurations.
36+
* <p><strong>Note:</strong> While relative redirects are recommended in the
37+
* RFC, under some configurations with reverse proxies they may not work.
3738
*
3839
* @author Rob Winch
3940
* @author Rossen Stoyanchev

spring-web/src/main/java/org/springframework/web/filter/RelativeRedirectResponseWrapper.java

+6-16
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,13 @@
1515
*/
1616
package org.springframework.web.filter;
1717

18-
import java.io.IOException;
19-
import javax.servlet.ServletResponse;
2018
import javax.servlet.http.HttpServletResponse;
2119
import javax.servlet.http.HttpServletResponseWrapper;
2220

2321
import org.springframework.http.HttpHeaders;
2422
import org.springframework.http.HttpStatus;
2523
import org.springframework.util.Assert;
24+
import org.springframework.web.util.WebUtils;
2625

2726
/**
2827
* A response wrapper used for the implementation of
@@ -44,7 +43,7 @@ private RelativeRedirectResponseWrapper(HttpServletResponse response, HttpStatus
4443

4544

4645
@Override
47-
public void sendRedirect(String location) throws IOException {
46+
public void sendRedirect(String location) {
4847
setStatus(this.redirectStatus.value());
4948
setHeader(HttpHeaders.LOCATION, location);
5049
}
@@ -53,20 +52,11 @@ public void sendRedirect(String location) throws IOException {
5352
public static HttpServletResponse wrapIfNecessary(HttpServletResponse response,
5453
HttpStatus redirectStatus) {
5554

56-
return (hasWrapper(response) ? response : new RelativeRedirectResponseWrapper(response, redirectStatus));
57-
}
55+
RelativeRedirectResponseWrapper wrapper =
56+
WebUtils.getNativeResponse(response, RelativeRedirectResponseWrapper.class);
5857

59-
private static boolean hasWrapper(ServletResponse response) {
60-
if (response instanceof RelativeRedirectResponseWrapper) {
61-
return true;
62-
}
63-
while (response instanceof HttpServletResponseWrapper) {
64-
response = ((HttpServletResponseWrapper) response).getResponse();
65-
if (response instanceof RelativeRedirectResponseWrapper) {
66-
return true;
67-
}
68-
}
69-
return false;
58+
return (wrapper != null ? response :
59+
new RelativeRedirectResponseWrapper(response, redirectStatus));
7060
}
7161

7262
}

spring-web/src/test/java/org/springframework/web/filter/ForwardedHeaderFilterTests.java

+10-6
Original file line numberDiff line numberDiff line change
@@ -433,18 +433,22 @@ public void sendRedirectWhenRequestOnlyAndNoXForwardedThenUsesRelativeRedirects(
433433
}
434434

435435
private String sendRedirect(final String location) throws ServletException, IOException {
436-
MockHttpServletResponse response = doWithFiltersAndGetResponse(this.filter, new OncePerRequestFilter() {
436+
Filter filter = new OncePerRequestFilter() {
437437
@Override
438-
protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain)
439-
throws ServletException, IOException {
440-
response.sendRedirect(location);
438+
protected void doFilterInternal(HttpServletRequest req, HttpServletResponse res,
439+
FilterChain chain) throws IOException {
440+
441+
res.sendRedirect(location);
441442
}
442-
});
443+
};
444+
MockHttpServletResponse response = doWithFiltersAndGetResponse(this.filter, filter);
443445
return response.getRedirectedUrl();
444446
}
445447

446448
@SuppressWarnings("serial")
447-
private MockHttpServletResponse doWithFiltersAndGetResponse(Filter... filters) throws ServletException, IOException {
449+
private MockHttpServletResponse doWithFiltersAndGetResponse(Filter... filters)
450+
throws ServletException, IOException {
451+
448452
MockHttpServletResponse response = new MockHttpServletResponse();
449453
FilterChain filterChain = new MockFilterChain(new HttpServlet() {}, filters);
450454
filterChain.doFilter(request, response);

0 commit comments

Comments
 (0)