[PATCH 0/3] --end-of-options marker

[PATCH 0/3] --end-of-options marker

[Jeff King]

It's hard for scripted uses of rev-list, etc, to avoid option injection
from untrusted arguments, because revision arguments must come before
any "--" separator. I.e.:

  git rev-list "$revision" -- "$path"

might mistake "$revision" for an option (with rev-list, that would make
it an error, but something like git-log would default to HEAD).

This series provides an alternative to "--" to stop option parsing
without indicating that further arguments are pathspecs.

  [1/3]: revision: allow --end-of-options to end option parsing
  [2/3]: parse-options: allow --end-of-options as a synonym for "--"
  [3/3]: gitcli: document --end-of-options

 Documentation/gitcli.txt | 6 ++++++
 parse-options.c          | 3 ++-
 revision.c               | 8 +++++++-
 t/t0040-parse-options.sh | 7 +++++++
 t/t4202-log.sh           | 7 +++++++
 t/t6000-rev-list-misc.sh | 8 ++++++++
 6 files changed, 37 insertions(+), 2 deletions(-)

[PATCH 1/3] revision: allow --end-of-options to end option parsing

[Jeff King]

There's currently no robust way to tell Git that a particular option is
meant to be a revision, and not an option. So if you have a branch
"refs/heads/--foo", you cannot just say:

  git rev-list --foo

You can say:

  git rev-list refs/heads/--foo

But that breaks down if you don't know the refname, and in particular if
you're a script passing along a value from elsewhere. In most programs,
you can use "--" to end option parsing, like this:

  some-prog -- "$revision"

But that doesn't work for the revision parser, because "--" is already
meaningful there: it separates revisions from pathspecs. So we need some
other marker to separate options from revisions.

This patch introduces "--end-of-options", which serves that purpose:

  git rev-list --oneline --end-of-options "$revision"

will work regardless of what's in "$revision" (well, if you say "--" it
may fail, but it won't do something dangerous, like triggering an
unexpected option).

The name is verbose, but that's probably a good thing; this is meant to
be used for scripted invocations where readability is more important
than terseness.

One alternative would be to introduce an explicit option to mark a
revision, like:

  git rev-list --oneline --revision="$revision"

That's slightly _more_ informative than this commit (because it makes
even something silly like "--" unambiguous). But the pattern of using a
separator like "--" is well established in git and in other commands,
and it makes some scripting tasks simpler like:

  git rev-list --end-of-options "$@"

There's no documentation in this patch, because it will make sense to
describe the feature once it is available everywhere (and support will
be added in further patches).

Signed-off-by: Jeff King <peff@peff.net>
---
 revision.c               | 8 +++++++-
 t/t6000-rev-list-misc.sh | 8 ++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index 07412297f0..51690e480d 100644
--- a/revision.c
+++ b/revision.c
@@ -2523,6 +2523,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	int i, flags, left, seen_dashdash, got_rev_arg = 0, revarg_opt;
 	struct argv_array prune_data = ARGV_ARRAY_INIT;
 	const char *submodule = NULL;
+	int seen_end_of_options = 0;
 
 	if (opt)
 		submodule = opt->submodule;
