{"thread":{"id":"17429","subject":"[PATCH] symbolic ref: refuse non-ref targets in HEAD","startedAt":"2009-01-29T04:52:05Z","lastAt":"2009-01-29T08:34:37Z","messageCount":6,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"102404","messageId":"20090129045205.GA31183@coredump.intra.peff.net","threadId":"17429","inReplyTo":null,"subject":"[PATCH] symbolic ref: refuse non-ref targets in HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-29T04:52:05Z","receivedAt":"2009-01-29T04:52:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When calling \"git symbolic-ref\" it is easy to forget that\nthe target must be a fully qualified ref. E.g., you might\naccidentally do:\n\n  $ git symbolic-ref HEAD master\n\nUnfortunately, this is very difficult to recover from,\nbecause the bogus contents of HEAD make git believe we are\nno longer in a git repository (as is_git_dir explicitly\nchecks for \"^refs/\" in the HEAD target). So immediately\ntrying to fix the situation doesn't work:\n\n  $ git symbolic-ref HEAD refs/heads/master\n  fatal: Not a git repository\n\nand one is left editing the .git/HEAD file manually.\n\nFurthermore, one might be tempted to use symbolic-ref to set\nup a detached HEAD:\n\n  $ git symbolic-ref HEAD `git rev-parse HEAD`\n\nwhich sets up an even more bogus HEAD:\n\n  $ cat .git/HEAD\n  ref: 1a9ace4f2ad4176148e61b5a85cd63d5604aac6d\n\nThis patch introduces a small safety valve to prevent the\nspecific case of anything not starting with refs/ to go into\nHEAD. The scope of the safety valve is intentionally very\nlimited, to make sure that we are not preventing any\nbehavior that would otherwise be valid (like pointing a\ndifferent symref than HEAD outside of refs/).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is a follow-up to a patch that I posted during the last release\nfreeze:\n\n  http://article.gmane.org/gmane.comp.version-control.git/103445\n\nI know that using symbolic-ref manually is rare, but both I and the\noriginal poster have been bitten by this (and figuring out what is going\non and fixing it is quite painful). But most importantly, I don't think\nthis can possibly hurt anyone trying to use this legitimately, since the\nexact thing it is protecting against corrupts your repo. :)\n\nPlease beware that running the test script on the current \"master\" will\nactually hose your git repo (test 3 kills the trash directory's\n.git/HEAD, which means test 4 thinks your parent .git/ is its current\nrepo). Maybe it makes sense to do a precautionary reset in between.\n\n builtin-symbolic-ref.c  |    2 ++\n t/t1401-symbolic-ref.sh |   27 +++++++++++++++++++++++++++\n 2 files changed, 29 insertions(+), 0 deletions(-)\n create mode 100755 t/t1401-symbolic-ref.sh\n\ndiff --git a/builtin-symbolic-ref.c b/builtin-symbolic-ref.c\nindex bfc78bb..46ea4b2 100644\n--- a/builtin-symbolic-ref.c\n+++ b/builtin-symbolic-ref.c\n@@ -44,6 +44,8 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)\n \t\tcheck_symref(argv[0], quiet);\n \t\tbreak;\n \tcase 2:\n+\t\tif (!strcmp(argv[0], \"HEAD\") && prefixcmp(argv[1], \"refs/\"))\n+\t\t\tdie(\"Refusing to point HEAD outside of refs/\");\n \t\tcreate_symref(argv[0], argv[1], msg);\n \t\tbreak;\n \tdefault:\ndiff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh\nnew file mode 100755\nindex 0000000..1f22009\n--- /dev/null\n+++ b/t/t1401-symbolic-ref.sh\n@@ -0,0 +1,27 @@\n+#!/bin/sh\n+\n+test_description='basic symbolic-ref tests'\n+. ./test-lib.sh\n+\n+test_expect_success 'symbolic-ref writes HEAD' '\n+\tgit symbolic-ref HEAD refs/heads/foo &&\n+\techo ref: refs/heads/foo >expect &&\n+\ttest_cmp expect .git/HEAD\n+'\n+\n+test_expect_success 'symbolic-ref reads HEAD' '\n+\techo refs/heads/foo >expect &&\n+\tgit symbolic-ref HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'symbolic-ref refuses non-ref branch for HEAD' '\n+\ttest_must_fail git symbolic-ref HEAD foo\n+'\n+\n+test_expect_success 'symbolic-ref refuses bare sha1' '\n+\techo content >file && git add file && git commit -m one\n+\ttest_must_fail git symbolic-ref HEAD `git rev-parse HEAD`\n+'\n+\n+test_done\n-- \n1.6.1.1.424.gac728\n"},{"id":"102413","messageId":"7vljsuh7kf.fsf@gitster.siamese.dyndns.org","threadId":"17429","inReplyTo":"20090129045205.GA31183@coredump.intra.peff.net","subject":"Re: [PATCH] symbolic ref: refuse non-ref targets in HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-29T07:53:20Z","receivedAt":"2009-01-29T07:53:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I know that using symbolic-ref manually is rare, but both I and the\n> original poster have been bitten by this (and figuring out what is going\n> on and fixing it is quite painful). But most importantly, I don't think\n> this can possibly hurt anyone trying to use this legitimately, since the\n> exact thing it is protecting against corrupts your repo. :)\n\nI generally do not like adding artificial limitation to plumbing like this\npatch does, because the end user making silly mistake using plumbing is a\nsign that there was something lacking in the Porcelain.\n\nBut for this particular case, I do not think any future usage of\nsymbolic-ref plubming will get inconvenienced with the change.  I would\neven suggest making the check tighter to insist on refs/heads/ (not just\nrefs/) and tighten validate_headref() in path.c to match.\n\n> Please beware that running the test script on the current \"master\" will\n> actually hose your git repo (test 3 kills the trash directory's\n> .git/HEAD, which means test 4 thinks your parent .git/ is its current\n> repo). Maybe it makes sense to do a precautionary reset in between.\n\nIn addition, perhaps it may make sense to use test_create_repo to go one\nlevel deeper before starting to play around, so that trash directory's\nrepository will prevent you from going any further up.\n"},{"id":"102415","messageId":"20090129080145.GA777@coredump.intra.peff.net","threadId":"17429","inReplyTo":"7vljsuh7kf.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] symbolic ref: refuse non-ref targets in HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-29T08:01:45Z","receivedAt":"2009-01-29T08:01:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 28, 2009 at 11:53:20PM -0800, Junio C Hamano wrote:\n\n> I generally do not like adding artificial limitation to plumbing like this\n> patch does, because the end user making silly mistake using plumbing is a\n> sign that there was something lacking in the Porcelain.\n\nNor do I. I only propose it in this case because\n\n  1. We cannot possibly be hurting somebody's workflow, since it\n     produces nonsensical results currently.\n\n  2. The damage it does is so annoying to recover from.\n\nI have no problem with symbolic-ref eventually learning to handle these\nsituations in some more sane manner; in the meantime, I think it makes\nsense to prevent nasty breakage. And I don't think we're closing any\ndoors for the future; the behavior will switch from \"you aren't allowed\nto do this\" to \"does something sensible\".\n\n> But for this particular case, I do not think any future usage of\n> symbolic-ref plubming will get inconvenienced with the change.  I would\n> even suggest making the check tighter to insist on refs/heads/ (not just\n> refs/) and tighten validate_headref() in path.c to match.\n\nReasonable. Updated series to follow.\n\n> > Please beware that running the test script on the current \"master\" will\n> > actually hose your git repo (test 3 kills the trash directory's\n> > .git/HEAD, which means test 4 thinks your parent .git/ is its current\n> > repo). Maybe it makes sense to do a precautionary reset in between.\n> \n> In addition, perhaps it may make sense to use test_create_repo to go one\n> level deeper before starting to play around, so that trash directory's\n> repository will prevent you from going any further up.\n\nThat sort of helps, but only by luck. Each test kills off one layer of\nrepo. So the first one kills the test_create_repo, and the second one\nkills the trash directory. Adding another test would kill off the main\nrepo. :) So you have to do something per-test. I'll do that in the\nre-roll.\n\n-Peff\n"},{"id":"102419","messageId":"20090129083015.GA6273@coredump.intra.peff.net","threadId":"17429","inReplyTo":"20090129080145.GA777@coredump.intra.peff.net","subject":"[PATCH 1/2] validate_headref: tighten ref-matching to just branches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-29T08:30:16Z","receivedAt":"2009-01-29T08:30:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we are trying to determine whether a directory contains\na git repository, one of the tests we do is to check whether\nHEAD is either a symlink or a symref into the \"refs/\"\nhierarchy, or a detached HEAD.\n\nWe can tighten this a little more, though: a non-detached\nHEAD should always point to a branch (since checking out\nanything else should result in detachment), so it is safe to\ncheck for \"refs/heads/\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nWhen I was writing the commit message, my spider sense tingled a little\nbit. I don't think this is an unreasonable thing to do, but I also don't\nknow that it helps in any meaningful way. There is no evidence of people\nhaving other stuff in their HEADs anyway.\n\n path.c |    4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex a074aea..108d9e9 100644\n--- a/path.c\n+++ b/path.c\n@@ -154,7 +154,7 @@ int validate_headref(const char *path)\n \t/* Make sure it is a \"refs/..\" symlink */\n \tif (S_ISLNK(st.st_mode)) {\n \t\tlen = readlink(path, buffer, sizeof(buffer)-1);\n-\t\tif (len >= 5 && !memcmp(\"refs/\", buffer, 5))\n+\t\tif (len >= 11 && !memcmp(\"refs/heads/\", buffer, 11))\n \t\t\treturn 0;\n \t\treturn -1;\n \t}\n@@ -178,7 +178,7 @@ int validate_headref(const char *path)\n \t\tlen -= 4;\n \t\twhile (len && isspace(*buf))\n \t\t\tbuf++, len--;\n-\t\tif (len >= 5 && !memcmp(\"refs/\", buf, 5))\n+\t\tif (len >= 11 && !memcmp(\"refs/heads/\", buf, 11))\n \t\t\treturn 0;\n \t}\n \n-- \n1.6.1.1.425.gdbb13\n"},{"id":"102420","messageId":"20090129083302.GB6273@coredump.intra.peff.net","threadId":"17429","inReplyTo":"20090129080145.GA777@coredump.intra.peff.net","subject":"[PATCH 2/2] symbolic ref: refuse non-ref targets in HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-29T08:33:02Z","receivedAt":"2009-01-29T08:33:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When calling \"git symbolic-ref\" it is easy to forget that\nthe target must be a fully qualified ref. E.g., you might\naccidentally do:\n\n  $ git symbolic-ref HEAD master\n\nUnfortunately, this is very difficult to recover from,\nbecause the bogus contents of HEAD make git believe we are\nno longer in a git repository (as is_git_dir explicitly\nchecks for \"^refs/heads/\" in the HEAD target). So\nimmediately trying to fix the situation doesn't work:\n\n  $ git symbolic-ref HEAD refs/heads/master\n  fatal: Not a git repository\n\nand one is left editing the .git/HEAD file manually.\n\nFurthermore, one might be tempted to use symbolic-ref to set\nup a detached HEAD:\n\n  $ git symbolic-ref HEAD `git rev-parse HEAD`\n\nwhich sets up an even more bogus HEAD:\n\n  $ cat .git/HEAD\n  ref: 1a9ace4f2ad4176148e61b5a85cd63d5604aac6d\n\nThis patch introduces a small safety valve to prevent the\nspecific case of anything not starting with refs/heads/ to\ngo into HEAD. The scope of the safety valve is intentionally\nvery limited, to make sure that we are not preventing any\nbehavior that would otherwise be valid (like pointing a\ndifferent symref than HEAD outside of refs/heads/).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nChanges from the original are:\n\n  - s,refs/,refs/heads/, in the code and commit message\n\n  - test non-ref and non-branch separately; under the current\n    implementation it is obvious the former will work if the latter\n    does, but I think by testing intent and not the implementation, the\n    tests are more future-proof\n\n  - tests make sure to restore validity of trash directory immediately\n    after running\n\n builtin-symbolic-ref.c  |    3 +++\n t/t1401-symbolic-ref.sh |   41 +++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 44 insertions(+), 0 deletions(-)\n create mode 100755 t/t1401-symbolic-ref.sh\n\ndiff --git a/builtin-symbolic-ref.c b/builtin-symbolic-ref.c\nindex bfc78bb..cafc4eb 100644\n--- a/builtin-symbolic-ref.c\n+++ b/builtin-symbolic-ref.c\n@@ -44,6 +44,9 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)\n \t\tcheck_symref(argv[0], quiet);\n \t\tbreak;\n \tcase 2:\n+\t\tif (!strcmp(argv[0], \"HEAD\") &&\n+\t\t    prefixcmp(argv[1], \"refs/heads/\"))\n+\t\t\tdie(\"Refusing to point HEAD outside of refs/heads/\");\n \t\tcreate_symref(argv[0], argv[1], msg);\n \t\tbreak;\n \tdefault:\ndiff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh\nnew file mode 100755\nindex 0000000..569f341\n--- /dev/null\n+++ b/t/t1401-symbolic-ref.sh\n@@ -0,0 +1,41 @@\n+#!/bin/sh\n+\n+test_description='basic symbolic-ref tests'\n+. ./test-lib.sh\n+\n+# If the tests munging HEAD fail, they can break detection of\n+# the git repo, meaning that further tests will operate on\n+# the surrounding git repo instead of the trash directory.\n+reset_to_sane() {\n+\techo ref: refs/heads/foo >.git/HEAD\n+}\n+\n+test_expect_success 'symbolic-ref writes HEAD' '\n+\tgit symbolic-ref HEAD refs/heads/foo &&\n+\techo ref: refs/heads/foo >expect &&\n+\ttest_cmp expect .git/HEAD\n+'\n+\n+test_expect_success 'symbolic-ref reads HEAD' '\n+\techo refs/heads/foo >expect &&\n+\tgit symbolic-ref HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'symbolic-ref refuses non-ref for HEAD' '\n+\ttest_must_fail git symbolic-ref HEAD foo\n+'\n+reset_to_sane\n+\n+test_expect_success 'symbolic-ref refuses non-branch for HEAD' '\n+\ttest_must_fail git symbolic-ref HEAD refs/foo\n+'\n+reset_to_sane\n+\n+test_expect_success 'symbolic-ref refuses bare sha1' '\n+\techo content >file && git add file && git commit -m one\n+\ttest_must_fail git symbolic-ref HEAD `git rev-parse HEAD`\n+'\n+reset_to_sane\n+\n+test_done\n-- \n1.6.1.1.425.gdbb13\n"},{"id":"102421","messageId":"20090129083437.GA6297@coredump.intra.peff.net","threadId":"17429","inReplyTo":"20090129080145.GA777@coredump.intra.peff.net","subject":"Re: [PATCH] symbolic ref: refuse non-ref targets in HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-29T08:34:37Z","receivedAt":"2009-01-29T08:34:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 29, 2009 at 03:01:45AM -0500, Jeff King wrote:\n\n> > In addition, perhaps it may make sense to use test_create_repo to go one\n> > level deeper before starting to play around, so that trash directory's\n> > repository will prevent you from going any further up.\n> \n> That sort of helps, but only by luck. Each test kills off one layer of\n> repo. So the first one kills the test_create_repo, and the second one\n> kills the trash directory. Adding another test would kill off the main\n> repo. :) So you have to do something per-test. I'll do that in the\n> re-roll.\n\nIt occurs to me you perhaps did mean to do something per-test, like:\n\n  test_create_repo foo && (cd foo && do_the_actual_test)\n\nThat would work, too.\n\n-Peff\n"}]}