Re: [PATCH] gitweb: protect blob and diff output lines from controls.

From: Junio C Hamano <junkio@cox.net>
Date: Nov 9, 2006, 09:55 UTC
Message-ID: <7vlkmlkkq8.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <200611091024.35019.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> writes:

Show 5 quoted lines

> Jakub Narebski wrote:
>> I'm not sure what quoting to choose for esc_attr, but there we could
>> use even --no-control-chars quoting (replacing any control character
>> by '?');  but perhaps in some cases like git_print_page_path
>> subroutine CEC is better.

To be honest, I do not have strong preference between the escaping style. If the gitweb cabal feel it is more natural to see "^L" in blobs and "\f" in path, I will very happily accept such a patch.

> I'm rambling. esc_attr is special case, because CGI does escapeHTML
> (and I hope also to_utf8) for us. Using <span class="cntrl">...</span>
> has also no sense. So there should be separate esc_attr_path subroutine
> I think.

Yes. It is unfortunate that there needs different types of quoting. I think the first step would be to stop calling esc_html in esc_path. I think it was a mistake, and I did not correct it when I started touching it.

Somehow I ended up spending sizeable part of my git day this week on fixing up blob/blame/tag/commit message view regarding this "make controls visible and safe" issues on the 'master' branch, but I have been consciously staying out of gitweb/ part of the system, primarily because there are many other people who are more interested and qualified in it than myself.

I'll step aside and try not to get in the way. There is another thing I noticed while testing it with an artifitial test that I haven't fixed, but I think you already know about it (when the commitdiff is completely empty except mode changes, we end up with unbalanced div). My test's tip can be found at 'gitweb-test-funny-char' branch temporarily in the git.git repository.