[PATCH] symbolic ref: refuse non-ref targets in HEAD

[PATCH] symbolic ref: refuse non-ref targets in HEAD

[Jeff King]

When calling "git symbolic-ref" it is easy to forget that
the target must be a fully qualified ref. E.g., you might
accidentally do:

  $ git symbolic-ref HEAD master

Unfortunately, this is very difficult to recover from,
because the bogus contents of HEAD make git believe we are
no longer in a git repository (as is_git_dir explicitly
checks for "^refs/" in the HEAD target). So immediately
trying to fix the situation doesn't work:

  $ git symbolic-ref HEAD refs/heads/master
  fatal: Not a git repository

and one is left editing the .git/HEAD file manually.

Furthermore, one might be tempted to use symbolic-ref to set
up a detached HEAD:

  $ git symbolic-ref HEAD `git rev-parse HEAD`

which sets up an even more bogus HEAD:

  $ cat .git/HEAD
  ref: 1a9ace4f2ad4176148e61b5a85cd63d5604aac6d

This patch introduces a small safety valve to prevent the
specific case of anything not starting with refs/ to go into
HEAD. The scope of the safety valve is intentionally very
limited, to make sure that we are not preventing any
behavior that would otherwise be valid (like pointing a
different symref than HEAD outside of refs/).

Signed-off-by: Jeff King <peff@peff.net>
---
This is a follow-up to a patch that I posted during the last release
freeze:

  http://article.gmane.org/gmane.comp.version-control.git/103445

I know that using symbolic-ref manually is rare, but both I and the
original poster have been bitten by this (and figuring out what is going
on and fixing it is quite painful). But most importantly, I don't think
this can possibly hurt anyone trying to use this legitimately, since the
exact thing it is protecting against corrupts your repo. :)

Please beware that running the test script on the current "master" will
actually hose your git repo (test 3 kills the trash directory's
.git/HEAD, which means test 4 thinks your parent .git/ is its current
repo). Maybe it makes sense to do a precautionary reset in between.

 builtin-symbolic-ref.c  |    2 ++
 t/t1401-symbolic-ref.sh |   27 +++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 0 deletions(-)
 create mode 100755 t/t1401-symbolic-ref.sh

diff --git a/builtin-symbolic-ref.c b/builtin-symbolic-ref.c
index bfc78bb..46ea4b2 100644
--- a/builtin-symbolic-ref.c
+++ b/builtin-symbolic-ref.c
@@ -44,6 +44,8 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
 		check_symref(argv[0], quiet);
 		break;
 	case 2:
+		if (!strcmp(argv[0], "HEAD") && prefixcmp(argv[1], "refs/"))
+			die("Refusing to point HEAD outside of refs/");
 		create_symref(argv[0], argv[1], msg);
 		break;
 	default:
diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
new file mode 100755
index 0000000..1f22009
--- /dev/null
+++ b/t/t1401-symbolic-ref.sh
@@ -0,0 +1,27 @@
+#!/bin/sh
+
+test_description='basic symbolic-ref tests'
+. ./test-lib.sh
+
+test_expect_success 'symbolic-ref writes HEAD' '
+	git symbolic-ref HEAD refs/heads/foo &&
+	echo ref: refs/heads/foo >expect &&
+	test_cmp expect .git/HEAD
+'
+
+test_expect_success 'symbolic-ref reads HEAD' '
+	echo refs/heads/foo >expect &&
+	git symbolic-ref HEAD >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'symbolic-ref refuses non-ref branch for HEAD' '
+	test_must_fail git symbolic-ref HEAD foo
+'
+
+test_expect_success 'symbolic-ref refuses bare sha1' '
+	echo content >file && git add file && git commit -m one
+	test_must_fail git symbolic-ref HEAD `git rev-parse HEAD`
+'
+
+test_done
-- 
1.6.1.1.424.gac728

Re: [PATCH] symbolic ref: refuse non-ref targets in HEAD

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I know that using symbolic-ref manually is rare, but both I and the
> original poster have been bitten by this (and figuring out what is going
> on and fixing it is quite painful). But most importantly, I don't think
> this can possibly hurt anyone trying to use this legitimately, since the
> exact thing it is protecting against corrupts your repo. :)

I generally do not like adding artificial limitation to plumbing like this
patch does, because the end user making silly mistake using plumbing is a
sign that there was something lacking in the Porcelain.

