Merge remote-tracking branch 'origin/main' into feature/new-config-system-2-recursive

Author: yanick

Changes

@@ -1,16 +1,21 @@
 {{$NEXT}}
 
     [ BUG FIXES ]
-    * None
+    * GH #1701: Split cookie values on & only (Yanick Champoux)
 
     [ ENHANCEMENTS ]
+    * GH #530: Make data censoring configurable (Yanick Champoux, David
+      Precious)
     * GH #1723: Enable use of a different Template Toolkit base class
       (Andy Beverley)
     * PR #1727: Don't create CPAN package files when generating new apps
       (Jason A. Crome)
+    * GH #1737: Add on_hook_exception for errors during hook processing
+      (Andy Beverley)
 
     [ DOCUMENTATION ]
-    * None
+    * GH #1342: Document skipping private methods in pod coverage tests
+      (Jason A. Crome)
 
     [ DEPRECATED ]
     * None

cpanfile

@@ -3,6 +3,7 @@ requires 'Attribute::Handlers';
 requires 'Carp';
 requires 'Clone';
 requires 'Config::Any';
+requires 'Data::Censor' => '0.04';
 requires 'Digest::SHA';
 requires 'Encode';
 requires 'Exporter', '5.57';

lib/Dancer2/ConfigReader.pm