@@ -2552,7 +2553,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		revarg_opt |= REVARG_CANNOT_BE_FILENAME;
 	for (left = i = 1; i < argc; i++) {
 		const char *arg = argv[i];
-		if (*arg == '-') {
+		if (!seen_end_of_options && *arg == '-') {
 			int opts;
 
 			opts = handle_revision_pseudo_opt(submodule,
@@ -2574,6 +2575,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 				continue;
 			}
 
+			if (!strcmp(arg, "--end-of-options")) {
+				seen_end_of_options = 1;
+				continue;
+			}
+
 			opts = handle_revision_opt(revs, argc - i, argv + i,
 						   &left, argv, opt);
 			if (opts > 0) {
diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 52a9e38d66..b8cf82349b 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh
@@ -140,4 +140,12 @@ test_expect_success '--header shows a NUL after each commit' '
 	test_cmp expect actual
 '
 
+test_expect_success 'rev-list --end-of-options' '
+	git update-ref refs/heads/--output=yikes HEAD &&
+	git rev-list --end-of-options --output=yikes >actual &&
+	test_path_is_missing yikes &&
+	git rev-list HEAD >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.23.0.rc1.436.g24d2e81391

[PATCH 2/3] parse-options: allow --end-of-options as a synonym for "--"

[Jeff King]

The revision option parser recently learned about --end-of-options, but
that's not quite enough for all callers. Some of them, like git-log,
pick out some options using parse_options(), and then feed the remainder
to setup_revisions(). For those cases we need to stop parse_options()
from finding more options when it sees --end-of-options, and to retain
that option in argv so that setup_revisions() can see it as well.

Let's handle this the same as we do "--". We can even piggy-back on the
handling of PARSE_OPT_KEEP_DASHDASH, because any caller that wants to
retain one will want to retain the other.

I've included two tests here. The "log" test covers "--source", which is
one of the options it handles with parse_options(), and would fail
before this patch. There's also a test that uses the parse-options
helper directly. That confirms that the option is handled correctly even
in cases without KEEP_DASHDASH or setup_revisions(). I.e., it is safe to
use --end-of-options in place of "--" in other programs.

Signed-off-by: Jeff King <peff@peff.net>
---
 parse-options.c          | 3 ++-
 t/t0040-parse-options.sh | 7 +++++++
 t/t4202-log.sh           | 7 +++++++
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/parse-options.c b/parse-options.c
index 87b26a1d92..b42f54d48b 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -780,7 +780,8 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 			continue;
 		}
 
-		if (!arg[2]) { /* "--" */
+		if (!arg[2] /* "--" */ ||
+		    !strcmp(arg + 2, "end-of-options")) {
 			if (!(ctx->flags & PARSE_OPT_KEEP_DASHDASH)) {
 				ctx->argc--;
 				ctx->argv++;
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index cebc77fab0..705a136ed9 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -399,4 +399,11 @@ test_expect_success 'GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS works' '
 		test-tool parse-options --ye
 '
 
+test_expect_success '--end-of-options treats remainder as args' '
+	test-tool parse-options \
+	    --expect="verbose: -1" \
+	    --expect="arg 00: --verbose" \
+	    --end-of-options --verbose
+'
+
 test_done
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index c20209324c..e88ccb04a9 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1707,4 +1707,11 @@ test_expect_success '--exclude-promisor-objects does not BUG-crash' '
 	test_must_fail git log --exclude-promisor-objects source-a
 '
 
+test_expect_success 'log --end-of-options' '
+       git update-ref refs/heads/--source HEAD &&
+       git log --end-of-options --source >actual &&
+       git log >expect &&
+       test_cmp expect actual
+'
+
 test_done
-- 
2.23.0.rc1.436.g24d2e81391

[PATCH 3/3] gitcli: document --end-of-options

[Jeff King]

Now that --end-of-options is available for any users of
setup_revisions() or parse_options(), which should be effectively
everywhere, we can guide people to use it for all their disambiguating
needs.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/gitcli.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt
index 1ed3ca33b7..4b32876b6e 100644
--- a/Documentation/gitcli.txt
+++ b/Documentation/gitcli.txt
@@ -37,6 +37,12 @@ arguments.  Here are the rules:
    file called HEAD in your work tree, `git diff HEAD` is ambiguous, and
    you have to say either `git diff HEAD --` or `git diff -- HEAD` to
    disambiguate.
+
+ * Because `--` disambiguates revisions and paths in some commands, it
+   cannot be used for those commands to separate options and revisions.
+   You can use `--end-of-options` for this (it also works for commands
+   that do not distinguish between revisions in paths, in which case it
+   is simply an alias for `--`).
 +
 When writing a script that is expected to handle random user-input, it is
 a good practice to make it explicit which arguments are which by placing
-- 
2.23.0.rc1.436.g24d2e81391

Re: [PATCH 0/3] --end-of-options marker

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It's hard for scripted uses of rev-list, etc, to avoid option injection
> from untrusted arguments, because revision arguments must come before
> any "--" separator. I.e.:
>
>   git rev-list "$revision" -- "$path"
>
> might mistake "$revision" for an option (with rev-list, that would make
> it an error, but something like git-log would default to HEAD).

Just to make sure I understand what I just read, let me paraphrase.
We would want to accept

	git rev-list --max-parents=4 \
		--end-of-options \
		--count -- docs/

so that '--count' would go thru the usual "as we have -- later, it
must be a rev and we do not even disambiguate.  What does get_sha1()
say it is?" and "docs/" would be taken as a pathspec.

"git rev-list --max-parents=4 --count -- docs/" would have treated
"--count" as an option and would error out due to lack of any
starting revision.

On the other hand, "git log --count -- docs/" would take "--count"
as an option, but does not complain about lack of any revs.  It just
starts digging from HEAD and ends up ignoring the "--count" branch
(or is this feature meant to support tags?  As far as I recall, we
do not allow branch names that begin with a dash).

> This series provides an alternative to "--" to stop option parsing
> without indicating that further arguments are pathspecs.
>
>   [1/3]: revision: allow --end-of-options to end option parsing
>   [2/3]: parse-options: allow --end-of-options as a synonym for "--"
>   [3/3]: gitcli: document --end-of-options
>
>  Documentation/gitcli.txt | 6 ++++++
>  parse-options.c          | 3 ++-
>  revision.c               | 8 +++++++-
>  t/t0040-parse-options.sh | 7 +++++++
>  t/t4202-log.sh           | 7 +++++++
>  t/t6000-rev-list-misc.sh | 8 ++++++++
>  6 files changed, 37 insertions(+), 2 deletions(-)

RE: [PATCH 0/3] --end-of-options marker

[Randall S. Becker]

On August 6, 2019 12:25 PM, Junio wrote:
> Jeff King <peff@peff.net> writes:
> 
> > It's hard for scripted uses of rev-list, etc, to avoid option
> > injection from untrusted arguments, because revision arguments must
> > come before any "--" separator. I.e.:
> >
> >   git rev-list "$revision" -- "$path"
> >
> > might mistake "$revision" for an option (with rev-list, that would
> > make it an error, but something like git-log would default to HEAD).
> 
> Just to make sure I understand what I just read, let me paraphrase.
> We would want to accept
> 
> 	git rev-list --max-parents=4 \
> 		--end-of-options \
> 		--count -- docs/
> 
> so that '--count' would go thru the usual "as we have -- later, it must be
a rev
> and we do not even disambiguate.  What does get_sha1() say it is?" and
> "docs/" would be taken as a pathspec.
> "git rev-list --max-parents=4 --count -- docs/" would have treated
"--count"
> as an option and would error out due to lack of any starting revision.
> 
> On the other hand, "git log --count -- docs/" would take "--count"
> as an option, but does not complain about lack of any revs.  It just
starts
> digging from HEAD and ends up ignoring the "--count" branch (or is this
> feature meant to support tags?  As far as I recall, we do not allow branch
> names that begin with a dash).
> 
> > This series provides an alternative to "--" to stop option parsing
> > without indicating that further arguments are pathspecs.

Would this offer the opportunity to, in the long term, supply options to
external diff engines, for example?

Something like git diff --end-of-options --diff-opt1 --diff-opt2 -- a b

I'm just noodling here, wondering why otherwise

git rev-list --max-parents=4  -- --count docs/

does not work. I thought -- was pretty specific in terms of turning off
interpretation. So is it not a defect that --count is being interpreted?

I have a fear for all my sub-teams who script with the assumption that --
has a specific meaning of stopping interpretation.

Slightly confused,
Randall

> >
> >   [1/3]: revision: allow --end-of-options to end option parsing
> >   [2/3]: parse-options: allow --end-of-options as a synonym for "--"
> >   [3/3]: gitcli: document --end-of-options
> >
> >  Documentation/gitcli.txt | 6 ++++++
> >  parse-options.c          | 3 ++-
> >  revision.c               | 8 +++++++-
> >  t/t0040-parse-options.sh | 7 +++++++
> >  t/t4202-log.sh           | 7 +++++++
> >  t/t6000-rev-list-misc.sh | 8 ++++++++
> >  6 files changed, 37 insertions(+), 2 deletions(-)

Re: [PATCH 0/3] --end-of-options marker

[Jeff King]

On Tue, Aug 06, 2019 at 09:24:38AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > It's hard for scripted uses of rev-list, etc, to avoid option injection
> > from untrusted arguments, because revision arguments must come before
> > any "--" separator. I.e.:
> >
> >   git rev-list "$revision" -- "$path"
> >
> > might mistake "$revision" for an option (with rev-list, that would make
> > it an error, but something like git-log would default to HEAD).
> 
> Just to make sure I understand what I just read, let me paraphrase.
> We would want to accept
> 
> 	git rev-list --max-parents=4 \
> 		--end-of-options \
> 		--count -- docs/
> 
> so that '--count' would go thru the usual "as we have -- later, it
> must be a rev and we do not even disambiguate.  What does get_sha1()
> say it is?" and "docs/" would be taken as a pathspec.

Yes, that's how I'd expect that to be parsed.

> "git rev-list --max-parents=4 --count -- docs/" would have treated
> "--count" as an option and would error out due to lack of any
> starting revision.

Right. Though some options may have an impact even before rev-list would
complain. For instance --output=foo (which is actually a diff option,
but revision.c handles both) opens and truncates "foo" before rev-list
realizes it doesn't have enough options.

> On the other hand, "git log --count -- docs/" would take "--count"
> as an option, but does not complain about lack of any revs.  It just
> starts digging from HEAD and ends up ignoring the "--count" branch

Correct.

> (or is this feature meant to support tags?  As far as I recall, we
> do not allow branch names that begin with a dash).

We do forbid them via "git branch", but they are not forbidden by
check-ref-format. So:

  git update-ref refs/heads/-foo $oid

works fine, and receive-pack is happy to accept it as a push.  I think
it might be reasonable to forbid that, but we'd have to accept potential
fallouts.

That said, for the purposes of option injection, it actually doesn't
matter whether the ref exists or not (or if it's even an allowed name).
The problem is that the caller is feeding what it _thinks_ will be
interpreted as a rev name, but it isn't. So yes, most of the time
treating "--count" as a rev is nonsense and will fail. But the important
thing is not so much that we do the right thing when you have a branch
(or tag) with a funny name, but that we _don't_ do something dangerous
or unexpected.

-Peff

Re: [PATCH 0/3] --end-of-options marker

[Jeff King]

On Tue, Aug 06, 2019 at 12:36:26PM -0400, Randall S. Becker wrote:

> > > This series provides an alternative to "--" to stop option parsing
> > > without indicating that further arguments are pathspecs.
> 
> Would this offer the opportunity to, in the long term, supply options to
> external diff engines, for example?
> 
> Something like git diff --end-of-options --diff-opt1 --diff-opt2 -- a b

I'd expect that to interpret "--diff-opt1" and "--diff-opt2" as
non-option arguments, which in the context of git-diff means endpoints
of the diff.

So no, I don't think you can use it like you're asking here.

> I'm just noodling here, wondering why otherwise
> 
> git rev-list --max-parents=4  -- --count docs/
> 
> does not work. I thought -- was pretty specific in terms of turning off
> interpretation. So is it not a defect that --count is being interpreted?

The command-line above means that "--count" is interpreted
(unambiguously) as a path. The problem is that if you want it to be
interpreted as a starting point for traversal, then it must come
_before_ the "--".

> I have a fear for all my sub-teams who script with the assumption that --
> has a specific meaning of stopping interpretation.

Nothing about "--" is changed by my series; it will still stop option
interpretation in rev-list and in other commands. But as before,
rev-list (and other Git commands that use the revision.c parser) use it
to separate revisions and pathspecs.  That's unlike how most other
programs use "--", but that ship sailed for Git in 2005.

-Peff

RE: [PATCH 0/3] --end-of-options marker

[Randall S. Becker]

On August 6, 2019 1:38 PM, Jeff King wrote:
> To: Randall S. Becker <rsbecker@nexbridge.com>
> Cc: 'Junio C Hamano' <gitster@pobox.com>; git@vger.kernel.org
> Subject: Re: [PATCH 0/3] --end-of-options marker
> 
> On Tue, Aug 06, 2019 at 12:36:26PM -0400, Randall S. Becker wrote:
> 
> > > > This series provides an alternative to "--" to stop option parsing
> > > > without indicating that further arguments are pathspecs.
> >
> > Would this offer the opportunity to, in the long term, supply options
> > to external diff engines, for example?
> >
> > Something like git diff --end-of-options --diff-opt1 --diff-opt2 -- a
> > b
> 
> I'd expect that to interpret "--diff-opt1" and "--diff-opt2" as non-option
> arguments, which in the context of git-diff means endpoints of the diff.
> 
> So no, I don't think you can use it like you're asking here.
> 
> > I'm just noodling here, wondering why otherwise
> >
> > git rev-list --max-parents=4  -- --count docs/
> >
> > does not work. I thought -- was pretty specific in terms of turning
> > off interpretation. So is it not a defect that --count is being interpreted?
> 
> The command-line above means that "--count" is interpreted
> (unambiguously) as a path. The problem is that if you want it to be
> interpreted as a starting point for traversal, then it must come _before_ the
> "--".
> 
> > I have a fear for all my sub-teams who script with the assumption that
> > -- has a specific meaning of stopping interpretation.
> 
> Nothing about "--" is changed by my series; it will still stop option
> interpretation in rev-list and in other commands. But as before, rev-list (and
> other Git commands that use the revision.c parser) use it to separate
> revisions and pathspecs.  That's unlike how most other programs use "--", but
> that ship sailed for Git in 2005.

Thanks for the explanation.

Randall

Re: [PATCH 0/3] --end-of-options marker

[SZEDER Gábor]

On Tue, Aug 06, 2019 at 01:38:17PM -0400, Jeff King wrote:
> Nothing about "--" is changed by my series; it will still stop option
> interpretation in rev-list and in other commands. But as before,
> rev-list (and other Git commands that use the revision.c parser) use it
> to separate revisions and pathspecs.  That's unlike how most other
> programs use "--", but that ship sailed for Git in 2005.

I'd like to draw attention to the oddball 'git filter-branch' command,
which uses '--' as a separator between 'filter-branch' and 'rev-list'
options.  Will it still work with this new option?  I think it will,
but not sure.

Re: [PATCH 0/3] --end-of-options marker

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1016 bytes --]

On 2019-08-06 at 14:38:30, Jeff King wrote:
> It's hard for scripted uses of rev-list, etc, to avoid option injection
> from untrusted arguments, because revision arguments must come before
> any "--" separator. I.e.:
> 
>   git rev-list "$revision" -- "$path"
> 
> might mistake "$revision" for an option (with rev-list, that would make
> it an error, but something like git-log would default to HEAD).
> 
> This series provides an alternative to "--" to stop option parsing
> without indicating that further arguments are pathspecs.

Sorry, I hadn't had a chance to look at this series in depth, but I was
wondering: could we not just accept two separate "--" arguments, and if
there are two of them, interpret the first with the traditional meaning
and the second with the Git-specific meaning? That would be much more
intuitive for folks, although I suspect it would take a little more work
in the options parser.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [PATCH 0/3] --end-of-options marker

[Jeff King]

On Tue, Aug 06, 2019 at 10:58:53PM +0000, brian m. carlson wrote:

> On 2019-08-06 at 14:38:30, Jeff King wrote:
> > It's hard for scripted uses of rev-list, etc, to avoid option injection
> > from untrusted arguments, because revision arguments must come before
> > any "--" separator. I.e.:
> > 
> >   git rev-list "$revision" -- "$path"
> > 
> > might mistake "$revision" for an option (with rev-list, that would make
> > it an error, but something like git-log would default to HEAD).
> > 
> > This series provides an alternative to "--" to stop option parsing
> > without indicating that further arguments are pathspecs.
> 
> Sorry, I hadn't had a chance to look at this series in depth, but I was
> wondering: could we not just accept two separate "--" arguments, and if
> there are two of them, interpret the first with the traditional meaning
> and the second with the Git-specific meaning? That would be much more
> intuitive for folks, although I suspect it would take a little more work
> in the options parser.

That also crossed my mind, but I think it opens up some complicated
corner cases.  For instance, if I'm parsing left-to-right and see "--",
how do I know which separator it is meant to be? I think the only rule
that makes sense is that you must have two "--", like:

  git rev-list [options] -- [revs] -- [paths]

but that means parsing the whole thing before we can interpret any of
it. What kinds of tricks can an attacker play by putting "--" in the
revs or paths areas? E.g., what does this mean:

  # expanded from "git rev-list -- $revs -- $paths"
  git rev-list -- --foo -- -- --bar --

I think if we at least choose the left-most "--" as the official
end-of-options then they can't inject an option (they can only inject a
rev as a path). I guess that's the same as with --end-of-options. But it
somehow feels less clear to me than a separate marker.

It also doesn't allow this:

  # allow paths and revs, with optional separator, but no more options
  git rev-list --end-of-options "$@"

though I'm not sure whether anybody cares.

-Peff

Re: [PATCH 0/3] --end-of-options marker

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2321 bytes --]

On 2019-08-06 at 23:43:20, Jeff King wrote:
> On Tue, Aug 06, 2019 at 10:58:53PM +0000, brian m. carlson wrote:
> > Sorry, I hadn't had a chance to look at this series in depth, but I was
> > wondering: could we not just accept two separate "--" arguments, and if
> > there are two of them, interpret the first with the traditional meaning
> > and the second with the Git-specific meaning? That would be much more
> > intuitive for folks, although I suspect it would take a little more work
> > in the options parser.
> 
> That also crossed my mind, but I think it opens up some complicated
> corner cases.  For instance, if I'm parsing left-to-right and see "--",
> how do I know which separator it is meant to be? I think the only rule
> that makes sense is that you must have two "--", like:
> 
>   git rev-list [options] -- [revs] -- [paths]

I was assuming that we wouldn't have a huge number of command-line
arguments and we'd check ahead, although that could of course cause some
pain when used with xargs, I suppose, especially on Linux with its huge
ARG_MAX.

> but that means parsing the whole thing before we can interpret any of
> it. What kinds of tricks can an attacker play by putting "--" in the
> revs or paths areas? E.g., what does this mean:
> 
>   # expanded from "git rev-list -- $revs -- $paths"
>   git rev-list -- --foo -- -- --bar --
> 
> I think if we at least choose the left-most "--" as the official
> end-of-options then they can't inject an option (they can only inject a
> rev as a path). I guess that's the same as with --end-of-options. But it
> somehow feels less clear to me than a separate marker.

I suppose if there's more than two, then interpret the first one as the
end-of-options marker, the second one in the traditional way, and any
subsequent ones as pathspecs matching the file "--". Writing such a
command line would be silly, but we'd fail secure.

> It also doesn't allow this:
> 
>   # allow paths and revs, with optional separator, but no more options
>   git rev-list --end-of-options "$@"
> 
> though I'm not sure whether anybody cares.

That's a good point. I don't have a strong view either way, but I
thought I'd ask about alternatives.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [PATCH 0/3] --end-of-options marker

[Taylor Blau]

On Wed, Aug 07, 2019 at 04:17:49AM +0000, brian m. carlson wrote:
> On 2019-08-06 at 23:43:20, Jeff King wrote:
> > On Tue, Aug 06, 2019 at 10:58:53PM +0000, brian m. carlson wrote:
> > > Sorry, I hadn't had a chance to look at this series in depth, but I was
> > > wondering: could we not just accept two separate "--" arguments, and if
> > > there are two of them, interpret the first with the traditional meaning
> > > and the second with the Git-specific meaning? That would be much more
> > > intuitive for folks, although I suspect it would take a little more work
> > > in the options parser.
> >
> > That also crossed my mind, but I think it opens up some complicated
> > corner cases.  For instance, if I'm parsing left-to-right and see "--",
> > how do I know which separator it is meant to be? I think the only rule
> > that makes sense is that you must have two "--", like:
> >
> >   git rev-list [options] -- [revs] -- [paths]
>
> I was assuming that we wouldn't have a huge number of command-line
> arguments and we'd check ahead, although that could of course cause some
> pain when used with xargs, I suppose, especially on Linux with its huge
> ARG_MAX.
>
> > but that means parsing the whole thing before we can interpret any of
> > it. What kinds of tricks can an attacker play by putting "--" in the
> > revs or paths areas? E.g., what does this mean:
> >
> >   # expanded from "git rev-list -- $revs -- $paths"
> >   git rev-list -- --foo -- -- --bar --
> >
> > I think if we at least choose the left-most "--" as the official
> > end-of-options then they can't inject an option (they can only inject a
> > rev as a path). I guess that's the same as with --end-of-options. But it
> > somehow feels less clear to me than a separate marker.

This is definitely the secure option among the two, but I'm not sure
that it makes me feel better about this alternative direction. I dislike
the ambiguity in having two '--'s, and I don't think that this is
something we ought to concern callers with.

'--end-of-options' is on the one hand, cumbersome to write, but it is
clear. I think that this is an acceptable trade-off, because we don't
expect users at the command line to ever type this. So, some extra
clarity in favor of a drop in convenience for a supposedly smaller
number of use cases seems like a favorable trade-off to me.

> I suppose if there's more than two, then interpret the first one as the
> end-of-options marker, the second one in the traditional way, and any
> subsequent ones as pathspecs matching the file "--". Writing such a
> command line would be silly, but we'd fail secure.
>
> > It also doesn't allow this:
> >
> >   # allow paths and revs, with optional separator, but no more options
> >   git rev-list --end-of-options "$@"
> >
> > though I'm not sure whether anybody cares.
>
> That's a good point. I don't have a strong view either way, but I
> thought I'd ask about alternatives.
> --
> brian m. carlson: Houston, Texas, US
> OpenPGP: https://keybase.io/bk2204

Thanks,
Taylor

Re: [PATCH 0/3] --end-of-options marker

[Jeff King]

On Tue, Aug 06, 2019 at 08:14:59PM +0200, SZEDER Gábor wrote:

> On Tue, Aug 06, 2019 at 01:38:17PM -0400, Jeff King wrote:
> > Nothing about "--" is changed by my series; it will still stop option
> > interpretation in rev-list and in other commands. But as before,
> > rev-list (and other Git commands that use the revision.c parser) use it
> > to separate revisions and pathspecs.  That's unlike how most other
> > programs use "--", but that ship sailed for Git in 2005.
> 
> I'd like to draw attention to the oddball 'git filter-branch' command,
> which uses '--' as a separator between 'filter-branch' and 'rev-list'
> options.  Will it still work with this new option?  I think it will,
> but not sure.

Good question.

Certainly "--" will work as it did before, since the code here only
changes behavior when it sees --end-of-options.

filter-branch doesn't use any of our parseopt infrastructure itself, so
it won't understand the new option itself. I.e., this won't work[1]:

  git filter-branch --end-of-options -this-is-a-branch-name

But since it passes rev-list options as-is, this does successfully pass
the name to rev-list:

  git filter-branch -- --end-of-options -this-is-a-branch-name

However, filter-branch itself seems to do some magic of its own with the
rev-list options, and will try to filter HEAD in that case. So I think
it needs further work to cover all cases correctly.

-Peff

[1] I think the first one there would work if filter-branch actually
    used "rev-parse --parseopt". E.g.:

      git rev-parse --parseopt -- --foo --end-of-options --bar <<-\EOF
      cmd
      --
      foo    an option
      bar    another option
      EOF

    yields:

      set -- --foo -- '--bar'

Re: [PATCH 0/3] --end-of-options marker

[Jeff King]

On Wed, Aug 07, 2019 at 04:17:49AM +0000, brian m. carlson wrote:

> > I think if we at least choose the left-most "--" as the official
> > end-of-options then they can't inject an option (they can only inject a
> > rev as a path). I guess that's the same as with --end-of-options. But it
> > somehow feels less clear to me than a separate marker.
> 
> I suppose if there's more than two, then interpret the first one as the
> end-of-options marker, the second one in the traditional way, and any
> subsequent ones as pathspecs matching the file "--". Writing such a
> command line would be silly, but we'd fail secure.

Yeah, I think that could work. I'd be a little concerned that the
implementation would end up complicated and confusing, just because
there are other parts of the code that treat "--" specially. That's not
a necessarily a reason to avoid it if there's a compelling reason, but I
think I favor a unique marker anyway (or at least am otherwise
ambivalent).

> That's a good point. I don't have a strong view either way, but I
> thought I'd ask about alternatives.

Discussion of alternatives is very welcome.

I think the most compelling alternative is the one I pointed out in one
of the commit messages:

  git rev-list --revision=<whatever>

which lets normal left-to-right parsing work without any complex
reasoning. It is harder to use with "$@", though.

Related, my proposal doesn't do anything for rev-parse. I think that:

  git rev-parse --end-of-options -xyz

should probably return:

  --end-of-options
  <oid of -xyz>

but I mostly consider that kind of use of rev-parse (pretending to be an
options parser for rev-list) to be vestigial. The main use of rev-parse
(in my experience) is "rev-parse --verify" to resolve a single name.

There are still some gaps there. For instance:

  git rev-parse --verify --foo

will treat "--foo" as an option (and then complain that there was no rev
argument). I don't think you can do anything too mischievous from this,
but it might be nice to tighten it up. I'm tempted to say that
"--verify" should complain if there isn't exactly one argument, but
technically things like this do work:

  git rev-parse --verify --sq "$rev"

  git rev-parse --verify --symbolic-full-name "$rev"

I don't know if anybody cares or not. We could perhaps work around it by
having --verify treat the final argument as a non-option, even if it
starts with "-". That would allow those cases, but:

  git rev-parse --verify --symbolic-full-name

would treat the latter as an argument (and currently that's always an
error anyway). Looking at rev-parse, there are other weird bits to
--verify, too. E.g., this:

  git rev-parse --verify a...b c

shows a...b, ignoring that --verify was given, and then eventually "c".

-Peff