But for this particular case, I do not think any future usage of
symbolic-ref plubming will get inconvenienced with the change.  I would
even suggest making the check tighter to insist on refs/heads/ (not just
refs/) and tighten validate_headref() in path.c to match.

> Please beware that running the test script on the current "master" will
> actually hose your git repo (test 3 kills the trash directory's
> .git/HEAD, which means test 4 thinks your parent .git/ is its current
> repo). Maybe it makes sense to do a precautionary reset in between.

In addition, perhaps it may make sense to use test_create_repo to go one
level deeper before starting to play around, so that trash directory's
repository will prevent you from going any further up.

Re: [PATCH] symbolic ref: refuse non-ref targets in HEAD

[Jeff King]

On Wed, Jan 28, 2009 at 11:53:20PM -0800, Junio C Hamano wrote:

> I generally do not like adding artificial limitation to plumbing like this
> patch does, because the end user making silly mistake using plumbing is a
> sign that there was something lacking in the Porcelain.

Nor do I. I only propose it in this case because

  1. We cannot possibly be hurting somebody's workflow, since it
     produces nonsensical results currently.

  2. The damage it does is so annoying to recover from.

I have no problem with symbolic-ref eventually learning to handle these
situations in some more sane manner; in the meantime, I think it makes
sense to prevent nasty breakage. And I don't think we're closing any
doors for the future; the behavior will switch from "you aren't allowed
to do this" to "does something sensible".

> But for this particular case, I do not think any future usage of
> symbolic-ref plubming will get inconvenienced with the change.  I would
> even suggest making the check tighter to insist on refs/heads/ (not just
> refs/) and tighten validate_headref() in path.c to match.

Reasonable. Updated series to follow.

> > Please beware that running the test script on the current "master" will
> > actually hose your git repo (test 3 kills the trash directory's
> > .git/HEAD, which means test 4 thinks your parent .git/ is its current
> > repo). Maybe it makes sense to do a precautionary reset in between.
> 
> In addition, perhaps it may make sense to use test_create_repo to go one
> level deeper before starting to play around, so that trash directory's
> repository will prevent you from going any further up.

That sort of helps, but only by luck. Each test kills off one layer of
repo. So the first one kills the test_create_repo, and the second one
kills the trash directory. Adding another test would kill off the main
repo. :) So you have to do something per-test. I'll do that in the
re-roll.

-Peff

[PATCH 1/2] validate_headref: tighten ref-matching to just branches

[Jeff King]

When we are trying to determine whether a directory contains
a git repository, one of the tests we do is to check whether
HEAD is either a symlink or a symref into the "refs/"
hierarchy, or a detached HEAD.

We can tighten this a little more, though: a non-detached
HEAD should always point to a branch (since checking out
anything else should result in detachment), so it is safe to
check for "refs/heads/".

Signed-off-by: Jeff King <peff@peff.net>
---
When I was writing the commit message, my spider sense tingled a little
bit. I don't think this is an unreasonable thing to do, but I also don't
know that it helps in any meaningful way. There is no evidence of people
having other stuff in their HEADs anyway.

 path.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/path.c b/path.c
index a074aea..108d9e9 100644
--- a/path.c
+++ b/path.c
@@ -154,7 +154,7 @@ int validate_headref(const char *path)
 	/* Make sure it is a "refs/.." symlink */
 	if (S_ISLNK(st.st_mode)) {
 		len = readlink(path, buffer, sizeof(buffer)-1);
-		if (len >= 5 && !memcmp("refs/", buffer, 5))
+		if (len >= 11 && !memcmp("refs/heads/", buffer, 11))
 			return 0;
 		return -1;
 	}
@@ -178,7 +178,7 @@ int validate_headref(const char *path)
 		len -= 4;
 		while (len && isspace(*buf))
 			buf++, len--;
-		if (len >= 5 && !memcmp("refs/", buf, 5))
+		if (len >= 11 && !memcmp("refs/heads/", buf, 11))
 			return 0;
 	}
 
-- 
1.6.1.1.425.gdbb13

[PATCH 2/2] symbolic ref: refuse non-ref targets in HEAD

[Jeff King]

When calling "git symbolic-ref" it is easy to forget that
the target must be a fully qualified ref. E.g., you might
accidentally do:

  $ git symbolic-ref HEAD master

