Skip to content

Commit

Permalink
Move Dancer2::Core::Role::ConfigReader to Path::Tiny:
Browse files Browse the repository at this point in the history
There are two important changes here.

The first defends against empty paths from the environment. We can
see that there was a situation in which we would have returned
an empty string (in case none of the three options worked) and
that's bad. We should fix it in the future.

The second is what might be a security issue. We use FileUtils'
path() which returns path based on root (/) if it's empty. There
is a situation in which the environments_location is empty (see
paragraph above) and in that case, we will accidentally use the
root directory. This does nothing if the file does not exist, but
this is now officially fixed with Path::Tiny and checking whether
it's empty or not.
  • Loading branch information
xsawyerx committed Feb 19, 2017
1 parent d49db05 commit 9e0db21
Showing 1 changed file with 23 additions and 7 deletions.
30 changes: 23 additions & 7 deletions lib/Dancer2/Core/Role/ConfigReader.pm
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,15 @@ package Dancer2::Core::Role::ConfigReader;

use Moo::Role;

use File::Spec;
use Carp 'croak';
use Path::Tiny ();
use Config::Any;
use Hash::Merge::Simple;
use Carp 'croak';
use Module::Runtime 'require_module';

use Dancer2::Core::Factory;
use Dancer2::Core;
use Dancer2::Core::Types;
use Dancer2::FileUtils 'path';

with 'Dancer2::Core::Role::HasLocation';

Expand All @@ -39,9 +38,19 @@ has environments_location => (
isa => Str,
lazy => 1,
default => sub {
$ENV{DANCER_ENVDIR}
|| File::Spec->catdir( $_[0]->config_location, 'environments' )
|| File::Spec->catdir( $_[0]->location, 'environments' );
# short circuit
defined $ENV{'DANCER_ENVDIR'}
and return $ENV{'DANCER_ENVDIR'};

my $self = shift;

foreach my $maybe_path ( $self->config_location, $self->location ) {
my $path = Path::Tiny::path($maybe_path, 'environments');
$path->exists && $path->is_dir
and return $path->stringify;
}

return '';
},
);

Expand Down Expand Up @@ -133,7 +142,14 @@ sub _build_config_files {
foreach my $file ( [ $location, "config.$ext" ],
[ $self->environments_location, "$running_env.$ext" ] )
{
my $path = path( @{$file} );
# This is a potential security problem,
# escaping outside the current directory
# It's now disabled
# FIXME: Commit separately
$file->[0] eq ''
and next;

my $path = Path::Tiny::path(@{$file})->stringify;
next if !-r $path;

push @files, $path;
Expand Down

0 comments on commit 9e0db21

Please sign in to comment.