Re: [PATCH] use filetest pragma to work with ACL

[Guillaume Castagnino <casta@xwing.info>]

[-- Attachment #1: Type: text/plain, Size: 2245 bytes --]

Hi,

Le mercredi 18 octobre 2017 à 17:24 -0400, Jeff King a écrit :
> On Wed, Oct 18, 2017 at 07:55:31PM +0000, Guillaume Castagnino wrote:
> 
> > From: Guillaume Castagnino <casta@xwing.info>
> > [...]
> 
> Stefan raised a few meta issues, all of which I agree with. But I had
> some questions about the patch itself:
> 
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index 9208f42ed1753..0ee7f304ce2b1 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -3072,6 +3072,7 @@ sub git_get_projects_list {
> >  				# only directories can be git
> > repositories
> >  				return unless (-d $_);
> >  				# need search permission
> > +				use filetest 'access';
> >  				return unless (-x $_);
> 
> This "use" will unconditionally at compile-time (such as "compile" is
> for perl, anyway). Which raises a few questions:
> 
>   - would we want to use "require" instead to avoid loading when we
>     don't enter this function?

I was under the impression that as this is a pragma affecting perl
“compiler”, you have to always use “use”, not require, as the pragma
initialisation has to be done before code is run.

>   - If the answer to the above is "no" (e.g., because we basically
>     always need it; I didn't check), should it go at the top of the
>     script with the other "use" directives?
> 
>     I think this is a scoped pragma, so what you have here affects
> only
>     this particular "-x". But wouldn't other uses of "-x" potentially
>     want the same benefit?

I quickly grepped the code, I did not see other calls that could
benefits from the pragma, but it could be indeed nice to move it at the
top to avoid future issues with such tests and POSIX ACL.

>   - Do all relevant versions of perl ship with filetest? According to
>     Module::Corelist, it first shipped with perl 5.6. In general I
> think
>     we treat that as a minimum for our perl scripts, though I do
> notice
>     that the gitweb script says "use 5.008". I'm not sure how
> realistic
>     that is.

So the CGI already requires perl 5.8, so it’s fine I think.

Attached a cleanup proposal and moving the use at the top.

Thanks
Guillaume

-- 
Guillaume Castagnino
    casta@xwing.info / guillaume@castagnino.org

[-- Attachment #2: 0001-gitweb-use-filetest-to-allow-ACLs.patch --]
[-- Type: text/x-patch, Size: 842 bytes --]

From 4d5a082970063b34d3dbdf5b9a577624310057d6 Mon Sep 17 00:00:00 2001
From: Guillaume Castagnino <casta@xwing.info>
Date: Thu, 19 Oct 2017 09:32:46 +0200
Subject: [PATCH] gitweb: use filetest to allow ACLs

In commit 46a1385 (gitweb: skip unreadable subdirectories, 2017-07-18)
we forgot to handle non-unix ACLs as well. Fix this.

Signed-off-by: Guillaume Castagnino <casta@xwing.info>
---
 gitweb/gitweb.perl | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 9208f42ed..6ac49eaf3 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -10,6 +10,8 @@
 use 5.008;
 use strict;
 use warnings;
+# handle ACL in file access tests
+use filetest 'access';
 use CGI qw(:standard :escapeHTML -nosticky);
 use CGI::Util qw(unescape);
 use CGI::Carp qw(fatalsToBrowser set_message);
-- 
2.14.2