XForwardedFilter : request.secure and request.scheme are not forced to "false" and "http" if X-Forwarded-Proto=http

GoogleCodeExporter commented 9 years ago

What steps will reproduce the problem?
1. User access web server via http, and web server proxy request to app
server via https

What is the expected output? What do you see instead?
isSecure() returns True, not False that I wanted

What version of the product are you using? On what operating system?
1.0.1, on Linux

Please provide any additional information below.
From the source code, it only handles case when x-forwarded-proto is https,
while doesn't do anything if x-forwarded-proto is http, so we have this
problem.

Original issue reported on code.google.com by caisd1...@gmail.com on 28 Jan 2010 at 10:35

GoogleCodeExporter commented 9 years ago

Comment by caisd1998,  Yesterday (27 hours ago)

Actually this filter can't handle following situation: User access web server 
via
http, and web server proxy request to app server via https, then isSecure() 
returns
True, not False that I wanted.

Original comment by cyrille....@gmail.com on 28 Jan 2010 at 10:47

Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

Comment by cyrille.leclerc,  Yesterday (19 hours ago)

I am surprised by your problem. I added a test case (1) that seem to reproduce 
what
you said and it works as expected.

Can you confirm that you declared the init-param "protocolHeader" with the value
"x-forwarded-proto" in web.xml ? Can you enable the debug level logs ont the
"fr.xebia.servlet.filter.XForwardedFilter" logger ?

(1) testIncomingRequestIsSecuredButProtocolHeaderSaysItIsNot() in
http://xebia-france.googlecode.com/svn/web/xebia-servlet-extras/trunk/src/test/j
ava/fr/xebia/servlet/filter/XForwardedFilterTest.java

Original comment by cyrille....@gmail.com on 28 Jan 2010 at 10:48

Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

Comment by caisd1998,  Today (33 minutes ago)

This is the output of my test: Incoming request /test/test.jsp with
originalRemoteAddr 'x.x.x.x', originalRemoteHost='x.x.x.x', 
originalSecure='true',
originalScheme='https', originalX-Forwarded-For?='y.y.y.y,
originalx-forwarded-proto?='http' will be seen as newRemoteAddr='y.y.y.y',
newRemoteHost='y.y.y.y', newScheme='https', newSecure='true',
newX-Forwarded-For?='null, newX-Forwarded-By?='null'

The logic to replace/update secure and scheme is in following: if 
(protocolHeader !=
null) {

    String protocolHeaderValue = request.getHeader(protocolHeader); if
(protocolHeaderValue != null &&
protocolHeaderSslValue.equalsIgnoreCase(protocolHeaderValue)) {

        xRequest.setSecure(true); xRequest.setScheme("https");
xRequest.setServerPort(httpsServerPort); 

    } 

} So it only handles case when x-forwarded-proto is https. The class is 
downloaded
from link above:
http://xebia-france.googlecode.com/svn/web/xebia-servlet-extras/tags/xebia-servl
et-extras-1.0.1/src/main/java/fr/xebia/servlet/filter/XForwardedFilter.java

Original comment by cyrille....@gmail.com on 28 Jan 2010 at 10:48

Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

Comment by caisd1998,  Today (28 minutes ago)

don't know why the format become mass, hope you can read and understand it.

Original comment by cyrille....@gmail.com on 28 Jan 2010 at 10:49

Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

Comment by cyrille.leclerc,  Today (20 minutes ago)

Hello, can you create a defect to have a better tracing of the issue please ?

Original comment by cyrille....@gmail.com on 28 Jan 2010 at 10:49

Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

Comment by caisd1998,  Today (20 minutes ago)

To fix it, after "String protocolHeaderValue = 
request.getHeader(protocolHeader);",
we can add this: if (protocolHeaderValue != null &&
protocolHeaderHttpValue.equalsIgnoreCase(protocolHeaderValue)) {

    xRequest.setSecure(false); xRequest.setScheme("http");
xRequest.setServerPort(httpServerPort); 

}

Here protocolHeaderHttpValue and httpServerPort are predefined values or loaded 
from
configuration.

Original comment by cyrille....@gmail.com on 28 Jan 2010 at 10:50

Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

Original comment by cyrille....@gmail.com on 28 Jan 2010 at 10:53

Changed title: XForwardedFilter : request.secure and request.scheme are not forced to "false" and "http" if X-Forwarded-Proto=http
Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

[deleted comment]

GoogleCodeExporter commented 9 years ago

I forgot to add the @Test annotation on the test :-(

I reproduced the problem and decided the following fix :

when xforwardedFor.protocolHeader is configured to X-Forwarded-For
 if X-Forwarded-Proto header is not defined 
    then don't overwrite request.secure, request.scheme and request.serverPort
 else if X-Forwarded-Proto header is equal to "https" 
    then request.secure=true, request.scheme=https, request.serverPort=443
 else request.secure=false, request.scheme=http, request.serverPort=80

Note : "X-Forwarded-For", 443 and 80 are configurables.

I HAVE ONE QUESTION :

Is it reasonable to NOT overwrite incoming request.secure, request.protocol and
request.serverPort if X-Forwarded-Proto header is null ?

Another approach would be to set assume missing header is similar to value http.

CAN YOU TEST THE TRUNK ?

Original comment by cyrille....@gmail.com on 28 Jan 2010 at 7:32

Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

I agree your changes, and yes, it is reasonable, so we get correct result if
accessing app directly.
Btw, you forgot to init HTTP_SERVER_PORT_PARAMETER.

Original comment by caisd1...@gmail.com on 29 Jan 2010 at 2:04

Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

I've verified your changes.

Original comment by caisd1...@gmail.com on 29 Jan 2010 at 2:18

Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

Sorry, I fixed the forgotten HTTP_SERVER_PORT_PARAMETER initialization issue.

Original comment by cyrille....@gmail.com on 29 Jan 2010 at 4:52

Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

Fixed in release 1.0.2. Can you try and confirm your problem is fixed ?
I still have to better document configuration parameters httpsServerPort and
httpServerPort.

Original comment by cyrille....@gmail.com on 30 Jan 2010 at 6:39

Changed state: Fixed
Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

confirmed

Original comment by caisd1...@gmail.com on 30 Jan 2010 at 9:20

Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

Original comment by cyrille....@gmail.com on 30 Jan 2010 at 11:26

Changed state: Verified
Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

Patch proposed to Tomcat project :
https://issues.apache.org/bugzilla/show_bug.cgi?id=48647

Original comment by cyrille....@gmail.com on 1 Feb 2010 at 12:29

Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

Patch has been applied in Tomcat and will be shipped in forthcoming 6.0.25
https://issues.apache.org/bugzilla/show_bug.cgi?id=48653

Original comment by cyrille....@gmail.com on 23 Feb 2010 at 9:18

Added labels: ****
Removed labels: ****

twarne / xebia-france

XForwardedFilter : request.secure and request.scheme are not forced to "false" and "http" if X-Forwarded-Proto=http #4