[PATCH] hgweb: fail if an invalid command was supplied in url path (issue4071)

[Mads Kiilerich]

On 09/22/2014 05:48 PM, Anton Shestakov wrote:
> # HG changeset patch
> # User Anton Shestakov <engored at ya.ru>
> # Date 1411397198 -32400
> #      Mon Sep 22 23:46:38 2014 +0900
> # Node ID e7a296512bdffc2238b6acceb6e2314a2842b900
> # Parent  5e16fe6fdd32124c3295db5ec40b076084cc5bd4
> hgweb: fail if an invalid command was supplied in url path (issue4071)
>
> Currently hgweb produces an http 400 error only if an invalid command was
> supplied using url query (i.e. "?cmd=badcmd").

FWIW, in my opinion:

The description will become a part of the changeset. Changesets have two 
personalities and can be seen as a change or a snapshot. Patches (like 
this mail) show the description before showing the change from the 
previous revision and it thus kind of make sense that it describes the 
situation before the patch. But actually the commit message is written 
after the change has been made so that is a bit backwards. And the 
changeset also designates a snapshot that includes the change and the 
description should thus describe the situation with the change.

I think the snapshot interpretation is the most important one, so to me 
"now" and "currently" refer to how it looks after the change. I prefer 
to explicitly refer to the situation before the change as "before" and 
use past tense. The description of result after the change can just use 
present tense and perhaps say "now" or refer to "after this change" or 
"with this change" or "this change will ..." or something like that.

That's my idea of best practice. Others might agree or disagree.

 From this perspective the description of this change is a bit confusing 
... but I think I got it ;-)

> If an invalid command was
> supplied as a url path fragment (i.e. "/badcmd/"), hgweb silently falls back to
> rendering the repo overview page ("/"). This is inconsistent and breaks some
> tools that rely on http status codes (as noted in the issue4071). So this patch
> makes hgweb fail in both cases with "400 no such method".
>
> That issue, however, is about a command with some arguments, such as revision
> and file ("/badcmd/tip/foo.txt"). It is possible to fix only that case, but
> this particular patch is more generic and will make hgweb produce 400 status
> code even if the command is not followed by any arguments ("/badcmd").
>
> diff --git a/mercurial/hgweb/hgweb_mod.py b/mercurial/hgweb/hgweb_mod.py
> --- a/mercurial/hgweb/hgweb_mod.py
> +++ b/mercurial/hgweb/hgweb_mod.py
> @@ -200,8 +200,6 @@ class hgweb(object):
>               # avoid accepting e.g. style parameter as command
>               if util.safehasattr(webcommands, cmd):
>                   req.form['cmd'] = [cmd]
> -            else:
> -                cmd = ''
>   
>               if cmd == 'static':
>                   req.form['file'] = ['/'.join(args)]

The handling of cmd in this method is not trivial. Can you explain 
(perhaps in the commit message) how this change will make the rest of 
the code do the right thing? How come we don't have to raise an error if 
the command is invalid? Can you convince us that nothing depends on the 
code path you remove?

/Mads