{"thread":{"id":"36341","subject":"Re: [PATCH] gitweb: Improve diffs when filenames contain problem characters","startedAt":"2014-04-03T08:18:34Z","lastAt":"2014-04-03T08:18:34Z","messageCount":1,"participants":["Jakub Narębski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"238319","messageId":"CANQwDwdfDQthiYa+-PemzBAgDLDZDDDf_pRzn1vV-7NRH9bjGw@mail.gmail.com","threadId":"36341","inReplyTo":"A60B35D8-0981-45C8-B7D9-B89C729A837D@kellerfarm.com","subject":"Re: [PATCH] gitweb: Improve diffs when filenames contain problem characters","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2014-04-03T08:18:34Z","receivedAt":"2014-04-03T08:18:34Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"[Forgot to hit Reply-to-all instead of Reply. Andrew, I'm sorry for\nduplicate email]\n\nOn Sat, Mar 29, 2014 at 2:53 PM, Andrew Keller wrote:\n>\n> When formatting a diff header line, be sure to escape the raw output from git\n> for use as HTML. This ensures that when \"problem characters\" (&, <, >, ?, etc.)\n> exist in filenames, gitweb displays them as text, rather than letting the\n> browser interpret them as HTML.\n\nActually gitweb tries to do the right thing, and in most cases it does\nHTML escaping correctly. The problem is only with *binary* files (this fact\nshould IMHO have been mentioned in commit message).\n\nThis issue is caused by two problems / errors. First, gitweb misclassify\n\"Binary files a/foo and b/bar differ\" as diff header, while it is untypical\nbut it is diff contents. Second, gitweb doesn't HTML-escape unknown\ndiff headers, assuming that it knows about all possible types.\n\nI have had those changes in my git repository, but I do not know if\nI have pushed it before the PC went down, and if I have it in backup.\n\n>\n> Reported-by: Dongsheng Song \n> Signed-off-by: Andrew Keller \n\nThank you for your work.\n\n> ---\n> Steps to reproduce:\n>\n> 1) Create a repository that contains a commit that adds a file:\n> * with an ampersand in the filename\n> * with binary contents\n> 2) Make the repository visible to gitweb\n> 3) In gitweb, navigate to the page that shows the diff for the commit\n> that added the file\n>\n> Behavior without patch:\n\nPage contains unescaped '&' instead of '&'\n\n> Page stops rendering when it gets to one of\n> the instances of the filename (in the diff header, to be specific), and\n> a light-red box appears a the top of the page, saying something like\n> this:\n>\n> This page contains the following errors:\n>\n> error on line 67 at column 22: xmlParseEntityRef: no name\n>\n> Below is a rendering of the page up to the first error.\n>\n>\n> (This particular error is what you get with an unescaped ampersand)\n\nThis is caused by the fact that some browsers use strict XML validation\nmode when receiving 'application/xml+xhtml' contents, and not 'text/html'.\nSo one might not notice it...\n\n> Behavior with patch: You see the ampersand in the file name, and the\n> page renders as one would expect.\n\nA question: it does not escapes HTML twice, i.e. you don't see\n'&' in rendered output ('&amp;' in page source)?\n\n> Other notes:\n>\n> Several helper methods exist for escaping HTML, and I was unsure\n> about which one to use. Although this patch fixes the problem,\n> it is possible that it may be more correct to use, for example, the\n> 'esc_html' helper method instead of interacting with $cgi directly.\n\nIt is preferred to use esc_html instead of $cgi->escapeHTML.\nEven better use esc_path to escape pathnames.\n\n> The first hunk in the diff seems to be all that's required to experience\n> the good behavior, however upon inspecting the code, it seems\n> prudent to also include the second one. It's a nearby section of code,\n> doing similar work, and is called from the same function as the\n> former, and with similar arguments.\n\nI wonder if it would not interfere with processing of diff contents\nby gitweb, for example adding links to pre-image and post-image\nversion of a file.\n\n> gitweb/gitweb.perl | 4 ++++\n> 1 files changed, 4 insertions(+), 0 deletions(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 79057b7..6c559f8 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -2223,6 +2223,8 @@ sub format_git_diff_header_line {\n> my $diffinfo = shift;\n> my ($from, $to) = @_;\n>\n> + $line = $cgi->escapeHTML($line);\n> +\n> if ($diffinfo->{'nparents'}) {\n> # combined diff\n> $line =~ s!^(diff (.*?) )\"?.*$!$1!;\n> @@ -2259,6 +2261,8 @@ sub format_extended_diff_header_line {\n> my $diffinfo = shift;\n> my ($from, $to) = @_;\n>\n> + $line = $cgi->escapeHTML($line);\n> +\n> # match \n> if ($line =~ s!^((copy|rename) from ).*$!$1! && $from->{'href'}) {\n> $line .= $cgi->a({-href=>$from->{'href'}, -class=>\"path\"},\n\nI'd have to examine code flow to check where it lands...\n\n-- \nJakub Narębski\n"}]}