ChristopherSchultz commented on a change in pull request #417:
URL: https://github.com/apache/tomcat/pull/417#discussion_r622458605
##########
File path: webapps/docs/changelog.xml
##########
@@ -143,6 +143,12 @@
request line, ensure that all the available data is included in the
error message. (markt)
</fix>
+ <fix>
+ <bug>65272</bug>: Restore the optional HTTP feature that allows
+ <code>LF</code> to be treated as a line terminator as well as the
Review comment:
This is specifically for a line-terminator for request-line and headers,
correct? Is it worth mentioning that specifically?
##########
File path: java/org/apache/coyote/http11/Http11InputBuffer.java
##########
@@ -550,10 +550,15 @@ boolean parseRequestLine(boolean keptAlive, int
connectionTimeout, int keepAlive
prevChr = chr;
chr = byteBuffer.get();
if (chr == Constants.CR) {
- // Possible end of request line. Need LF next.
+ // Possible end of request line. Need LF next else invalid.
} else if (prevChr == Constants.CR && chr == Constants.LF) {
+ // CRLF is the standard line terminator
end = pos - 1;
parsingRequestLineEol = true;
+ } else if (chr == Constants.LF) {
Review comment:
Is it worth "fingerprinting" a message to determine if it is
self-consistent? That is, a message is allowable with LF-terminated
request/headers but _only_ if *all* of its headers and the request line are
terminated with LF (only)? Or is that being unnecessarily picky and
complicating the code?
##########
File path: test/org/apache/coyote/http11/TestHttp11InputBuffer.java
##########
@@ -687,24 +691,6 @@ public void testInvalidEndOfRequestLine01() {
}
- @Test
Review comment:
I would leave this test in, but change its nature (i.e. require that the
response code is 200). Or is your assertion that the "correct" test is below in
`test/org/apache/coyote/http11/TestHttp11InputBufferCRLF.java `?
##########
File path: java/org/apache/coyote/http11/Http11InputBuffer.java
##########
@@ -841,7 +846,8 @@ private HeaderParseStatus parseHeader() throws IOException {
if (chr == Constants.CR && prevChr != Constants.CR) {
Review comment:
This appears to essentially ignore (all?) CR characters in headers. I'm
not sure that's what we want.
--
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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
