check_refname_format allows refs with components that begin with -, even though `git tag` does not

check_refname_format allows refs with components that begin with -, even though `git tag` does not

[Demi M. Obenour]

[-- Attachment #1.1.1: Type: text/plain, Size: 927 bytes --]

If I try to create a Git tag with a name beginning with `-`,
Git complains.  However, Git does not check that a repository does
not have tags containing `-`.  This almost led to a vulnerability
in the QubesOS `verify-git-tag` script.  Fortunately, this was not
exploitable, as neither `git tag -v`, `git verify-tag --raw`, nor
`git describe` have options that are useful to an attacker.

Since this could cause vulnerabilities in other programs, I initially
reported it as an embargoed security bug, but was told to post it
publicly.

The best idea I had for a fix is to print names beginning with `-`
using the fully-qualified form, such as "refs/tags/-a".  Also, `--`
is used as a delimiter in many commands, and can’t be escaped,
so disallowing it might be a good idea.

In the long run, I hope to see leading dashes banned entirely, but
backwards compatibility might prevent that.

Sincerely,

Demi

[-- Attachment #1.1.2: OpenPGP_0xB288B55FFF9C22C1.asc --]
[-- Type: application/pgp-keys, Size: 4041 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: check_refname_format allows refs with components that begin with -, even though `git tag` does not

[Junio C Hamano]

"Demi M. Obenour" <athena@invisiblethingslab.com> writes:

> If I try to create a Git tag with a name beginning with `-`,
> Git complains.  However, Git does not check that a repository does
> not have tags containing `-`.

This is quite deliberate.  The command line parser of "git checkout"
and friends long lacked way to say "switch to THAT BRANCH whose name
begins with a hyphen" etc., and preventing tags and branches whose
name begins with a hyphen from created at the Porcelain level was a
way to stop users from hurting themselves.  

These funny names are supported at the plumbing level primarily
because we have historically allowed them and suddenly forbidding
their use would break existing repository.  A secondary reason is to
have a way to learn the current value of and then remove them, so
people with these funnily named branches and tags can "rename" them.

> This almost led to a vulnerability in the QubesOS `verify-git-tag`
> script.

Scripts need to be careful about their inputs, period.

> The best idea I had for a fix is to print names beginning with `-`
> using the fully-qualified form, such as "refs/tags/-a".  Also, `--`
> is used as a delimiter in many commands, and can’t be escaped,
> so disallowing it might be a good idea.

I do not think there is anything to fix.

Command line parsers of some commands may have to learn how to
disambiguate such a strangely named tags and branches, though.  Some
commands do not know --end-of-options convention, for example.

Thanks.


[Further reading]

https://lore.kernel.org/git/7v62pjo4km.fsf@alter.siamese.dyndns.org/
https://lore.kernel.org/git/7vsk262vla.fsf@alter.siamese.dyndns.org/

Re: check_refname_format allows refs with components that begin with -, even though `git tag` does not

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 10 2020, Demi M. Obenour wrote:

> If I try to create a Git tag with a name beginning with `-`,
> Git complains.  However, Git does not check that a repository does
> not have tags containing `-`.  This almost led to a vulnerability
> in the QubesOS `verify-git-tag` script.  Fortunately, this was not
> exploitable, as neither `git tag -v`, `git verify-tag --raw`, nor
> `git describe` have options that are useful to an attacker.
>
> Since this could cause vulnerabilities in other programs, I initially
> reported it as an embargoed security bug, but was told to post it
> publicly.
>
> The best idea I had for a fix is to print names beginning with `-`
> using the fully-qualified form, such as "refs/tags/-a".  Also, `--`
> is used as a delimiter in many commands, and can’t be escaped,
> so disallowing it might be a good idea.
>
> In the long run, I hope to see leading dashes banned entirely, but
> backwards compatibility might prevent that.

[Re-posted from the very brief discussion on the security list]

I'm inclined to say that we might as well remove the porcelain
limitation of these "-" tags and branches. In reality we support them,
fsck doesn't even warn about them, and git-check-ref-format(1) doesn't
document them as forbidden.

From reading 63486240108 ("disallow branch names that start with a
hyphen", 2010-09-14) and 4f0accd638b ("tag: disallow '-' as tag name",
2011-05-10) I don't see why we'd need to forbid these. A WIP patch of
removing the checks from strbuf_check_branch_ref() and
strbuf_check_tag_ref() fails just one test.

The reason we forbade these seems to have been mainly to avoid the UI
confusion of "git checkout -" or "git checkout -foo" and "git checkout
-- -foo" not working. We can just make the "-" a special-case, and since
then we've had "git switch -- -foo" which works, and you can do "git
checkout refs/heads/-foo".

I'm probably missing some porcelain bits but with my trivial patch "git
[opts] [tag|branch] -- -foo" works.

So I think we should just allow these, since at best it just leads to a
false sense of security.

FWIW this is the very basic WIP patch I was playing with. Fails just one
test that explicitly tests for a branch named "-naster" with
check-ref-format, should probably still disallow refs named just "-" (or
maybe not...).

diff --git a/builtin/tag.c b/builtin/tag.c
index ecf011776dc..86088838bb4 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -369,9 +369,6 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
 
 static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
 {
-	if (name[0] == '-')
-		return -1;
-
 	strbuf_reset(sb);
 	strbuf_addf(sb, "refs/tags/%s", name);
 
diff --git a/sha1-name.c b/sha1-name.c
index 0b23b86ceb4..9a01138336c 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -1592,7 +1592,7 @@ int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
 	 */
 	strbuf_splice(sb, 0, 0, "refs/heads/", 11);
 
-	if (*name == '-' ||
+	if (!strcmp(name, "-") ||
 	    !strcmp(sb->buf, "refs/heads/HEAD"))
 		return -1;
 

Re: check_refname_format allows refs with components that begin with -, even though `git tag` does not

[Jeff King]

On Tue, Nov 10, 2020 at 12:09:55PM -0800, Junio C Hamano wrote:

> > The best idea I had for a fix is to print names beginning with `-`
> > using the fully-qualified form, such as "refs/tags/-a".  Also, `--`
> > is used as a delimiter in many commands, and can’t be escaped,
> > so disallowing it might be a good idea.
> 
> I do not think there is anything to fix.
> 
> Command line parsers of some commands may have to learn how to
> disambiguate such a strangely named tags and branches, though.  Some
> commands do not know --end-of-options convention, for example.

I think there is at least one thing to fix: rev-parse never learned
about --end-of-options. It also seems to have some weirdness around
"--".

Here's a series which fixes it.

  [1/3]: rev-parse: don't accept options after dashdash
  [2/3]: rev-parse: put all options under the "-" check
  [3/3]: rev-parse: handle --end-of-options

 Documentation/git-rev-parse.txt |   8 ++-
 builtin/rev-parse.c             | 100 ++++++++++++++++++--------------
 t/t1503-rev-parse-verify.sh     |  13 +++++
 t/t1506-rev-parse-diagnosis.sh  |  25 ++++++++
 4 files changed, 99 insertions(+), 47 deletions(-)

-Peff

[PATCH 1/3] rev-parse: don't accept options after dashdash

[Jeff King]

Because of the order in which we check options in rev-parse, there are a
few options we accept even after a "--". This is wrong, because the
whole point of "--" is to say "everything after here is a path". Let's
move the "did we see a dashdash" check (it's called "as_is" in the code)
to the top of the parsing loop.

Note there is one subtlety here. The options are ordered so that some
are checked before we even see if we're in a repository (they continue
the loop, and if we get past a certain point, then we do the repository
setup). By moving the as_is check higher, it's also in that "before
setup" section, even though it might look at the repository via
verify_filename(). However, this works out: we'd never set as_is until
we parse "--", and we don't parse that until after doing the setup.

An alternative here to avoid the subtlety is to put the as_is check at
the top of the post-setup options. But then every pre-setup option would
have to remember to check "if (!as_is && !strcmp(...))". So while this
is a bit magical, it's harder for future code to get wrong.

Signed-off-by: Jeff King <peff@peff.net>
---
I guess it could also shove them all into an:

  if (!as_is) {
	if (!strcmp("--local-env-vars"))
  }

conditional. I do end up having to do that later for end-of-options
anyway.

 builtin/rev-parse.c            | 11 ++++++-----
 t/t1506-rev-parse-diagnosis.sh |  9 +++++++++
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index ed200c8af1..293428fa0d 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -622,6 +622,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 
+		if (as_is) {
+			if (show_file(arg, output_prefix) && as_is < 2)
+				verify_filename(prefix, arg, 0);
+			continue;
+		}
+
 		if (!strcmp(arg, "--local-env-vars")) {
 			int i;
 			for (i = 0; local_repo_env[i]; i++)
@@ -655,11 +661,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 			i++;
 			continue;
 		}
-		if (as_is) {
-			if (show_file(arg, output_prefix) && as_is < 2)
-				verify_filename(prefix, arg, 0);
-			continue;
-		}
 		if (!strcmp(arg,"-n")) {
 			if (++i >= argc)
 				die("-n requires an argument");
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index 3e657e693b..2ed5d50059 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -254,4 +254,13 @@ test_expect_success 'escaped char does not trigger wildcard rule' '
 	test_must_fail git rev-parse "foo\\*bar"
 '
 
+test_expect_success 'arg after dashdash not interpreted as option' '
+	cat >expect <<-\EOF &&
+	--
+	--local-env-vars
+	EOF
+	git rev-parse -- --local-env-vars >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.29.2.640.g9e24689a4c

[PATCH 2/3] rev-parse: put all options under the "-" check

[Jeff King]

The option-parsing loop of rev-parse checks whether the first character
of an arg is "-". If so, then it enters a series of conditionals
checking for individual options. But some options are inexplicably
outside of that outer conditional.

This doesn't produce the wrong behavior; the conditional is actually
redundant with the individual option checks, and it's really only its
fallback "continue" that we care about. But we should at least be
consistent.

One obvious alternative is that we could get rid of the conditional
entirely. But we'll be using the extra block it provides in the next
patch.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/rev-parse.c | 47 ++++++++++++++++++++++-----------------------
 1 file changed, 23 insertions(+), 24 deletions(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 293428fa0d..79689286d8 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -652,30 +652,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 			did_repo_setup = 1;
 		}
 
-		if (!strcmp(arg, "--git-path")) {
-			if (!argv[i + 1])
-				die("--git-path requires an argument");
-			strbuf_reset(&buf);
-			puts(relative_path(git_path("%s", argv[i + 1]),
-					   prefix, &buf));
-			i++;
-			continue;
-		}
-		if (!strcmp(arg,"-n")) {
-			if (++i >= argc)
-				die("-n requires an argument");
-			if ((filter & DO_FLAGS) && (filter & DO_REVS)) {
-				show(arg);
-				show(argv[i]);
-			}
-			continue;
-		}
-		if (starts_with(arg, "-n")) {
-			if ((filter & DO_FLAGS) && (filter & DO_REVS))
-				show(arg);
-			continue;
-		}
-
 		if (*arg == '-') {
 			if (!strcmp(arg, "--")) {
 				as_is = 2;
@@ -684,6 +660,29 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 					show_file(arg, 0);
 				continue;
 			}
+			if (!strcmp(arg, "--git-path")) {
+				if (!argv[i + 1])
+					die("--git-path requires an argument");
+				strbuf_reset(&buf);
+				puts(relative_path(git_path("%s", argv[i + 1]),
+						   prefix, &buf));
+				i++;
+				continue;
+			}
+			if (!strcmp(arg,"-n")) {
+				if (++i >= argc)
+					die("-n requires an argument");
+				if ((filter & DO_FLAGS) && (filter & DO_REVS)) {
+					show(arg);
+					show(argv[i]);
+				}
+				continue;
+			}
+			if (starts_with(arg, "-n")) {
+				if ((filter & DO_FLAGS) && (filter & DO_REVS))
+					show(arg);
+				continue;
+			}
 			if (!strcmp(arg, "--default")) {
 				def = argv[++i];
 				if (!def)
-- 
2.29.2.640.g9e24689a4c

[PATCH 3/3] rev-parse: handle --end-of-options

[Jeff King]

We taught rev-list a new way to separate options from revisions in
19e8789b23 (revision: allow --end-of-options to end option parsing,
2019-08-06), but rev-parse uses its own parser. It should know about
--end-of-options not only for consistency, but because it may be
presented with similarly ambiguous cases. E.g., if a caller does:

  git rev-parse "$rev" -- "$path"

to parse an untrusted input, then it will get confused if $rev contains
an option-like string like "--local-env-vars". Or even "--not-real",
which we'd keep as an option to pass along to rev-list.

Or even more importantly:

  git rev-parse --verify "$rev"

can be confused by options, even though its purpose is safely parsing
untrusted input. On the plus side, it will always fail the --verify
part, as it will not have parsed a revision, so the caller will
generally "fail closed" rather than continue to use the untrusted
string. But it will still trigger whatever option was in "$rev"; this
should be mostly harmless, since rev-parse options are all read-only,
but I didn't carefully audit all paths.

This patch lets callers write:

  git rev-parse --end-of-options "$rev" -- "$path"

and:

  git rev-parse --verify --end-of-options "$rev"

which will both treat "$rev" always as a revision parameter. The latter
is a bit clunky. It would be nicer if we had defined "--verify" to
require that its next argument be the revision. But we have not
historically done so, and:

  git rev-parse --verify -q "$rev"

does currently work. I added a test here to confirm that we didn't break
that.

A few implementation notes:

 - We don't document --end-of-options explicitly in commands, but rather
   in gitcli(7). So I didn't give it its own section in git-rev-parse(1).
   But I did call it out specifically in the --verify section, and
   include it in the examples, which should show best practices.

 - We don't have to re-indent the main option-parsing block, because we
   can combine our "did we see end of options" check with "does it start
   with a dash". The exception is the pre-setup options, which need
   their own block.

 - We do however have to pull the "--" parsing out of the "does it start
   with dash" block, because we want to parse it even if we've seen
   --end-of-options.

 - We'll leave "--end-of-options" in the output. This is probably not
   technically necessary, as a careful caller will do:

     git rev-parse --end-of-options $revs -- $paths

   and anything in $revs will be resolved to an object id. However, it
   does help a slightly less careful caller like:

     git rev-parse --end-of-options $revs_or_paths

   where a path "--foo" will remain in the output as long as it also
   exists on disk. In that case, it's helpful to retain --end-of-options
   to get passed along to rev-list, s it would otherwise see just
   "--foo".

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-rev-parse.txt |  8 +++--
 builtin/rev-parse.c             | 56 +++++++++++++++++++--------------
 t/t1503-rev-parse-verify.sh     | 13 ++++++++
 t/t1506-rev-parse-diagnosis.sh  | 16 ++++++++++
 4 files changed, 68 insertions(+), 25 deletions(-)

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 19b12b6d43..5013daa6ef 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -109,6 +109,10 @@ names an existing object that is a commit-ish (i.e. a commit, or an
 annotated tag that points at a commit).  To make sure that `$VAR`
 names an existing object of any type, `git rev-parse "$VAR^{object}"`
 can be used.
++
+Note that if you are verifying a name from an untrusted source, it is
+wise to use `--end-of-options` so that the name argument is not mistaken
+for another option.
 
 -q::
 --quiet::
@@ -446,15 +450,15 @@ $ git rev-parse --verify HEAD
 * Print the commit object name from the revision in the $REV shell variable:
 +
 ------------
-$ git rev-parse --verify $REV^{commit}
+$ git rev-parse --verify --end-of-options $REV^{commit}
 ------------
 +
 This will error out if $REV is empty or not a valid revision.
 
 * Similar to above:
 +
 ------------
-$ git rev-parse --default master --verify $REV
+$ git rev-parse --default master --verify --end-of-options $REV
 ------------
 +
 but if $REV is empty, the commit object name from master will be printed.
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 79689286d8..69ba7326cf 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -595,6 +595,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 	struct object_context unused;
 	struct strbuf buf = STRBUF_INIT;
 	const int hexsz = the_hash_algo->hexsz;
+	int seen_end_of_options = 0;
 
 	if (argc > 1 && !strcmp("--parseopt", argv[1]))
 		return cmd_parseopt(argc - 1, argv + 1, prefix);
@@ -628,21 +629,23 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 
-		if (!strcmp(arg, "--local-env-vars")) {
-			int i;
-			for (i = 0; local_repo_env[i]; i++)
-				printf("%s\n", local_repo_env[i]);
-			continue;
-		}
-		if (!strcmp(arg, "--resolve-git-dir")) {
-			const char *gitdir = argv[++i];
-			if (!gitdir)
-				die("--resolve-git-dir requires an argument");
-			gitdir = resolve_gitdir(gitdir);
-			if (!gitdir)
-				die("not a gitdir '%s'", argv[i]);
-			puts(gitdir);
-			continue;
+		if (!seen_end_of_options) {
+			if (!strcmp(arg, "--local-env-vars")) {
+				int i;
+				for (i = 0; local_repo_env[i]; i++)
+					printf("%s\n", local_repo_env[i]);
+				continue;
+			}
+			if (!strcmp(arg, "--resolve-git-dir")) {
+				const char *gitdir = argv[++i];
+				if (!gitdir)
+					die("--resolve-git-dir requires an argument");
+				gitdir = resolve_gitdir(gitdir);
+				if (!gitdir)
+					die("not a gitdir '%s'", argv[i]);
+				puts(gitdir);
+				continue;
+			}
 		}
 
 		/* The rest of the options require a git repository. */
@@ -652,14 +655,15 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 			did_repo_setup = 1;
 		}
 
-		if (*arg == '-') {
-			if (!strcmp(arg, "--")) {
-				as_is = 2;
-				/* Pass on the "--" if we show anything but files.. */
-				if (filter & (DO_FLAGS | DO_REVS))
-					show_file(arg, 0);
-				continue;
-			}
+		if (!strcmp(arg, "--")) {
+			as_is = 2;
+			/* Pass on the "--" if we show anything but files.. */
+			if (filter & (DO_FLAGS | DO_REVS))
+				show_file(arg, 0);
+			continue;
+		}
+
+		if (!seen_end_of_options && *arg == '-') {
 			if (!strcmp(arg, "--git-path")) {
 				if (!argv[i + 1])
 					die("--git-path requires an argument");
@@ -937,6 +941,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				puts(the_hash_algo->name);
 				continue;
 			}
+			if (!strcmp(arg, "--end-of-options")) {
+				seen_end_of_options = 1;
+				if (filter & (DO_FLAGS | DO_REVS))
+					show_file(arg, 0);
+				continue;
+			}
 			if (show_flag(arg) && verify)
 				die_no_single_rev(quiet);
 			continue;
diff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh
index 492edffa9c..dc9fe3cbf1 100755
--- a/t/t1503-rev-parse-verify.sh
+++ b/t/t1503-rev-parse-verify.sh
@@ -144,4 +144,17 @@ test_expect_success SYMLINKS 'ref resolution not confused by broken symlinks' '
 	test_must_fail git rev-parse --verify broken
 '
 
+test_expect_success 'options can appear after --verify' '
+	git rev-parse --verify HEAD >expect &&
+	git rev-parse --verify -q HEAD >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'verify respects --end-of-options' '
+	git update-ref refs/heads/-tricky HEAD &&
+	git rev-parse --verify HEAD >expect &&
+	git rev-parse --verify --end-of-options -tricky >actual &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index 2ed5d50059..e2ae15a2cf 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -263,4 +263,20 @@ test_expect_success 'arg after dashdash not interpreted as option' '
 	test_cmp expect actual
 '
 
+test_expect_success 'arg after end-of-options not interpreted as option' '
+	test_must_fail git rev-parse --end-of-options --not-real -- 2>err &&
+	test_i18ngrep bad.revision.*--not-real err
+'
+
+test_expect_success 'end-of-options still allows --' '
+	cat >expect <<-EOF &&
+	--end-of-options
+	$(git rev-parse --verify HEAD)
+	--
+	path
+	EOF
+	git rev-parse --end-of-options HEAD -- path >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.29.2.640.g9e24689a4c

Re: [PATCH 3/3] rev-parse: handle --end-of-options

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This patch lets callers write:
>
>   git rev-parse --end-of-options "$rev" -- "$path"
>
> and:
>
>   git rev-parse --verify --end-of-options "$rev"
>
> which will both treat "$rev" always as a revision parameter.

Nice.  The only iffy case I can think of is that we can never have
"--" to specify a rev, because with "git cmd -- -- path" we don't
know which double-dash among the two is the disambiguator that makes
the other double-dash to be either rev or path, but that is not a
new problem with this change.

> +test_expect_success 'verify respects --end-of-options' '
> +	git update-ref refs/heads/-tricky HEAD &&
> +	git rev-parse --verify HEAD >expect &&
> +	git rev-parse --verify --end-of-options -tricky >actual &&
> +	test_cmp expect actual
> +'

;-)  Or refs/heads/--tricky?

The whole thing looked good.  Thanks.

Re: [PATCH 3/3] rev-parse: handle --end-of-options

[Demi M. Obenour]

[-- Attachment #1.1.1: Type: text/plain, Size: 758 bytes --]

On 11/10/20 5:23 PM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>> This patch lets callers write:
>>
>>   git rev-parse --end-of-options "$rev" -- "$path"
>>
>> and:
>>
>>   git rev-parse --verify --end-of-options "$rev"
>>
>> which will both treat "$rev" always as a revision parameter.
> 
> Nice.  The only iffy case I can think of is that we can never have
> "--" to specify a rev, because with "git cmd -- -- path" we don't
> know which double-dash among the two is the disambiguator that makes
> the other double-dash to be either rev or path, but that is not a
> new problem with this change.

I think Git should at least reject refs named "--", since they cause
problems in so many places.

Sincerely,

Demi

[-- Attachment #1.1.2: OpenPGP_0xB288B55FFF9C22C1.asc --]
[-- Type: application/pgp-keys, Size: 4041 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] rev-parse: handle --end-of-options

[Jeff King]

On Tue, Nov 10, 2020 at 02:23:34PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > This patch lets callers write:
> >
> >   git rev-parse --end-of-options "$rev" -- "$path"
> >
> > and:
> >
> >   git rev-parse --verify --end-of-options "$rev"
> >
> > which will both treat "$rev" always as a revision parameter.
> 
> Nice.  The only iffy case I can think of is that we can never have
> "--" to specify a rev, because with "git cmd -- -- path" we don't
> know which double-dash among the two is the disambiguator that makes
> the other double-dash to be either rev or path, but that is not a
> new problem with this change.

Yeah. It is sufficient to me that "rev-parse" (or any command) does not
do a bad thing when somebody malicious asks about "--", even if the repo
contents themselves are sane and do not have "--". If somebody has a
not-sane repo with refs/heads/-- and the worst case is that they have
trouble accessing it without a fully-qualified name, then I find it hard
to sympathize. ;)

> > +test_expect_success 'verify respects --end-of-options' '
> > +	git update-ref refs/heads/-tricky HEAD &&
> > +	git rev-parse --verify HEAD >expect &&
> > +	git rev-parse --verify --end-of-options -tricky >actual &&
> > +	test_cmp expect actual
> > +'
> 
> ;-)  Or refs/heads/--tricky?

Yeah, arguably that is a better name. It could even be a real option
name, though once the bug is fixed none of it matters. :)

-Peff