You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Konstantin Kolinko <kn...@gmail.com> on 2009/12/20 05:10:09 UTC
Re: svn commit: r892293 - in /tomcat/trunk/java: javax/servlet/http/
org/apache/catalina/authenticator/ org/apache/catalina/core/
org/apache/catalina/realm/ org/apache/catalina/valves/ org/apache/jasper/resources/
2009/12/18 <ma...@apache.org>:
> Author: markt
> Date: Fri Dec 18 16:13:28 2009
> New Revision: 892293
>
> URL: http://svn.apache.org/viewvc?rev=892293&view=rev
> Log:
> Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=47963
> Anything that could end up in an HTTP header must meet the requirements of RFC2616.
I think this approach to fixing this issue is wrong.
1. It is not these messages but the ones in
org/apache/tomcat/util/http/res/LocalStrings*.properties
that should adhere to RFC2616.
You should add that compliance comment to those files.
HttpMessages.getMessage(int) returns those strings.
2. There are plenty of 3-rd party libraries that call sendError(code.
message) and those won't be fixed by this patch.
One example that I saw was sendError(code. exception.getMessage()).
3. The result of this patch will be that error messages on the Tomcat
error pages for Japanese language will not be translated. (Or for
Russian language, if I ever propose it). I am not happy with it.
Especially these two messages, as error page 503 is hard to customize:
+# Note: The following value may be used in HTTP headers and as such may only
+# contain TEXT as defined by RFC 2616. Since Japanese language messages
+# do not meet this requirement, English text is used.
+applicationDispatcher.isUnavailable=Servlet {0} is currently unavailable
+# Note: The following value may be used in HTTP headers and as such may only
+# contain TEXT as defined by RFC 2616. Since Japanese language messages
+# do not meet this requirement, English text is used.
+standardContext.isUnavailable=This application is not currently available
4. Regarding alternative approaches.
We already do scanning and replacing '\n' and '\r's in the message. E.g.
write(message.replace('\n', ' ').replace('\r', ' '));
Maybe we can replace this with a custom function that performs safeguarding?
What approach would be better:
- For non-ISO8859-1 characters (code > 255):
a) replace non-ISO8859-1 characters with '?'
b) replace non-ISO8859-1 characters with space
c) throw an exception and fallback to the default message.
d) provide an option, to choose a) vs. c) vs. current legacy behavior
- For ASCII characters with codes 0..32, 127:
replace with spaces
All the above makes sense only if USE_CUSTOM_STATUS_MSG_IN_HEADER
option was explicitly set to true. Otherwise the
HttpMessages.getMessage(int) message is used and no safeguarding takes
place.
So it was user's choice that they need this feature, and are ready to
pay their performance overhead for safeguarding.
I am in favor of a) option.
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r892293 - in /tomcat/trunk/java: javax/servlet/http/
org/apache/catalina/authenticator/ org/apache/catalina/core/
org/apache/catalina/realm/ org/apache/catalina/valves/ org/apache/jasper/resources/
Posted by Konstantin Kolinko <kn...@gmail.com>.
2009/12/20 Mark Thomas <ma...@apache.org>:
>
> I don't view an HTTP header where the message is a long string of ????
> characters as better. In that case I think the use English text is a
> better option. Neither do I think we want too many configuration options
> here.
>
> What do you think to the following as an approach:
>
> 1. Revert r892293
> 2. Write an isSafeForHttpHeader() function
> 3. Replace all instances of
> if (org.apache.coyote.Constants.USE_CUSTOM_STATUS_MSG_IN_HEADER) {
> with
> if (org.apache.coyote.Constants.USE_CUSTOM_STATUS_MSG_IN_HEADER &&
> isSafeForHttpHeader(response.getMessage())) {
>
> That would allow i18n to be used in the body of the response and in the
> HTTP headers if it were safe. If it were not safe, the default would be
> used. The user any pays the performance hit of the isSafeForHttpHeader()
> test if they elect to use custom messages.
>
> Mark
I like it.
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r892293 - in /tomcat/trunk/java: javax/servlet/http/
org/apache/catalina/authenticator/ org/apache/catalina/core/ org/apache/catalina/realm/
org/apache/catalina/valves/ org/apache/jasper/resources/
Posted by Mark Thomas <ma...@apache.org>.
On 20/12/2009 04:10, Konstantin Kolinko wrote:
> 2009/12/18 <ma...@apache.org>:
>> Author: markt
>> Date: Fri Dec 18 16:13:28 2009
>> New Revision: 892293
>>
>> URL: http://svn.apache.org/viewvc?rev=892293&view=rev
>> Log:
>> Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=47963
>> Anything that could end up in an HTTP header must meet the requirements of RFC2616.
>
> I think this approach to fixing this issue is wrong.
>
> 1. It is not these messages
This I disagree with. The messages can end up being used in HTTP headers
and some of them used to violate RFC2616. That needs fixing. I agree
that there are other ways we could fix this.
> but the ones in
> org/apache/tomcat/util/http/res/LocalStrings*.properties
> that should adhere to RFC2616.
> You should add that compliance comment to those files.
Agreed.
> 2. There are plenty of 3-rd party libraries that call sendError(code.
> message) and those won't be fixed by this patch.
Not our problem although if whatever we end up implementing works around
them great. If our solution doesn't work around broken libraries I am
fine with that too.
> 3. The result of this patch will be that error messages on the Tomcat
> error pages for Japanese language will not be translated.
Correct. Although I note that a number of messages would have been in
English before this patch anyway.
> (Or for
> Russian language, if I ever propose it). I am not happy with it.
>
> Especially these two messages, as error page 503 is hard to customize:
>
> +# Note: The following value may be used in HTTP headers and as such may only
> +# contain TEXT as defined by RFC 2616. Since Japanese language messages
> +# do not meet this requirement, English text is used.
> +applicationDispatcher.isUnavailable=Servlet {0} is currently unavailable
>
> +# Note: The following value may be used in HTTP headers and as such may only
> +# contain TEXT as defined by RFC 2616. Since Japanese language messages
> +# do not meet this requirement, English text is used.
> +standardContext.isUnavailable=This application is not currently available
>
> 4. Regarding alternative approaches.
>
> We already do scanning and replacing '\n' and '\r's in the message. E.g.
> write(message.replace('\n', ' ').replace('\r', ' '));
>
> Maybe we can replace this with a custom function that performs safeguarding?
Anything is possible ;)
The fundamental issue is that the error message on the error page comes
from response.getMessage() and we use the same text in the HTTP header.
The first should use i18n, the second must comply with RFC2616. Those
requirements are not compatible.
> What approach would be better:
> - For non-ISO8859-1 characters (code > 255):
> a) replace non-ISO8859-1 characters with '?'
> b) replace non-ISO8859-1 characters with space
> c) throw an exception and fallback to the default message.
> d) provide an option, to choose a) vs. c) vs. current legacy behavior
>
> - For ASCII characters with codes 0..32, 127:
> replace with spaces
>
> All the above makes sense only if USE_CUSTOM_STATUS_MSG_IN_HEADER
> option was explicitly set to true. Otherwise the
> HttpMessages.getMessage(int) message is used and no safeguarding takes
> place.
> So it was user's choice that they need this feature, and are ready to
> pay their performance overhead for safeguarding.
The user may not be expecting the performance hit. That would need
adding to the docs.
> I am in favor of a) option.
I don't view an HTTP header where the message is a long string of ????
characters as better. In that case I think the use English text is a
better option. Neither do I think we want too many configuration options
here.
What do you think to the following as an approach:
1. Revert r892293
2. Write an isSafeForHttpHeader() function
3. Replace all instances of
if (org.apache.coyote.Constants.USE_CUSTOM_STATUS_MSG_IN_HEADER) {
with
if (org.apache.coyote.Constants.USE_CUSTOM_STATUS_MSG_IN_HEADER &&
isSafeForHttpHeader(response.getMessage())) {
That would allow i18n to be used in the body of the response and in the
HTTP headers if it were safe. If it were not safe, the default would be
used. The user any pays the performance hit of the isSafeForHttpHeader()
test if they elect to use custom messages.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org