@@ -145,7 +145,7 @@ sub _build_config_readers {
 
     my @config_reader_names = $ENV{'DANCER_CONFIG_READERS'}
                             ? (split qr{,}msx, $ENV{'DANCER_CONFIG_READERS'})
-                            : ( q{Dancer2::ConfigReader::File::Simple} );
+                            : ( q{Dancer2::ConfigReader::Config::Any} );
 
     warn "ConfigReaders to use: @config_reader_names\n" if $ENV{DANCER_CONFIG_VERBOSE};
     return [
@@ -164,7 +164,7 @@ __END__
 
 This class provides a C<config> attribute that 
 is populated by executing one or more B<ConfigReader> packages. 
-The default ConfigReader used by default is C<Dancer2::ConfigReader::File::Simple>.
+The default ConfigReader used by default is C<Dancer2::ConfigReader::Config::Any>.
 
 Also provides a C<setting()> method which is supposed to be used by externals to
 read/write config entries.
@@ -187,7 +187,7 @@ if the first config reader returns
         subitem1: subcontent1
         subitem2: subcontent2
 
-and  the second returns 
+and the second returns 
 
     item2: content9
     item3:
@@ -213,14 +213,13 @@ then the final config is
             subsubitem5: subsubcontent5
     item4: content4
 
-
 =head2 Configuring the ConfigReaders via DANCER_CONFIG_READERS 
 
 You can control which B<ConfigReader>
 class or classes to use to create the config
 via the C<DANCER_CONFIG_READERS> environment.
 
-    DANCER_CONFIG_READERS='Dancer2::ConfigReader::File::Simple,Dancer2::ConfigReader::CustomConfig'
+    DANCER_CONFIG_READERS='Dancer2::ConfigReader::Config::Any,Dancer2::ConfigReader::CustomConfig'
 
 If you want several, separate them with a comma (",").
 
@@ -234,22 +233,22 @@ instantiated and added to the list of configurations to merge. This way you can,
             path: /path/to/sqlite.db 
             table: config
 
-The default ConfigReader L<Dancer2::ConfigReader::File::Simple> will pick that file and proceed to instantiate C<Dancer2::ConfigReader::SQLite> 
+The default ConfigReader L<Dancer2::ConfigReader::::Config::Any> will pick that file and proceed to instantiate C<Dancer2::ConfigReader::SQLite> 
 with the provided parameters.
 
 C<additional_config_readers> can take one or a list of reader configurations, which can be either the name of the ConfigReader's class, or the 
 key/value pair of the class name and its constructor's arguments.
 
 =head2 Creating your own custom B<ConfigReader> classes.
 
-Here's an example extending class C<Dancer2::ConfigReader::File::Simple>.
+Here's an example extending class C<Dancer2::ConfigReader::Config::Any>.
 
-    package Dancer2::ConfigReader::FileExtended;
+    package Dancer2::ConfigReader::Config::Any::Extended;
     use Moo;
-    extends 'Dancer2::ConfigReader::File::Simple';
+    extends 'Dancer2::ConfigReader::Config::Any';
     has name => (
         is      => 'ro',
-        default => sub {'FileExtended'},
+        default => sub {'Config::Any::Extended'},
     );
     around read_config => sub {
         my ($orig, $self) = @_;
@@ -258,7 +257,7 @@ Here's an example extending class C<Dancer2::ConfigReader::File::Simple>.
         return $config;
     };
 
-Another (more complex) example is in the file C<Dancer2::ConfigReader::File::Simple>.
+Another (more complex) example is in class C<Dancer2::ConfigReader::Config::Any>.
 
 =head1 ATTRIBUTES
 

lib/Dancer2/ConfigReader/File/Simple.pm renamed to lib/Dancer2/ConfigReader/Config/Any.pm

@@ -1,5 +1,5 @@
 # ABSTRACT: Config reader for files
-package Dancer2::ConfigReader::File::Simple;
+package Dancer2::ConfigReader::Config::Any;
 
 use Moo;
 
@@ -20,7 +20,7 @@ has name => (
     is      => 'ro',
     isa     => Str,
     lazy    => 0,
-    default => sub {'File::Simple'},
+    default => sub {'Config::Any'},
 );
 
 has config_files => (
@@ -127,7 +127,7 @@ random parts of the file config with environmental variables:
 
     use Carp 'croak';
 
-    extends 'Dancer2::ConfigReader::File::Simple';
+    extends 'Dancer2::ConfigReader::Config::Any';
 
     has name => (
         is      => 'ro',
@@ -191,7 +191,7 @@ random parts of the file config with environmental variables:
 
 =attr name
 
-The name of the Config Reader class: C<File::Simple>.
+The name of the Config Reader class: C<Config::Any>.
 
 =attr location
 

lib/Dancer2/Core/App.pm

@@ -800,6 +800,7 @@ sub supported_hooks {
       core.app.before_request
       core.app.after_request
       core.app.route_exception
+      core.app.hook_exception
       core.app.before_file_render
       core.app.after_file_render
       core.error.before
@@ -819,6 +820,7 @@ sub hook_aliases {
         before_error           => 'core.error.before',
         after_error            => 'core.error.after',
         on_route_exception     => 'core.app.route_exception',
+        on_hook_exception      => 'core.app.hook_exception',
 
         before_file_render         => 'core.app.before_file_render',
         after_file_render          => 'core.app.after_file_render',
@@ -1235,7 +1237,26 @@ sub compile_hooks {
                 eval  { $EVAL_SHIM->($hook,@_); 1; }
                 or do {
                     my $err = $@ || "Zombie Error";
-                    $app->cleanup;
+                    my $is_hook_exception = $position eq 'core.app.hook_exception';
+                    # Don't execute the hook_exception hook if the exception
+                    # has been generated from a hook exception handler itself,
+                    # thus preventing potentially recursive code.
+                    $app->execute_hook( 'core.app.hook_exception', $app, $err )
+                        unless $is_hook_exception;
+                    my $is_halted = $app->response->is_halted; # Capture before cleanup
+                    # We can't cleanup if we're in the hook for a hook
+                    # exception, as this would clear the custom response that
+                    # may have been set by the hook. However, there is no need
+                    # to do so, as the upper hook that called this hook
+                    # exception will perform the cleanup instead anyway
+                    $app->cleanup
+                        unless $is_hook_exception;
+                    # Allow the hook function to halt the response, thus
+                    # retaining any response it may have set. Otherwise the
+                    # croak from this function will overwrite any content that
+                    # may have been set by the hook
+                    return if $is_halted;
+                    # Default behavior if nothing else defined
                     $app->log('error', "Exception caught in '$position' filter: $err");
                     croak "Exception caught in '$position' filter: $err";
                 };

lib/Dancer2/Core/Error.pm

@@ -8,7 +8,7 @@ use Dancer2::Core::HTTP;
 use Data::Dumper;
 use Dancer2::FileUtils qw/path open_file/;
 use Sub::Quote;
-use Module::Runtime 'require_module';
+use Module::Runtime qw/ require_module use_module /;
 use Ref::Util qw< is_hashref >;
 use Clone qw(clone);
 
@@ -48,6 +48,52 @@ has title => (
     builder => '_build_title',
 );
 
+has censor => (
+    is => 'ro',
+    isa => CodeRef,
+    lazy => 1, 
+    default => sub {
+        my $self = shift;
+
+        if( my $custom = $self->has_app && $self->app->setting('error_censor') ) {
+
+            if( is_hashref $custom ) {
+                die "only one key can be set for the 'error_censor' setting\n" 
+                    if 1 != keys %$custom;
+
+                my( $class, $args ) = %$custom;
+
+                my $censor = use_module($class)->new(%$args);
+
+                return sub {
+                    $censor->censor(@_);
+                }
+            }
+
+            my $coderef = eval '\&'.$custom;
+
+            # it's already defined? Nice! We're done
+            return $coderef if $coderef;
+
+            my $module = $custom =~ s/::[^:]*?$//r;
+
+            require_module($module);
+
+            return eval '\&'.$custom;
+        }
+
+        # reminder: update POD below if changing the config here
+         my $data_censor = use_module('Data::Censor')->new(
+             sensitive_fields => qr/pass|card.?num|pan|secret/i,
+             replacement => "Hidden (looks potentially sensitive)",
+         );
+
+         return sub {
+             $data_censor->censor(@_);
+         };
+    }
+);
+
 sub _build_title {
     my ($self) = @_;
     my $title = 'Error ' . $self->status;
@@ -367,11 +413,11 @@ sub backtrace {
 }
 
 sub dumper {
-    my $obj = shift;
+    my ($self,$obj) = @_;
 
     # Take a copy of the data, so we can mask sensitive-looking stuff:
     my $data     = clone($obj);
-    my $censored = _censor( $data );
+    my $censored = $self->censor->( $data );
 
     #use Data::Dumper;
     my $dd = Data::Dumper->new( [ $data ] );
@@ -399,7 +445,7 @@ sub environment {
     my $env = $self->has_app && $self->app->has_request && $self->app->request->env;
 
     # Get a sanitised dump of the settings, session and environment
-    $_ = $_ ? dumper($_) : '<i>undefined</i>' for $settings, $session, $env;
+    $_ = $_ ? $self->dumper($_) : '<i>undefined</i>' for $settings, $session, $env;
 
     return <<"END_HTML";
 <div class="title">Stack</div><pre class="content">$stack</pre>
@@ -423,37 +469,6 @@ sub get_caller {
 
 # private
 
-# Given a hashref, censor anything that looks sensitive.  Returns number of
-# items which were "censored".
-
-sub _censor {
-    my $hash = shift;
-    my $visited = shift || {};
-
-    unless ( $hash && is_hashref($hash) ) {
-        carp "_censor given incorrect input: $hash";
-        return;
-    }
-
-    my $censored = 0;
-    for my $key ( keys %$hash ) {
-        if ( is_hashref( $hash->{$key} ) ) {
-            if (!$visited->{ $hash->{$key} }) {
-                # mark the new ref as visited
-                $visited->{ $hash->{$key} } = 1;
-
-                $censored += _censor( $hash->{$key}, $visited );
-            }
-        }
-        elsif ( $key =~ /(pass|card?num|pan|secret)/i ) {
-            $hash->{$key} = "Hidden (looks potentially sensitive)";
-            $censored++;
-        }
-    }
-
-    return $censored;
-}
-
 # Replaces the entities that are illegal in (X)HTML.
 sub _html_encode {
     my $value = shift;
@@ -523,6 +538,60 @@ This is only an attribute getter, you'll have to set it at C<new>.
 
 The message of the error page.
 
+=attr censor 
+
+The function to use to censor error messages. By default it uses the C<censor> method of L<Data::Censor>"
+
+         # default censor function used by `error_censor` 
+         # is equivalent to
+         sub MyApp::censor {
+             Data::Censor->new(
+                sensitive_fields => qr/pass|card.?num|pan|secret/i,
+                replacement      => "Hidden (looks potentially sensitive)",
+            )->censor(@_);
+         }
+         setting error_censor => 'MyApp::censor';
+
+It can be configured via the app setting C<error_censor>. If provided, 
+C<error_censor> has to be the fully qualified name of the censor 
+function. That function is expected to take in the data as a hashref, 
+modify it in place and return the number of items 'censored'.
+
+For example, using L<Data::Censor>.
+
+    # in config.yml
+    error_censor: MyApp::Censor::censor
+
+    # in MyApp::Censor
+    package MyApp::Censor;
+
+    use Data::Censor;
+
+    my $data_censor = Data::Censor->new(
+        sensitive_fields => [ qw(card_number password hush) ],
+        replacement => '(Sensitive data hidden)',
+    );
+
+    sub censor { $data_censor->censor(@_) }
+
+    1;
+
+
+As a shortcut, C<error_censor> can also be the key/value combo of 
+a class and the arguments for its constructor. The created object 
+is expected to have a method C<censor>. For example, the use of 
+L<Data::Censor> above could also have been done via the config 
+
+    error_censor:
+        Data::Censor: 
+            sensitive_fields:
+                - card_number 
+                - password 
+                - hush 
+            replacement: '(Sensitive data hidden)'
+
+
+
 =method throw($response)
 
 Populates the content of the response with the error's information.

lib/Dancer2/Core/Request.pm

@@ -579,7 +579,7 @@ sub _build_cookies {
         $cookies->{$name} = Dancer2::Core::Cookie->new(
             name  => $name,
             # HTTP::XSCookies v0.17+ will do the split and return an arrayref
-            value => (is_arrayref($value) ? $value : [split(/[&;]/, $value)])
+            value => is_arrayref($value) ? $value : [split '&', $value ]
         );
     }
     return $cookies;

lib/Dancer2/Core/Role/ConfigReader.pm

@@ -64,8 +64,7 @@ Config can be created by reading configuration
 files, from environment variables, by fetching
 it from a cloud service, or any other means.
 
-By default, the config loader
-which is used, is C<Dancer2::ConfigReader::File::Simple>
+Default config reader is C<Dancer2::ConfigReader::Config::Any>
 but user can create his own config reader
 if he wants to replace or augment
 the default method of config creation.