Re: [RFC PATCH 0/2] upload-pack.c: limit allowed filter choices

[Taylor Blau <me@ttaylorr.com>]

On Wed, Mar 18, 2020 at 06:18:25AM -0400, Jeff King wrote:
> On Tue, Mar 17, 2020 at 02:39:05PM -0600, Taylor Blau wrote:
>
> > Hi Christian,
> >
> > Of course, I would be happy to send along our patches. They are included
> > in the series below, and correspond roughly to what we are running at
> > GitHub. (For us, there have been a few more clean-ups and additional
> > patches, but I squashed them into 2/2 below).
> >
> > The approach is roughly that we have:
> >
> >   - 'uploadpack.filter.allow' -> specifying the default for unspecified
> >     filter choices, itself defaulting to true in order to maintain
> >     backwards compatibility, and
> >
> >   - 'uploadpack.filter.<filter>.allow' -> specifying whether or not each
> >     filter kind is allowed or not. (Originally this was given as 'git
> >     config uploadpack.filter=blob:none.allow true', but this '=' is
> >     ambiguous to configuration given over '-c', which itself uses an '='
> >     to separate keys from values.)
>
> One thing that's a little ugly here is the embedded dot in the
> subsection (i.e., "filter.<filter>"). It makes it look like a four-level
> key, but really there is no such thing in Git.  But everything else we
> tried was even uglier.
>
> I think we want to declare a real subsection for each filter and not
> just "uploadpack.filter.<filter>". That gives us room to expand to other
> config options besides "allow" later on if we need to.
>
> We don't want to claim "uploadpack.allow" and "uploadpack.<filter>.allow";
> that's too generic.
>
> Likewise "filter.allow" is too generic.

I wonder. A multi-valued 'uploadpack.filter.allow' *might* solve some
problems, but the more I turn it over in my head, the more that I think
that it's creating more headaches for us than it's removing.

On the pro's side, is that we could have this be a multi-valued key
where each value is the name of an allowed filter. I guess that would
solve the subsection-naming problem, but it is admittedly generic, not
to mention the fact that we already *use* this key to specify a default
value for missing 'uploadpack.filter.<filter>.allow' values. For that
reason, it seems like a non-starter to me.

> We could do "uploadpackfilter.allow" and "uploadpackfilter.<filter>.allow",
> but that's both ugly _and_ separates these options from the rest of
> uploadpack.*.
>
> We could use a character besides ".", which would reduce confusion. But
> what? Using colon is kind of ugly, because it's already syntactically
> significant in filter names, and you get:
>
>   uploadpack.filter:blob:none.allow
>
> We tried equals, like:
>
>   uploadpack.filter=blob:none.allow
>
> but there's an interesting side effect. Doing:
>
>   git -c uploadpack.filter=blob:none.allow=true upload-pack ...
>
> doesn't work, because the "-c" parser ends the key at the first "=". As
> it should, because otherwise we'd get confused by an "=" in a value.
> This is a failing of the "-c" syntax; it can't represent values with
> "=". Fixing it would be awkward, and I've never seen it come up in
> practice outside of this (you _could_ have a branch with a funny name
> and try to do "git -c branch.my=funny=branch.remote=origin" or
> something, but the lack of bug reports suggests nobody is that
> masochistic).

Thanks for adding some more detail to this decision.

Another thing we could do is just simply use a different character. It
may be a little odd, but it keeps the filter-related variables in their
own sub-section, allowing us to add more configuration sub-variables in
the future. I guess that calling it something like:

  $ git config uploadpack.filter@blob:none.allow <true|false>

is a little strange (i.e., why '@' over '#'? There's certainly no
precedent here that I can think of...), but maybe it is slightly
less-weird than a pseudo-four-level key.

> So...maybe the extra dot is the last bad thing?
>
> > I noted in the second patch that there is the unfortunate possibility of
> > encountering a SIGPIPE when trying to write the ERR sideband back to a
> > client who requested a non-supported filter. Peff and I have had some
> > discussion off-list about resurrecting SZEDZER's work which makes room
> > in the buffer by reading one packet back from the client when the server
> > encounters a SIGPIPE. It is for this reason that I am marking the series
> > as 'RFC'.
>
> For reference, the patch I was thinking of was this:
>
>   https://lore.kernel.org/git/20190830121005.GI8571@szeder.dev/

Thanks.

> -Peff

Thanks,
Taylor