[PATCH] hgweb: require files and directory links to begin with 'path:'

[Angel Ezquerra]

On Thu, Mar 21, 2013 at 11:31 AM, Pierre-Yves David
<pierre-yves.david at logilab.fr> wrote:
> On Wed, Mar 20, 2013 at 11:15:41PM +0100, Angel Ezquerra wrote:
>> # HG changeset patch
>> # User Angel Ezquerra <angel.ezquerra at gmail.com>
>> # Date 1363809088 -3600
>> #      Wed Mar 20 20:51:28 2013 +0100
>> # Node ID 84f24bbe0c3e97e4e1753c894e3c963d1e6c6d63
>> # Parent  136516cd3d6902aaa2edc9befc65763c56a6dbfc
>> hgweb: require files and directory links to begin with 'path:'
>>
>> If they don't the server will reply with a 403 HTTP forbidden error. This gets
>> rid of the need to explicitly check for the known pattern types.
>
> I really fails to grasp why you complexify for whole argument process for the
> shake of being able to replay a 403 in some rare error case.
>
> In my opinion, the "file" argument should be documented as a plain path and
> processed as such. It make not sense to allows any pattern here. They should
> not be recognised. Returning a 404 in this case is fine by me for the shake of
> simplicity.

I'm confused. I thought this patch _simplified_ the code. If we did
not have to remove the extra starting slash the code would be as
simple as doing a simple file.startswith('path:') check.

Note that without this check archival will happily return an empty
archive. The user cannot tell whether its input patter is wrong or the
file simply doesn't exist.

>> Note that the templater has access to a "path" variable which is a path to the
>> current file or directory relative to the root of the repository, and which
>> begins with a "/". However archival.archive() expects 'path:' to not begin with
>> a '/'. To cope with this webcommands.archive must remove the extra '/' which is
>> passed by the templater.
>>
>> diff --git a/mercurial/hgweb/webcommands.py b/mercurial/hgweb/webcommands.py
>> --- a/mercurial/hgweb/webcommands.py
>> +++ b/mercurial/hgweb/webcommands.py
>> @@ -822,12 +822,11 @@
>>      file = req.form.get('file', None)
>>      if file:
>>          file = file[0]
>> -        patandfile = file.split(':')
>> -        if len(patandfile) > 1 and patandfile[0].lower() in ('glob', 'relglob',
>> -                'path', 'relpath', 're', 'relre', 'set'):
>> -            msg = 'Archive pattern not allowed: %s' % file
>> +        if not file.lower().startswith('path:/'):
>> +            msg = "Archive path must begin with 'path:'"
>>              raise ErrorResponse(HTTP_FORBIDDEN, msg)
>> -        pats = ['path:' + file]
>> +        # The file path has an extra "/" that must be removed
>> +        pats = ['path:' + file[6:]]
>
> consider using lstrip('/')
>

Neat, I did not know about that.

Should I resend the patch with this little change or are you agains
the patch as a whole?

Cheers,

Angel