Re: TagLibraryInfo final fields
On Tue, May 30, 2017 at 5:34 PM, Mark Thomas <ma...@apache.org> wrote: > On 30/05/17 13:55, Katya Todorova wrote: > > Hello, > > > > Looking at javax.servlet.jsp.tagext.TagLibraryInfo, uri and prefix > fields > > are declared final while according to the spec and tomcat documentation > > they should not be. > > > > Is that intentionally done? > No. It is a bug. Please open a Bugzilla issue for this. > > Mark Here it is: https://bz.apache.org/bugzilla/show_bug.cgi?id=61137 > > > > Thank you, > > Katya > > > > http://svn.apache.org/viewvc/tomcat/trunk/java/javax/servlet/jsp/tagext/ > > TagLibraryInfo.java?view=markup > > https://docs.oracle.com/javaee/7/api/javax/servlet/ > > jsp/tagext/TagLibraryInfo.html > > https://tomcat.apache.org/tomcat-7.0-doc/jspapi/javax/ > servlet/jsp/tagext/TagLibraryInfo.html > > > > > - > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > >
TagLibraryInfo final fields
Hello, Looking at javax.servlet.jsp.tagext.TagLibraryInfo, uri and prefix fields are declared final while according to the spec and tomcat documentation they should not be. Is that intentionally done? Thank you, Katya http://svn.apache.org/viewvc/tomcat/trunk/java/javax/servlet/jsp/tagext/ TagLibraryInfo.java?view=markup https://docs.oracle.com/javaee/7/api/javax/servlet/ jsp/tagext/TagLibraryInfo.html https://tomcat.apache.org/tomcat-7.0-doc/jspapi/javax/servlet/jsp/tagext/TagLibraryInfo.html
Re: Host header validation
On Wed, Apr 5, 2017 at 3:57 PM, Mark Thomas <ma...@apache.org> wrote: > On 05/04/2017 07:50, Katya Todorova wrote: > >> >>> Applied. Many thanks. >>> >>> If you'd like to work on this further then can I suggest you take a look >>> at Konstantin's comments: >>> >>> http://markmail.org/message/vp5voob7elspflax >>> >> >> >> I looked at the comments and it seems there are things to be clarified >> before going in this direction: >> - should we introduce a flag for turn on/off validation and in which cases >> > > Currently, the validation isn't used at the point where the header is > parsed. > > I'd prefer not to add an option to disable this check. It just seems like > the wrong thing to do. > > I'm currently thinking that we could add the validation and log failures > (rather than return a 400 response) warning that a future release will > start rejecting the requests. That should prompt users to contact us with > any false positives. I think logging failure instead of returning 400 at that point is a good approach. How much time do you think would be enough to simply log before enforcing these checks? (asking out of curiosity) Also, would it be possible to have this functionality in tomcat 7/8? > - zone id support in IPv6 addresses >> > > - IPvFuture support (for this one Konstantin has already proposed to be >> postponed for a while) >> > > Looking at the spec, I think we can parse IPvFuture now. We should > probably log any IPvFuture values with a request to report the use case to > us so we can update the parser to handle specific instances rather tan the > general case. > > If you think this is the right time to work on the first two, let me know >> and I can prepare a patch. >> > > I think that would be great. BUT. That isn't my decision to make. One of > the key principles of the Apache Way is that contributors choose what they > want to work on. There isn't a project leader or a management team > assigning tasks. If you want to work on this then absolutely, go ahead. If > there is some other aspect you'd rather be working on then by all means > work on that. > > The Tomcat community is always willing to provide some pointers to > suitable tasks where people new to Tomcat can get started but that > shouldn't be see as assigning areas to work on. Thank you for clarifying the process. I decided to look at these things because they are related to what I've already done and would, in a way, complete the effort around host parsing. I was asking about IPvFuture in particular because it seems the specification will evolve and host parsing code would need to be adapted accordingly, probably incrementally. While the rest of the host definition seems pretty stable and in use. That said, I'm not interested in particular in IPvFuture implementation - I thought it would help me getting to know Tomcat better - if there's anything else that is more important/urgent and is suitable for someone new to Tomcat, please share some pointers. I don't see this as task assignment but more as providing the context/priority information that I currently lack. Kind regards, Katya Other possibilities are: >> >>> >>> - performance improvements for the Host header validation >>> >>> - improving code coverage generally for any of the HTTP parsing code >>> >>> - any that attracts your interest >>> >> >> >> I'm looking at the code coverage and will take a look at host validation >> performance. >> > > Fantastic. (With the caveat of you'd rather do something else then that > would be fantastic too.) > > > Kind regards, > > Mark > > - > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > >
Re: Host header validation
> > Applied. Many thanks. > > If you'd like to work on this further then can I suggest you take a look > at Konstantin's comments: > > http://markmail.org/message/vp5voob7elspflax I looked at the comments and it seems there are things to be clarified before going in this direction: - should we introduce a flag for turn on/off validation and in which cases - zone id support in IPv6 addresses - IPvFuture support (for this one Konstantin has already proposed to be postponed for a while) If you think this is the right time to work on the first two, let me know and I can prepare a patch. Other possibilities are: > > - performance improvements for the Host header validation > > - improving code coverage generally for any of the HTTP parsing code > > - any that attracts your interest I'm looking at the code coverage and will take a look at host validation performance. Kind regards, Katya
Re: Host header validation
> You can either create a pull request on github or create a Bugzilla > issue and attach a patch. > Mark > I've created a separate pull request for leading zeros issue since I think it requires additional discussion whether to be submitted or not. Although this fix honors the specification, it leads to different behavior in case of IPv4 address and IPv6 address with IPv4 part. This may cause confusion: https://github.com/apache/tomcat/pull/49 The rest of identified host parser issues are fixed here: https://github.com/apache/tomcat/pull/48 Kind regards, Katya
Re: Host header validation
> > I recommend using the code coverage reports as a guide. > > https://ci.apache.org/projects/tomcat/tomcat9/coverage/ > > and add test cases if they increase code coverage. Hmm. It looks like > there is some low hanging fruit in the parsing code to improve coverage. > > You can run the tests with code coverage locally too. > Thank you for the guidance, I'll add only these test cases that increases code coverage. > Other than that, the usual advice applies. Test limits, either side of > limits, etc > > Generally, I try and reduce copy and paste as much as possible. @RunWith > has been really good for that. There is probably scope to use it more > throughout the Tomcat tests. > > Kind regards, > > Mark > > > - > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > > Is there an issue tracking this change where I can attach the patch when I'm ready? Or should I do so here? Thank you, Katya
Re: Host header validation
On Tue, Mar 28, 2017 at 5:45 PM, Mark Thomas <ma...@apache.org> wrote: > On 28/03/17 15:23, Katya Todorova wrote: > > Hi, > >> r1787662 adds Host header validation along with a fair number of unit > tests. > >> It includes a performance test which indicates - on my machine at least > >> - that the performance impact is in the noise. I'd like to see better > >> performance for full IPv6 addresses but the current code looks to be > >> acceptable. > >> The validation is not yet integrated into the request processing. My > >> primary reason for not integrating it is that it will trigger a 400 > >> response if the header is invalid and I don't want to incorrectly reject > >> valid headers. Therefore I have a request. Please try and break these > >> new parsers. > > > > > > I’ve looked at the new http host parsers and tried some test data. > > Most of the test cases have already been covered but still several > > issues popped up: > > Thanks for the additional test cases. This is exactly the sort of > feedback I was looking for. > > Would you like to get more involved in Tomcat development? If so, > turning these into a patch for the unit tests could be good place to > start. You'll need to mark the tests with @Ignore for now until the > underlying bugs are fixed. For bonus points, fix the bugs in the parser > so the tests pass. > I'll prepare a patch. Would you prefer to keep the test cases more compact or more extensive? E.g. for testing IPv6 with less components than expected we may have one test case with only 3 components (for example) or we may have many test cases - respectively with one, two, ..., seven components? The latter approach would make the test class a lot bigger but might help to catch some corner cases. Let me know what you think. Katya > Mark
Re: Host header validation
On Tue, Mar 28, 2017 at 5:45 PM, Mark Thomas <ma...@apache.org> wrote: > On 28/03/17 15:23, Katya Todorova wrote: > > Hi, > >> r1787662 adds Host header validation along with a fair number of unit > tests. > >> It includes a performance test which indicates - on my machine at least > >> - that the performance impact is in the noise. I'd like to see better > >> performance for full IPv6 addresses but the current code looks to be > >> acceptable. > >> The validation is not yet integrated into the request processing. My > >> primary reason for not integrating it is that it will trigger a 400 > >> response if the header is invalid and I don't want to incorrectly reject > >> valid headers. Therefore I have a request. Please try and break these > >> new parsers. > > > > > > I’ve looked at the new http host parsers and tried some test data. > > Most of the test cases have already been covered but still several > > issues popped up: > > Thanks for the additional test cases. This is exactly the sort of > feedback I was looking for. > > Would you like to get more involved in Tomcat development? If so, > turning these into a patch for the unit tests could be good place to > start. You'll need to mark the tests with @Ignore for now until the > underlying bugs are fixed. For bonus points, fix the bugs in the parser > so the tests pass. > > Mark > > > > > > - IPv6 addresses containing ::: are considered valid while they should > > not be - e.g . [:::::::::] > > > > (except when “:::” are located in the end , in that case the host is > > rejected as invalid) > > > > - IPv4 part of IPv6 addresses should not contain leading zeros > > according to the following part of the specification: > > > > IPv4address = dec-octet "." dec-octet "." dec-octet "." dec-octet > > > > dec-octet = DIGIT ; 0-9 > > > > / %x31-39 DIGIT ; 10-99 > > > > / "1" 2DIGIT; 100-199 > > > > / "2" %x30-34 DIGIT ; 200-249 > > > > / "25" %x30-35 ; 250-255 > > > > However, whether leading zeros are permitted or not seems to be a > > matter of a recommendation rather than a strict rule. This may lead > > to ambiguity since many sources over the internet consider 01.02.03.04 > > as valid but [::01.02.03.04] as invalid. > > > > - IPv6 Host containing any symbol other than : after ] is considered > > valid though these trailing symbols after the ] are ignored > > > > e.g. [::1]’, [::1] a > > > > - It seems that compression just before the IPv4 part is not handled > correctly. > > > >This one is considered invalid but should be valid > [a:b:c:d:e::1.2.3.4] > > > > Most of the test data has been taken from here: > > > > [1] http://home.deds.nl/~aeron/regex/invalid_ipv6.txt > > [2] http://home.deds.nl/~aeron/regex/valid_ipv6.txt > > > > > >> Please commit any values you test with. > > > > > >> Once we are happy with the quality of these parsers, I'll integrate them > >> into the request processing. > >> Mark > > > > Kind regards, > > > > Katya > > > > > - > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > >
Re: Host header validation
Hi, > r1787662 adds Host header validation along with a fair number of unit tests. > It includes a performance test which indicates - on my machine at least > - that the performance impact is in the noise. I'd like to see better > performance for full IPv6 addresses but the current code looks to be > acceptable. > The validation is not yet integrated into the request processing. My > primary reason for not integrating it is that it will trigger a 400 > response if the header is invalid and I don't want to incorrectly reject > valid headers. Therefore I have a request. Please try and break these > new parsers. I’ve looked at the new http host parsers and tried some test data. Most of the test cases have already been covered but still several issues popped up: - IPv6 addresses containing ::: are considered valid while they should not be - e.g . [:::::::::] (except when “:::” are located in the end , in that case the host is rejected as invalid) - IPv4 part of IPv6 addresses should not contain leading zeros according to the following part of the specification: IPv4address = dec-octet "." dec-octet "." dec-octet "." dec-octet dec-octet = DIGIT ; 0-9 / %x31-39 DIGIT ; 10-99 / "1" 2DIGIT; 100-199 / "2" %x30-34 DIGIT ; 200-249 / "25" %x30-35 ; 250-255 However, whether leading zeros are permitted or not seems to be a matter of a recommendation rather than a strict rule. This may lead to ambiguity since many sources over the internet consider 01.02.03.04 as valid but [::01.02.03.04] as invalid. - IPv6 Host containing any symbol other than : after ] is considered valid though these trailing symbols after the ] are ignored e.g. [::1]’, [::1] a - It seems that compression just before the IPv4 part is not handled correctly. This one is considered invalid but should be valid [a:b:c:d:e::1.2.3.4] Most of the test data has been taken from here: [1] http://home.deds.nl/~aeron/regex/invalid_ipv6.txt [2] http://home.deds.nl/~aeron/regex/valid_ipv6.txt > Please commit any values you test with. > Once we are happy with the quality of these parsers, I'll integrate them > into the request processing. > Mark Kind regards, Katya
