{"thread":{"id":"45655","subject":"[PATCH 0/3] quarantine-push loose ends","startedAt":"2017-04-10T22:11:07Z","lastAt":"2017-04-10T22:52:05Z","messageCount":5,"participants":["Jeff King","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"316527","messageId":"20170410221058.2ao64wedg2pa6uc2@sigill.intra.peff.net","threadId":"45655","inReplyTo":null,"subject":"[PATCH 0/3] quarantine-push loose ends","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-10T22:10:58Z","receivedAt":"2017-04-10T22:11:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 10, 2017 at 05:14:24PM -0400, Jeff King wrote:\n\n> > Also, I think this whole thing could really do with some documentation\n> > in githooks(5). E.g. what hooks does it apply for? The test is just\n> > for pre-receive but the patch changes run_update_hook(), does it also\n> > take effect for update? Ditto the caveat about update-ref.\n> \n> My thinking was that the cases where the effects were user-visible were\n> sufficiently insane that nobody would need to know or care when the\n> feature was in use.\n\nI guess it can't hurt to write about it, though. Here's that, plus a\ncleanup on the stray tmp_objdir_env() call you noticed. The final patch\nprovides some safety for the ref-update case. My assumption all along\nhas been that nobody would want to update random refs from inside the\npre-receive hook. This doubles down on that by making it forbidden. I\ndon't think that's a big loss, because doing so now is extremely\ndangerous. If that assumption is wrong, the correct path forward is to\nmake the quarantining configurable.\n\n  [1/3]: receive-pack: drop tmp_objdir_env from run_update_hook\n  [2/3]: receive-pack: document user-visible quarantine effects\n  [3/3]: refs: reject ref updates while GIT_QUARANTINE_PATH is set\n\n Documentation/git-receive-pack.txt | 29 +++++++++++++++++++++++++++++\n Documentation/githooks.txt         |  3 +++\n builtin/receive-pack.c             |  1 -\n refs.c                             |  6 ++++++\n t/t5547-push-quarantine.sh         | 11 +++++++++++\n 5 files changed, 49 insertions(+), 1 deletion(-)\n\n"},{"id":"316528","messageId":"20170410221322.perffycei6z6ahqc@sigill.intra.peff.net","threadId":"45655","inReplyTo":"20170410221058.2ao64wedg2pa6uc2@sigill.intra.peff.net","subject":"[PATCH 1/3] receive-pack: drop tmp_objdir_env from run_update_hook","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-10T22:13:22Z","receivedAt":"2017-04-10T22:13:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since 722ff7f87 (receive-pack: quarantine objects until\npre-receive accepts, 2016-10-03), we have to feed the\npre-receive hook the tmp_objdir environment, so that git\nprograms run from the hook know where to find the objects.\n\nThat commit modified run_update_hook() to do the same, but\nthere it is a noop. By the time we get to the update hooks,\nwe have already migrated the objects from quarantine, and so\ntmp_objdir_env() will always return NULL. We can drop this\nuseless call.\n\nNote that the ordering here and the lack of support for the\nupdate hook is intentional. The update hook calls are\ninterspersed with actual ref updates, and we must migrate\nthe objects before any refs are updated (since otherwise\nthose refs would appear broken to outside processes). So the\nonly other options are:\n\n  - remain in quarantine for the _first_ ref, but not the\n    others. This is sufficiently confusing that it can be\n    rejected outright.\n\n  - run all the individual update hooks first, then migrate,\n    then update all the refs. But this changes the repository\n    state that the update hooks see (i.e., whether or not\n    refs from the same push are updated yet or not).\n\nSo the functionality is fine and remains unchanged with this\npatch; we're just cleaning up a useless and confusing line\nof code.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/receive-pack.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex aca9c33d8..b4469b3cc 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -772,7 +772,6 @@ static int run_update_hook(struct command *cmd)\n \tproc.stdout_to_stderr = 1;\n \tproc.err = use_sideband ? -1 : 0;\n \tproc.argv = argv;\n-\tproc.env = tmp_objdir_env(tmp_objdir);\n \n \tcode = start_command(&proc);\n \tif (code)\n-- \n2.12.2.952.g759391acc\n\n"},{"id":"316529","messageId":"20170410221339.5fs26rzca6wk73de@sigill.intra.peff.net","threadId":"45655","inReplyTo":"20170410221058.2ao64wedg2pa6uc2@sigill.intra.peff.net","subject":"[PATCH 2/3] receive-pack: document user-visible quarantine effects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-10T22:13:39Z","receivedAt":"2017-04-10T22:13:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 722ff7f87 (receive-pack: quarantine objects until\npre-receive accepts, 2016-10-03) changed the underlying\ndetails of how we take in objects. This is mostly\ntransparent to the user, but there are a few things they\nmight notice. Let's document them.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/git-receive-pack.txt | 28 ++++++++++++++++++++++++++++\n Documentation/githooks.txt         |  3 +++\n 2 files changed, 31 insertions(+)\n\ndiff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt\nindex 0ccd5fbc7..7267ecfbe 100644\n--- a/Documentation/git-receive-pack.txt\n+++ b/Documentation/git-receive-pack.txt\n@@ -114,6 +114,8 @@ will be performed, and the update, post-receive and post-update\n hooks will not be invoked either.  This can be useful to quickly\n bail out if the update is not to be supported.\n \n+See the notes on the quarantine environment below.\n+\n update Hook\n -----------\n Before each ref is updated, if $GIT_DIR/hooks/update file exists\n@@ -214,6 +216,32 @@ if the repository is packed and is served via a dumb transport.\n \texec git update-server-info\n \n \n+Quarantine Environment\n+----------------------\n+\n+When `receive-pack` takes in objects, they are placed into a temporary\n+\"quarantine\" directory within the `$GIT_DIR/objects` directory and\n+migrated into the main object store only after the `pre-receive` hook\n+has completed. If the push fails before then, the temporary directory is\n+removed entirely.\n+\n+This has a few user-visible effects and caveats:\n+\n+  1. Pushes which fail due to problems with the incoming pack, missing\n+     objects, or due to the `pre-receive` hook will not leave any\n+     on-disk data. This is usually helpful to prevent repeated failed\n+     pushes from filling up your disk, but can make debugging more\n+     challenging.\n+\n+  2. Any objects created by the `pre-receive` hook will be created in\n+     the quarantine directory (and migrated only if it succeeds).\n+\n+  3. The `pre-receive` hook MUST NOT update any refs to point to\n+     quarantined objects. Other programs accessing the repository will\n+     not be able to see the objects (and if the pre-receive hook fails,\n+     those refs would become corrupted).\n+\n+\n SEE ALSO\n --------\n linkgit:git-send-pack[1], linkgit:gitnamespaces[7]\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 9565dc3fd..32343ae29 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -256,6 +256,9 @@ environment variables will not be set. If the client selects\n to use push options, but doesn't transmit any, the count variable\n will be set to zero, `GIT_PUSH_OPTION_COUNT=0`.\n \n+See the section on \"Quarantine Environment\" in\n+linkgit:git-receive-pack[1] for some caveats.\n+\n [[update]]\n update\n ~~~~~~\n-- \n2.12.2.952.g759391acc\n\n"},{"id":"316530","messageId":"20170410221412.3rxmkwgofc6aqi54@sigill.intra.peff.net","threadId":"45655","inReplyTo":"20170410221058.2ao64wedg2pa6uc2@sigill.intra.peff.net","subject":"[PATCH 3/3] refs: reject ref updates while GIT_QUARANTINE_PATH is set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-10T22:14:12Z","receivedAt":"2017-04-10T22:14:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As documented in git-receive-pack(1), updating a ref from\nwithin the pre-receive hook is dangerous and can corrupt\nyour repo. This patch forbids ref updates entirely during\nthe hook to make it harder for adventurous hook writers to\nshoot themselves in the foot.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/git-receive-pack.txt |  3 ++-\n refs.c                             |  6 ++++++\n t/t5547-push-quarantine.sh         | 11 +++++++++++\n 3 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt\nindex 7267ecfbe..86a4b32f0 100644\n--- a/Documentation/git-receive-pack.txt\n+++ b/Documentation/git-receive-pack.txt\n@@ -239,7 +239,8 @@ This has a few user-visible effects and caveats:\n   3. The `pre-receive` hook MUST NOT update any refs to point to\n      quarantined objects. Other programs accessing the repository will\n      not be able to see the objects (and if the pre-receive hook fails,\n-     those refs would become corrupted).\n+     those refs would become corrupted). For safety, any ref updates\n+     from within `pre-receive` are automatically rejected.\n \n \n SEE ALSO\ndiff --git a/refs.c b/refs.c\nindex 0272e332c..62b405d0b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1516,6 +1516,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n {\n \tstruct ref_store *refs = get_ref_store(NULL);\n \n+\tif (getenv(GIT_QUARANTINE_ENVIRONMENT)) {\n+\t\tstrbuf_addstr(err,\n+\t\t\t      _(\"ref updates forbidden inside quarantine environment\"));\n+\t\treturn -1;\n+\t}\n+\n \treturn refs->be->transaction_commit(refs, transaction, err);\n }\n \ndiff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh\nindex af9fcd833..113c87007 100755\n--- a/t/t5547-push-quarantine.sh\n+++ b/t/t5547-push-quarantine.sh\n@@ -58,4 +58,15 @@ test_expect_success 'push to repo path with path separator (colon)' '\n \tgit push \"$(pwd)/xxx${pathsep}yyy.git\" HEAD\n '\n \n+test_expect_success 'updating a ref from quarantine is forbidden' '\n+\tgit init --bare update.git &&\n+\twrite_script update.git/hooks/pre-receive <<-\\EOF &&\n+\tread old new refname\n+\tgit update-ref refs/heads/unrelated $new\n+\texit 1\n+\tEOF\n+\ttest_must_fail git push update.git HEAD &&\n+\tgit -C update.git fsck\n+'\n+\n test_done\n-- \n2.12.2.952.g759391acc\n"},{"id":"316531","messageId":"CACBZZX6TONCXyCbepDcSQuxoBKJAzVWPrBzUsozg5p2Hj-+JCg@mail.gmail.com","threadId":"45655","inReplyTo":"20170410221058.2ao64wedg2pa6uc2@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] quarantine-push loose ends","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-10T22:51:39Z","receivedAt":"2017-04-10T22:52:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Tue, Apr 11, 2017 at 12:10 AM, Jeff King <peff@peff.net> wrote:\n> On Mon, Apr 10, 2017 at 05:14:24PM -0400, Jeff King wrote:\n>\n>> > Also, I think this whole thing could really do with some documentation\n>> > in githooks(5). E.g. what hooks does it apply for? The test is just\n>> > for pre-receive but the patch changes run_update_hook(), does it also\n>> > take effect for update? Ditto the caveat about update-ref.\n>>\n>> My thinking was that the cases where the effects were user-visible were\n>> sufficiently insane that nobody would need to know or care when the\n>> feature was in use.\n>\n> I guess it can't hurt to write about it, though. Here's that, plus a\n> cleanup on the stray tmp_objdir_env() call you noticed. The final patch\n> provides some safety for the ref-update case. My assumption all along\n> has been that nobody would want to update random refs from inside the\n> pre-receive hook. This doubles down on that by making it forbidden. I\n> don't think that's a big loss, because doing so now is extremely\n> dangerous. If that assumption is wrong, the correct path forward is to\n> make the quarantining configurable.\n\nThanks a lot. Cleared up all the questions I have, and now we have\npermanent docs & more testing for them.\n\n>   [1/3]: receive-pack: drop tmp_objdir_env from run_update_hook\n>   [2/3]: receive-pack: document user-visible quarantine effects\n>   [3/3]: refs: reject ref updates while GIT_QUARANTINE_PATH is set\n>\n>  Documentation/git-receive-pack.txt | 29 +++++++++++++++++++++++++++++\n>  Documentation/githooks.txt         |  3 +++\n>  builtin/receive-pack.c             |  1 -\n>  refs.c                             |  6 ++++++\n>  t/t5547-push-quarantine.sh         | 11 +++++++++++\n>  5 files changed, 49 insertions(+), 1 deletion(-)\n>\n"}]}