{"thread":{"id":"21229","subject":"[PATCH] gitweb: fix esc_param","startedAt":"2009-10-13T19:51:36Z","lastAt":"2009-10-14T09:13:58Z","messageCount":7,"participants":["Giuseppe Bilotta","Stephen Boyd","Jakub Narebski","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"124879","messageId":"1255463496-21617-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"21229","inReplyTo":null,"subject":"[PATCH] gitweb: fix esc_param","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-10-13T19:51:36Z","receivedAt":"2009-10-13T19:51:36Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"The custom CGI escaping done in esc_param failed to escape UTF-8\nproperly. Fix by using CGI::escape on each sequence of matched\ncharacters instead of sprintf()ing a custom escaping for each byte.\n\nAdditionally, the space -> + escape was being escaped due to greedy\nmatching on the first substitution. Fix by adding space to the\nlist of characters not handled on the first substitution.\n\nFinally, remove an unnecessary escaping of the + sign.\n---\n gitweb/gitweb.perl | 3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\nThe issues with this routine were exposed by Stephen's\n\"author as search link\" patch. This should fix them.\n\nSince the idea of esc_param is to replicate CGI::escape except for the /\ncharacter (if I read the comment correclty), a possible alternative\nwould be to just use CGI::escape on the whole string and then undo the\nescaping for the / character.\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 6237865..6593e5c 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1115,8 +1115,7 @@ sub to_utf8 {\n # correct, but quoted slashes look too horrible in bookmarks\n sub esc_param {\n \tmy $str = shift;\n-\t$str =~ s/([^A-Za-z0-9\\-_.~()\\/:@])/sprintf(\"%%%02X\", ord($1))/eg;\n-\t$str =~ s/\\+/%2B/g;\n+\t$str =~ s/([^A-Za-z0-9\\-_.~()\\/:@ ]+)/CGI::escape($1)/eg;\n \t$str =~ s/ /\\+/g;\n \treturn $str;\n }\n-- \n1.6.3.rc1.192.gdbfcb\n"},{"id":"124917","messageId":"4AD525CA.8090102@gmail.com","threadId":"21229","inReplyTo":"1255463496-21617-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH] gitweb: fix esc_param","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2009-10-14T01:13:46Z","receivedAt":"2009-10-14T01:13:46Z","isPatch":true,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"Giuseppe Bilotta wrote:\n> The custom CGI escaping done in esc_param failed to escape UTF-8\n> properly. Fix by using CGI::escape on each sequence of matched\n> characters instead of sprintf()ing a custom escaping for each byte.\n>\n> Additionally, the space -> + escape was being escaped due to greedy\n> matching on the first substitution. Fix by adding space to the\n> list of characters not handled on the first substitution.\n>\n> Finally, remove an unnecessary escaping of the + sign.\n>\n\nSignoff?\n\nThis works great for my purposes. Thanks.\n"},{"id":"124940","messageId":"cb7bb73a0910132319p27b4f362sf5bffa2afe6e5849@mail.gmail.com","threadId":"21229","inReplyTo":"4AD525CA.8090102@gmail.com","subject":"Re: [PATCH] gitweb: fix esc_param","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-10-14T06:19:22Z","receivedAt":"2009-10-14T06:19:22Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Wed, Oct 14, 2009 at 3:13 AM, Stephen Boyd wrote:\n> Giuseppe Bilotta wrote:\n>> The custom CGI escaping done in esc_param failed to escape UTF-8\n>> properly. Fix by using CGI::escape on each sequence of matched\n>> characters instead of sprintf()ing a custom escaping for each byte.\n>>\n>> Additionally, the space -> + escape was being escaped due to greedy\n>> matching on the first substitution. Fix by adding space to the\n>> list of characters not handled on the first substitution.\n>>\n>> Finally, remove an unnecessary escaping of the + sign.\n>>\n>\n> Signoff?\n\nDoh.\n\nSigned-off-by: Giuseppe Bilotta \n\n(If this isn't enough, let me know)\n\n> This works great for my purposes. Thanks.\n\nCan you also check if this fixes the branch name issue you mentioned\nin the other branch? (And/or do you have a repository exposing the\nproblem if not?)\n\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"124941","messageId":"4AD56FD9.70602@gmail.com","threadId":"21229","inReplyTo":"cb7bb73a0910132319p27b4f362sf5bffa2afe6e5849@mail.gmail.com","subject":"Re: [PATCH] gitweb: fix esc_param","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2009-10-14T06:29:45Z","receivedAt":"2009-10-14T06:29:45Z","isPatch":true,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"Giuseppe Bilotta wrote:\n> On Wed, Oct 14, 2009 at 3:13 AM, Stephen Boyd wrote:\n>> This works great for my purposes. Thanks.\n>\n> Can you also check if this fixes the branch name issue you mentioned\n> in the other branch? (And/or do you have a repository exposing the\n> problem if not?)\n\n(We're jumping back and forth between threads haha)\n\nSorry, it doesn't. But this diff fixes the first part of the problem.\nThere are still problems with the RSS/Atom feed names being mangled. The\nbranch name I'm using is gitwéb, but I imagine any utf8 character will fail.\n\nI see the title and the actual text being mangled without this patch.\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 4b21ad2..910c370 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1500,9 +1500,10 @@ sub format_ref_marker {\n -href => href(\n action=>$dest_action,\n hash=>$dest\n- )}, $name);\n+ )}, esc_html($name));\n \n- $markers .= \" \" .\n+ my $title_ref = esc_html($ref);\n+ $markers .= \" \" .\n $link . \"\";\n }\n }\n"},{"id":"124949","messageId":"200910141023.15475.jnareb@gmail.com","threadId":"21229","inReplyTo":"1255463496-21617-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH] gitweb: fix esc_param","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-10-14T08:23:14Z","receivedAt":"2009-10-14T08:23:14Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, 13 Oct 2009, Giuseppe Bilotta wrote:\n\n> The custom CGI escaping done in esc_param failed to escape UTF-8\n> properly. Fix by using CGI::escape on each sequence of matched\n> characters instead of sprintf()ing a custom escaping for each byte.\n\nHmmm... I wonder if this bug isn't caused by failing to mark some\ninput as utf8 using to_utf8() subroutine... or by using \nbinmode $fd, ':utf8' on $fd from opening git-rev-list, after ensuring\nthat it outputs utf8 by --encoding=utf8 (or is it only git-log that\naccepts that option?).\n\n> \n> Additionally, the space -> + escape was being escaped due to greedy\n> matching on the first substitution. Fix by adding space to the\n> list of characters not handled on the first substitution.\n\nThanks.\n\n> \n> Finally, remove an unnecessary escaping of the + sign.\n\nSignoff?\n\n> ---\n> gitweb/gitweb.perl | 3 +--\n> 1 files changed, 1 insertions(+), 2 deletions(-)\n> \n> The issues with this routine were exposed by Stephen's\n> \"author as search link\" patch. This should fix them.\n> \n> Since the idea of esc_param is to replicate CGI::escape except for the /\n> character (if I read the comment correclty), a possible alternative\n> would be to just use CGI::escape on the whole string and then undo the\n> escaping for the / character.\n\nWell, that and widely used but non-standard (well, not using percent\nencoding) escaping of space with '+'; CGI::escape(\" \") is %20, not '+'.\nSe we would have to turn '%2F' into '/', and '%20' into '+'.\n\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 6237865..6593e5c 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1115,8 +1115,7 @@ sub to_utf8 {\n> # correct, but quoted slashes look too horrible in bookmarks\n> sub esc_param {\n> \tmy $str = shift;\n> -\t$str =~ s/([^A-Za-z0-9\\-_.~()\\/:@])/sprintf(\"%%%02X\", ord($1))/eg;\n> -\t$str =~ s/\\+/%2B/g;\n> +\t$str =~ s/([^A-Za-z0-9\\-_.~()\\/:@ ]+)/CGI::escape($1)/eg;\n> \t$str =~ s/ /\\+/g;\n> \treturn $str;\n> }\n> -- \n> 1.6.3.rc1.192.gdbfcb\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"124950","messageId":"200910141103.19295.jnareb@gmail.com","threadId":"21229","inReplyTo":"4AD56FD9.70602@gmail.com","subject":"Re: [PATCH] gitweb: fix esc_param","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-10-14T09:03:18Z","receivedAt":"2009-10-14T09:03:18Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, 14 Oct 2009, Stephen Boyd wrote:\n> Giuseppe Bilotta wrote:\n>> On Wed, Oct 14, 2009 at 3:13 AM, Stephen Boyd wrote:\n>>\n>>> This works great for my purposes. Thanks.\n>>>\n>> Can you also check if this fixes the branch name issue you mentioned\n>> in the other branch? (And/or do you have a repository exposing the\n>> problem if not?)\n> \n> (We're jumping back and forth between threads haha)\n> \n> Sorry, it doesn't. But this diff fixes the first part of the problem.\n> There are still problems with the RSS/Atom feed names being mangled. The\n> branch name I'm using is gitwéb, but I imagine any utf8 character will fail.\n\nThat it is (probably) not because of lack of esc_html, but because\nof lack of to_utf8. Grrr... we really should convert to utf8 (usually\njust mark as utf8) on reading input, to avoid such kind of errors.\n\n> \n> I see the title and the actual text being mangled without this patch.\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 4b21ad2..910c370 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1500,9 +1500,10 @@ sub format_ref_marker {\n> -href => href(\n> action=>$dest_action,\n> hash=>$dest\n> - )}, $name);\n> + )}, esc_html($name));\n\nHmmm... to_utf8 would be enough here, but for the fact that \ngit-check-ref-format doesn't prohibit '<', '>' in ref names, \nso it is possible that somebody somewhere is using such strange\nref names (e.g. branch named '' is valid branch name).\n\n> \n> - $markers .= \" \" .\n> + my $title_ref = esc_html($ref);\n> + $markers .= \" \" .\n\nHere it would be really useful to have esc_attr (which would do to_utf8\nplus escaping of '\"' and '%', quote and escape characters), although\nI guess that full HTML escaping done with esc_html wouldn't do anything\nbad.\n\n> $link . \"\";\n> }\n> }\n\n-- \nJakub Narebski\nPoland\n"},{"id":"124957","messageId":"7vpr8qe3s9.fsf@alter.siamese.dyndns.org","threadId":"21229","inReplyTo":"1255463496-21617-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH] gitweb: fix esc_param","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-14T09:13:58Z","receivedAt":"2009-10-14T09:13:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta writes:\n\n> ...\n> Finally, remove an unnecessary escaping of the + sign.\n>\n> Signed-off-by: Giuseppe Bilotta \n\nThanks.\n"}]}