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

Clean method parseRange #174

Closed
wants to merge 1 commit into from
Closed

Clean method parseRange #174

wants to merge 1 commit into from

Conversation

zhanhb
Copy link
Contributor

@zhanhb zhanhb commented Jun 23, 2019

No description provided.

@markt-asf
Copy link
Contributor

I've started to put together a unit test for this. I've committed an initial version. It is parameterised so adding additional tests should be simple.
I've taken a quick look at the range parsing and I am wondering about adding a new parser to org.apache.tomcat.util.http.parser for Range headers. MediaType looks to be a reaosnable class to bse it on.

@zhanhb
Copy link
Contributor Author

zhanhb commented Jun 24, 2019

It's not easy for me to run a single test case now. I execute curl instead.
I'll try to run single test case instead of execute command curl.

@zhanhb
Copy link
Contributor Author

zhanhb commented Jun 24, 2019

// bytes is the only range unit supported
if (!rangeHeader.startsWith("bytes")) {
response.sendError(HttpServletResponse.SC_BAD_REQUEST);
return null;
}

How abouting chaning Line 1391 to

if (!rangeHeader.startsWith("bytes=")) {

Definition in rfc 2068 and rfc 7233 are same, syntax of request header 'Range' will always start with that. Client should not send malformed range headers such as Range: bytes 1-2 or even Range: bytes11-20(we will parse that as if bytes=1-20).

@markt-asf
Copy link
Contributor

I've re-written the Range header parsing using the code in org.apache.tomcat.util.http.parser and adding a new parser for Range. There is scope for further code reduction but that will involve API changes to the Default servlet so I'll leave that for Tomcat 10.
I think I have caught all the issues in Range parsing you identified but if you have further test cases to add then please do so.

@zhanhb
Copy link
Contributor Author

zhanhb commented Jun 25, 2019

Here's a test case for PUT, no other cases yet now. (Maybe need to migrate to PATCH in the future)

# got 201 or 204
curl -i -X PUT --header 'Content-Range: bytes=1-5/4' --data 'abcdef' 'http://some-url-served-by-tomcat-default-servlet-and-not-readonly'

@zhanhb
Copy link
Contributor Author

zhanhb commented Jun 25, 2019

If strong weak tag is implemented, maybe strong etag validator can be applied on header If-Range. Just maybe, we can see on the client side, chrome doesn't do strict as specified in rfc 7233(will send weak etag in header If-Range).

@zhanhb
Copy link
Contributor Author

zhanhb commented Jun 25, 2019

@zhanhb
Copy link
Contributor Author

zhanhb commented Jun 25, 2019

My suggestion for multiple range support: support is enough, don't let it be an entrance for DDOS attack.

For this feature is specified in rfc document, many http server will supported this. But on the client side, a user agent who has a good ability to parse multi-range response usually doesn't need this feature. Single range is enough for them or capability reason.

@markt-asf
Copy link
Contributor

Partial PUT vs PATCH should be a separate topic. It will require careful consideration as some clients may depend on PUT.

Support for multiple ranges should also be a separate discussion as should issues with If-Range.

@zhanhb zhanhb closed this Jun 26, 2019
@zhanhb zhanhb deleted the polish branch June 29, 2019 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants