From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 0/3] --end-of-options marker
Date: Tue, 6 Aug 2019 13:33:50 -0400 [thread overview]
Message-ID: <20190806173349.GA4961@sigill.intra.peff.net> (raw)
In-Reply-To: <xmqqa7cml0s9.fsf@gitster-ct.c.googlers.com>
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
next prev parent reply other threads:[~2019-08-06 17:33 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-06 14:38 [PATCH 0/3] --end-of-options marker Jeff King
2019-08-06 14:39 ` [PATCH 1/3] revision: allow --end-of-options to end option parsing Jeff King
2019-08-06 14:40 ` [PATCH 2/3] parse-options: allow --end-of-options as a synonym for "--" Jeff King
2019-08-06 14:40 ` [PATCH 3/3] gitcli: document --end-of-options Jeff King
2019-08-06 16:24 ` [PATCH 0/3] --end-of-options marker Junio C Hamano
2019-08-06 16:36 ` Randall S. Becker
2019-08-06 17:38 ` Jeff King
2019-08-06 17:58 ` Randall S. Becker
2019-08-06 18:14 ` SZEDER Gábor
2019-08-08 10:03 ` Jeff King
2019-08-06 17:33 ` Jeff King [this message]
2019-08-06 22:58 ` brian m. carlson
2019-08-06 23:43 ` Jeff King
2019-08-07 4:17 ` brian m. carlson
2019-08-07 16:54 ` Taylor Blau
2019-08-08 10:28 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190806173349.GA4961@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).