Skip to content

Commit 4da43de

Browse files
rstoyanchevrstoyanchev
committed
Remove individual detection of forwarded headers
This commit removes all places where forwarded headers are checked implicitly, on an ad-hoc basis. ForwardedHeaderFilter is expected to be used instead providing centralized control over using or discarding such headers. Issue: SPR-16668
1 parent 82a8e42 commit 4da43de

File tree

14 files changed

+251
-299
lines changed

14 files changed

+251
-299
lines changed

Diff for: ‎spring-test/src/main/java/org/springframework/mock/web/reactive/function/server/MockServerRequest.java

+1-20
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@
3838
import org.springframework.http.HttpHeaders;
3939
import org.springframework.http.HttpMethod;
4040
import org.springframework.http.HttpRange;
41-
import org.springframework.http.HttpRequest;
4241
import org.springframework.http.MediaType;
4342
import org.springframework.http.codec.HttpMessageReader;
4443
import org.springframework.http.codec.multipart.Part;
@@ -141,7 +140,7 @@ public URI uri() {
141140

142141
@Override
143142
public UriBuilder uriBuilder() {
144-
return UriComponentsBuilder.fromHttpRequest(new ServerRequestAdapter());
143+
return UriComponentsBuilder.fromUri(this.uri);
145144
}
146145

147146
@Override
@@ -571,22 +570,4 @@ private OptionalLong toOptionalLong(long value) {
571570

572571
}
573572

574-
private final class ServerRequestAdapter implements HttpRequest {
575-
576-
@Override
577-
public String getMethodValue() {
578-
return methodName();
579-
}
580-
581-
@Override
582-
public URI getURI() {
583-
return MockServerRequest.this.uri;
584-
}
585-
586-
@Override
587-
public HttpHeaders getHeaders() {
588-
return MockServerRequest.this.headers.headers;
589-
}
590-
}
591-
592573
}

Diff for: ‎spring-web/src/main/java/org/springframework/web/cors/reactive/CorsUtils.java

+13-13
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616

1717
package org.springframework.web.cors.reactive;
1818

19+
import java.net.URI;
20+
1921
import org.springframework.http.HttpHeaders;
2022
import org.springframework.http.HttpMethod;
2123
import org.springframework.http.server.reactive.ServerHttpRequest;
@@ -49,28 +51,26 @@ public static boolean isPreFlightRequest(ServerHttpRequest request) {
4951
}
5052

