[PATCH V2 STABLE] serve: move hg-ssh readonly logic into hg serve

[Yuya Nishihara]

On Wed, 10 May 2017 08:50:38 -0700, Durham Goode wrote:
> On 5/10/17 7:29 AM, Yuya Nishihara wrote:
> > On Tue, 9 May 2017 15:48:50 -0700, Durham Goode wrote:
> >> On 4/28/17 6:27 AM, Yuya Nishihara wrote:
> >>> On Thu, 27 Apr 2017 10:13:21 -0400, Augie Fackler wrote:
> >>>> On Wed, Apr 26, 2017 at 06:24:52PM -0700, Durham Goode wrote:
> >>>>> # HG changeset patch
> >>>>> # User Durham Goode <durham at fb.com>
> >>>>> # Date 1493255976 25200
> >>>>> #      Wed Apr 26 18:19:36 2017 -0700
> >>>>> # Branch stable
> >>>>> # Node ID b1964bbc387fb53b4152f19d6e929309e3f21ac6
> >>>>> # Parent  6e0368b6e0bb2aa5210daec091c0200583553a78
> >>>>> serve: move hg-ssh readonly logic into hg serve
> >>>>
> >>>> This looks good, but I'm very hesitant to add a new feature like this
> >>>> 4 days before a release. Does anyone feel strongly that this should be
> >>>> in 4.2?
> >>>
> >>> Agreed. The feature seems good, but no need to hurry to pick it up.
> >>>
> >>>> +        if realcmd == 'serve' and '--read-only' in req.args:
> >>>> +            req.args.remove('--read-only')
> >>>
> >>> This increases the risk of wrong command parsing. We can't say --read-only
> >>> is always a flag.
> >>>
> >>>> +            req.ui.setconfig('hooks', 'pretxnopen.readonlyrejectpush',
> >>>> +                             rejectpush, 'dispatch')
> >>>> +            req.ui.setconfig('hooks', 'prepushkey.readonlyrejectpush',
> >>>> +                             rejectpush, 'dispatch')
> >>>
> >>> And --read-only won't work as expected in command server since there are write
> >>> operations other than push.
> >>
> >> Are there write operations that wouldn't trigger the pretxnopen hook?
> >
> > Perhaps, "hg rollback" ?
> >
> >> Alternatively, I could rename this flag to be
> >> --add-hgssh-push-blocking-hooks, remove it from the hg server command
> >> definition entirely (just watch for it at the dispatch level and strip
> >> it like I currently do). That way we can maintain the current pattern
> >> while letting it be a special path for hg-ssh only.
> >>
> >> And if someone passes --add-hgssh-push-blocking-hooks to hg serve in a
> >> position that isn't a flag, well it'll just break for them.
> >
> > Another idea is adding --unsafe-stdio option so the frontend script may do
> > anything on the 'hg serve --stdio' process.
> 
> I'm not sure I understand this proposal.  If we have --unsafe-stdio, 
> doesn't that imply --stdio is safe and can block write operations? It 
> sounds like we don't have a way to block things like rollback, so we 
> couldn't guarantee --stdio is safe.

I meant we could add --unsafe-stdio (or --unchecked-stdio) as an alias of
--stdio. A frontend script (like hg-ssh) may bypass the constraints of
'serve --stdio' by using 'serve --unsafe-stdio'. This might sound evil, but
I think it's okay as long as 'serve <whatever>' is not an API of the ssh
wire protocol.