[PATCH 2 of 3 V6] hgweb: teach archive how to download a specific directory or file

[Angel Ezquerra]

On Wed, Mar 13, 2013 at 3:40 AM, Kevin Bullock
<kbullock+mercurial at ringworld.org> wrote:
> On 12 Mar 2013, at 5:45 PM, Angel Ezquerra wrote:
>
>> On Tue, Mar 12, 2013 at 11:10 PM, Angel Ezquerra
>> <angel.ezquerra at gmail.com> wrote:
>>> On Tue, Mar 12, 2013 at 11:08 PM, Kevin Bullock
>>> <kbullock+mercurial at ringworld.org> wrote:
>>>> On 12 Mar 2013, at 4:47 PM, Angel Ezquerra wrote:
>>>>
>>>>> On Tue, Mar 12, 2013 at 6:29 PM, Pierre-Yves David
>>>>> <pierre-yves.david at logilab.fr> wrote:
>>>>>> On Wed, Feb 27, 2013 at 06:06:33PM +0100, Angel Ezquerra wrote:
>>>>>>> +        if ':' in file:
>>>>>>> +            msg = 'Archive pattern not allowed: %s' % file
>>>>>>> +            raise ErrorResponse(HTTP_FORBIDDEN, msg)
>>>>>>> +        pats = ['path:' + file]
>>>>>>
>>>>>> What about file with ":" is there name ?
>>>>>
>>>>> I'd say that a user that created such a file deserves to not be able
>>>>> to download it ;-)
>>>>>
>>>>> Jokes aside, the alternative is to explicitly test for the different
>>>>> patterns (e.g. relpath:, relre:, etc). The problem with that is that
>>>>> if we ever introduce a new pattern we may forget to update this filter
>>>>> and then let a malicious user use such a pattern.
>>>>>
>>>>> So I don't know what is the best solution to this... Suggestions?
>>>>
>>>> If you unconditionally prefix the filename with 'path:', it will be interpreted as a path (even if it contains a ':' later in the name). Then you'd presumably fall back on a 'file not found' error instead of the 403 Forbidden, which might even be preferable from a security standpoint.
>>>>
>>>
>>> OK, I will do as you suggest. Thanks!
>>>
>>> Angel
>>
>> Ummm, actually that does not work that well because the server will
>> simply return an empty archive file.
>
> That sounds like a bug.

Kevin,

I checked archival.archive() and it does not check whether the number
of files matching the pattern is 0.

I cannot tell if that is the desired behavior or a bug.

In any case I think that is something that, if needs fixing, should be
fixed on a separate patch?

Also, I personally like the fact that we give a 403 error. IMHO it
tells you exactly what is the problem without compromising the
security of the server because we append "path:" to the requested file
path anyway (unless there is some way to feed '\b' into the requested
path?).

So I'd vote to take the latest version of my patch series (V7!) which
explicitly checks for known patterns, and I would deal with the
possible archive.archival bug separately.

Cheers,

Angel