Re: [PATCH] Introduce a filter-path argument to git-daemon, for doing custom path transformations

From: Johan Sørensen <johan@johansorensen.com>
Date: Mar 12, 2009, 19:06 UTC
Message-ID: <9e0f31700903121206m3adbabacra655c5d340365f43@mail.gmail.com>
In-Reply-To: <alpine.DEB.1.00.0903121218000.10279@pacific.mpi-cbg.de>

On Thu, Mar 12, 2009 at 12:29 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

(all my comments below refer to my latest patch)

> More importantly, you might want to point out the security concerns of
> running a script with the full permissions of git-daemon.  (AFAICT from
> your patch you are not dropping any privileges at any point.)

Do you really think this is needed? It doesn't seem like running the hook scripts does anything more than trusting the script author and permissions of the hook scripts (?). I see the path-filter script exactly the same way, with the exception of having to double-check the user supplied path the script receives.

> Which brings me to another idea: we have quite a few places in Git where
> we use regular expressions.  Would they not be enough for your use case?

Hmm, please do elaborate on your idea. If you mean being able to supply a bunch of regexp mappings when starting the daemon then it wouldn't cut it for me; I'm in need of something more dynamic.

Show 5 quoted lines

>> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
>> index 36f00ae..efd1687 100644
>> --- a/Documentation/git-daemon.txt
>> +++ b/Documentation/git-daemon.txt
>> @@ -71,6 +72,18 @@ OPTIONS

[snip]

> Please keep the lines shorter than 81 characters.

I believe the longest line I've added in the docs is 77.

> But there is more: what about concurrent accesses?

The external path-filter script is run from the execute(), which is forked+exec'ed for each incoming connection to the daemon, so that would mean a concurrency of one in that child-process, unless I've missed something in the code path?

Show 6 quoted lines

>> +     read(filter_cmd.out, result, sizeof(result) - 1);
>
> No error checking?
>
> BTW we do have strbuf_read(), which would solve your "static char *"
> problem nicely.

I'm using strbuf_read() now, but this being my very first git patch, I may have misunderstood the api slightly?

> And your code would be even easier to read if your
> run_path_filter_script() would never return NULL, but the unchanged path
> instead.

Done.

Thanks for giving my patch the run-through. I'm still curious to hear what people think about the idea in general though!

>
> Ciao,
> Dscho
>

Cheers, JS