Re: [PR] Make status code configurable [tomcat]
markt-asf commented on PR #723: URL: https://github.com/apache/tomcat/pull/723#issuecomment-2288295442 Ah, understood. My view is that this is a neat trick but it isn't sufficient. There are a bunch of places where Tomcat calls `response.sendError(400,"reason")` where we also want the connection to be closed. e.g. `CoyoteAdapter`. I don't see how we can differentiate between those calls and calls from the application that do not need to be fatal. I keep coming back to the view that applications should not be using 400 responses for this. Those applications that need to use HTTP response codes should define their own 4xx status code(s). -- 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: dev-unsubscr...@tomcat.apache.org 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
(tomcat) branch main updated (d2546e873d -> 3dcd0bea99)
This is an automated email from the ASF dual-hosted git repository. markt pushed a change to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git from d2546e873d Fix typo add 3dcd0bea99 Fix regression in reading chunked request bodies No new revisions were added by this update. Summary of changes: .../coyote/http11/filters/ChunkedInputFilter.java | 10 ++ .../http11/filters/TestChunkedInputFilter.java | 115 +++-- webapps/docs/changelog.xml | 11 ++ 3 files changed, 130 insertions(+), 6 deletions(-) - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
(tomcat) 02/02: Fix regression in reading chunked request bodies
This is an automated email from the ASF dual-hosted git repository. markt pushed a commit to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/tomcat.git commit adc2a3bbe9b56d66048da65d16c842da17d79bdf Author: Mark Thomas AuthorDate: Wed Aug 14 10:53:04 2024 +0100 Fix regression in reading chunked request bodies available() was returning a non-zero value when there was no data to read. In some circumstances this could cause a blocking read to block waiting for more data rather than return the data it had already received. --- .../coyote/http11/filters/ChunkedInputFilter.java | 10 ++ .../http11/filters/TestChunkedInputFilter.java | 115 +++-- webapps/docs/changelog.xml | 11 ++ 3 files changed, 130 insertions(+), 6 deletions(-) diff --git a/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java b/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java index 36389576d4..a967ef65a8 100644 --- a/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java +++ b/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java @@ -198,6 +198,16 @@ public class ChunkedInputFilter implements InputFilter, ApplicationBufferHandler if (readChunk != null) { available = readChunk.remaining(); } + +// Handle some edge cases +if (available == 1 && parseState == ParseState.CHUNK_BODY_CRLF) { + // Either just the CR or just the LF are left in the buffer. There is no data to read. +available = 0; +} else if (available == 2 && !crFound && parseState == ParseState.CHUNK_BODY_CRLF) { + // Just CRLF is left in the buffer. There is no data to read. +available = 0; +} + if (available == 0) { // No data buffered here. Try the next filter in the chain. return buffer.available(); diff --git a/test/org/apache/coyote/http11/filters/TestChunkedInputFilter.java b/test/org/apache/coyote/http11/filters/TestChunkedInputFilter.java index b264092ed0..5bb587d620 100644 --- a/test/org/apache/coyote/http11/filters/TestChunkedInputFilter.java +++ b/test/org/apache/coyote/http11/filters/TestChunkedInputFilter.java @@ -16,9 +16,13 @@ */ package org.apache.coyote.http11.filters; +import java.io.BufferedReader; import java.io.IOException; import java.io.InputStream; +import java.io.InputStreamReader; import java.io.PrintWriter; +import java.nio.charset.StandardCharsets; +import java.util.concurrent.TimeUnit; import jakarta.servlet.ServletException; import jakarta.servlet.http.HttpServlet; @@ -36,7 +40,6 @@ import org.apache.catalina.startup.TomcatBaseTest; public class TestChunkedInputFilter extends TomcatBaseTest { -private static final String LF = "\n"; private static final int EXT_SIZE_LIMIT = 10; @Test @@ -117,16 +120,16 @@ public class TestChunkedInputFilter extends TomcatBaseTest { SimpleHttpClient.CRLF + "Connection: close" + SimpleHttpClient.CRLF + SimpleHttpClient.CRLF + -"3" + (chunkHeaderUsesCRLF ? SimpleHttpClient.CRLF : LF) + -"a=0" + (chunkUsesCRLF ? SimpleHttpClient.CRLF : LF) + +"3" + (chunkHeaderUsesCRLF ? SimpleHttpClient.CRLF : SimpleHttpClient.LF) + +"a=0" + (chunkUsesCRLF ? SimpleHttpClient.CRLF : SimpleHttpClient.LF) + "4" + SimpleHttpClient.CRLF + "&b=1" + SimpleHttpClient.CRLF + "0" + SimpleHttpClient.CRLF + "x-trailer1: Test", "Value1" + -(firstheaderUsesCRLF ? SimpleHttpClient.CRLF : LF) + +(firstheaderUsesCRLF ? SimpleHttpClient.CRLF : SimpleHttpClient.LF) + "x-trailer2: TestValue2" + -(secondheaderUsesCRLF ? SimpleHttpClient.CRLF : LF) + -(endUsesCRLF ? SimpleHttpClient.CRLF : LF) }; +(secondheaderUsesCRLF ? SimpleHttpClient.CRLF : SimpleHttpClient.LF) + +(endUsesCRLF ? SimpleHttpClient.CRLF : SimpleHttpClient.LF) }; TrailerClient client = new TrailerClient(tomcat.getConnector().getLocalPort()); @@ -777,4 +780,104 @@ public class TestChunkedInputFilter extends TomcatBaseTest { Assert.assertNull(client.getResponseLine()); } } + + +private static class BodyReadLineServlet extends HttpServlet { +private static final long serialVersionUID = 1L; + +@Override +protected void doPost(HttpServletRequest req, HttpServletResponse resp) +throws ServletException, IOException { + +int lineCount = 0; +int pauseCount = 0; + +// Read the body one line at a time. There should be ~1s between reads. +try (InputStream is = req.getInputStream(); +InputStreamReader isr = new InputStreamReader(is, StandardCharsets.UTF_8); +BufferedReader br = n
(tomcat) branch 10.1.x updated (abcb2b1faf -> adc2a3bbe9)
This is an automated email from the ASF dual-hosted git repository. markt pushed a change to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/tomcat.git from abcb2b1faf Add test case for null property new a6d6e3dd7e Fix typo new adc2a3bbe9 Fix regression in reading chunked request bodies The 2 revisions listed above as "new" are entirely new to this repository and will be described in separate emails. The revisions listed as "add" were already present in the repository and have only been added to this reference. Summary of changes: .../coyote/http11/filters/ChunkedInputFilter.java | 10 ++ .../http11/filters/TestChunkedInputFilter.java | 115 +++-- webapps/docs/changelog.xml | 13 ++- 3 files changed, 131 insertions(+), 7 deletions(-) - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
(tomcat) 01/02: Fix typo
This is an automated email from the ASF dual-hosted git repository. markt pushed a commit to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/tomcat.git commit a6d6e3dd7ef0aa624b2427d79a58396ecdaba5e8 Author: Mark Thomas AuthorDate: Tue Aug 13 11:48:43 2024 +0100 Fix typo --- webapps/docs/changelog.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 5ba613df47..01bececc19 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -960,7 +960,7 @@ Make asynchronous error handling more robust. Ensure that once the call to AsyncListener.onError() has returned to the container, only container threads can access the AsyncContext. This -protects against various race conditions that woudl otherwise occur if +protects against various race conditions that would otherwise occur if application threads continued to access the AsyncContext. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
(tomcat) branch 9.0.x updated: Fix regression in reading chunked request bodies
This is an automated email from the ASF dual-hosted git repository. markt pushed a commit to branch 9.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git The following commit(s) were added to refs/heads/9.0.x by this push: new 1d0e79ad72 Fix regression in reading chunked request bodies 1d0e79ad72 is described below commit 1d0e79ad723ec181a3650c5d9c9ad9dd62178333 Author: Mark Thomas AuthorDate: Wed Aug 14 10:53:04 2024 +0100 Fix regression in reading chunked request bodies available() was returning a non-zero value when there was no data to read. In some circumstances this could cause a blocking read to block waiting for more data rather than return the data it had already received. --- .../coyote/http11/filters/ChunkedInputFilter.java | 10 ++ .../http11/filters/TestChunkedInputFilter.java | 115 +++-- webapps/docs/changelog.xml | 11 ++ 3 files changed, 130 insertions(+), 6 deletions(-) diff --git a/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java b/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java index 36389576d4..a967ef65a8 100644 --- a/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java +++ b/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java @@ -198,6 +198,16 @@ public class ChunkedInputFilter implements InputFilter, ApplicationBufferHandler if (readChunk != null) { available = readChunk.remaining(); } + +// Handle some edge cases +if (available == 1 && parseState == ParseState.CHUNK_BODY_CRLF) { + // Either just the CR or just the LF are left in the buffer. There is no data to read. +available = 0; +} else if (available == 2 && !crFound && parseState == ParseState.CHUNK_BODY_CRLF) { + // Just CRLF is left in the buffer. There is no data to read. +available = 0; +} + if (available == 0) { // No data buffered here. Try the next filter in the chain. return buffer.available(); diff --git a/test/org/apache/coyote/http11/filters/TestChunkedInputFilter.java b/test/org/apache/coyote/http11/filters/TestChunkedInputFilter.java index 9a0d41eff6..6f9e906080 100644 --- a/test/org/apache/coyote/http11/filters/TestChunkedInputFilter.java +++ b/test/org/apache/coyote/http11/filters/TestChunkedInputFilter.java @@ -16,9 +16,13 @@ */ package org.apache.coyote.http11.filters; +import java.io.BufferedReader; import java.io.IOException; import java.io.InputStream; +import java.io.InputStreamReader; import java.io.PrintWriter; +import java.nio.charset.StandardCharsets; +import java.util.concurrent.TimeUnit; import javax.servlet.ServletException; import javax.servlet.http.HttpServlet; @@ -36,7 +40,6 @@ import org.apache.catalina.startup.TomcatBaseTest; public class TestChunkedInputFilter extends TomcatBaseTest { -private static final String LF = "\n"; private static final int EXT_SIZE_LIMIT = 10; @Test @@ -117,16 +120,16 @@ public class TestChunkedInputFilter extends TomcatBaseTest { SimpleHttpClient.CRLF + "Connection: close" + SimpleHttpClient.CRLF + SimpleHttpClient.CRLF + -"3" + (chunkHeaderUsesCRLF ? SimpleHttpClient.CRLF : LF) + -"a=0" + (chunkUsesCRLF ? SimpleHttpClient.CRLF : LF) + +"3" + (chunkHeaderUsesCRLF ? SimpleHttpClient.CRLF : SimpleHttpClient.LF) + +"a=0" + (chunkUsesCRLF ? SimpleHttpClient.CRLF : SimpleHttpClient.LF) + "4" + SimpleHttpClient.CRLF + "&b=1" + SimpleHttpClient.CRLF + "0" + SimpleHttpClient.CRLF + "x-trailer1: Test", "Value1" + -(firstheaderUsesCRLF ? SimpleHttpClient.CRLF : LF) + +(firstheaderUsesCRLF ? SimpleHttpClient.CRLF : SimpleHttpClient.LF) + "x-trailer2: TestValue2" + -(secondheaderUsesCRLF ? SimpleHttpClient.CRLF : LF) + -(endUsesCRLF ? SimpleHttpClient.CRLF : LF) }; +(secondheaderUsesCRLF ? SimpleHttpClient.CRLF : SimpleHttpClient.LF) + +(endUsesCRLF ? SimpleHttpClient.CRLF : SimpleHttpClient.LF) }; TrailerClient client = new TrailerClient(tomcat.getConnector().getLocalPort()); @@ -777,4 +780,104 @@ public class TestChunkedInputFilter extends TomcatBaseTest { Assert.assertNull(client.getResponseLine()); } } + + +private static class BodyReadLineServlet extends HttpServlet { +private static final long serialVersionUID = 1L; + +@Override +protected void doPost(HttpServletRequest req, HttpServletResponse resp) +throws ServletException, IOException { + +int lineCount = 0; +int pauseCount = 0; + +// Read the body one line at a time. There should be ~1s between reads. +try (InputStream i
(tomcat) branch 9.0.x updated: Fix typo
This is an automated email from the ASF dual-hosted git repository. markt pushed a commit to branch 9.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git The following commit(s) were added to refs/heads/9.0.x by this push: new 660dfdf2e3 Fix typo 660dfdf2e3 is described below commit 660dfdf2e34287f2a8c7a83fcc815a45190c2f85 Author: Mark Thomas AuthorDate: Tue Aug 13 11:48:43 2024 +0100 Fix typo --- webapps/docs/changelog.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 2f3f85194b..ce55fa406c 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -890,7 +890,7 @@ Make asynchronous error handling more robust. Ensure that once the call to AsyncListener.onError() has returned to the container, only container threads can access the AsyncContext. This -protects against various race conditions that woudl otherwise occur if +protects against various race conditions that would otherwise occur if application threads continued to access the AsyncContext. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Cookie parsing and upcoming updates to RFC6265
Hi all, The IETF HTTP working group is working on RFC 6265bis (the RFC that will replace RFC 6265). I have been reviewing the changes to see what impact they might have on Tomcat and our users. There are a few changes (e.g. SameSite) we have already implemented. There are quite a few changes that I think don't impact us. And then there is this: Cookie: apple Current Tomcat interprets that as name="apple" value="" RFC 6265 says any name-value-pair from a Set-Cookie string without an "=" should be ignored and the Cookie headers should always use = between the name and the value. RFC 6265bis would required name="", value="apple" when using the relaxed (receiver) parsing. The strict (sender) syntax does not allow a cookie without a name. RFC 6265bis does appear to be consistent with browser intention [1] (at least intentions 10 years ago anyway). So we are currently: - accepting a cookie RFC 6265 says we should ignore - interpreting it the opposite way to apparent browser intention - interpreting it the opposite way to likely RFC 6265bis requirements Given the above, I do wonder to what extent applications are actually using these cookies. So, what should we do? I think we need a new configuration option named "noEqualsCookie" (suggestions for a better name welcome) with three options: - ignore - name - value Tomcat 9, 10 & 11 have the default set to name so there is no change. Tomcat 12 has the default set to value. Thoughts? Mark [1] https://lists.apache.org/thread/w2ovto22r4mbvh0o307fvljvbkfsvzb4 - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: Cookie parsing and upcoming updates to RFC6265
ср, 14 авг. 2024 г. в 17:29, Mark Thomas : > > Hi all, > > The IETF HTTP working group is working on RFC 6265bis (the RFC that will > replace RFC 6265). I have been reviewing the changes to see what impact > they might have on Tomcat and our users. Links: https://datatracker.ietf.org/doc/html/rfc6265 https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis/ Generally, I think we can do nothing at our end until that paper is approved. Though now is a good time to file comments (complaints, concerns) to IETF. > There are a few changes (e.g. SameSite) we have already implemented. > > There are quite a few changes that I think don't impact us. > > And then there is this: > > Cookie: apple > > Current Tomcat interprets that as name="apple" value="" > > RFC 6265 says any name-value-pair from a Set-Cookie string without an > "=" should be ignored and the Cookie headers should always use = between > the name and the value. Interesting, if I read 5.2 (That section is about processing Set-Cookie by a User-Agent. I was looking at what browsers do at their end.) it first parses that as empty name and value=apple, and then they have "5. If the name string is empty, ignore the set-cookie-string entirely." In RFC 6265bis this step is removed. I think the following: 1) Generally, when encountering an invalid "apple" string there are the following options: a) error out, e.g. throw IAE, or in some other way resulting in sendError(400) b) ignore / drop it The option of a) in case of cookies would be wrong, as cookies may creep in into user's storage and it makes a site inaccessible. 2) Interpreting that case as name="" value="apple" leaves a place for programmatic processing. It decouples parsing and error handling. E.g. if the HttpServletRequest had a getCookie(name) method (in addition to the existing getCookies()), some "security hardening Filter" could do if (getCookie("") != null) { request.sendError(400); } Using an empty name clearly groups those cookies as some "oddity", and allows one to retrieve them at once. There are places where the spec takes advantage of this to define steps in its algorithms. E.g. the step that I cited above from the current RFC6265 5. If the name string is empty, ignore the set-cookie-string entirely. E.g. in RFC6265bis section "5.7. Storage Model" step 22: https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis/#section-5.7 22. If the cookie-name is empty and either of the following conditions are true, abort these steps and ignore the cookie entirely: * the cookie-value begins with a case-insensitive match for the string "__Secure-" * the cookie-value begins with a case-insensitive match for the string "__Host-" 3) Interpreting that case as name="apple" value="" lacks the advantage of such grouping. (btw That is how cookie attributes are parsed. E.g. "Secure".) 4) I cannot think of a valid use case of handling such cookies. An abstract "Security hardening filter"? Will it block access to the site? Or will it use this feature to "finger-print" users' browsers? Anyway, I think it is a waste of time to support such use cases. (Though we cannot block them, as the same data are available via getHeaders, and parsed manually). I think that 1) We would better switch to "ignore" mode right now, in all supported versions. 2) The "empty name" option seems to be the correct behaviour, But as the majority of web applications would not need this feature, I think that\ "ignore" would better remain the default behaviour. Best regards, Konstantin Kolinko - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 69262] New: Application can fail to start if both jakarta.el::jakarta.el-api and org.apache.tomcat.embed::tomcat-embed-el are on the classpath
https://bz.apache.org/bugzilla/show_bug.cgi?id=69262 Bug ID: 69262 Summary: Application can fail to start if both jakarta.el::jakarta.el-api and org.apache.tomcat.embed::tomcat-embed-el are on the classpath Product: Tomcat 10 Version: 10.1.26 Hardware: All OS: All Status: NEW Severity: normal Priority: P2 Component: EL Assignee: dev@tomcat.apache.org Reporter: skylar.sut...@gmail.com Target Milestone: -- If both the jakarta.el::jakarta.el-api and org.apache.tomcat.embed::tomcat-embed-el JARs are on the classpath, the application can fail to start. Both JARs contain a jakarta.el.ExpressionFactory, and if the jakarta.el-api version gets prioritized higher on the classpath, it will trump the tomcat-embed-el version. This may manifest differently for each application, but in our case it manifested as a "java.lang.ClassNotFoundException: com.sun.el.ExpressionFactoryImpl" at startup. ref: https://github.com/jakartaee/expression-language/blob/master/api/src/main/java/jakarta/el/ExpressionFactory.java ref: https://github.com/apache/tomcat/blob/main/java/jakarta/el/ExpressionFactory.java Given that jakarta.el-api is the API spec and tomcat-embed-el is the implementation, it should be assumed that a) both may be on the classpath and b) that it is safe for both to be on the classpath (e.g. foo-api and foo-impl). In reviewing the jakarta.el-api version of the ExpressionFactory, their documentation outlines a clear workflow of how to tell the Factory what your implementation class is. I belive the tomcat-embed-el ExpressionFactory should NOT be an override of the API class, but rather something like "ExpressionFactoryImpl" which instructions to ExpressionFactory that it should use ExpressionFactoryImpl. An example POM below shows a valid use case that triggered this scenario: a springboot application, running on Tomcat Embed (Jasper), using the JSTL tag reference implementation. The springboot WAR is generated with jakarta.el-api prioritized higher than tomcat-embed-el in the "classpath.idx" file, which determines the classpath order for a springboot application. Adding an for "el-api" to "jstl-api" will resolve the issue and allow the application to start. e.g. jakarta.servlet.jsp.jstl jakarta.servlet.jsp.jstl-api runtime jakarta.el jakarta.el-api Broken Example: http://maven.apache.org/POM/4.0.0"; xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd";> 4.0.0 com.foo bar 1.0.0-SNAPSHOT foo-bar war org.springframework.boot spring-boot-starter-parent 3.3.2 3.2.5 org.springframework.boot spring-boot org.springframework.boot spring-boot-starter-web runtime org.springframework.boot spring-boot-autoconfigure org.apache.tomcat.embed tomcat-embed-core org.apache.tomcat.embed tomcat-embed-jasper jakarta.servlet.jsp.jstl jakarta.servlet.jsp.jstl-api runtime jakarta.el jakarta.el-api org.glassfish.web jakarta.servlet.jsp.jstl runtime org.springframework.boot spring-boot-maven-plugin com.foo.bar.Application WAR -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.ap
[Bug 69262] Application can fail to start if both jakarta.el::jakarta.el-api and org.apache.tomcat.embed::tomcat-embed-el are on the classpath
https://bz.apache.org/bugzilla/show_bug.cgi?id=69262 skylar.sut...@gmail.com changed: What|Removed |Added CC||skylar.sut...@gmail.com -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Make status code configurable [tomcat]
adwsingh commented on PR #723: URL: https://github.com/apache/tomcat/pull/723#issuecomment-2289602433 @markt-asf I think Tomcat reserving existing HTTP 4xx responses solely for its internal use is very restrictive to the application. What do you feel about, adding a new field in the CoyoteResponse called shouldCloseConnection that defaults to true and we set it to false once we are sure we are entering application layer. On how we detect we are entering the application layer is something I would need to deep dive on. -- 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: dev-unsubscr...@tomcat.apache.org 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