This is an archive of the discontinued Mercurial Phabricator instance.

Differential D210

pushvars: move fb extension pushvars to core
ClosedPublic

Authored by pulkit on Aug 1 2017, 8:08 PM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Commits
rHGdb3dc11356ed: pushvars: move fb extension pushvars to core
Summary

pushvars extension in fbext adds a --pushvars flag to push command using which
one send strings to server which becomes environment variables there prepended
with HG_USERVAR_. These variables can then be used to run hooks on the server.
The extension is moved directly to core and unbundling of the strings and
converting them to environment variables at server is disabled by default for
security reasons. One can turn that on by following config:

[push]
pushvars.server = true

This patch also adds the test for the extension.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

pulkit created this revision.Aug 1 2017, 8:08 PM
akushner added a subscriber: akushner.Aug 1 2017, 9:02 PM
akushner added inline comments.
tests/test-pushvars.t
8

Please use /bin/sh

9–10

I didn't see 'grep -E' in the test base, but do see egrep. Consider this instead:

env |  egrep "^HG_USERVAR_(DEBUG|BYPASS_REVIEW)" | sort
akushner added inline comments.Aug 1 2017, 9:04 PM
mercurial/commands.py
4011

s/passed/pass/

4012–4016

This should definitely be opt-in, vs. opt-out.

pulkit added inline comments.Aug 1 2017, 9:20 PM
mercurial/commands.py
4012–4016

Sorry but I don't understand what you mean here. Do I need to change the documentation part or the change the flag part. Currently I have set the default of the flag to True.

akushner added a comment.Aug 1 2017, 9:28 PM

@pulkit - see what others say. People might disagree with me on this, but I'd rather be cautious on a feature like this. Even though the variables passed are all prepended with HG_USERVAR so I don't see a way how this could cause problems.

mercurial/commands.py
4012–4016

I think people might have concerns about sending variables to the server so I think this should be default off and have the documentation tell how to enable it.

I would write the documentation such that the meaning would be "if you want to enable this on your server, add the following"

For reference, I created this feature since I used to use a similar feature in git where you'd do the following convolution:

git push --receive-pack='env ALLOW_CRAZY_FILENAMES=true git-receive-pack' "
durin42 added a subscriber: durin42.Aug 2 2017, 11:14 AM

I've often wanted a way to have extra arguments to push commands (etc), but I'm not sure that just exporting all environment variables is exactly the interface I want. Could we accomplish something similar by having an --extra-args= flag that delivers a part with an arbitrary payload that can then be used (or not) by hooks as they see fit? Or is that too challenging?

(The environment variable model also feels like it fits poorly with http servers, in my brain.)

akushner added a comment.Aug 2 2017, 12:40 PM

@durin42 - This isn't exporting all the shell variables.... it works almost exactly like you are suggesting

Could we accomplish something similar by having an --extra-args= flag that delivers a part with an arbitrary payload that can then be used (or not) by hooks as they see fit?

How we use this feature:

hg push --pushvars "BYPASS_LARGE_FILE_CHECK=true"

and then the hook that usually balks when someone tries to upload some ridiculously sized binary does something like

if [[ $HG_USERVAR_BYPASS_LARGE_FILE_CHECK == true ]]; then 
  # Don't bail and allow what we usually don't allow
fi

Notice that the HG_USERVAR is prepended to the "BYPASS_LARGE_FILE_CHECK" var? The user can't override normal shell variables with this.

durin42 added a comment.Aug 2 2017, 1:05 PM
In D210#3506, @akushner wrote:

@durin42 - This isn't exporting all the shell variables.... it works almost exactly like you are suggesting

Could we accomplish something similar by having an --extra-args= flag that delivers a part with an arbitrary payload that can then be used (or not) by hooks as they see fit?

How we use this feature:

hg push --pushvars "BYPASS_LARGE_FILE_CHECK=true"

and then the hook that usually balks when someone tries to upload some ridiculously sized binary does something like

if [[ $HG_USERVAR_BYPASS_LARGE_FILE_CHECK == true ]]; then 
  # Don't bail and allow what we usually don't allow
fi

Right, that's roughly what I expected. Could it be done using HG_EXTRA_ARGS that was set to whatever --extra-args contained?

Notice that the HG_USERVAR is prepended to the "BYPASS_LARGE_FILE_CHECK" var? The user can't override normal shell variables with this.

Sure, but environment variables are still global state, so its got nonzero concurrency concerns. I'm also *extremely* uncomfortable shipping *all* environment variables because people put credentials in them on a regular basis, so if we do go with the "ship an environment variable" approach, I think the user should be specifying which variables to send.

pulkit added a comment.Aug 2 2017, 1:25 PM

Notice that the HG_USERVAR is prepended to the "BYPASS_LARGE_FILE_CHECK" var? The user can't override normal shell variables with this.

Sure, but environment variables are still global state, so its got nonzero concurrency concerns. I'm also *extremely* uncomfortable shipping *all* environment variables because people put credentials in them on a regular basis, so if we do go with the "ship an environment variable" approach, I think the user should be specifying which variables to send.

This is not sending all the environment variables of a user. Only the key, values passed with --pushvars are send to server and server has an option whether to unbundle them or not. The keys passed with --pushvars are prepended with HG_USERVAR. I am sorry if the commit description is confusing.

akushner added inline comments.Aug 2 2017, 2:50 PM
mercurial/commands.py
4012
The --pushvars option sends strings to the server that become environment variables prepended with HG_USERVAR_. For example, '--pushvars ENABLE_FEATURE=true', provides the server side hooks with 'HG_USERVAR_ENABLE_FEATURE=true' as part of their environment.

Pushvars can provide for user-overridable hooks as well as set debug levels. One example is having a hook that blocks commits containing conflict markers, but enables the user to override the hook if the file is using conflict markers for testing purposes or the file format has strings that look like conflict markers.

To enable this feature on your server, add the following to your configuration file:

[push]
pushvars.server = true
quark added a subscriber: quark.Aug 2 2017, 3:23 PM
quark added inline comments.
mercurial/commands.py


3973
Maybe add (ADVANCED) so it gets hidden in --help without --verbose.


4012
To better fit the rst format, it might be:



.. container:: verbose

   The --pushvars option sends strings to the server that become environment
   variables prepended with HG_USERVAR_. For example, ``--pushvars
   ENABLE_FEATURE=true`` provides the server side hooks with
   ``HG_USERVAR_ENABLE_FEATURE=true`` as part of their environment.

   Pushvars can provide for user-overridable hooks as well as set debug levels.
   One example is having a hook that blocks commits containing conflict
   markers, but enables the user to override the hook if the file is using
   conflict markers for testing purposes or the file format has strings that
   look like conflict markers.

   By default, servers will ignore `--pushvars`. To enable it, set
   ``pushvars.server`` set to ``true`` server-side.
quark added inline comments.Aug 2 2017, 4:10 PM
mercurial/commands.py


4012
Oops, set to should be changed to to.
pulkit edited the summary of this revision. (Show Details)Aug 2 2017, 5:02 PM
pulkit updated this revision to Diff 512.
pulkit updated this revision to Diff 514.Aug 2 2017, 8:39 PM
durin42 accepted this revision.Aug 4 2017, 5:50 PM
I like this, it opens the doors to some things I've wanted to do for a while.
This revision is now accepted and ready to land.Aug 4 2017, 5:50 PM
Closed by commit rHGdb3dc11356ed: pushvars: move fb extension pushvars to core (authored by pulkit).  ·  Explain WhyAug 4 2017, 6:01 PM
This revision was automatically updated to reflect the committed changes.
Revision Contents
Changeset List
PathPackages
Mmercurial/bundle2.py (14 lines)
Mmercurial/commands.py (37 lines)
Mmercurial/exchange.py (8 lines)
Mtests/test-completion.t (2 lines)
AMtests/test-pushvars.t (71 lines)
Diff 512
mercurial/bundle2.py
        if len(node) < 20 or len(fnode) < 20:
        if len(node) < 20 or len(fnode) < 20:

            op.ui.debug('ignoring incomplete received .hgtags fnodes data\n')
            op.ui.debug('ignoring incomplete received .hgtags fnodes data\n')

            break
            break

        cache.setfnode(node, fnode)
        cache.setfnode(node, fnode)

        count += 1
        count += 1




    cache.write()
    cache.write()

    op.ui.debug('applied %i hgtags fnodes cache entries\n' % count)
    op.ui.debug('applied %i hgtags fnodes cache entries\n' % count)



@parthandler('pushvars')

def bundle2getvars(op, part):

    '''unbundle a bundle2 containing shellvars on the server'''

    # An option to disable unbundling on server-side for security reasons

    if op.ui.configbool('push', 'pushvars.server', False):

        hookargs = {}

        for key, value in part.advisoryparams:

            key = key.upper()

            # We want pushed variables to have USERVAR_ prepended so we know

            # they came from the --pushvar flag.

            key = "USERVAR_" + key

            hookargs[key] = value

        op.addhookargs(hookargs)

mercurial/commands.py
    [('f', 'force', None, _('force push')),
    [('f', 'force', None, _('force push')),

    ('r', 'rev', [],
    ('r', 'rev', [],

     _('a changeset intended to be included in the destination'),
     _('a changeset intended to be included in the destination'),

     _('REV')),
     _('REV')),

    ('B', 'bookmark', [], _("bookmark to push"), _('BOOKMARK')),
    ('B', 'bookmark', [], _("bookmark to push"), _('BOOKMARK')),

    ('b', 'branch', [],
    ('b', 'branch', [],

     _('a specific branch you would like to push'), _('BRANCH')),
     _('a specific branch you would like to push'), _('BRANCH')),

    ('', 'new-branch', False, _('allow pushing a new branch')),
    ('', 'new-branch', False, _('allow pushing a new branch')),

    ('', 'pushvars', [], _('variables that can be sent to server (ADVANCED)')),

quarkUnsubmitted
Not Done
Maybe add (ADVANCED) so it gets hidden in --help without --verbose.
quark: Maybe add `(ADVANCED)` so it gets hidden in `--help` without `--verbose`.
    ] + remoteopts,
    ] + remoteopts,

    _('[-f] [-r REV]... [-e CMD] [--remotecmd CMD] [DEST]'))
    _('[-f] [-r REV]... [-e CMD] [--remotecmd CMD] [DEST]'))

def push(ui, repo, dest=None, **opts):
def push(ui, repo, dest=None, **opts):

    """push changes to the specified destination
    """push changes to the specified destination




    Push changesets from the local repository to the specified
    Push changesets from the local repository to the specified

    destination.
    destination.




    If -B/--bookmark is used, the specified bookmarked revision, its
    If -B/--bookmark is used, the specified bookmarked revision, its

    ancestors, and the bookmark will be pushed to the remote
    ancestors, and the bookmark will be pushed to the remote

    repository. Specifying ``.`` is equivalent to specifying the active
    repository. Specifying ``.`` is equivalent to specifying the active

    bookmark's name.
    bookmark's name.




    Please see :hg:`help urls` for important details about ``ssh://``
    Please see :hg:`help urls` for important details about ``ssh://``

    URLs. If DESTINATION is omitted, a default path will be used.
    URLs. If DESTINATION is omitted, a default path will be used.




    .. container:: verbose

akushnerUnsubmitted
Not Done
s/passed/pass/
akushner: s/passed/pass/


akushnerUnsubmitted
Not Done
The --pushvars option sends strings to the server that become environment variables prepended with HG_USERVAR_. For example, '--pushvars ENABLE_FEATURE=true', provides the server side hooks with 'HG_USERVAR_ENABLE_FEATURE=true' as part of their environment.

Pushvars can provide for user-overridable hooks as well as set debug levels. One example is having a hook that blocks commits containing conflict markers, but enables the user to override the hook if the file is using conflict markers for testing purposes or the file format has strings that look like conflict markers.

To enable this feature on your server, add the following to your configuration file:

[push]
pushvars.server = true



akushner: ```
The --pushvars option sends strings to the server that become environment variables…
quarkUnsubmitted
Not Done
To better fit the rst format, it might be:



.. container:: verbose

   The --pushvars option sends strings to the server that become environment
   variables prepended with HG_USERVAR_. For example, ``--pushvars
   ENABLE_FEATURE=true`` provides the server side hooks with
   ``HG_USERVAR_ENABLE_FEATURE=true`` as part of their environment.

   Pushvars can provide for user-overridable hooks as well as set debug levels.
   One example is having a hook that blocks commits containing conflict
   markers, but enables the user to override the hook if the file is using
   conflict markers for testing purposes or the file format has strings that
   look like conflict markers.

   By default, servers will ignore `--pushvars`. To enable it, set
   ``pushvars.server`` set to ``true`` server-side.
quark: To better fit the rst format, it might be:

```
.. container:: verbose

   The --pushvars…
quarkUnsubmitted
Not Done
Oops, set to should be changed to to.
quark: Oops, `set to` should be changed to `to`.
        The --pushvars option sends strings to the server that become

        environment variables prepended with ``HG_USERVAR_``. For example,

        ``--pushvars ENABLE_FEATURE=true``, provides the server side hooks with

        ``HG_USERVAR_ENABLE_FEATURE=true`` as part of their environment.

akushnerUnsubmitted
Not Done
This should definitely be opt-in, vs. opt-out.
akushner: This should definitely be opt-in, vs. opt-out.
pulkitAuthorUnsubmitted
Not Done
Sorry but I don't understand what you mean here. Do I need to change the documentation part or the change the flag part. Currently I have set the default of the flag to True.
pulkit: Sorry but I don't understand what you mean here. Do I need to change the documentation part or…
akushnerUnsubmitted
Not Done
I think people might have concerns about sending variables to the server so I think this should be default off and have the documentation tell how to enable it.



I would write the documentation such that the meaning would be "if you want to enable this on your server, add the following"



For reference, I created this feature since I used to use a similar feature in git where you'd do the following convolution:



git push --receive-pack='env ALLOW_CRAZY_FILENAMES=true git-receive-pack' "
akushner: I think people might have concerns about sending variables to the server so I think this should…


        pushvars can provide for user-overridable hooks as well as set debug

        levels. One example is having a hook that blocks commits containing

        conflict markers, but enables the user to override the hook if the file

        is using conflict markers for testing purposes or the file format has

        strings that look like conflict markers.



        By default, servers will ignore `--pushvars`. To enable it add the

        following to your configuration file



            [push]

            pushvars.server = true



    Returns 0 if push was successful, 1 if nothing to push.
    Returns 0 if push was successful, 1 if nothing to push.

    """
    """




    opts = pycompat.byteskwargs(opts)
    opts = pycompat.byteskwargs(opts)

    if opts.get('bookmark'):
    if opts.get('bookmark'):

        ui.setconfig('bookmarks', 'pushing', opts['bookmark'], 'push')
        ui.setconfig('bookmarks', 'pushing', opts['bookmark'], 'push')

        for b in opts['bookmark']:
        for b in opts['bookmark']:

            # translate -B options to -r so changesets get pushed
            # translate -B options to -r so changesets get pushed

        c = repo['']
        c = repo['']

        subs = c.substate # only repos that are committed
        subs = c.substate # only repos that are committed

        for s in sorted(subs):
        for s in sorted(subs):

            result = c.sub(s).push(opts)
            result = c.sub(s).push(opts)

            if result == 0:
            if result == 0:

                return not result
                return not result

    finally:
    finally:

        del repo._subtoppath
        del repo._subtoppath



    pushvars = opts.get('pushvars')

    if pushvars:

        shellvars = {}

        for raw in pushvars:

            if '=' not in raw:

                msg = ("unable to parse variable '%s', should follow "

                        "'KEY=VALUE' or 'KEY=' format")

                raise error.Abort(msg % raw)

            k, v = raw.split('=', 1)

            shellvars[k] = v



        repo._shellvars = shellvars



    pushop = exchange.push(repo, other, opts.get('force'), revs=revs,
    pushop = exchange.push(repo, other, opts.get('force'), revs=revs,

                           newbranch=opts.get('new_branch'),
                           newbranch=opts.get('new_branch'),

                           bookmarks=opts.get('bookmark', ()),
                           bookmarks=opts.get('bookmark', ()),

                           opargs=opts.get('opargs'))
                           opargs=opts.get('opargs'))




    if pushvars:

        del repo._shellvars



    result = not pushop.cgresult
    result = not pushop.cgresult




    if pushop.bkresult is not None:
    if pushop.bkresult is not None:

        if pushop.bkresult == 2:
        if pushop.bkresult == 2:

            result = 2
            result = 2

        elif not result and pushop.bkresult:
        elif not result and pushop.bkresult:

            result = 2
            result = 2




