{"thread":{"id":"22560","subject":"[PATCH 0/2] gitweb: Improve escaping","startedAt":"2010-02-07T20:47:50Z","lastAt":"2010-02-17T16:26:53Z","messageCount":5,"participants":["Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"133887","messageId":"20100207204539.11146.36972.stgit@localhost.localdomain","threadId":"22560","inReplyTo":null,"subject":"[PATCH 0/2] gitweb: Improve escaping","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-02-07T20:47:50Z","receivedAt":"2010-02-07T20:47:50Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Those two patches are dependent on each other only because of single\nchange in second patch, showcasing the feature.\n---\nJakub Narebski (2):\n      gitweb: esc_html (short) error message in die_error\n      gitweb: Protect escaping functions against calling on undef\n\n gitweb/gitweb.perl |   11 +++++++++--\n 1 files changed, 9 insertions(+), 2 deletions(-)\n\n-- \nJakub Narebski\nShadeHawk on #git\nPoland\n"},{"id":"133888","messageId":"20100207204803.11146.57731.stgit@localhost.localdomain","threadId":"22560","inReplyTo":"20100207204539.11146.36972.stgit@localhost.localdomain","subject":"[PATCH 1/2] gitweb: esc_html (short) error message in die_error","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-02-07T20:51:18Z","receivedAt":"2010-02-07T20:51:18Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"The error message (second argument to die_error) is meant to be short,\none-line text description of given error.  A few callers call\ndie_error with error message containing unescaped user supplied data\n($hash, $file_name).  Instead of forcing callers to escape data,\nsimply call esc_html on the parameter.\n\nNote that optional third parameter, which contains detailed error\ndescription, is meant to be HTML formatted, and therefore should be\nnot escaped.\n\nWhile at it update esc_html synopsis/usage, and bring default error\ndescription to read 'Internal Server Error' (titlecased).\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nAlternate solution would be to do escaping in call sites for die_error.\n\nBut in error messages shown on 'die' via CGI::Carp qw(fatalsToBrowser)\nthe error message is HTML escaped, so in fact we are following here\nthis \"calling convention\".\n\nNote that for any patch relied on 'Internal server error' as error\nmessage, it must be changed to not fail wrongly.\n\n gitweb/gitweb.perl |    4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex a5bc359..e393f65 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3374,7 +3374,7 @@ sub git_footer_html {\n \t      \"</html>\";\n }\n \n-# die_error(<http_status_code>, <error_message>)\n+# die_error(<http_status_code>, <error_message>[, <detailed_html_description>])\n # Example: die_error(404, 'Hash not found')\n # By convention, use the following status codes (as defined in RFC 2616):\n # 400: Invalid or missing CGI parameters, or\n@@ -3389,7 +3389,7 @@ sub git_footer_html {\n #      or down for maintenance).  Generally, this is a temporary state.\n sub die_error {\n \tmy $status = shift || 500;\n-\tmy $error = shift || \"Internal server error\";\n+\tmy $error = esc_html(shift || \"Internal Server Error\");\n \tmy $extra = shift;\n \n \tmy %http_responses = (\n"},{"id":"133889","messageId":"20100207205130.11146.30664.stgit@localhost.localdomain","threadId":"22560","inReplyTo":"20100207204539.11146.36972.stgit@localhost.localdomain","subject":"[PATCH 2/2] gitweb: Protect escaping functions against calling on undef","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-02-07T20:52:25Z","receivedAt":"2010-02-07T20:52:25Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"This is a bit of future-proofing esc_html and friends: when called\nwith undefined value they would now would return undef... which would\nprobably mean that error would still occur, but closer to the source\nof problem.\n\nThis means that we can safely use\n  esc_html(shift) || \"Internal Server Error\"\nin die_error() instead of\n  esc_html(shift || \"Internal Server Error\")\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nWithout the second part (i.e. the change in die_error) those two\npatches would be totally independent.\n\n gitweb/gitweb.perl |    9 ++++++++-\n 1 files changed, 8 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex e393f65..c10967c 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1145,6 +1145,7 @@ sub validate_refname {\n # in utf-8 thanks to \"binmode STDOUT, ':utf8'\" at beginning\n sub to_utf8 {\n \tmy $str = shift;\n+\treturn undef unless defined $str;\n \tif (utf8::valid($str)) {\n \t\tutf8::decode($str);\n \t\treturn $str;\n@@ -1157,6 +1158,7 @@ sub to_utf8 {\n # correct, but quoted slashes look too horrible in bookmarks\n sub esc_param {\n \tmy $str = shift;\n+\treturn undef unless defined $str;\n \t$str =~ s/([^A-Za-z0-9\\-_.~()\\/:@ ]+)/CGI::escape($1)/eg;\n \t$str =~ s/ /\\+/g;\n \treturn $str;\n@@ -1165,6 +1167,7 @@ sub esc_param {\n # quote unsafe chars in whole URL, so some charactrs cannot be quoted\n sub esc_url {\n \tmy $str = shift;\n+\treturn undef unless defined $str;\n \t$str =~ s/([^A-Za-z0-9\\-_.~();\\/;?:@&=])/sprintf(\"%%%02X\", ord($1))/eg;\n \t$str =~ s/\\+/%2B/g;\n \t$str =~ s/ /\\+/g;\n@@ -1176,6 +1179,8 @@ sub esc_html {\n \tmy $str = shift;\n \tmy %opts = @_;\n \n+\treturn undef unless defined $str;\n+\n \t$str = to_utf8($str);\n \t$str = $cgi->escapeHTML($str);\n \tif ($opts{'-nbsp'}) {\n@@ -1190,6 +1195,8 @@ sub esc_path {\n \tmy $str = shift;\n \tmy %opts = @_;\n \n+\treturn undef unless defined $str;\n+\n \t$str = to_utf8($str);\n \t$str = $cgi->escapeHTML($str);\n \tif ($opts{'-nbsp'}) {\n@@ -3389,7 +3396,7 @@ sub git_footer_html {\n #      or down for maintenance).  Generally, this is a temporary state.\n sub die_error {\n \tmy $status = shift || 500;\n-\tmy $error = esc_html(shift || \"Internal Server Error\");\n+\tmy $error = esc_html(shift) || \"Internal Server Error\";\n \tmy $extra = shift;\n \n \tmy %http_responses = (\n"},{"id":"134867","messageId":"201002171721.17339.jnareb@gmail.com","threadId":"22560","inReplyTo":"20100207204803.11146.57731.stgit@localhost.localdomain","subject":"Re: [PATCH 1/2] gitweb: esc_html (short) error message in die_error","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-02-17T16:21:16Z","receivedAt":"2010-02-17T16:21:16Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 7 Feb 2010, Jakub Narebski wrote:\n\n> The error message (second argument to die_error) is meant to be short,\n> one-line text description of given error.  A few callers call\n> die_error with error message containing unescaped user supplied data\n> ($hash, $file_name).  Instead of forcing callers to escape data,\n> simply call esc_html on the parameter.\n> \n> Note that optional third parameter, which contains detailed error\n> description, is meant to be HTML formatted, and therefore should be\n> not escaped.\n> \n> While at it update esc_html synopsis/usage, and bring default error\n> description to read 'Internal Server Error' (titlecased).\n> \n> Signed-off-by: Jakub Narebski <jnareb@gmail.com>\n> ---\n> Alternate solution would be to do escaping in call sites for die_error.\n> \n> But in error messages shown on 'die' via CGI::Carp qw(fatalsToBrowser)\n> the error message is HTML escaped, so in fact we are following here\n> this \"calling convention\".\n> \n> Note that for any patch relied on 'Internal server error' as error\n> message, it must be changed to not fail wrongly.\n\nPing!\n\nOn one hand it introduces different treatment for second parameter to\ndie_error (error message), which is now being HTML-escaped, and optional\nthird parameter to die_error (detailed description), which is taken as\nformatted and not escaped.  On the other hand this is simple solution\nnot requiring changes in callsites, and allowing easy extending error\nmessages with relevant information.\n\nAnother related issue is how detailed should be our one-line error\nmessages, and what data should they include.  User provided input\n($file_name, $hash, etc.) should be safe if HTML-escaped.  But what\nabout adding errno message: \"$!\"?  It can help in debugging the\nproblem, but theoretically it exposes information to a possible\nattacker...\n\nWhat do you think about it?\n-- \nJakub Narebski\nPoland\n"},{"id":"134868","messageId":"201002171726.53418.jnareb@gmail.com","threadId":"22560","inReplyTo":"20100207205130.11146.30664.stgit@localhost.localdomain","subject":"Re: [PATCH 2/2] gitweb: Protect escaping functions against calling on undef","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-02-17T16:26:53Z","receivedAt":"2010-02-17T16:26:53Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 7 Feb 2010, Jakub Narebski wrote:\n\n> This is a bit of future-proofing esc_html and friends: when called\n> with undefined value they would now would return undef... which would\n> probably mean that error would still occur, but closer to the source\n> of problem.\n> \n> This means that we can safely use\n>   esc_html(shift) || \"Internal Server Error\"\n> in die_error() instead of\n>   esc_html(shift || \"Internal Server Error\")\n> \n> Signed-off-by: Jakub Narebski <jnareb@gmail.com>\n> ---\n> Without the second part (i.e. the change in die_error) those two\n> patches would be totally independent.\n\nPing!\n\nAll but last chunk (last chunk depends on PATCH 1/2) are a bit of\nhardening / defensive programming... although currently not necessary\n(gitweb doesn't pass undef to its escaping subroutines... I think).\n\n-- \nJakub Narebski\nPoland\n"}]}