GuiceFilter breaks dispatching to jsp if jasper is being used to compile the jsp

[gissuebot]

From brianm%[email protected] on May 12, 2009 18:42:37

The Jasper JSP compiler uses request.getServletPath() to name the JSP file.
When dispatched to a JSP under the filter the servlet path is either empty
or /, which leads to an error when generating a class name for the jsp file
before it can be compiled.

This is happening in 2.0- r943

Original issue: http://code.google.com/p/google-guice/issues/detail?id=372

[gissuebot]

From brianm%[email protected] on May 12, 2009 15:43:53

probably, if dispatching to a servlet, the request and response should be wrapped and
made to look like the dispatch was actually to a servlet by the container.

I am seeing this under jetty 6.1.16

[gissuebot]

From dhanji on May 12, 2009 16:07:21

Yea this is a known issue--I've just not had any time to fix it up =(

Note that this is a side effect of returning the wrong path in the requestDispatcher for JSP forwards. It affects
Guice Servlets using request dispatcher to forward to JSPs too, for example...

Brownie points if you willing to submit a patch!

[gissuebot]

From Leigh.Klotz on July 28, 2009 15:48:22

Is the problem somewhere near //noinspection OverlyComplexAnonymousInnerClass in
ServletDefinition.jav?

[gissuebot]

From landon.wainwright on August 24, 2009 11:37:06

Yeah this is very annoying. I have been trying to solve it for a while now.Are there
any work arounds that I can use in the mean time to direct the response to a JSP?

[gissuebot]

From tobiasapt on December 08, 2009 15:01:40

I've come up with a work around that works for Jasper.  Jasper looks for a request
attribute "org.apache.catalina.jsp_file" and uses that before it checks
request.getServletPath().  I tried simply setting the attribute but it gets blown
away at some point during during the request.

public class JSPFixGuiceFilter extends GuiceFilter{
       @Override
       public void doFilter(ServletRequest request,
                       ServletResponse response, FilterChain filterChain)
                       throws IOException, ServletException {
                       request = new
HttpServletRequestWrapper((HttpServletRequest)request){
                               @Override
                               public Object getAttribute(String name) {
                                       if("org.apache.catalina.jsp_file".equals(name)){
                                               return super.getServletPath();
                                       }
                                       return super.getAttribute(name);
                               }
                       };

                       super.doFilter(request, response, filterChain);
               
       }
}

[gissuebot]

From burke.eric on December 21, 2009 13:56:29

This fails if MyServlet is managed by Guice. If this is just a normal unmanaged
servlet configured in web.xml, it works fine:

@Singleton
public class MyServlet extends HttpServlet {
    protected void doGet(HttpServletRequest req, HttpServletResponse res)
            throws ServletException, IOException {
        req.getRequestDispatcher("/WEB-INF/protected_page.html").forward(req, res);
    }
}

In Tomcat I receive "HTTP Status 404 - /WEB-INF/protected_page.html" with description
"The requested resource (/WEB-INF/protected_page.html) is not available."

[gissuebot]

From burke.eric on December 21, 2009 14:42:00

I'm attaching a web app that demonstrates a problem dispatching to a static HTML file
under the WEB-INF directory. I tried to make it easy to compile and run, see the
README.txt in the ZIP file. The embedded JARs are from trunk in Guice Subversion on
12/21/2009, revision 1134

Binary attachments: guice_bug.zip, guice_bug.war

[gissuebot]

From dhanji on December 30, 2009 16:06:43

Can this be considered fixed (I believe r1135 fixes it, but plz verify)?

I will mark as fixed, plz reopen if there is still a problem.

Status: Fixed
Owner: dhanji

[gissuebot]

From reynirhubner on June 24, 2010 08:46:32

I just tested this out. It does not work as intended.
I checked out the source today from trunk, built it and used it for the test.

I have a filter that dispatches the request to a servlet.
RequestDispatcher reqDispatcher = req.getRequestDispatcher("/servletName/c2/?param1=1);
reqDispatcher.forward(request, response);

The dispatch almost works, except, when I check the value of request.getPathInfo(), I get "/servletName/c2/?param1=1" but should get "/servletName/c2/", as the request parameters should not be included...

[gissuebot]

From dhanji on October 08, 2010 06:20:48

Can you submit a test case for this please? That would be the best way for us to verify.

Thanks!

Status: Accepted

[gissuebot]

From [email protected] on April 03, 2011 19:38:11

I've encountered this too, on Jetty 7.2.2 + Guice 3.0 + Stripes 1.5.3.

The original request matches a pattern ("/admin/*" in my case), and gets wrapped in a guice ServletDefinition$2 object. When the handling servlet does a forward, jetty wraps the guice request wrapper and then invokes setServletPath to point it to the new servlet. In my case, the forward is to "/WEB-INF/jsp/overview.jsp", and that string is what is passed to request.setServletPath(...).

The jasper JspServlet then calls request.getServletPath() to find the jsp to render; this is handled by ServletDefinition$2 which calls super.getServletPath() to get the correct new path, but it then calls UriPatternType.extract("/WEB-INF/jsp/overview.jsp") which returns the path to the original servlet, ie "/admin"!

Result is that the jsp cannot be found.

[gissuebot]

From ori.schwartz on April 04, 2011 19:42:31

There's definitely a bug here.

Here's the simplest test case (I'm using tomcat 5.5.33):

* Use Guice ServletModule to configure two servlets, one of which just dispatches to the other:

    protected void configureServlets()
    {
        serve( "/dispatcher" ).with( DispatcherServlet.class );
        serve( "/dispatchee" ).with( DispatcheeServlet.class );
    }

* Dispatchee outputs text for verification, dispatcher redirects like this:

        protected void doGet( HttpServletRequest req, HttpServletResponse resp )
            throws ServletException, IOException
        {
            this.getServletContext().getRequestDispatcher( "/dispatchee" )
                .forward( req, resp );
        }

* Invoking /dispatcher yields a blank page with a 404 status that is incorrectly served up by Tomcat's default servlet.

* When configuring via traditional web.xml instead of guice, everything works as expected.

* I did some debugging and it looks like Tomcat's internal pattern matching inside its RequestDispatcher implementation isn't even aware of the servlet mapping that Guice configured.

Attachment: gist
   GuiceRequestDispatcherBug.java

[gissuebot]

From [email protected] on April 04, 2011 20:01:39

re#12: I think that's a different issue from the one discussed in the other comments on this bugreport.

Tomcat definitely isn't aware of the mappings define via serve(x).with(y). The whole design is that when the original request comes in, the servlet container will not find a matching servlet (because there is no matching entry in web.xml), so will select the "default servlet" as the target. However it will run the Guice filter first, as that is mapped to "/*". And the guice filter then arranges for the request to be sent to the configured servlet instead of passing it to the (default) servlet that the web container chose.

I would have thought that forwarding from one Guice-configured servlet to another would be supported (eg by creating a custom RequestDispatcher), but it isn't really necessary for most users.

Forwarding from a guice-configured servlet to the container's JSP or static-resource servlet is critical, and this is the issue discussed in this bugreport. Note that comment#11 is about using a wildcard-suffix (eg "/admin/"). The existing code works fine with wildcard-prefixes (eg ".do"), because UriPatternType.extract returns null in this case, causing ServletDefinition$2.getServletPath() to return the (correct) path from the parent class.

[gissuebot]

From ori.schwartz on April 05, 2011 09:00:59

Thanks Simon (#13), that explains it. You are right, this is not the same as the JSP issue discussed in this report.

Should I open a separate issue for the behavior I reported? It seems like an important fix to me. Lots of apps use ServletContext#getRequestDispatcher to forward requests internally.

[gissuebot]

From sberlin on April 05, 2011 09:46:00

Yes, please open a separate issue.  If someone is able to also attach a patch with a testcase & fix that:
 a) Fails before applying the fix
and
 b) Succeeds after applying the fix

That will help very much in fixing this.

[gissuebot]

From ori.schwartz on April 05, 2011 09:57:15

I've opened up issue 621 for the above comment #12 (forward method in ServeltContext#getRequestDispatcher).

[gissuebot]

From valotas on June 03, 2011 13:59:38

re#11: serveRegex("/admin/.*").with(YourAdminServlet.class) should work. At least that solved the same problem I had with a servlet mapped to a wildcard-suffix path.

[gissuebot]

From valotas on June 05, 2011 12:54:35

Attached a possible solution with its test.

I'm not sure if it is the best possible one as I do not know if a custom HttpServletRequest that tries to mimic servlet container behavior of computing stuff like the request paths is needed by the guice-servlet module internals.

Attachment: gist
   ServletDefinition.java
   ServletDefinitionPathsTest.java

[gissuebot]

From sberlin on June 05, 2011 12:59:16

Would you be able to attach a patch instead of the new file as a whole?  Thanks! (Patches are much easier to review.)

[gissuebot]

From valotas on June 05, 2011 13:09:30

If you can help me on how to do that! Never had to do it before!

[gissuebot]

From sberlin on June 05, 2011 13:16:50

If you're using Eclipse there should be a Team -> Create Patch option on the file or project (right-click to find it).  If you're using SVN directly, use 'svn diff > patch.txt'.  You have to have the code checked out from SVN (either directly, through Eclipse, or through whatever IDE you're using).  It's harder if you just are modifying the src jars (but possible.. you need a diff tool & the original & modified files).

See: https://code.google.com/p/google-guice/source/checkout for how to get the source from SVN.

[gissuebot]

From valotas on June 05, 2011 13:41:48

Ok, attached you can find a patch file!

Attachment: gist
   372patch.txt

[gissuebot]

From valotas on September 11, 2011 10:38:40

It's been a while! Has this been fixed? Is there any problem (or any other problem) regarding the patch provided? I would be glad to investigate more if that is the case!

[gissuebot]

From sberlin on October 16, 2011 09:33:09

Is it possible to write a test-case also?  I prefer not to apply patches without tests attached, especially when I don't know the code as well.  The test will make sure the problem doesn't regress, and also highlight what was wrong in the first place.

Owner: sberlin

[gissuebot]

From valotas on October 16, 2011 13:08:06

Isn't the ServletDefinitionPathsTest enough?

This is what I added:
pathInfoWithServletStyleMatching("/path/some/path/of.jsp", "/path", "/thing/*", "/some/path/of.jsp", "/some/path/of.jsp");

It should fail if you try to run it without the ServletDefinition changes

[gissuebot]

From arman.vartan on April 08, 2014 07:50:27

After 3.5 years this patch has neither found its way into any minor v3 release nor any work as been done to fix this issue in the v4 betas.

As this is not a minor bug and has been addressed in several other tickets, please revise the status of this ticket.

[gissuebot]

From valotas on April 08, 2014 07:56:06

It looks like the servlet module is not a high prio one :(

[gissuebot]

From sberlin on April 08, 2014 07:59:15

Sorry, this fell off our radar.  (3+ years without anyone commenting on it helps do that.)  Is the patch from #22 still valid against the latest servlet code?

[gissuebot]

From valotas on April 08, 2014 08:08:09

Well, last comment was mine, explaining where to find the test requested. The fact that you requested a test case means that you did not review the patch that you requested me :)

Anyway, I can not confirm now that the patch will work against the latest code at the trunk (as mentioned by the development team, there are quite some changes to the guice internals).

[gissuebot]

From sberlin on April 08, 2014 08:09:44

I remember looking at it, but missed the fact that a test was added (given that the test was just a single line) -- I was expecting a whole method.  I'll try to find some time to apply it & confirm it doesn't expose other problems.

[gissuebot]

From valotas on April 08, 2014 08:17:09

No problem. Happy to have helped :)

[gissuebot]

From valotas on April 08, 2014 08:22:11

Have though in mind what I've said to my previous response:

"I'm not sure if it is the best possible one as I do not know if a custom HttpServletRequest that tries to mimic servlet container behavior of computing stuff like the request paths is needed by the guice-servlet module internals"

I really do not know of a better way, but in order to make sure that the guice-servlet does not break the servlet specifications is to write tests against the servlet specifications :/

[gissuebot]

From arman.vartan on April 08, 2014 08:22:40

Wow, that was fast. Thanks guys :)

[gissuebot]

From sberlin on April 10, 2014 06:31:35

I have a few questions about the patch:
  1) Should pathInfo.contains(servletPath) instead be pathInfo.startsWith(servletPath)?  That seems safer and it passes the test, but I don't know the intent.
  2) The comment says (paraphrased) "If pathInfo doesn't contain servletPath ... then return null".  But the code doesn't do that.  The code returns pathInfo as-is (with the contextPath stripped) if it doesn't contain the servlet path.  Is that intended?

[gissuebot]

From valotas on April 10, 2014 07:50:13

I think that you are right. I can not remember my intention on the code I wrote 3y ago :)

I'll try to have a look at it the following days

[gissuebot]

From sberlin on April 10, 2014 13:28:56

Can you confirm if putting
 else { // !pathInfo.startsWith(servletPath)
   pathInfo = null;
 }

works for you too?  Based on the javadoc for getPathInfo, it seems incorrect to have pathInfo be the same as the servletPath.

[gissuebot]

From sberlin on April 10, 2014 16:28:14

Fixed by r=c35ebc2ce88f & r=f1abba38c7f5.

Status: Fixed