After forward from SAM injected request returns wrong servlet path #927

Closed
arjantijms opened this Issue Jul 14, 2016 · 5 comments

Comments

Projects
None yet
5 participants
@arjantijms
Contributor

arjantijms commented Jul 14, 2016

If a SAM forwards to a Servlet that uses a CDI bean with an injected HttpServletRequest, then calling getServletPath on this HttpServletRequest returns the servlet path of the original servlet, not the forwarded servlet.

This can be reproduced by running the test case at https://github.com/javaee-samples/javaee7-samples/tree/master/jaspic/dispatching-jsf-cdi

Deploying the .war build by that module to Payara 4.1.1.162 and requesting http://localhost:8080/jaspic-dispatching-jsf-cdi/public/servlet the following is shown:

response from forwardedServlet - Called from CDI
servletPath via Servlet - /forwardedServlet
servletPath via CDI - /public/servlet

It should however be:

response from forwardedServlet - Called from CDI
servletPath via Servlet - /forwardedServlet
servletPath via CDI - /forwardedServlet

On WildFly 10.0.0 and Liberty 16.0.0.2 the above is indeed shown.

@davidweaver

This comment has been minimized.

Show comment
Hide comment
@davidweaver

davidweaver Jul 20, 2016

Contributor

This has been raised internally to be corrected (PAYARA-917)

Contributor

davidweaver commented Jul 20, 2016

This has been raised internally to be corrected (PAYARA-917)

@smillidge

This comment has been minimized.

Show comment
Hide comment
@smillidge

smillidge Sep 3, 2016

Contributor

OK I've spent some time in this code investigating.

  1. This is nothing to do with JASPIC as the "error" happens without any JASPIC i.e. injecting a request scoped bean into a simple forwarded Servlet.
  2. I'm not sure if this is a bug without testing a lot on other servers.

The problem arises from how Payara implements Servlet forwards and includes. Payara always creates a HttpServletRequestWrapper for an include or forward and this is passed to the servlet.
However the WeldListener servlet in order to initialise the HttpServletRequest in the RequestScope listens for a ServletRequestEvent. This event is only fired, as defined by the spec, at the start of request tracing and is therefore passed the original unwrapped HttpServletRequest object which is then stored in the request scope. This original unwrapped HttpServletRequest instance is returned to the injected RequestScoped bean and therefore has the original path.

While it is pretty simple to correct the path issue by storing a special request attribute for the current path and returning that in response to getServletPath() to fix this issue (which I will do) the underlying issue remains.

I read the CDI spec but it's pretty silent on what happens if a ServletRequestWrapper is used somewhere in the processing pipeline and then the HttpServletRequest is subsequently injected into a request scoped bean. Should you receive the request wrapper or the original request?

Contributor

smillidge commented Sep 3, 2016

OK I've spent some time in this code investigating.

  1. This is nothing to do with JASPIC as the "error" happens without any JASPIC i.e. injecting a request scoped bean into a simple forwarded Servlet.
  2. I'm not sure if this is a bug without testing a lot on other servers.

The problem arises from how Payara implements Servlet forwards and includes. Payara always creates a HttpServletRequestWrapper for an include or forward and this is passed to the servlet.
However the WeldListener servlet in order to initialise the HttpServletRequest in the RequestScope listens for a ServletRequestEvent. This event is only fired, as defined by the spec, at the start of request tracing and is therefore passed the original unwrapped HttpServletRequest object which is then stored in the request scope. This original unwrapped HttpServletRequest instance is returned to the injected RequestScoped bean and therefore has the original path.

While it is pretty simple to correct the path issue by storing a special request attribute for the current path and returning that in response to getServletPath() to fix this issue (which I will do) the underlying issue remains.

I read the CDI spec but it's pretty silent on what happens if a ServletRequestWrapper is used somewhere in the processing pipeline and then the HttpServletRequest is subsequently injected into a request scoped bean. Should you receive the request wrapper or the original request?

smillidge added a commit to smillidge/Payara that referenced this issue Sep 3, 2016

@arjantijms

This comment has been minimized.

Show comment
Hide comment
@arjantijms

arjantijms Sep 4, 2016

Contributor
  1. This is nothing to do with JASPIC as the "error" happens without any JASPIC i.e. injecting a request scoped bean into a simple forwarded Servlet.

Good find! I'll try to add a test for this too in the Java EE 7 samples project.

  1. I'm not sure if this is a bug without testing a lot on other servers.

I'd like to do a new report on a larger number of servers wrt to JASPIC tests from the EE 7 samples project soon, so then for a somewhat larger number of servers it will become clear how they behave here.

However the WeldListener servlet in order to initialise the HttpServletRequest in the RequestScope listens for a ServletRequestEvent. This event is only fired, as defined by the spec, at the start of request tracing and is therefore passed the original unwrapped HttpServletRequest object which is then stored in the request scope.

From what I understood, the CDI spec is not super clear on how the per request setup should actually be done. Weld provides the listener as a convenience for integrators. It does mention somewhere that if used it should be the first listener invoked (which may require some container specific code to guarantee this).

Also, "the start of request tracing" is not entirely clear either to everyone. Many vendors take it to be before authorization and authentication checks are done, but Liberty and Weblogic take it to be afterwards (see https://java.net/projects/servlet-spec/lists/users/archive/2015-03/message/1).

This means that on some servers the request has already been seen and can even be wrapped before the ServletRequestEvent is thrown :(

I read the CDI spec but it's pretty silent on what happens if a ServletRequestWrapper is used somewhere in the processing pipeline and then the HttpServletRequest is subsequently injected into a request scoped bean. Should you receive the request wrapper or the original request?

This is a very good observation, and in fact a short while ago I've been discussing exactly this point with Romain Manni-Bucau from TomEE. Long story short; it indeed wasn't clear which version of the wrapped request you would get.

Would also be good to check if other CDI build-in beans like the one for the injected Principal is capable of changing when mid-request logout and a new login takes place.

For now I will at least start a discussing for this on the CDI spec mailing list.

Contributor

arjantijms commented Sep 4, 2016

  1. This is nothing to do with JASPIC as the "error" happens without any JASPIC i.e. injecting a request scoped bean into a simple forwarded Servlet.

Good find! I'll try to add a test for this too in the Java EE 7 samples project.

  1. I'm not sure if this is a bug without testing a lot on other servers.

I'd like to do a new report on a larger number of servers wrt to JASPIC tests from the EE 7 samples project soon, so then for a somewhat larger number of servers it will become clear how they behave here.

However the WeldListener servlet in order to initialise the HttpServletRequest in the RequestScope listens for a ServletRequestEvent. This event is only fired, as defined by the spec, at the start of request tracing and is therefore passed the original unwrapped HttpServletRequest object which is then stored in the request scope.

From what I understood, the CDI spec is not super clear on how the per request setup should actually be done. Weld provides the listener as a convenience for integrators. It does mention somewhere that if used it should be the first listener invoked (which may require some container specific code to guarantee this).

Also, "the start of request tracing" is not entirely clear either to everyone. Many vendors take it to be before authorization and authentication checks are done, but Liberty and Weblogic take it to be afterwards (see https://java.net/projects/servlet-spec/lists/users/archive/2015-03/message/1).

This means that on some servers the request has already been seen and can even be wrapped before the ServletRequestEvent is thrown :(

I read the CDI spec but it's pretty silent on what happens if a ServletRequestWrapper is used somewhere in the processing pipeline and then the HttpServletRequest is subsequently injected into a request scoped bean. Should you receive the request wrapper or the original request?

This is a very good observation, and in fact a short while ago I've been discussing exactly this point with Romain Manni-Bucau from TomEE. Long story short; it indeed wasn't clear which version of the wrapped request you would get.

Would also be good to check if other CDI build-in beans like the one for the injected Principal is capable of changing when mid-request logout and a new login takes place.

For now I will at least start a discussing for this on the CDI spec mailing list.

@smillidge

This comment has been minimized.

Show comment
Hide comment
@smillidge

smillidge Sep 4, 2016

Contributor

An interesting test would be to create a Servlet Filter which wraps the request and then see what request is received in Request Scoped Beans in other servers that utilise Weld. It could be that Liberty and WildFly don't wrap servlet requests during forwarding hence the difference in behaviour.

Contributor

smillidge commented Sep 4, 2016

An interesting test would be to create a Servlet Filter which wraps the request and then see what request is received in Request Scoped Beans in other servers that utilise Weld. It could be that Liberty and WildFly don't wrap servlet requests during forwarding hence the difference in behaviour.

@arjantijms

This comment has been minimized.

Show comment
Hide comment
@arjantijms

arjantijms Sep 4, 2016

Contributor

An interesting test would be to create a Servlet Filter which wraps the request and then see what request is received in Request Scoped Beans in other servers that utilise Weld

From the top of my head I think they will receive the original request, not the wrapped one, but certainly something to double check. Indeed, if Liberty and WildFly somehow make the original request instance aware of every forward and include, it would likely automatically work for request instances that were previously stored.

There's quite an amount of research that can be done here to find out what's exactly happening.

Contributor

arjantijms commented Sep 4, 2016

An interesting test would be to create a Servlet Filter which wraps the request and then see what request is received in Request Scoped Beans in other servers that utilise Weld

From the top of my head I think they will receive the original request, not the wrapped one, but certainly something to double check. Indeed, if Liberty and WildFly somehow make the original request instance aware of every forward and include, it would likely automatically work for request instances that were previously stored.

There's quite an amount of research that can be done here to find out what's exactly happening.

@smillidge smillidge closed this in 46a4a3e Sep 13, 2016

lprimak pushed a commit to flowlogix/Payara that referenced this issue Jul 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment