Skip to content
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

Corrupted POST data of type x-www-form-urlencoded after forwardOn404 #4867

Closed
peter20210924 opened this issue Sep 24, 2021 · 5 comments · Fixed by #5669
Closed

Corrupted POST data of type x-www-form-urlencoded after forwardOn404 #4867

peter20210924 opened this issue Sep 24, 2021 · 5 comments · Fixed by #5669

Comments

@peter20210924
Copy link

peter20210924 commented Sep 24, 2021

Dear community, In the most recent jersey 2.35 I observed a possible bug that causes my application to fail:

After POSTing application/x-www-form-urlencoded content to my application AFTER applying a org.glassfish.jersey.servlet.ServletContainer filter along with forwardOn404, the first byte is always missing.

The problem seems to be caused by calling containerRequest.hasEntity() in WebComponent.filterFormParameters(). I guess that EntityInputStream.isEmpty() consumes one byte from the original request input stream to check if there is some content but, if content (one byte) is found, it does only push it back to EntityInputStream.input but not the original input stream of the servlet request that will be forwarded later on.

The jersey code around seems to be quite complex, so fixing won't be that easy for me. It would be great if you could fix this issue soon. Thank you.

(Meanwhile I'll try to add two more filters to circumvent the problem: The first one changes the content type to application/x-www-form-urlencoded_fixme and the second reverts it afterwards to application/x-www-form-urlencoded. This should hopefully work.)

@jansupol
Copy link
Contributor

jansupol commented Oct 6, 2021

The EntityInputStream is in Jersey just because we want to be able to read the first byte multiple times. I am confused about the forwardOn404 option, whether it is actually involved in the issue, or not. Can you possibly provide a reproducer?

@gluser1357
Copy link

gluser1357 commented Sep 5, 2023

Hi, so, I'm really not able to reproduce this bug from 2021 exactly. Nevertheless, I found a related one that interferres with httpRequest.getInputStream() (same method that was called when creating EntityInputStream in Jersey, at least while POSTing x-www-form-urlencoded data): In short, Apache WicketFilter acts differently when just httpRequest.getInputStream() (without reading any data) has been called by a filter in advance. See details in readme.txt of my demo at https://github.com/gluser1357/jerseyissues/tree/issue-4867-wicket10 (runnable on Jetty 11 or Tomcat 10.1).

Since the issue seems not related to Jersey as demonstrated in the demo I think this issue 4867 (originally created by us in 2021) can be closed (but please read upcoming comment before).

@gluser1357
Copy link

gluser1357 commented Sep 7, 2023

Some news here: I posted the issue to the Apache Wicket Team and the guys were so kind and debugged it. The problem is actually not Wicket-related. In short:

  • in case of "Content-Type: application/x-www-form-urlencoded"
  • calling getInputStream() (like it is done when Jersey creates EntityInputStream), even without reading data from the stream,
  • causes subsequent calls to the getParameter*() Servlet API to NOT return any form parameters (e. g. in subsequent servlet filters like WicketFilter).

See quickstarter and explaination here: https://issues.apache.org/jira/browse/WICKET-7071.

This behaviour is not clear and not explained in the Servlet API docs (https://jakarta.ee/specifications/servlet/6.0/jakarta-servlet-spec-6.0#when-parameters-are-available) but leads to errors e. g. if Jersey is used as servlet filter before other filters in a filter chain. One solution that would help me would be to make Jersey not calling getInputStream() in case of non-matching paths and FILTER_FORWARD_ON_404=true. E. g. by a lazy initialization of/within the wrapping EntityInputStream.

Jersey team: Could you imagine such a lazy initialization of EntityInputStream which would avoid unneeded getInputStream() calls?

UPDATE: For sake of completeness I added another example (quickstart) on github: https://github.com/gluser1357/jerseyissues/tree/getinputstream-and-form-data. It allows quickly to test

  • the result of request.getParameter*() after passing a Jersey filter with no matching path
  • to demonstrate the unexplained behaviour of Tomcat 10.1 and Jetty when calling request.getParameter*() after calling request.getInputStream().

I'll further try to contact the other teams involved in this issue (Jetty, Tomcat, Jakarta spec). Maybe someone else wants to support here?

UPDATE 2: Question: Can we alternatively workaround the whole issue by simply call getParameterMap() in a servlet filter before any other filters like Jersey come into play, or would we run in new trouble by doing this? See ReadParamsIfFormEncodedFilter code and comments in https://issues.apache.org/jira/browse/WICKET-7071 (in the comments).

@michaz
Copy link

michaz commented Jun 2, 2024

Actually, this issue is more general:

Consider any Request that passes through Jersey-as-a-Filter, doesn't match, and is forwarded down the filter chain because of forwardOn404.

From the point of view of any downstream Filter or Servlet, this Request is already partially consumed. Any call to req.getReader() will fail, because Jersey already called req.getInputStream().

Either this method or getReader() may be called to read the body, not both.

On the other hand, calling req.getInputStream() again will work fine, because nothing has actually been read yet. It's just that the choice for InputStream and against Reader has been made, but downstream code can't know that.

The code looks like it maybe just isn't aware that the call to req.getInputStream() is destructive. Could it easily be deferred until it really needs to be made, say, by wrapping the entire req in the RequestContext instead of picking the inputStream?

@gluser1357
Copy link

Great to hear about #5669! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants