New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Consider "wss" and "https" for secure flag in Forwarded header checks #27097
Comments
@jhyot Thanks for raising this issue. At least one of the Spring Framework code snippets you're pointing at is outdated. I'm not sure I completely understand the issue here. We do have a couple of places with "hardcoded" https values, but we seem to cover the scheme part dynamically, without restricting it to "<http|https>". Could you produce a code snippet or a sample application that shows the wrong behavior here? The comment you're pointing to seems to indicate that Spring's filter did set the scheme as "wss" and that Tomcat didn't support that back then. Now the Tomcat handler supports it properly so I'm wondering what needs to be fixed on our side.
I've written the following test and it's green; am I missing something here? void wssForwardedProtocol() {
MockHttpServletRequest request = new MockHttpServletRequest();
request.addHeader("X-Forwarded-Proto", "wss");
request.addHeader("X-Forwarded-Host", "84.198.58.199");
request.addHeader("X-Forwarded-Port", 443);
request.setScheme("http");
request.setServerName("example.com");
request.setServerPort(80);
request.setRequestURI("/rest/mobile/users/1");
HttpRequest httpRequest = new ServletServerHttpRequest(request);
UriComponents result = UriComponentsBuilder.fromHttpRequest(httpRequest).build();
assertThat(result.getScheme()).isEqualTo("wss");
assertThat(result.getHost()).isEqualTo("84.198.58.199");
assertThat(result.getPort()).isEqualTo(-1);
assertThat(result.getPath()).isEqualTo("/rest/mobile/users/1");
}
With that last section, it seems we're tackling a different issue; from supporting "wss" as a protocol value, to somehow translating it differently in the resulting request. Wouldn't that behavior break the expected behavior of the forwarded headers? Could you be more specific and explain what library fails and how? |
I think the main issue exists at https://github.com/spring-projects/spring-framework/blob/main/spring-web/src/main/java/org/springframework/web/filter/ForwardedHeaderFilter.java#L242
with a clearly unexpected result for the scheme value "wss". |
This fails for me for the port which is 443 because this check for https && 443 doesn't null it out. Also this test fails because the @Test
public void forwardedRequestWss() throws Exception {
this.request.setRequestURI("/path");
this.request.addHeader(X_FORWARDED_PROTO, "wss");
this.request.addHeader(X_FORWARDED_HOST, "84.198.58.199");
this.filter.doFilter(this.request, new MockHttpServletResponse(), this.filterChain);
HttpServletRequest actual = (HttpServletRequest) this.filterChain.getRequest();
assertThat(actual.isSecure()).isTrue();
} |
@rstoyanchev Thank you, that is what I mean. You can also add
to your test, since that currently fails as well (as it is coupled to the spring-framework/spring-web/src/main/java/org/springframework/web/filter/ForwardedHeaderFilter.java Line 244 in 171b283
Yes I guess that can be seen as a separate issue. Should I open a new ticket to discuss why possibly translating "wss" to "https" might make sense? |
These should be fixed now. As for translating "wss" to "https", that's not straight forward and as Brian said, probably not something we would want to change. Our CORS checks do take into account "wss" though so that should work. |
Spring's
ForwardedHeaderFilter
handles the header valueX-Forwarded-Proto: wss
incorrectly. The class has hardcoded checks for "http" and "https", which is in line with the widely held assumption that these are the only valid values (see e.g. MDN and other sources when searching for the header name on the web).But
X-Forwarded-Proto
is not standardized, and at least one popular proxy, Traefik, has decided to use "ws" and "wss" in the header, see traefik/traefik#6388. We have run into problems with Spring because of that. Also Tomcat has added "ws"/"wss" as supported values: apache/tomcat#311I suggest that "wss" should be a supported value and handled consistently like "https".
ForwardedHeaderFilter
and its dependencyUriComponentsBuilder.fromHttpRequest(request)
have hardcoded checks for "http" and "https" in multiple places ([1], [2]). Depending on which otherX-Forwarded-*
headers are present, it maps a request on port 443 andX-Forwarded-Proto: wss
to the followingServletRequest
properties:scheme=wss, port=80, secure=false
orscheme=wss, port=443, secure=false
. This seems clearly incorrect.Based on my observations I would propose that
X-Forwarded-Proto: wss
gets mapped into theServletRequest.scheme
value "https" (and "ws" -> "http") because that seems the most compatible way. Leaving "wss" in theServletRequest.scheme
seems to lead to problems. A same-origin check that compares the value of theOrigin
header (where "https" would be the protocol) with the servlet request url would fail. I also know of at least one library that fails completely when it encountersscheme=wss
in theServletRequest
object.The text was updated successfully, but these errors were encountered: