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

[Angel Ezquerra]

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:
>> # HG changeset patch
>> # User Angel Ezquerra <angel.ezquerra at gmail.com>
>> # Date 1360493525 -3600
>> # Node ID b9fda7cbf2c239cecd667159391befb440edba80
>> # Parent  a1aab0b30144b6d9b383e8021d24eecd5861c84e
>> hgweb: teach archive how to download a specific directory or file
>>
>> The archive web command now takes into account the "file" request entry, if one
>> is provided.
>>
>> The provided "file" is processed as a "path" corresponding to a directory or
>> file that will be downloaded.
>>
>> With this change hgweb can to process requests such as:
>>
>>     http://mercurial.selenic.com/hg/archive/tip.zip/mercurial/templates
>>
>> This will download all files on the mercurial/templates directory as a zip file.
>> It is not possible to specify file patterns. Those will be rejected with a 403
>> reply fromthe server.
>>
>> Note that this is a first step to add support for downloading directories from
>> the web interface. Currently the only way to use this feature is by manually
>> constructing the URL that you want to download. We will have to modify the
>> archiveentry map entry on the different templates so that it adds the current
>> folder path to the archive links.
>>
>> diff --git a/mercurial/hgweb/webcommands.py b/mercurial/hgweb/webcommands.py
>> --- a/mercurial/hgweb/webcommands.py
>> +++ b/mercurial/hgweb/webcommands.py
>> @@ -816,6 +816,17 @@
>>      if cnode == key or key == 'tip':
>>          arch_version = short(cnode)
>>      name = "%s-%s" % (reponame, arch_version)
>> +
>> +    ctx = webutil.changectx(web.repo, req)
>> +    pats = []
>> +    file = req.form.get('file', None)
>> +    if file:
>> +        file = file[0]
>> +        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?


>> diff --git a/tests/test-archive.t b/tests/test-archive.t
>> --- a/tests/test-archive.t
>> +++ b/tests/test-archive.t
>> @@ -100,6 +100,13 @@
>>        testing: test-archive-2c0277f05ed4/baz/bletch   OK
>>        testing: test-archive-2c0277f05ed4/foo   OK
>>    No errors detected in compressed data of archive.zip.
>> +  $ python getarchive.py "$TIP" gz baz | gunzip | tar tf - 2>/dev/null
>> +  test-archive-2c0277f05ed4/baz/bletch
>
> Is that the test for a directory of a file?
>
> You should test the other too.

You are right. That was testing archiving a single file. I'll add
another test that archives a directory.

Good catch!

>
>> +test that we reject unsafe patterns
>> +
>> +  $ python getarchive.py "$TIP" gz relre:baz
>> +  HTTP Error 403: Archive pattern not allowed: relre:baz
>>
>>    $ "$TESTDIR/killdaemons.py" $DAEMON_PIDS
>
> Patches looks great otherwise!

Thanks for the review. Once we decide the best way to reject patterns
(if the current approach is not acceptable) I will resend the series
with the additional test.

Cheers,

Angel