You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2021/05/26 23:12:20 UTC
[GitHub] [trafficserver] shinrich opened a new pull request #7887: Fail parsing the request line version if there are too many characters
shinrich opened a new pull request #7887:
URL: https://github.com/apache/trafficserver/pull/7887
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #7887: Fail parsing the request line version if there are too many characters
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7887:
URL: https://github.com/apache/trafficserver/pull/7887#issuecomment-849212786
Running autest for testing text parser sounds like a great idea 😛
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7887: Fail parsing the request line version if there are too many characters
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7887:
URL: https://github.com/apache/trafficserver/pull/7887#issuecomment-853069532
@maskit that is a very good point. I have adjusted the PR to only change the default value of proxy.config.http.strict_uri_parsing from 0 to 1. Probably having a default of 0 (disabled) made sense 15 years ago when many more clients played very loose with the spec, but these days good clients should be within spec and we should be in the more secure stance out of the box.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #7887: Fail parsing the request line version if there are too many characters
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #7887:
URL: https://github.com/apache/trafficserver/pull/7887#discussion_r641664494
##########
File path: tests/gold_tests/headers/gold/bad_good_request_v10.gold
##########
@@ -0,0 +1,22 @@
+HTTP/1.0 400 Invalid HTTP Request
+Date: ``
+Server: ATS/``
+Cache-Control: no-store
+Content-Type: text/html
+Content-Language: en
+Content-Length: 219
+
+<HTML>
+<HEAD>
+<TITLE>Bad Request</TITLE>
+</HEAD>
+
+<BODY BGCOLOR="white" FGCOLOR="black">
+<H1>Bad Request</H1>
+<HR>
+
+<FONT FACE="Helvetica,Arial"><B>
+Description: Could not process this request.
+</B></FONT>
+<HR>
+</BODY>
Review comment:
Ah, OK. That makes sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7887: Fail parsing the request line version if there are too many characters
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7887:
URL: https://github.com/apache/trafficserver/pull/7887#issuecomment-853385977
Hmm, looks like a fair number of our clients like to use non-RFC compliant characters in their requests. It seems like having a middle ground check for white space and control characters would be useful. I'm experimenting with that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on a change in pull request #7887: Fail parsing the request line version if there are too many characters
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7887:
URL: https://github.com/apache/trafficserver/pull/7887#discussion_r641650167
##########
File path: proxy/hdrs/URL.cc
##########
@@ -1194,6 +1194,12 @@ url_is_strictly_compliant(const char *start, const char *end)
} // namespace UrlImpl
using namespace UrlImpl;
+bool
+url_is_compliant(const char *start, const char *end)
+{
+ return url_is_strictly_compliant(start, end);
+}
+
Review comment:
Have to do this wrapper because url_is_strictly_compliant is in the UrlImpl space. I tried taking it out and it wouldn't build because of a missing hash table maybe because it is calling ParseRules?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on a change in pull request #7887: Fail parsing the request line version if there are too many characters
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7887:
URL: https://github.com/apache/trafficserver/pull/7887#discussion_r641579631
##########
File path: tests/gold_tests/headers/gold/bad_good_request_v10.gold
##########
@@ -0,0 +1,22 @@
+HTTP/1.0 400 Invalid HTTP Request
+Date: ``
+Server: ATS/``
+Cache-Control: no-store
+Content-Type: text/html
+Content-Language: en
+Content-Length: 219
+
+<HTML>
+<HEAD>
+<TITLE>Bad Request</TITLE>
+</HEAD>
+
+<BODY BGCOLOR="white" FGCOLOR="black">
+<H1>Bad Request</H1>
+<HR>
+
+<FONT FACE="Helvetica,Arial"><B>
+Description: Could not process this request.
+</B></FONT>
+<HR>
+</BODY>
Review comment:
But the problem of the excludes test is if the following request is processed in a different way than I would expect, the excludes test would not catch it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #7887: Fail parsing the request line version if there are too many characters
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #7887:
URL: https://github.com/apache/trafficserver/pull/7887#discussion_r640767269
##########
File path: tests/gold_tests/headers/gold/bad_good_request_v10.gold
##########
@@ -0,0 +1,22 @@
+HTTP/1.0 400 Invalid HTTP Request
+Date: ``
+Server: ATS/``
+Cache-Control: no-store
+Content-Type: text/html
+Content-Language: en
+Content-Length: 219
+
+<HTML>
+<HEAD>
+<TITLE>Bad Request</TITLE>
+</HEAD>
+
+<BODY BGCOLOR="white" FGCOLOR="black">
+<H1>Bad Request</H1>
+<HR>
+
+<FONT FACE="Helvetica,Arial"><B>
+Description: Could not process this request.
+</B></FONT>
+<HR>
+</BODY>
Review comment:
I wonder whether this gold file is verifying too much? Any small changes in this output, even by one character, will cause it to fail. Perhaps:
```
HTTP/1.0 400 Invalid HTTP Request
Date: ``
Server: ATS/``
Cache-Control: no-store
Content-Type: text/html
Content-Language: en
Content-Length: ``
``
<TITLE>Bad Request</TITLE>
``
Description: Could not process this request.
``
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich closed pull request #7887: Fail parsing the request line version if there are too many characters
Posted by GitBox <gi...@apache.org>.
shinrich closed pull request #7887:
URL: https://github.com/apache/trafficserver/pull/7887
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7887: Fail parsing the request line version if there are too many characters
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7887:
URL: https://github.com/apache/trafficserver/pull/7887#issuecomment-853385977
Hmm, looks like a fair number of our clients like to use non-RFC compliant characters in their requests. It seems like having a middle ground check for white space and control characters would be useful. I'm experimenting with that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on a change in pull request #7887: Fail parsing the request line version if there are too many characters
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7887:
URL: https://github.com/apache/trafficserver/pull/7887#discussion_r641578094
##########
File path: tests/gold_tests/headers/gold/bad_good_request_v10.gold
##########
@@ -0,0 +1,22 @@
+HTTP/1.0 400 Invalid HTTP Request
+Date: ``
+Server: ATS/``
+Cache-Control: no-store
+Content-Type: text/html
+Content-Language: en
+Content-Length: 219
+
+<HTML>
+<HEAD>
+<TITLE>Bad Request</TITLE>
+</HEAD>
+
+<BODY BGCOLOR="white" FGCOLOR="black">
+<H1>Bad Request</H1>
+<HR>
+
+<FONT FACE="Helvetica,Arial"><B>
+Description: Could not process this request.
+</B></FONT>
+<HR>
+</BODY>
Review comment:
I agree to a certain extent, but the test is also ensuring that the following request is not processed which the wildcard at the end would not catch. Guess I could also do this via Contains and Excludes tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #7887: Fail parsing the request line version if there are too many characters
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7887:
URL: https://github.com/apache/trafficserver/pull/7887#discussion_r643572717
##########
File path: proxy/hdrs/HTTP.cc
##########
@@ -1088,6 +1088,11 @@ http_parser_parse_req(HTTPParser *parser, HdrHeap *heap, HTTPHdrImpl *hh, const
}
url_end += 1;
+ // Make sure we didn't pickup odd characters in the url between the method and version
+ if (!url_is_compliant(url_start, url_end)) {
Review comment:
The same check will be done later when `url_parse` is called, if `strict_uri_parsing` is `true`. In other words, there might be a case we want to accept an invalid URL? Adding this check here effectively makes the flag void.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7887: Fail parsing the request line version if there are too many characters
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7887:
URL: https://github.com/apache/trafficserver/pull/7887#issuecomment-868720340
Will be opening a different PR which adds an intermediate option to the strict url parsing setting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org