Skip Menu |

Preferred bug tracker

Please visit the preferred bug tracker to report your issue.

This queue is for tickets about the Perl-Critic CPAN distribution.

Report information
The Basics
Id: 37881
Status: resolved
Priority: 0/
Queue: Perl-Critic
People
Owner: Nobody in particular
Requestors: EDAVIS [...] cpan.org
Cc:
AdminCc:
Bug Information
Severity: Wishlist
Broken in: (no value)
Fixed in: (no value)

History Show all quoted text
  Wed Jul 23 10:38:28 2008 EDAVIS [...] cpan.org - Ticket created
Subject: Feature: unsafe policies
As I understand it Perl::Critic parses the program with PPI. It would be useful to have a check that 'perl -c' passes. This is not much of a policy, since the program has to be syntactically correct to run at all, but it can still be useful when batch-verifying programs (as in a version control system pre-commit hook or as part of a test suite). Now 'perl -c' evaluates some of the code so it is not safe to run it on arbitrary input. So I suggest this and similar policies would be turned on with an --unsafe command line flag. Another unsafe policy would be to check that Perl programs support --help, --version etc. (PBP page 303.) To do this you must run the program and see what it does. If you like the idea I'll write a patch when I have some time.
  Wed Jul 23 11:03:52 2008 perl [...] galumph.com - Correspondence added
Subject: Re: [rt.cpan.org #37881] Feature: unsafe policies
Date: Wed, 23 Jul 2008 08:03:24 -0700
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <perl [...] galumph.com>
EDAVIS via RT wrote: Show quoted text
> As I understand it Perl::Critic parses the program with PPI.
See Perl::Critic::Dynamic.
  Wed Jul 23 11:03:56 2008 The RT System itself - Status changed from 'new' to 'open'
  Wed Jul 23 11:46:20 2008 EDAVIS [...] cpan.org - Correspondence added
Show quoted text
>See Perl::Critic::Dynamic.
Yes, that's exactly what I was looking for. I see that Perl::Critic::Policy::Dynamic::ValidateAgainstSymbolTable for example has a stern warning against using it on untrusted code. I still think it would be useful to move that warning into the code rather than the documentation. Policies could mark themselves as 'unsafe' and perlcritic will not include them unless --unsafe is passed on the command line.
  Wed Jul 23 11:47:13 2008 perl [...] galumph.com - Correspondence added
Subject: Re: [rt.cpan.org #37881] Feature: unsafe policies
Date: Wed, 23 Jul 2008 08:46:53 -0700
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <perl [...] galumph.com>
Elliot Shank via RT wrote: Show quoted text
> EDAVIS via RT wrote:
>> As I understand it Perl::Critic parses the program with PPI.
> > See Perl::Critic::Dynamic.
Also, there is no way something unsafe is going into core. Hell, I don't include P::C::Dynamic in {Task|Bundle}::P::C. (I do mention it in the docs, though.)
  Wed Jul 23 11:51:59 2008 perl [...] galumph.com - Correspondence added
Subject: Re: [rt.cpan.org #37881] Feature: unsafe policies
Date: Wed, 23 Jul 2008 08:51:48 -0700
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <perl [...] galumph.com>
EDAVIS via RT wrote: Show quoted text
> Policies could mark themselves as 'unsafe' and perlcritic will not > include them unless --unsafe is passed on the command line.
No. Not if I have anything to say about it.
  Wed Jul 23 17:57:51 2008 jeff [...] imaginative-software.com - Correspondence added
Subject: Re: [rt.cpan.org #37881] Feature: unsafe policies
Date: Wed, 23 Jul 2008 14:57:33 -0700
To: bug-Perl-Critic [...] rt.cpan.org
From: Jeffrey Thalhammer <jeff [...] imaginative-software.com>
On Jul 23, 2008, at 8:46 AM, EDAVIS via RT wrote: Show quoted text
> Queue: Perl-Critic > Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=37881 > > > I still think it would be useful to move that warning into the code > rather than the documentation. Policies could mark themselves as > 'unsafe' and perlcritic will not include them unless --unsafe is > passed > on the command line.
Hi Ed- As Elliot said, Perl-Critic-Dynamic is where we've decided to put any non-static (i.e. "unsafe") Policies. I think a Policy that simply checks whether the file compiles would be a nice addition to that distro (even though all Dynamic policies would probably do this implicitly anyway). If you care to write such a Policy, I'd be happy to ship it in the Dynamic distro. You'll have to deal with @INC in some way -- see Perl::Critic::Policy::Dynamic::ValidateAgainstSymbolTable for some ideas on that. All the Policies in the Dynamic distro have the "dynamic" theme. So you could use the --theme option to switch them all on/off, which I believe is the intention of your --unsafe switch. If you have Perl- Critic-Dynamic distro installed, then you should be able to do something like this... $> perlcritic SomeFile.pm #Run with all Policies, including dynamic ones $> perlcritic --theme='! dynamic' SomeFile.pm #Run with all Policies, except dynamic ones $> perlcritic --theme='dynamic' SomeFile.pm #Run with dynamic policies only Note that setting the --theme implies that --severity=1. So you should explicitly set --severity to whatever you want it to be. You can also set the default values for these options in your .perlcriticrc file. Does that help? -Jeff
  Thu Jul 24 04:29:53 2008 EDAVIS [...] cpan.org - Correspondence added
OK. My feeling is that executing perl code is too dangerous to do without explicitly asking for it. Merely installing a new module should not flip perlcritic's behaviour from safe to unsafe; many package systems including perl's CPAN shell pull in dependencies automatically. But I can see that others disagree on this. You could, however, add a note to the perlcritic manual page to say that perlcritic may be dangerous to use on untrusted code, depending on what policy modules are installed. I'll write a couple of dynamic policies when I get round to it.
  Thu Jul 24 13:21:36 2008 jeff [...] imaginative-software.com - Correspondence added
Subject: Re: [rt.cpan.org #37881] Feature: unsafe policies
Date: Thu, 24 Jul 2008 10:21:10 -0700
To: bug-Perl-Critic [...] rt.cpan.org
From: Jeffrey Thalhammer <jeff [...] imaginative-software.com>
On Jul 24, 2008, at 1:29 AM, EDAVIS via RT wrote: Show quoted text
> Queue: Perl-Critic > Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=37881 > > > OK. My feeling is that executing perl code is too dangerous to do > without explicitly asking for it. Merely installing a new module > should > not flip perlcritic's behaviour from safe to unsafe; many package > systems including perl's CPAN shell pull in dependencies > automatically.
Ok, you've convinced me. But as AFAIK, there is only one Dynamic policy in circulation right now, so I'm not making this a high priority at the moment. But if there were more Dynamic policies floating around, then I would definitely bump it up (wink, wink). -Jeff
  Thu Jul 24 19:29:09 2008 perl [...] galumph.com - Correspondence added
Subject: Re: [rt.cpan.org #37881] Feature: unsafe policies
Date: Thu, 24 Jul 2008 16:28:55 -0700
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <perl [...] galumph.com>
Jeffrey Thalhammer via RT wrote: Show quoted text
> Ok, you've convinced me. But as AFAIK, there is only one Dynamic policy > in circulation right now, so I'm not making this a high priority at > the moment.
To be honest, I don't like that one, not because of what it's checking for, but because it's unsafe. If you haven't noticed, I haven't done anything to maintain that distro, e.g. it doesn't have an xt directory. I like that P::C is always "safe". You can't make it do bad things, except by installing things like That Policy. Actually, for dynamic policies, I'd run them in a sandbox, preferably in a chroot jail with no network access. Which is why P::C::Dynamic isn't pulled in by by Task:: or Bundle::. I'd prefer that P::C doesn't even acknowledge the concept of dynamic policies. If you want something dynamic, hook it into B::Lint or something.
  Wed Apr 21 22:11:50 2010 ELLIOTJS [...] cpan.org - Severity Normal changed to Wishlist
  Thu Sep 30 19:05:32 2010 thaljef [...] cpan.org - Correspondence added
RT-Send-CC: perl [...] galumph.com
I just released Perl-Critic-Dynamic-0.05, which marks all Policies that inherit from Perl::Critic::DynamicPolicy as "unsafe". When used with the latest perlcritic(1), you must set the --allow-unsafe switch for any "unsafe" policy to be loaded.
  Thu Sep 30 19:05:34 2010 thaljef [...] cpan.org - Status changed from 'open' to 'resolved'