Mark,
On 3/22/23 07:38, Mark Thomas wrote:
Any more thoughts on this?
There hasn't been much movement from the spec EG on this, so my current
thinking is to revert this change for 10.1.x and earlier to wait and see
what the Servlet EG decides.
I'd like to leave our changes in, but I understand that Konstantin has a
good point about silently discarding parameters.
There is no particular reason not to implement option (c) (throw
RuntimeException if the maximum number of parameters is exceeded).
Anyone affected by it can change the setting, and an appropriate error
message can direct operators to that setting to make it easy.
-chris
On 15/03/2023 15:05, Mark Thomas wrote:
On 15/03/2023 11:22, Konstantin Kolinko wrote:
ср, 15 мар. 2023 г. в 13:29, Konstantin Kolinko
<knst.koli...@gmail.com>:
ср, 15 мар. 2023 г. в 13:15, Konstantin Kolinko
<knst.koli...@gmail.com>:
ср, 15 мар. 2023 г. в 12:07, Mark Thomas <ma...@apache.org>:
On 14/03/2023 21:13, Christopher Schultz wrote:
On 3/14/23 13:57, Mark Thomas wrote:
On 09/03/2023 14:23, Christopher Schultz wrote:
I would go for a 1000 limit for all currently-supported versions.
It's
*very* easy to raise the limit if it interferes with a specific
application's functions.
I *would* add an entry in the "notable changes" for each release
e.g.
https://tomcat.apache.org/migration-10.1.html#Tomcat_10.1.x_noteable_changes
Makes sense.
I'll do that.
-1 unless the behaviour of "silently dropping extra parameters" is
changed as well.
Silent loss of data is not what I want to see in production.
Fair point. Although I'll note that that is exactly what happens if
the current limit is exceeded. I accept that, by lowering the limit,
it is now more likely that limit will be exceeded. How much more
likely I don't know and I don't think we have any reasonable way to
determine.
Also, the failure isn't completely silent. There will be an INFO log
message the first time it happens in a 24 hour period.
<snip/>
Proposals:
1. I think that maxParameterCount would better be configured
per-Context.
The count of parameters is a property of a specific web application.
Makes sense. As an migration path for 10.1.x, 9.0.x and 8.5.x, do we
want to make the Connector attribute the default to be used if a value
is not explicitly set on the Context? That makes the new feature
backwards compatible. We can remove the Connector setting in 11.0.x.
2. I wonder if we can make handling of the errors configurable.
I think that the following options are possible:
a) Drop parameters that exceeded the limit, or failed to decode.
This is what we do now.
b) If there is any error, ignore all parameters and behave as if none
were provided.
I'm wary of doing anything that will cause currently working
applications to start breaking.
c) Blow up by throwing a RuntimeException for any call to
Request.getParameter() methods.
It may be an IllegalStateException.
This topic (error handling in parameters) is currently under
discussion in the Servlet EG
(https://github.com/jakartaee/servlet/issues/431). That discussion
isn't particularly active but it is one of the current servlet issues
on my TODO list so there will hopefully be some progress.
My first thought was to go with c). I know that it contradicts with
Servlet API JavaDoc, but if it is configurable then it is a possible
option. I suppose that a web application should have error handling
configured and should be able to deal with errors.
If we go with c), it requires adding try/catch to safeguard
getParameter() calls in the following classes of Tomcat:
- org.apache.catalina.filters.FailedRequestFilter
- org.apache.catalina.valves.ExtendedAccessLogValve
(The ExtendedAccessLogValve can be configured to log the value of a
parameter.)
3. I propose to change the default behaviour to b), "ignoring all
parameters".
The loss of data will be clearly visible to the applications. It would
not go unnoted.
In an ideal world, the Servlet spec would have opted for c) from the
start.
I wonder if it might not be better to revert this change for 10.1.x
and earlier until the Servlet EG resolves #431 and then reconsider our
options with (potentially) a new default behaviour in 11.0.x.
If we don't revert then, of the current options:
My concern with both b) and c) is that they could break applications
that currently work. I don't like doing that if we don't have to in a
point release.
That leaves a). My main concern with a) is how to raise visibility of
exceeding the limit. What if we changed the way UserDataHelper works
(or introduced something new) that limited the number of log messages
per period and thereby avoided the DoS risk via excessive logging but
still generated enough log messages to raise awareness of the issue.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
