Page MenuHomePhabricator

pushvars: move fb extension pushvars to core
ClosedPublic

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

Details

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pulkit created this revision.Aug 1 2017, 8:08 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.

@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' "

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.)

@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.

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
This revision was automatically updated to reflect the committed changes.