From: Jeff King <peff@peff.net>
Date: Wed, 18 Mar 2020 10:18:25 GMT
Subject: Re: [RFC PATCH 0/2] upload-pack.c: limit allowed filter choices
Message-ID: <20200318101825.GB1227946@coredump.intra.peff.net>
In-Reply-To: <cover.1584477196.git.me@ttaylorr.com>

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.

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).

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/

-Peff