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 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 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
git@vger.kernel.org list mirror (unofficial, one of many) This inbox may be cloned and mirrored by anyone: git clone --mirror https://public-inbox.org/git git clone --mirror http://ou63pmih66umazou.onion/git git clone --mirror http://czquwvybam4bgbro.onion/git git clone --mirror http://hjrcffqmbrq6wope.onion/git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V1 git git/ https://public-inbox.org/git \ git@vger.kernel.org public-inbox-index git Example config snippet for mirrors. Newsgroups are available over NNTP: nntp://news.public-inbox.org/inbox.comp.version-control.git nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git nntp://news.gmane.io/gmane.comp.version-control.git note: .onion URLs require Tor: https://www.torproject.org/ code repositories for the project(s) associated with this inbox: https://80x24.org/mirrors/git.git AGPL code for this site: git clone https://public-inbox.org/public-inbox.git