5153
/**
52-
* Check if the request is a same-origin one, based on {@code Origin}, {@code Host},
53-
* {@code Forwarded}, {@code X-Forwarded-Proto}, {@code X-Forwarded-Host} and
54-
* @code X-Forwarded-Port} headers.
54+
* Check if the request is a same-origin one, based on {@code Origin}, and
55+
* {@code Host} headers.
56+
*
57+
* <p><strong>Note:</strong> as of 5.1 this method ignores
58+
* {@code "Forwarded"} and {@code "X-Forwarded-*"} headers that specify the
59+
* client-originated address. Consider using the {@code ForwardedHeaderFilter}
60+
* to extract and use, or to discard such headers.
61+
*
5562
* @return {@code true} if the request is a same-origin one, {@code false} in case
5663
* of a cross-origin request
57-
* <p><strong>Note:</strong> this method uses values from "Forwarded"
58-
* (<a href="http://tools.ietf.org/html/rfc7239">RFC 7239</a>),
59-
* "X-Forwarded-Host", "X-Forwarded-Port", and "X-Forwarded-Proto" headers,
60-
* if present, in order to reflect the client-originated address.
61-
* Consider using the {@code ForwardedHeaderFilter} in order to choose from a
62-
* central place whether to extract and use, or to discard such headers.
63-
* See the Spring Framework reference for more on this filter.
6464
*/
6565
public static boolean isSameOrigin(ServerHttpRequest request) {
6666
String origin = request.getHeaders().getOrigin();
6767
if (origin == null) {
6868
return true;
6969
}
7070

71-
UriComponents actualUrl = UriComponentsBuilder.fromHttpRequest(request).build();
72-
String actualHost = actualUrl.getHost();
73-
int actualPort = getPort(actualUrl.getScheme(), actualUrl.getPort());
71+
URI uri = request.getURI();
72+
String actualHost = uri.getHost();
73+
int actualPort = getPort(uri.getScheme(), uri.getPort());
7474
Assert.notNull(actualHost, "Actual request host must not be null");
7575
Assert.isTrue(actualPort != -1, "Actual request port must not be undefined");
7676

Diff for: ‎spring-web/src/main/java/org/springframework/web/util/WebUtils.java

+17-48
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,10 @@
1818

1919
import java.io.File;
2020
import java.io.FileNotFoundException;
21+
import java.net.URI;
2122
import java.util.Collection;
2223
import java.util.Enumeration;
23-
import java.util.LinkedHashSet;
2424
import java.util.Map;
25-
import java.util.Set;
2625
import java.util.StringTokenizer;
2726
import java.util.TreeMap;
2827
import javax.servlet.ServletContext;
@@ -138,16 +137,6 @@ public abstract class WebUtils {
138137
/** Key for the mutex session attribute */
139138
public static final String SESSION_MUTEX_ATTRIBUTE = WebUtils.class.getName() + ".MUTEX";
140139

141-
private static final Set<String> FORWARDED_HEADER_NAMES = new LinkedHashSet<>(5);
142-
143-
static {
144-
FORWARDED_HEADER_NAMES.add("Forwarded");
145-
FORWARDED_HEADER_NAMES.add("X-Forwarded-Host");
146-
FORWARDED_HEADER_NAMES.add("X-Forwarded-Port");
147-
FORWARDED_HEADER_NAMES.add("X-Forwarded-Proto");
148-
FORWARDED_HEADER_NAMES.add("X-Forwarded-Prefix");
149-
}
150-
151140

152141
/**
153142
* Set a system property to the web application root directory.
@@ -677,13 +666,12 @@ public static MultiValueMap<String, String> parseMatrixVariables(String matrixVa
677666
* Check the given request origin against a list of allowed origins.
678667
* A list containing "*" means that all origins are allowed.
679668
* An empty list means only same origin is allowed.
680-
* <p><strong>Note:</strong> this method may use values from "Forwarded"
681-
* (<a href="http://tools.ietf.org/html/rfc7239">RFC 7239</a>),
682-
* "X-Forwarded-Host", "X-Forwarded-Port", and "X-Forwarded-Proto" headers,
683-
* if present, in order to reflect the client-originated address.
684-
* Consider using the {@code ForwardedHeaderFilter} in order to choose from a
685-
* central place whether to extract and use, or to discard such headers.
686-
* See the Spring Framework reference for more on this filter.
669+
*
670+
* <p><strong>Note:</strong> as of 5.1 this method ignores
671+
* {@code "Forwarded"} and {@code "X-Forwarded-*"} headers that specify the
672+
* client-originated address. Consider using the {@code ForwardedHeaderFilter}
673+
* to extract and use, or to discard such headers.
674+
*
687675
* @return {@code true} if the request origin is valid, {@code false} otherwise
688676
* @since 4.1.5
689677
* @see <a href="https://tools.ietf.org/html/rfc6454">RFC 6454: The Web Origin Concept</a>
@@ -708,13 +696,12 @@ else if (CollectionUtils.isEmpty(allowedOrigins)) {
708696
* Check if the request is a same-origin one, based on {@code Origin}, {@code Host},
709697
* {@code Forwarded}, {@code X-Forwarded-Proto}, {@code X-Forwarded-Host} and
710698
* @code X-Forwarded-Port} headers.
711-
* <p><strong>Note:</strong> this method uses values from "Forwarded"
712-
* (<a href="http://tools.ietf.org/html/rfc7239">RFC 7239</a>),
713-
* "X-Forwarded-Host", "X-Forwarded-Port", and "X-Forwarded-Proto" headers,
714-
* if present, in order to reflect the client-originated address.
715-
* Consider using the {@code ForwardedHeaderFilter} in order to choose from a
716-
* central place whether to extract and use, or to discard such headers.
717-
* See the Spring Framework reference for more on this filter.
699+
*
700+
* <p><strong>Note:</strong> as of 5.1 this method ignores
701+
* {@code "Forwarded"} and {@code "X-Forwarded-*"} headers that specify the
702+
* client-originated address. Consider using the {@code ForwardedHeaderFilter}
703+
* to extract and use, or to discard such headers.
704+
718705
* @return {@code true} if the request is a same-origin one, {@code false} in case
719706
* of cross-origin request
720707
* @since 4.2
@@ -735,37 +722,19 @@ public static boolean isSameOrigin(HttpRequest request) {
735722
scheme = servletRequest.getScheme();
736723
host = servletRequest.getServerName();
737724
port = servletRequest.getServerPort();
738-
if (containsForwardedHeaders(servletRequest)) {
739-
UriComponents actualUrl = new UriComponentsBuilder()
740-
.scheme(scheme).host(host).port(port)
741-
.adaptFromForwardedHeaders(headers)
742-
.build();
743-
scheme = actualUrl.getScheme();
744-
host = actualUrl.getHost();
745-
port = actualUrl.getPort();
746-
}
747725
}
748726
else {
749-
UriComponents actualUrl = UriComponentsBuilder.fromHttpRequest(request).build();
750-
scheme = actualUrl.getScheme();
751-
host = actualUrl.getHost();
752-
port = actualUrl.getPort();
727+
URI uri = request.getURI();
728+
scheme = uri.getScheme();
729+
host = uri.getHost();
730+
port = uri.getPort();
753731
}
754732

755733
UriComponents originUrl = UriComponentsBuilder.fromOriginHeader(origin).build();
756734
return (ObjectUtils.nullSafeEquals(host, originUrl.getHost()) &&
757735
getPort(scheme, port) == getPort(originUrl.getScheme(), originUrl.getPort()));
758736
}
759737

760-
private static boolean containsForwardedHeaders(HttpServletRequest request) {
761-
for (String headerName : FORWARDED_HEADER_NAMES) {
762-
if (request.getHeader(headerName) != null) {
763-
return true;
764-
}
765-
}
766-
return false;
767-
}
768-
769738
private static int getPort(@Nullable String scheme, int port) {
770739
if (port == -1) {
771740
if ("http".equals(scheme) || "ws".equals(scheme)) {

Diff for: ‎spring-web/src/test/java/org/springframework/web/cors/reactive/CorsUtilsTests.java

+53-27
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2015 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.
@@ -16,15 +16,19 @@
1616

1717
package org.springframework.web.cors.reactive;
1818

19+
import java.util.concurrent.atomic.AtomicReference;
20+
1921
import org.junit.Test;
22+
import reactor.core.publisher.Mono;
2023

2124
import org.springframework.http.HttpHeaders;
25+
import org.springframework.http.server.reactive.ServerHttpRequest;
2226
import org.springframework.mock.http.server.reactive.test.MockServerHttpRequest;
27+
import org.springframework.mock.web.test.server.MockServerWebExchange;
28+
import org.springframework.web.filter.reactive.ForwardedHeaderFilter;
2329

24-
import static org.junit.Assert.assertFalse;
25-
import static org.junit.Assert.assertTrue;
26-
import static org.springframework.mock.http.server.reactive.test.MockServerHttpRequest.get;
27-
import static org.springframework.mock.http.server.reactive.test.MockServerHttpRequest.options;
30+
import static org.junit.Assert.*;
31+
import static org.springframework.mock.http.server.reactive.test.MockServerHttpRequest.*;
2832

2933
/**
3034
* Test case for reactive {@link CorsUtils}.
@@ -35,19 +39,19 @@ public class CorsUtilsTests {
3539

3640
@Test
3741
public void isCorsRequest() {
38-
MockServerHttpRequest request = get("/").header(HttpHeaders.ORIGIN, "http://domain.com").build();
42+
ServerHttpRequest request = get("/").header(HttpHeaders.ORIGIN, "http://domain.com").build();
3943
assertTrue(CorsUtils.isCorsRequest(request));
4044
}
4145

4246
@Test
4347
public void isNotCorsRequest() {
44-
MockServerHttpRequest request = get("/").build();
48+
ServerHttpRequest request = get("/").build();
4549
assertFalse(CorsUtils.isCorsRequest(request));
4650
}
4751

4852
@Test
4953
public void isPreFlightRequest() {
50-
MockServerHttpRequest request = options("/")
54+
ServerHttpRequest request = options("/")
5155
.header(HttpHeaders.ORIGIN, "http://domain.com")
5256
.header(HttpHeaders.ACCESS_CONTROL_REQUEST_METHOD, "GET")
5357
.build();
@@ -56,7 +60,7 @@ public void isPreFlightRequest() {
5660

5761
@Test
5862
public void isNotPreFlightRequest() {
59-
MockServerHttpRequest request = get("/").build();
63+
ServerHttpRequest request = get("/").build();
6064
assertFalse(CorsUtils.isPreFlightRequest(request));
6165

6266
request = options("/").header(HttpHeaders.ORIGIN, "http://domain.com").build();
@@ -68,31 +72,35 @@ public void isNotPreFlightRequest() {
6872

6973
@Test // SPR-16262
7074
public void isSameOriginWithXForwardedHeaders() {
71-
assertTrue(checkSameOriginWithXForwardedHeaders("mydomain1.com", -1, "https", null, -1, "https://mydomain1.com"));
72-
assertTrue(checkSameOriginWithXForwardedHeaders("mydomain1.com", 123, "https", null, -1, "https://mydomain1.com"));
73-
assertTrue(checkSameOriginWithXForwardedHeaders("mydomain1.com", -1, "https", "mydomain2.com", -1, "https://mydomain2.com"));
74-
assertTrue(checkSameOriginWithXForwardedHeaders("mydomain1.com", 123, "https", "mydomain2.com", -1, "https://mydomain2.com"));
75-
assertTrue(checkSameOriginWithXForwardedHeaders("mydomain1.com", -1, "https", "mydomain2.com", 456, "https://mydomain2.com:456"));
76-
assertTrue(checkSameOriginWithXForwardedHeaders("mydomain1.com", 123, "https", "mydomain2.com", 456, "https://mydomain2.com:456"));
75+
String server = "mydomain1.com";
76+
testWithXForwardedHeaders(server, -1, "https", null, -1, "https://mydomain1.com");
77+
testWithXForwardedHeaders(server, 123, "https", null, -1, "https://mydomain1.com");
78+
testWithXForwardedHeaders(server, -1, "https", "mydomain2.com", -1, "https://mydomain2.com");
79+
testWithXForwardedHeaders(server, 123, "https", "mydomain2.com", -1, "https://mydomain2.com");
80+
testWithXForwardedHeaders(server, -1, "https", "mydomain2.com", 456, "https://mydomain2.com:456");
81+
testWithXForwardedHeaders(server, 123, "https", "mydomain2.com", 456, "https://mydomain2.com:456");
7782
}
7883

7984
@Test // SPR-16262
8085
public void isSameOriginWithForwardedHeader() {
81-
assertTrue(checkSameOriginWithForwardedHeader("mydomain1.com", -1, "proto=https", "https://mydomain1.com"));
82-
assertTrue(checkSameOriginWithForwardedHeader("mydomain1.com", 123, "proto=https", "https://mydomain1.com"));
83-
assertTrue(checkSameOriginWithForwardedHeader("mydomain1.com", -1, "proto=https; host=mydomain2.com", "https://mydomain2.com"));
84-
assertTrue(checkSameOriginWithForwardedHeader("mydomain1.com", 123, "proto=https; host=mydomain2.com", "https://mydomain2.com"));
85-
assertTrue(checkSameOriginWithForwardedHeader("mydomain1.com", -1, "proto=https; host=mydomain2.com:456", "https://mydomain2.com:456"));
86-
assertTrue(checkSameOriginWithForwardedHeader("mydomain1.com", 123, "proto=https; host=mydomain2.com:456", "https://mydomain2.com:456"));
86+
String server = "mydomain1.com";
87+
testWithForwardedHeader(server, -1, "proto=https", "https://mydomain1.com");
88+
testWithForwardedHeader(server, 123, "proto=https", "https://mydomain1.com");
89+
testWithForwardedHeader(server, -1, "proto=https; host=mydomain2.com", "https://mydomain2.com");
90+
testWithForwardedHeader(server, 123, "proto=https; host=mydomain2.com", "https://mydomain2.com");
91+
testWithForwardedHeader(server, -1, "proto=https; host=mydomain2.com:456", "https://mydomain2.com:456");
92+
testWithForwardedHeader(server, 123, "proto=https; host=mydomain2.com:456", "https://mydomain2.com:456");
8793
}
8894

89-
private boolean checkSameOriginWithXForwardedHeaders(String serverName, int port, String forwardedProto, String forwardedHost, int forwardedPort, String originHeader) {
95+
private void testWithXForwardedHeaders(String serverName, int port,
96+
String forwardedProto, String forwardedHost, int forwardedPort, String originHeader) {
97+
9098
String url = "http://" + serverName;
9199
if (port != -1) {
92100
url = url + ":" + port;
93101
}
94-
MockServerHttpRequest.BaseBuilder<?> builder = get(url)
95-
.header(HttpHeaders.ORIGIN, originHeader);
102+
103+
MockServerHttpRequest.BaseBuilder<?> builder = get(url).header(HttpHeaders.ORIGIN, originHeader);
96104
if (forwardedProto != null) {
97105
builder.header("X-Forwarded-Proto", forwardedProto);
98106
}
@@ -102,18 +110,36 @@ private boolean checkSameOriginWithXForwardedHeaders(String serverName, int port
102110
if (forwardedPort != -1) {
103111
builder.header("X-Forwarded-Port", String.valueOf(forwardedPort));
104112
}
105-
return CorsUtils.isSameOrigin(builder.build());
113+
114+
ServerHttpRequest request = adaptFromForwardedHeaders(builder);
115+
assertTrue(CorsUtils.isSameOrigin(request));
106116
}
107117

108-
private boolean checkSameOriginWithForwardedHeader(String serverName, int port, String forwardedHeader, String originHeader) {
118+
private void testWithForwardedHeader(String serverName, int port,
119+
String forwardedHeader, String originHeader) {
120+
109121
String url = "http://" + serverName;
110122
if (port != -1) {
111123
url = url + ":" + port;
112124
}
125+
113126
MockServerHttpRequest.BaseBuilder<?> builder = get(url)
114127
.header("Forwarded", forwardedHeader)
115128
.header(HttpHeaders.ORIGIN, originHeader);
116-
return CorsUtils.isSameOrigin(builder.build());
129+
130+
ServerHttpRequest request = adaptFromForwardedHeaders(builder);
131+
assertTrue(CorsUtils.isSameOrigin(request));
132+
}
133+
134+
// SPR-16668
135+
private ServerHttpRequest adaptFromForwardedHeaders(MockServerHttpRequest.BaseBuilder<?> builder) {
136+
AtomicReference<ServerHttpRequest> requestRef = new AtomicReference<>();
137+
MockServerWebExchange exchange = MockServerWebExchange.from(builder);
138+
new ForwardedHeaderFilter().filter(exchange, exchange2 -> {
139+
requestRef.set(exchange2.getRequest());
140+
return Mono.empty();
141+
}).block();
142+
return requestRef.get();
117143
}
118144

119145
}

0 commit comments

Comments
 (0)