{"thread":{"id":"25113","subject":"[PATCH (bugfix)] gitweb: Add and use prep_attr helper","startedAt":"2010-09-15T20:34:12Z","lastAt":"2010-09-15T21:16:34Z","messageCount":4,"participants":["Jakub Narebski","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"150746","messageId":"201009152234.14253.jnareb@gmail.com","threadId":"25113","inReplyTo":null,"subject":"[PATCH (bugfix)] gitweb: Add and use prep_attr helper","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-15T20:34:12Z","receivedAt":"2010-09-15T20:34:12Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"One of features of CGI.pm module is HTML generation.  The HTML\ngeneration subroutines / methods automatically escape values of\nattributes of HTML elements... but in older versions didn't do it\nfully, not removing / replacing control characters.  Control\ncharacters are forbidden in XML / XHTML, and some browsers\n(e.g. Firefox) do not skip over them but display error instead of page\nin strict XHTML mode (XHTML DTD + application/xml+html mimetype).\n\nThis issue was noticed by Uwe Kleine-König and resolved to be a\nproblem with control characters in XHTML output by Ævar Arnfjörð\nBjarmason in format_search_author() subroutine.\n\n\nIntroduce prep_attr subroutine to deal with this problem, and use it\nin all places where we use data from outside gitweb in attributes of\nHTML elements, where those HTML elements are generated by CGI.pm\nsubroutines / methods (this usually means 'title' attribute).\n\nWhile at it simplify format_search_author() a bit, and use spaces\nfor align.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nThis is a more generic replacement for\n  [PATCH] gitweb: Harden format_search_author()\n  http://thread.gmane.org/gmane.comp.version-control.git/152379/focus=152543\nfrom 2010-08-03\n\n gitweb/gitweb.perl |   62 +++++++++++++++++++++++++++++++---------------------\n 1 files changed, 37 insertions(+), 25 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex a85e2f6..da20209 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1346,6 +1346,22 @@ sub esc_param {\n \treturn $str;\n }\n \n+# prepare 'attribute => value' pair for CGI.pm HTML generation,\n+# by escaping/quoting/replacing/removing values which are invalid in (X)HTML,\n+# and which older versions of CGI.pm do not quote correctly\n+#\n+# USAGE:\n+#   $cgi->a({-href => ..., prep_attr(-title => $title)}, esc_html($contents));\n+#   $cgi->a({-href => ..., -title => prep_attr($title)}, esc_html($contents));\n+sub prep_attr {\n+\tmy ($attr_name, $value) = @_ > 1 ? @_ : (undef, @_);\n+\n+\t$value = to_utf8($value);\n+\t$value =~ s/[[:cntrl:]]/?/g;\n+\n+\treturn defined $attr_name ? ($attr_name => $value) : $value;\n+}\n+\n # quote unsafe chars in whole URL, so some characters cannot be quoted\n sub esc_url {\n \tmy $str = shift;\n@@ -1555,8 +1571,7 @@ sub chop_and_escape_str {\n \tif ($chopped eq $str) {\n \t\treturn esc_html($chopped);\n \t} else {\n-\t\t$str =~ s/[[:cntrl:]]/?/g;\n-\t\treturn $cgi->span({-title=>$str}, esc_html($chopped));\n+\t\treturn $cgi->span({prep_attr(-title=>$str)}, esc_html($chopped));\n \t}\n }\n \n@@ -1778,9 +1793,8 @@ sub format_subject_html {\n \t$extra = '' unless defined($extra);\n \n \tif (length($short) < length($long)) {\n-\t\t$long =~ s/[[:cntrl:]]/?/g;\n \t\treturn $cgi->a({-href => $href, -class => \"list subject\",\n-\t\t                -title => to_utf8($long)},\n+\t\t                prep_attr(-title => $long)},\n \t\t       esc_html($short)) . $extra;\n \t} else {\n \t\treturn $cgi->a({-href => $href, -class => \"list subject\"},\n@@ -1856,23 +1870,20 @@ sub format_search_author {\n \tmy ($author, $searchtype, $displaytext) = @_;\n \tmy $have_search = gitweb_check_feature('search');\n \n-\tif ($have_search) {\n-\t\tmy $performed = \"\";\n-\t\tif ($searchtype eq 'author') {\n-\t\t\t$performed = \"authored\";\n-\t\t} elsif ($searchtype eq 'committer') {\n-\t\t\t$performed = \"committed\";\n-\t\t}\n-\n-\t\treturn $cgi->a({-href => href(action=>\"search\", hash=>$hash,\n-\t\t\t\tsearchtext=>$author,\n-\t\t\t\tsearchtype=>$searchtype), class=>\"list\",\n-\t\t\t\ttitle=>\"Search for commits $performed by $author\"},\n-\t\t\t\t$displaytext);\n+\treturn $displaytext unless ($have_search);\n \n-\t} else {\n-\t\treturn $displaytext;\n+\tmy $performed = \"\";\n+\tif ($searchtype eq 'author') {\n+\t\t$performed = \"authored\";\n+\t} elsif ($searchtype eq 'committer') {\n+\t\t$performed = \"committed\";\n \t}\n+\n+\tmy $title = \"Search for commits $performed by $author\";\n+\treturn $cgi->a({-href => href(action=>\"search\", hash=>$hash,\n+\t                              searchtext=>$author, searchtype=>$searchtype),\n+\t                -class=>\"list\", prep_attr(-title=>$title)},\n+\t               $displaytext);\n }\n \n # format the author name of the given commit with the given tag\n@@ -3559,7 +3570,7 @@ sub git_footer_html {\n \t\tforeach my $format qw(RSS Atom) {\n \t\t\t$href_params{'action'} = lc($format);\n \t\t\tprint $cgi->a({-href => href(%href_params),\n-\t\t\t              -title => \"$href_params{'-title'} $format feed\",\n+\t\t\t              prep_attr(-title => \"$href_params{'-title'} $format feed\"),\n \t\t\t              -class => $feed_class}, $format).\"\\n\";\n \t\t}\n \n@@ -3827,17 +3838,17 @@ sub git_print_page_path {\n \t\t\t$fullname .= ($fullname ? '/' : '') . $dir;\n \t\t\tprint $cgi->a({-href => href(action=>\"tree\", file_name=>$fullname,\n \t\t\t                             hash_base=>$hb),\n-\t\t\t              -title => $fullname}, esc_path($dir));\n+\t\t\t              prep_attr(-title => $fullname)}, esc_path($dir));\n \t\t\tprint \" / \";\n \t\t}\n \t\tif (defined $type && $type eq 'blob') {\n \t\t\tprint $cgi->a({-href => href(action=>\"blob_plain\", file_name=>$file_name,\n \t\t\t                             hash_base=>$hb),\n-\t\t\t              -title => $name}, esc_path($basename));\n+\t\t\t              prep_attr(-title => $name)}, esc_path($basename));\n \t\t} elsif (defined $type && $type eq 'tree') {\n \t\t\tprint $cgi->a({-href => href(action=>\"tree\", file_name=>$file_name,\n \t\t\t                             hash_base=>$hb),\n-\t\t\t              -title => $name}, esc_path($basename));\n+\t\t\t              prep_attr(-title => $name)}, esc_path($basename));\n \t\t\tprint \" / \";\n \t\t} else {\n \t\t\tprint esc_path($basename);\n@@ -3981,7 +3992,8 @@ sub git_print_tree_entry {\n \t\t\t\t\tprint \" -> \" .\n \t\t\t\t\t      $cgi->a({-href => href(action=>\"object\", hash_base=>$hash_base,\n \t\t\t\t\t                             file_name=>$norm_target),\n-\t\t\t\t\t               -title => $norm_target}, esc_path($link_target));\n+\t\t\t\t\t               prep_attr(-title => $norm_target)},\n+\t\t\t\t\t              esc_path($link_target));\n \t\t\t\t} else {\n \t\t\t\t\tprint \" -> \" . esc_path($link_target);\n \t\t\t\t}\n@@ -4687,7 +4699,7 @@ sub git_project_list_body {\n \t\tprint \"<td>\" . $cgi->a({-href => href(project=>$pr->{'path'}, action=>\"summary\"),\n \t\t                        -class => \"list\"}, esc_html($pr->{'path'})) . \"</td>\\n\" .\n \t\t      \"<td>\" . $cgi->a({-href => href(project=>$pr->{'path'}, action=>\"summary\"),\n-\t\t                        -class => \"list\", -title => $pr->{'descr_long'}},\n+\t\t                        -class => \"list\", prep_attr(-title => $pr->{'descr_long'})},\n \t\t                        esc_html($pr->{'descr'})) . \"</td>\\n\" .\n \t\t      \"<td><i>\" . chop_and_escape_str($pr->{'owner'}, 15) . \"</i></td>\\n\";\n \t\tprint \"<td class=\\\"\". age_class($pr->{'age'}) . \"\\\">\" .\n-- \n1.7.2.1\n"},{"id":"150747","messageId":"AANLkTi=o2PFB=JkO5wYHCLAHCO=Ebe63mAc6WB8GfktH@mail.gmail.com","threadId":"25113","inReplyTo":"201009152234.14253.jnareb@gmail.com","subject":"Re: [PATCH (bugfix)] gitweb: Add and use prep_attr helper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-15T20:40:41Z","receivedAt":"2010-09-15T20:40:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"2010/9/15 Jakub Narebski <jnareb@gmail.com>:\n> One of features of CGI.pm module is HTML generation.  The HTML\n> generation subroutines / methods automatically escape values of\n> attributes of HTML elements... but in older versions didn't do it\n> fully, not removing / replacing control characters.  Control\n> characters are forbidden in XML / XHTML, and some browsers\n> (e.g. Firefox) do not skip over them but display error instead of page\n> in strict XHTML mode (XHTML DTD + application/xml+html mimetype).\n>\n> This issue was noticed by Uwe Kleine-König and resolved to be a\n> problem with control characters in XHTML output by Ævar Arnfjörð\n> Bjarmason in format_search_author() subroutine.\n\nFWIW this looks good to me, but perhaps instead of using \"?\" for\neverything in ASCII from \\0 to \" \" it would be better to display the\nhuman-readable escape sequence, like \"\\r\" or \"\\b\".\n\nGitweb already does this (although I don't have a link handy) if you\ncheck in a file with Windows newlines, it'll display \"\\r\" in a little\nbox.\n\nMaybe that should be done everywhere? I don't know.\n"},{"id":"150756","messageId":"7vsk1a684v.fsf@alter.siamese.dyndns.org","threadId":"25113","inReplyTo":"AANLkTi=o2PFB=JkO5wYHCLAHCO=Ebe63mAc6WB8GfktH@mail.gmail.com","subject":"Re: [PATCH (bugfix)] gitweb: Add and use prep_attr helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-09-15T20:53:36Z","receivedAt":"2010-09-15T20:53:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> FWIW this looks good to me, but perhaps instead of using \"?\" for\n> everything in ASCII from \\0 to \" \" it would be better to display the\n> human-readable escape sequence, like \"\\r\" or \"\\b\".\n>\n> Gitweb already does this...\n\nGood point ;-).\n"},{"id":"150759","messageId":"201009152316.36225.jnareb@gmail.com","threadId":"25113","inReplyTo":"AANLkTi=o2PFB=JkO5wYHCLAHCO=Ebe63mAc6WB8GfktH@mail.gmail.com","subject":"Re: [PATCH (bugfix)] gitweb: Add and use prep_attr helper","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-15T21:16:34Z","receivedAt":"2010-09-15T21:16:34Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, 15 Sep 2010, Ævar Arnfjörð Bjarmason wrote:\n> 2010/9/15 Jakub Narebski <jnareb@gmail.com>:\n> > One of features of CGI.pm module is HTML generation.  The HTML\n> > generation subroutines / methods automatically escape values of\n> > attributes of HTML elements... but in older versions didn't do it\n> > fully, not removing / replacing control characters.  Control\n> > characters are forbidden in XML / XHTML, and some browsers\n> > (e.g. Firefox) do not skip over them but display error instead of page\n> > in strict XHTML mode (XHTML DTD + application/xml+html mimetype).\n> >\n> > This issue was noticed by Uwe Kleine-König and resolved to be a\n> > problem with control characters in XHTML output by Ævar Arnfjörð\n> > Bjarmason in format_search_author() subroutine.\n> \n> FWIW this looks good to me, but perhaps instead of using \"?\" for\n> everything in ASCII from \\0 to \" \" it would be better to display the\n> human-readable escape sequence, like \"\\r\" or \"\\b\".\n\nWell, the advantage of this version is that it can be changed in single\nplace, i.e. in prep_attr(), see updated patch below.\n \n> Gitweb already does this (although I don't have a link handy) if you\n> check in a file with Windows newlines, it'll display \"\\r\" in a little\n> box.\n> \n> Maybe that should be done everywhere? I don't know.\n\nNote that we have three different situations:\n1. Output is HTML fragment (esc_html, esc_path)\n2. Output is attribute of HTML element, i.e. text (prep_attr)\n3. Output is HTTP header (in snapshot_name)\n   for proposed filename\n\nBut having the same result for 1 and 2 might be good idea.\n\n-- >8 --\nSubject: [PATCH] gitweb: Add and use prep_attr helper\n\nOne of features of CGI.pm module is HTML generation.  The HTML\ngeneration subroutines / methods automatically escape values of\nattributes of HTML elements... but in older versions didn't do it\nfully, not removing / replacing control characters.  Control\ncharacters are forbidden in XML / XHTML, and some browsers\n(e.g. Firefox) do not skip over them but display error instead of page\nin strict XHTML mode (XHTML DTD + application/xml+html mimetype).\n\nThis issue was noticed by Uwe Kleine-König and resolved to be a\nproblem with control characters in XHTML output by Ævar Arnfjörð\nBjarmason in format_search_author() subroutine.\n\n\nIntroduce prep_attr subroutine to deal with this problem, and use it\nin all places where we use data from outside gitweb in attributes of\nHTML elements, where those HTML elements are generated by CGI.pm\nsubroutines / methods (this usually means 'title' attribute).\n\nWhile at it simplify format_search_author() a bit, and use spaces\nfor align.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nIntediff (indented)\n\n  diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n  index da20209..92a634a 100755\n  --- a/gitweb/gitweb.perl\n  +++ b/gitweb/gitweb.perl\n  @@ -1357,7 +1357,7 @@ sub prep_attr {\n          my ($attr_name, $value) = @_ > 1 ? @_ : (undef, @_);\n   \n          $value = to_utf8($value);\n  -       $value =~ s/[[:cntrl:]]/?/g;\n  +       $value =~ s/([[:cntrl:]])/quot_cec($1, '-nohtml' => 1)/eg;\n   \n          return defined $attr_name ? ($attr_name => $value) : $value;\n   }\n\n\n gitweb/gitweb.perl |   62 +++++++++++++++++++++++++++++++---------------------\n 1 files changed, 37 insertions(+), 25 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex a85e2f6..92a634a 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1346,6 +1346,22 @@ sub esc_param {\n \treturn $str;\n }\n \n+# prepare 'attribute => value' pair for CGI.pm HTML generation,\n+# by escaping/quoting/replacing/removing values which are invalid in (X)HTML,\n+# and which older versions of CGI.pm do not quote correctly\n+#\n+# USAGE:\n+#   $cgi->a({-href => ..., prep_attr(-title => $title)}, esc_html($contents));\n+#   $cgi->a({-href => ..., -title => prep_attr($title)}, esc_html($contents));\n+sub prep_attr {\n+\tmy ($attr_name, $value) = @_ > 1 ? @_ : (undef, @_);\n+\n+\t$value = to_utf8($value);\n+\t$value =~ s/([[:cntrl:]])/quot_cec($1, '-nohtml' => 1)/eg;\n+\n+\treturn defined $attr_name ? ($attr_name => $value) : $value;\n+}\n+\n # quote unsafe chars in whole URL, so some characters cannot be quoted\n sub esc_url {\n \tmy $str = shift;\n@@ -1555,8 +1571,7 @@ sub chop_and_escape_str {\n \tif ($chopped eq $str) {\n \t\treturn esc_html($chopped);\n \t} else {\n-\t\t$str =~ s/[[:cntrl:]]/?/g;\n-\t\treturn $cgi->span({-title=>$str}, esc_html($chopped));\n+\t\treturn $cgi->span({prep_attr(-title=>$str)}, esc_html($chopped));\n \t}\n }\n \n@@ -1778,9 +1793,8 @@ sub format_subject_html {\n \t$extra = '' unless defined($extra);\n \n \tif (length($short) < length($long)) {\n-\t\t$long =~ s/[[:cntrl:]]/?/g;\n \t\treturn $cgi->a({-href => $href, -class => \"list subject\",\n-\t\t                -title => to_utf8($long)},\n+\t\t                prep_attr(-title => $long)},\n \t\t       esc_html($short)) . $extra;\n \t} else {\n \t\treturn $cgi->a({-href => $href, -class => \"list subject\"},\n@@ -1856,23 +1870,20 @@ sub format_search_author {\n \tmy ($author, $searchtype, $displaytext) = @_;\n \tmy $have_search = gitweb_check_feature('search');\n \n-\tif ($have_search) {\n-\t\tmy $performed = \"\";\n-\t\tif ($searchtype eq 'author') {\n-\t\t\t$performed = \"authored\";\n-\t\t} elsif ($searchtype eq 'committer') {\n-\t\t\t$performed = \"committed\";\n-\t\t}\n-\n-\t\treturn $cgi->a({-href => href(action=>\"search\", hash=>$hash,\n-\t\t\t\tsearchtext=>$author,\n-\t\t\t\tsearchtype=>$searchtype), class=>\"list\",\n-\t\t\t\ttitle=>\"Search for commits $performed by $author\"},\n-\t\t\t\t$displaytext);\n+\treturn $displaytext unless ($have_search);\n \n-\t} else {\n-\t\treturn $displaytext;\n+\tmy $performed = \"\";\n+\tif ($searchtype eq 'author') {\n+\t\t$performed = \"authored\";\n+\t} elsif ($searchtype eq 'committer') {\n+\t\t$performed = \"committed\";\n \t}\n+\n+\tmy $title = \"Search for commits $performed by $author\";\n+\treturn $cgi->a({-href => href(action=>\"search\", hash=>$hash,\n+\t                              searchtext=>$author, searchtype=>$searchtype),\n+\t                -class=>\"list\", prep_attr(-title=>$title)},\n+\t               $displaytext);\n }\n \n # format the author name of the given commit with the given tag\n@@ -3559,7 +3570,7 @@ sub git_footer_html {\n \t\tforeach my $format qw(RSS Atom) {\n \t\t\t$href_params{'action'} = lc($format);\n \t\t\tprint $cgi->a({-href => href(%href_params),\n-\t\t\t              -title => \"$href_params{'-title'} $format feed\",\n+\t\t\t              prep_attr(-title => \"$href_params{'-title'} $format feed\"),\n \t\t\t              -class => $feed_class}, $format).\"\\n\";\n \t\t}\n \n@@ -3827,17 +3838,17 @@ sub git_print_page_path {\n \t\t\t$fullname .= ($fullname ? '/' : '') . $dir;\n \t\t\tprint $cgi->a({-href => href(action=>\"tree\", file_name=>$fullname,\n \t\t\t                             hash_base=>$hb),\n-\t\t\t              -title => $fullname}, esc_path($dir));\n+\t\t\t              prep_attr(-title => $fullname)}, esc_path($dir));\n \t\t\tprint \" / \";\n \t\t}\n \t\tif (defined $type && $type eq 'blob') {\n \t\t\tprint $cgi->a({-href => href(action=>\"blob_plain\", file_name=>$file_name,\n \t\t\t                             hash_base=>$hb),\n-\t\t\t              -title => $name}, esc_path($basename));\n+\t\t\t              prep_attr(-title => $name)}, esc_path($basename));\n \t\t} elsif (defined $type && $type eq 'tree') {\n \t\t\tprint $cgi->a({-href => href(action=>\"tree\", file_name=>$file_name,\n \t\t\t                             hash_base=>$hb),\n-\t\t\t              -title => $name}, esc_path($basename));\n+\t\t\t              prep_attr(-title => $name)}, esc_path($basename));\n \t\t\tprint \" / \";\n \t\t} else {\n \t\t\tprint esc_path($basename);\n@@ -3981,7 +3992,8 @@ sub git_print_tree_entry {\n \t\t\t\t\tprint \" -> \" .\n \t\t\t\t\t      $cgi->a({-href => href(action=>\"object\", hash_base=>$hash_base,\n \t\t\t\t\t                             file_name=>$norm_target),\n-\t\t\t\t\t               -title => $norm_target}, esc_path($link_target));\n+\t\t\t\t\t               prep_attr(-title => $norm_target)},\n+\t\t\t\t\t              esc_path($link_target));\n \t\t\t\t} else {\n \t\t\t\t\tprint \" -> \" . esc_path($link_target);\n \t\t\t\t}\n@@ -4687,7 +4699,7 @@ sub git_project_list_body {\n \t\tprint \"<td>\" . $cgi->a({-href => href(project=>$pr->{'path'}, action=>\"summary\"),\n \t\t                        -class => \"list\"}, esc_html($pr->{'path'})) . \"</td>\\n\" .\n \t\t      \"<td>\" . $cgi->a({-href => href(project=>$pr->{'path'}, action=>\"summary\"),\n-\t\t                        -class => \"list\", -title => $pr->{'descr_long'}},\n+\t\t                        -class => \"list\", prep_attr(-title => $pr->{'descr_long'})},\n \t\t                        esc_html($pr->{'descr'})) . \"</td>\\n\" .\n \t\t      \"<td><i>\" . chop_and_escape_str($pr->{'owner'}, 15) . \"</i></td>\\n\";\n \t\tprint \"<td class=\\\"\". age_class($pr->{'age'}) . \"\\\">\" .\n-- \n1.7.2.1\n"}]}