Re: [PATCH 4/n] gitweb: Secure against commit-ish/tree-ish with the same name as path

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Jakub Narebski <jnareb@xxxxxxxxx> writes:

>> [PATCH/RFC 1/n] gitweb: Better git-unquoting and gitweb-quoting of p...
>> 
>> Marked preliminary, perhaps need some discussion and rerolling
>> but I haven't looked at it.
>
> I'm not sure if without this patch (well, the unquote part) gitweb
> can work with filenames which git quotes using escape sequences,

I am reasonably sure it wouldn't, and it sounded like you wanted
to fix it better than the preliminary one, so I think we are in
agreement.

>> [PATCH 2/n] gitweb: Use '&iquot;' instead of '?' in esc_path
>> 
>> Discussed; we agreed that showing byte values in different
>> colors is preferable.  Waiting for re-roll.
>
> The problem with using text color or background color is that
> the filenames tends to be shown with different color and background
> color: "tree" view, parts of difftree, parts of diff header, etc.
> Perhaps text-decoration: overline;? Just kidding...

Use of overstrike may actually not be a bad thing.  It _is_
unusual situation after all.

>> [PATCH 3/n] gitweb: Use 's' regexp modifier to secure against filena...
>> 
>> I looked at it although haven't said anything yet.  Probably a
>> safe and good change but I wonder how LF at the end of the line
>> matches /...(.+)$/s pattern; iow, if we do not use -z does it
>> still do the right thing?  Otherwise I suspect you would perhaps
>> need to chomp?
>
> We always pass chomped lines. First chunk is unnecessary (we care only
> for type), without second "tree" view look strange for files with
> embedded newline in filename.

The codepath affected by the first chunk does not chomp, which
was what I was referring to.  So in the meantime will apply only
the second hunk.

>> [PATCH 4/n] gitweb: Secure against commit-ish/tree-ish with the same...
>> 
>> Good fix and even improves readability; will apply after
>> dropping -- from ls-tree args.

I just applied this.  I'll be pushing out a "master" update
sometime today, and do not expect to be able to get to your "n
turned out to be ten" series, so it might be worthwhile to
reroll the remaining bits that you still care about on top of
what I push out tonight to make sure we are on the same page.

Preferably:

 - you should avoid making a series out of more-or-less
   unrelated things;

 - if you are doing related things in one series, do not send
   half-baked early parts out until you are finished and are
   confident with it.  If you do not know how many patches you
   need to complete that logically single topic yet, that is a
   sure sign that you are not done.  Instead, finish writing and
   testing it, and if your test finds an earlier mistake,
   especially a trivial one, go back and fix it in the earlier
   patch in the series.  Everybody makes mistakes so fixing up
   before submission is a norm, and other people do not have to
   be forced to see your "oops" in the development history.

Thanks.

-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]