Unfortunately, this is very difficult to recover from,
because the bogus contents of HEAD make git believe we are
no longer in a git repository (as is_git_dir explicitly
checks for "^refs/heads/" in the HEAD target). So
immediately trying to fix the situation doesn't work:

  $ git symbolic-ref HEAD refs/heads/master
  fatal: Not a git repository

and one is left editing the .git/HEAD file manually.

Furthermore, one might be tempted to use symbolic-ref to set
up a detached HEAD:

  $ git symbolic-ref HEAD `git rev-parse HEAD`

which sets up an even more bogus HEAD:

  $ cat .git/HEAD
  ref: 1a9ace4f2ad4176148e61b5a85cd63d5604aac6d

This patch introduces a small safety valve to prevent the
specific case of anything not starting with refs/heads/ to
go into HEAD. The scope of the safety valve is intentionally
very limited, to make sure that we are not preventing any
behavior that would otherwise be valid (like pointing a
different symref than HEAD outside of refs/heads/).

Signed-off-by: Jeff King <peff@peff.net>
---
Changes from the original are:

  - s,refs/,refs/heads/, in the code and commit message

  - test non-ref and non-branch separately; under the current
    implementation it is obvious the former will work if the latter
    does, but I think by testing intent and not the implementation, the
    tests are more future-proof

  - tests make sure to restore validity of trash directory immediately
    after running

 builtin-symbolic-ref.c  |    3 +++
 t/t1401-symbolic-ref.sh |   41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 0 deletions(-)
 create mode 100755 t/t1401-symbolic-ref.sh

diff --git a/builtin-symbolic-ref.c b/builtin-symbolic-ref.c
index bfc78bb..cafc4eb 100644
--- a/builtin-symbolic-ref.c
+++ b/builtin-symbolic-ref.c
@@ -44,6 +44,9 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
 		check_symref(argv[0], quiet);
 		break;
 	case 2:
+		if (!strcmp(argv[0], "HEAD") &&
+		    prefixcmp(argv[1], "refs/heads/"))
+			die("Refusing to point HEAD outside of refs/heads/");
 		create_symref(argv[0], argv[1], msg);
 		break;
 	default:
diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
new file mode 100755
index 0000000..569f341
--- /dev/null
+++ b/t/t1401-symbolic-ref.sh
@@ -0,0 +1,41 @@
+#!/bin/sh
+
+test_description='basic symbolic-ref tests'
+. ./test-lib.sh
+
+# If the tests munging HEAD fail, they can break detection of
+# the git repo, meaning that further tests will operate on
+# the surrounding git repo instead of the trash directory.
+reset_to_sane() {
+	echo ref: refs/heads/foo >.git/HEAD
+}
+
+test_expect_success 'symbolic-ref writes HEAD' '
+	git symbolic-ref HEAD refs/heads/foo &&
+	echo ref: refs/heads/foo >expect &&
+	test_cmp expect .git/HEAD
+'
+
+test_expect_success 'symbolic-ref reads HEAD' '
+	echo refs/heads/foo >expect &&
+	git symbolic-ref HEAD >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'symbolic-ref refuses non-ref for HEAD' '
+	test_must_fail git symbolic-ref HEAD foo
+'
+reset_to_sane
+
+test_expect_success 'symbolic-ref refuses non-branch for HEAD' '
+	test_must_fail git symbolic-ref HEAD refs/foo
+'
+reset_to_sane
+
+test_expect_success 'symbolic-ref refuses bare sha1' '
+	echo content >file && git add file && git commit -m one
+	test_must_fail git symbolic-ref HEAD `git rev-parse HEAD`
+'
+reset_to_sane
+
+test_done
-- 
1.6.1.1.425.gdbb13

Re: [PATCH] symbolic ref: refuse non-ref targets in HEAD

[Jeff King]

On Thu, Jan 29, 2009 at 03:01:45AM -0500, Jeff King wrote:

> > In addition, perhaps it may make sense to use test_create_repo to go one
> > level deeper before starting to play around, so that trash directory's
> > repository will prevent you from going any further up.
> 
> That sort of helps, but only by luck. Each test kills off one layer of
> repo. So the first one kills the test_create_repo, and the second one
> kills the trash directory. Adding another test would kill off the main
> repo. :) So you have to do something per-test. I'll do that in the
> re-roll.

It occurs to me you perhaps did mean to do something per-test, like:

  test_create_repo foo && (cd foo && do_the_actual_test)

That would work, too.

-Peff