Patchwork [1,of,2,V2] ui: introduce an experimental dict of exportable environment variables

login
register
mail settings
Submitter Matt Harbison
Date Jan. 18, 2017, 4:50 a.m.
Message ID <5a03e25ec0c0417e915b.1484715046@Envy>
Download mbox | patch
Permalink /patch/18241/
State Accepted
Headers show

Comments

Matt Harbison - Jan. 18, 2017, 4:50 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1484712312 18000
#      Tue Jan 17 23:05:12 2017 -0500
# Node ID 5a03e25ec0c0417e915b2014995bd83443ef97ec
# Parent  923336cf8b8afdb41746ecef8a39d773bd5538bf
ui: introduce an experimental dict of exportable environment variables

Care needs to be taken to prevent leaking potentially sensitive environment
variables through hgweb, if template support for environment variables is to be
introduced.  There are a few ideas about the API for preventing accidental
leaking [1].  Option 3 seems best from the POV of not needing to configure
anything in the normal case.  I couldn't figure out how to do that, so guard it
with an experimental option for now.

[1] https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-January/092383.html
Yuya Nishihara - Jan. 18, 2017, 1:52 p.m.
On Tue, 17 Jan 2017 23:50:46 -0500, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1484712312 18000
> #      Tue Jan 17 23:05:12 2017 -0500
> # Node ID 5a03e25ec0c0417e915b2014995bd83443ef97ec
> # Parent  923336cf8b8afdb41746ecef8a39d773bd5538bf
> ui: introduce an experimental dict of exportable environment variables

This looks good as an experimental implementation, so queued, thanks.
I found a few minor problems, which can be fixed later.

> Care needs to be taken to prevent leaking potentially sensitive environment
> variables through hgweb, if template support for environment variables is to be
> introduced.  There are a few ideas about the API for preventing accidental
> leaking [1].  Option 3 seems best from the POV of not needing to configure
> anything in the normal case.  I couldn't figure out how to do that, so guard it
> with an experimental option for now.
> 
> [1] https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-January/092383.html

In addition to hgweb, we'll probably need to consider the case where hg
command is executed behind a third-party web application. A web frontend may
pass a revset provided by user for example, which seems a valid use case.

> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -147,6 +147,15 @@
>  
>              self.httppasswordmgrdb = urlreq.httppasswordmgrwithdefaultrealm()
>  
> +        allowed = self.configlist('experimental', 'exportableenviron')
> +        if '*' in allowed:
> +            self._exportableenviron = self.environ
> +        else:
> +            self._exportableenviron = {}
> +            for k in allowed:
> +                if k in self.environ:
> +                    self._exportableenviron[k] = self.environ[k]

Perhaps s/self.environ/encoding.environ/ would be better since self.environ
can be a WSGI-request environ. (FWIW, I have no idea why we need to carry
around WSGI environ by ui.)

And we'll need to build the dict by fixconfig(), not by __init__().
Matt Harbison - Jan. 19, 2017, 2:32 a.m.
On Wed, 18 Jan 2017 08:52:43 -0500, Yuya Nishihara <yuya@tcha.org> wrote:

> On Tue, 17 Jan 2017 23:50:46 -0500, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1484712312 18000
>> #      Tue Jan 17 23:05:12 2017 -0500
>> # Node ID 5a03e25ec0c0417e915b2014995bd83443ef97ec
>> # Parent  923336cf8b8afdb41746ecef8a39d773bd5538bf
>> ui: introduce an experimental dict of exportable environment variables
>
> This looks good as an experimental implementation, so queued, thanks.
> I found a few minor problems, which can be fixed later.
>
>> Care needs to be taken to prevent leaking potentially sensitive  
>> environment
>> variables through hgweb, if template support for environment variables  
>> is to be
>> introduced.  There are a few ideas about the API for preventing  
>> accidental
>> leaking [1].  Option 3 seems best from the POV of not needing to  
>> configure
>> anything in the normal case.  I couldn't figure out how to do that, so  
>> guard it
>> with an experimental option for now.
>>
>> [1]  
>> https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-January/092383.html
>
> In addition to hgweb, we'll probably need to consider the case where hg
> command is executed behind a third-party web application. A web frontend  
> may
> pass a revset provided by user for example, which seems a valid use case.

OK.  But this is well beyond anything I've done or understand, so I'll  
need pointers if there's anything specific to this case.

>> --- a/mercurial/ui.py
>> +++ b/mercurial/ui.py
>> @@ -147,6 +147,15 @@
>>
>>              self.httppasswordmgrdb =  
>> urlreq.httppasswordmgrwithdefaultrealm()
>>
>> +        allowed = self.configlist('experimental', 'exportableenviron')
>> +        if '*' in allowed:
>> +            self._exportableenviron = self.environ
>> +        else:
>> +            self._exportableenviron = {}
>> +            for k in allowed:
>> +                if k in self.environ:
>> +                    self._exportableenviron[k] = self.environ[k]
>
> Perhaps s/self.environ/encoding.environ/ would be better since  
> self.environ
> can be a WSGI-request environ. (FWIW, I have no idea why we need to carry
> around WSGI environ by ui.)

OK.  I wasn't sure what it was, and just assumed it was some existing  
filtering of variables.

> And we'll need to build the dict by fixconfig(), not by __init__().

Is there a way to know if ui is being used by hgweb or a web app?  I'd  
prefer this --config setting go away, at least in the default case.  Maybe  
it's useful if someone wants to opt in some vars for hgweb, for some  
reason.
Yuya Nishihara - Jan. 19, 2017, 2:58 p.m.
On Wed, 18 Jan 2017 21:32:34 -0500, Matt Harbison wrote:
> On Wed, 18 Jan 2017 08:52:43 -0500, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Tue, 17 Jan 2017 23:50:46 -0500, Matt Harbison wrote:
> >> # HG changeset patch
> >> # User Matt Harbison <matt_harbison@yahoo.com>
> >> # Date 1484712312 18000
> >> #      Tue Jan 17 23:05:12 2017 -0500
> >> # Node ID 5a03e25ec0c0417e915b2014995bd83443ef97ec
> >> # Parent  923336cf8b8afdb41746ecef8a39d773bd5538bf
> >> ui: introduce an experimental dict of exportable environment variables
> >
> > This looks good as an experimental implementation, so queued, thanks.
> > I found a few minor problems, which can be fixed later.
> >
> >> Care needs to be taken to prevent leaking potentially sensitive  
> >> environment
> >> variables through hgweb, if template support for environment variables  
> >> is to be
> >> introduced.  There are a few ideas about the API for preventing  
> >> accidental
> >> leaking [1].  Option 3 seems best from the POV of not needing to  
> >> configure
> >> anything in the normal case.  I couldn't figure out how to do that, so  
> >> guard it
> >> with an experimental option for now.
> >>
> >> [1]  
> >> https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-January/092383.html
> >
> > In addition to hgweb, we'll probably need to consider the case where hg
> > command is executed behind a third-party web application. A web frontend  
> > may
> > pass a revset provided by user for example, which seems a valid use case.
> 
> OK.  But this is well beyond anything I've done or understand, so I'll  
> need pointers if there's anything specific to this case.

Sorry for vague comment. I was thinking about which command options should
be considered safe for malicious input. For instance,

 a) a web application may accept ?rev=%REV% and execute "hg log -r'%REV%'",
    which seems fine as long as '%REV%' is shell-escaped appropriately.
 b) but if we have 'search_in_template(pattern, template)' revset and
    '{envvars}' template, (a) no longer be true.

So I saw (b) had the same story for 'shell(command)' revset/fileset proposed
before. We shouldn't allow shell injection in revset/fileset. Instead, we could
define an external revset/fileset command in config file.

Then, this question came up:

 c) how about -T? is a template fragment considered unsafe?

> > And we'll need to build the dict by fixconfig(), not by __init__().
> 
> Is there a way to know if ui is being used by hgweb or a web app?  I'd  
> prefer this --config setting go away, at least in the default case.  Maybe  
> it's useful if someone wants to opt in some vars for hgweb, for some  
> reason.

If hgweb is only the case, maybe we can overwrite ui._exportableenviron by
hgweb.

Patch

diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -147,6 +147,15 @@ 
 
             self.httppasswordmgrdb = urlreq.httppasswordmgrwithdefaultrealm()
 
+        allowed = self.configlist('experimental', 'exportableenviron')
+        if '*' in allowed:
+            self._exportableenviron = self.environ
+        else:
+            self._exportableenviron = {}
+            for k in allowed:
+                if k in self.environ:
+                    self._exportableenviron[k] = self.environ[k]
+
     @classmethod
     def load(cls):
         """Create a ui and load global and user configs"""
@@ -1211,6 +1220,12 @@ 
                 " update your code.)") % version
         self.develwarn(msg, stacklevel=2, config='deprec-warn')
 
+    def exportableenviron(self):
+        """The environment variables that are safe to export, e.g. through
+        hgweb.
+        """
+        return self._exportableenviron
+
     @contextlib.contextmanager
     def configoverride(self, overrides, source=""):
         """Context manager for temporary config overrides