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

BlockingServletIo assumes the request's InputStream is unmolested... #10

Open
JustAHappyKid opened this issue Feb 18, 2020 · 4 comments
Labels
enhancement New feature or request module:servlet

Comments

@JustAHappyKid
Copy link

JustAHappyKid commented Feb 18, 2020

I'm not very familiar with the HTTP-servlet standard, so perhaps http4s is technically in-the-right in assuming the ServletInputStream has not yet been read from. However, it would certainly make the library more user-friendly (and have saved me hours) if the BlockingServletIo raised something like an IllegalStateException -- including a pertinent error message -- if the input-stream's isFinished method returns true out-the-gate.

In my case, I had a javax.servlet.Filter registered that was calling getParameterMap on the HttpServletRequest (to do some primitive malicious-request rejection), before passing off the request object off to the http4s stack... It looks like this was invoking Jetty's implementation of getParameterMap, which reads "POST" parameters (so to speak) from the request body.

The result is that I would end up with an empty Map when doing something like req.decode[UrlForm] { f => val params = f.values; /* ... */ } and no sign of what had gone wrong.

@rossabaker
Copy link
Member

That behavior of consuming the stream is unfortunately correct per servlet spec.

One thing I'm not sure of is whether isFinished returns true on an empty stream before an attempt to read. If not, we could probably do something like you suggest (for the cost of a boolean check per request on everybody). If empty bodies start isFinished, then I don't think there is anything we can do here.

@JustAHappyKid
Copy link
Author

Good question. I'll try to get you an answer -- what does isFinished return for an empty stream (empty-body request) before any attempt to read from the stream has been made?

@JustAHappyKid
Copy link
Author

Sorry I didn't follow-up on this sooner. It appears as though isFinished returns false even on an empty-bodied HTTP request, so long as read hasn't been called on the InputStream.

I set up an experiment like this in a servlet Filter:

println("isFinished: " + request.getInputStream.isFinished)
request.getInputStream.read()
println("isFinished: " + request.getInputStream.isFinished)

If I then issue a HEAD request using curl (e.g., curl --head http://localhost:8080/) the first call to isFinished returns false and the second true.

@rossabaker
Copy link
Member

Okay, that's encouraging. In jetty, there's a sychronization, but it should be pretty cheap in the grand scheme of things. I'd be fine with adding this check. Would you like to give it a try?

@rossabaker rossabaker added enhancement New feature or request module:servlet labels Apr 11, 2021
@rossabaker rossabaker transferred this issue from http4s/http4s Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request module:servlet
Projects
None yet
Development

No branches or pull requests

2 participants