mercurial/exchange.py
                if ret:
                if ret:

                    ui.status(bookmsgmap[action][0] % book)
                    ui.status(bookmsgmap[action][0] % book)

                else:
                else:

                    ui.warn(bookmsgmap[action][1] % book)
                    ui.warn(bookmsgmap[action][1] % book)

                    if pushop.bkresult is not None:
                    if pushop.bkresult is not None:

                        pushop.bkresult = 1
                        pushop.bkresult = 1

    return handlereply
    return handlereply




@b2partsgenerator('pushvars', idx=0)

def _getbundlesendvars(pushop, bundler):

    '''send shellvars via bundle2'''

    if getattr(pushop.repo, '_shellvars', ()):

        part = bundler.newpart('pushvars')



        for key, value in pushop.repo._shellvars.iteritems():

            part.addparam(key, value, mandatory=False)




def _pushbundle2(pushop):
def _pushbundle2(pushop):

    """push data to the remote using bundle2
    """push data to the remote using bundle2




    The only currently supported type of data is changegroup but this will
    The only currently supported type of data is changegroup but this will

    evolve in the future."""
    evolve in the future."""

    bundler = bundle2.bundle20(pushop.ui, bundle2.bundle2caps(pushop.remote))
    bundler = bundle2.bundle20(pushop.ui, bundle2.bundle2caps(pushop.remote))

    pushback = (pushop.trmanager
    pushback = (pushop.trmanager

tests/test-completion.t
  commit: addremove, close-branch, amend, secret, edit, interactive, include, exclude, message, logfile, date, user, subrepos
  commit: addremove, close-branch, amend, secret, edit, interactive, include, exclude, message, logfile, date, user, subrepos

  diff: rev, change, text, git, binary, nodates, noprefix, show-function, reverse, ignore-all-space, ignore-space-change, ignore-blank-lines, unified, stat, root, include, exclude, subrepos
  diff: rev, change, text, git, binary, nodates, noprefix, show-function, reverse, ignore-all-space, ignore-space-change, ignore-blank-lines, unified, stat, root, include, exclude, subrepos

  export: output, switch-parent, rev, text, git, binary, nodates
  export: output, switch-parent, rev, text, git, binary, nodates

  forget: include, exclude
  forget: include, exclude

  init: ssh, remotecmd, insecure
  init: ssh, remotecmd, insecure

  log: follow, follow-first, date, copies, keyword, rev, removed, only-merges, user, only-branch, branch, prune, patch, git, limit, no-merges, stat, graph, style, template, include, exclude
  log: follow, follow-first, date, copies, keyword, rev, removed, only-merges, user, only-branch, branch, prune, patch, git, limit, no-merges, stat, graph, style, template, include, exclude

  merge: force, rev, preview, tool
  merge: force, rev, preview, tool

  pull: update, force, rev, bookmark, branch, ssh, remotecmd, insecure
  pull: update, force, rev, bookmark, branch, ssh, remotecmd, insecure

  push: force, rev, bookmark, branch, new-branch, ssh, remotecmd, insecure
  push: force, rev, bookmark, branch, new-branch, pushvars, ssh, remotecmd, insecure

  remove: after, force, subrepos, include, exclude
  remove: after, force, subrepos, include, exclude

  serve: accesslog, daemon, daemon-postexec, errorlog, port, address, prefix, name, web-conf, webdir-conf, pid-file, stdio, cmdserver, templates, style, ipv6, certificate, subrepos
  serve: accesslog, daemon, daemon-postexec, errorlog, port, address, prefix, name, web-conf, webdir-conf, pid-file, stdio, cmdserver, templates, style, ipv6, certificate, subrepos

  status: all, modified, added, removed, deleted, clean, unknown, ignored, no-status, terse, copies, print0, rev, change, include, exclude, subrepos, template
  status: all, modified, added, removed, deleted, clean, unknown, ignored, no-status, terse, copies, print0, rev, change, include, exclude, subrepos, template

  summary: remote
  summary: remote

  update: clean, check, merge, date, rev, tool
  update: clean, check, merge, date, rev, tool

  addremove: similarity, subrepos, include, exclude, dry-run
  addremove: similarity, subrepos, include, exclude, dry-run

  archive: no-decode, prefix, rev, type, subrepos, include, exclude
  archive: no-decode, prefix, rev, type, subrepos, include, exclude

  backout: merge, commit, no-commit, parent, rev, edit, tool, include, exclude, message, logfile, date, user
  backout: merge, commit, no-commit, parent, rev, edit, tool, include, exclude, message, logfile, date, user

tests/test-pushvars.t
  • This file was added.
Setup



  $ PYTHONPATH=$TESTDIR/..:$PYTHONPATH

  $ export PYTHONPATH



  $ cat > $TESTTMP/pretxnchangegroup.sh << EOF

  > #!/bin/sh

  > env | egrep "^HG_USERVAR_(DEBUG|BYPASS_REVIEW)" | sort

akushnerUnsubmitted
Not Done
Please use /bin/sh
akushner: Please use /bin/sh
  > exit 0

  > EOF

akushnerUnsubmitted
Not Done
I didn't see 'grep -E' in the test base, but do see egrep. Consider this instead:



env |  egrep "^HG_USERVAR_(DEBUG|BYPASS_REVIEW)" | sort
akushner: I didn't see 'grep -E' in the test base, but do see egrep. Consider this instead:

```
env |…
  $ chmod +x $TESTTMP/pretxnchangegroup.sh

  $ cat >> $HGRCPATH << EOF

  > [hooks]

  > pretxnchangegroup = $TESTTMP/pretxnchangegroup.sh

  > [experimental]

  > bundle2-exp = true

  > EOF



  $ hg init repo

  $ hg clone -q repo child

  $ cd child



Test pushing vars to repo with pushvars.server not set



  $ echo b > a

  $ hg commit -Aqm a

  $ hg push --pushvars "DEBUG=1" --pushvars "BYPASS_REVIEW=true"

  pushing to $TESTTMP/repo (glob)

  searching for changes

  adding changesets

  adding manifests

  adding file changes

  added 1 changesets with 1 changes to 1 files



Setting pushvars.sever = true and then pushing.



  $ echo [push] >> $HGRCPATH

  $ echo "pushvars.server = true" >> $HGRCPATH

  $ echo b >> a

  $ hg commit -Aqm a

  $ hg push --pushvars "DEBUG=1" --pushvars "BYPASS_REVIEW=true"

  pushing to $TESTTMP/repo

  searching for changes

  adding changesets

  adding manifests

  adding file changes

  added 1 changesets with 1 changes to 1 files

  HG_USERVAR_BYPASS_REVIEW=true

  HG_USERVAR_DEBUG=1



Test pushing var with empty right-hand side



  $ echo b >> a

  $ hg commit -Aqm a

  $ hg push --pushvars "DEBUG="

  pushing to $TESTTMP/repo (glob)

  searching for changes

  adding changesets

  adding manifests

  adding file changes

  added 1 changesets with 1 changes to 1 files

  HG_USERVAR_DEBUG=



Test pushing bad vars



  $ echo b >> a

  $ hg commit -Aqm b

  $ hg push --pushvars "DEBUG"

  pushing to $TESTTMP/repo (glob)

  abort: unable to parse variable 'DEBUG', should follow 'KEY=VALUE' or 'KEY=' format

  [255]