[Rt-commit] rt branch, 5.0/use-json-in-system-config-edit-ui, created. rt-5.0.0-115-gcbe5408ed6

? sunnavy sunnavy at bestpractical.com
Wed Nov 11 10:21:50 EST 2020


The branch, 5.0/use-json-in-system-config-edit-ui has been created
        at  cbe5408ed6ce6123cedfdc35a5a91cec711f39ae (commit)

- Log -----------------------------------------------------------------
commit 13316b2242a2968a95ad576ecca7e63307f6e941
Author: sunnavy <sunnavy at bestpractical.com>
Date:   Tue Nov 10 04:41:23 2020 +0800

    Mark config items containing regexp immutable
    
    We are going to switch to JSON format, which doesn't support regexp :/
    
    Note that "path" in @StaticRoots is regexp, so we need to mark it too.

diff --git a/lib/RT/Config.pm b/lib/RT/Config.pm
index 59ce078f1c..843dfc663f 100644
--- a/lib/RT/Config.pm
+++ b/lib/RT/Config.pm
@@ -646,6 +646,7 @@ our %META;
 
     RTAddressRegexp => {
         Type    => 'SCALAR',
+        Immutable => 1,
         PostLoadCheck => sub {
             my $self = shift;
             my $value = $self->Get('RTAddressRegexp');
@@ -1762,9 +1763,6 @@ our %META;
     DefaultSearchResultOrderBy => {
         Widget => '/Widgets/Form/String',
     },
-    EmailSubjectTagRegex => {
-        Widget => '/Widgets/Form/String',
-    },
     EmailOutputEncoding => {
         Widget => '/Widgets/Form/String',
     },
@@ -1902,7 +1900,26 @@ our %META;
     },
     ShowMobileSite => {
         Widget => '/Widgets/Form/Boolean',
-    }
+    },
+    StaticRoots => {
+        Type      => 'ARRAY',
+        Immutable => 1,
+    },
+    EmailSubjectTagRegex => {
+        Immutable => 1,
+    },
+    ExtractSubjectTagMatch => {
+        Immutable => 1,
+    },
+    ExtractSubjectTagNoMatch => {
+        Immutable => 1,
+    },
+    WebNoAuthRegex => {
+        Immutable => 1,
+    },
+    SelfServiceRegex => {
+        Immutable => 1,
+    },
 );
 my %OPTIONS = ();
 my @LOADED_CONFIGS = ();

commit 692a6436d47a7ed5d1c327d47bda3b7fff0c06dd
Author: Dianne Skoll <dianne at bestpractical.com>
Date:   Wed Aug 19 14:44:00 2020 -0400

    Use "JSON" format for array/hash items in system config edit web UI
    
    This is for security reasons, to prevent users from executing arbitrary
    code there.
    
    For complicated configuration items that couldn't be serialized as
    JSON(like regex or coderef), we keep showing them as readonly Perl code.
    
    Internally in the database, the items are still saved as Perl data
    structures.

diff --git a/share/html/Admin/Tools/Config/Elements/Option b/share/html/Admin/Tools/Config/Elements/Option
index f10e84e284..ae0ac68b7a 100644
--- a/share/html/Admin/Tools/Config/Elements/Option
+++ b/share/html/Admin/Tools/Config/Elements/Option
@@ -46,18 +46,31 @@
 %#
 %# END BPS TAGGED BLOCK }}}
 <%PERL>
-
+use JSON;
 use Data::Dumper;
 my $stringify = sub {
     my $value = shift;
-    return "" if !defined($value);
+    return ('', '') if !defined($value);
 
+    my $output;
     local $Data::Dumper::Terse = 1;
     local $Data::Dumper::Indent = 2;
     local $Data::Dumper::Sortkeys = 1;
-    my $output = Dumper $value;
-    chomp $output;
-    return $output;
+    my $format = '';
+    if (ref $value) {
+        eval { $output = to_json($value, {utf8 => 1, pretty => 1}) };
+        if ( $@ ) {
+            $output = Dumper($value);
+            $format = 'code';
+        }
+        else {
+            $format = 'json';
+        }
+    } else {
+        $output = $value;
+    }
+    chomp($output);
+    return($output, $format);
 };
 
 my $doc_version = $RT::VERSION;
@@ -68,20 +81,18 @@ my $meta = RT->Config->Meta( $name );
 return if $meta->{Invisible} || $meta->{Deprecated};
 return if $name =~ /Password/i && $name !~ /MinimumPasswordLength/;
 
-my $has_execute_code = $session{CurrentUser}->HasRight(Right => 'ExecuteCode', Object => RT->System);
-
 my $raw_value = RT->Config->Get( $name );
-my $val = $stringify->($raw_value);
+my ($val, $format) = $stringify->($raw_value);
 my $doc_url = "https://docs.bestpractical.com/rt/$doc_version/RT_Config.html#$option->{Help}";
-my $widget = $meta->{'Widget'} || '/Widgets/Form/Code';
-my $is_code = $widget eq '/Widgets/Form/Code';
+my $widget = $meta->{'Widget'} || '/Widgets/Form/JSON';
 
+# CustomDateRanges could contain subrefs, but we supply limited/safe
+# widgets that contain plain inputs only.
 my $is_immutable = $meta->{Immutable}
                 || $meta->{Obfuscate}
-                || ($is_code && $val =~ s/sub \{ "DUMMY" \}/sub { ... }/g)
-                || ($is_code && !$has_execute_code);
+                || ( $format eq 'code' && $widget ne '/Widgets/Form/CustomDateRanges' );
 
-my $current_value = $is_code ? $val : $raw_value;
+my $current_value = $format ? $val : $raw_value;
 my $args   = $meta->{'WidgetArguments'} || {};
 if ($widget eq '/Widgets/Form/Boolean') {
     %$args = (
@@ -112,8 +123,8 @@ my $row_end = qq{</div></div>};
 
 <!-- start option <% $name %> -->
 % if ( $meta->{EditLink} ) {
-% if ($widget eq '/Widgets/Form/MultilineString' || $widget eq '/Widgets/Form/Code') {
-<% $row_start |n %><textarea disabled class="<% $is_code ? 'code' : '' %> form-control" rows="6" cols="80"><% $current_value %></textarea><br />
+% if ($widget eq '/Widgets/Form/MultilineString' || $widget eq '/Widgets/Form/Code' || $widget eq '/Widgets/Form/JSON' ) {
+<% $row_start |n %><textarea disabled class="<% $format %> form-control" rows="6" cols="80"><% $current_value %></textarea><br />
 % } else {
 <% $row_start |n %><input type="text" disabled value="<% $current_value %>" class="form-control" /><br/>
 % }
@@ -127,8 +138,8 @@ my $row_end = qq{</div></div>};
 <div class="text-right"><em><% loc('Must modify in config file' ) %></em></div>
 <% $row_end |n%>
 % } elsif ( $is_immutable ) {
-% if ($widget eq '/Widgets/Form/MultilineString' || $widget eq '/Widgets/Form/Code') {
-<% $row_start |n %><textarea disabled class="<% $is_code ? 'code' : '' %> form-control" rows="6" cols="80"><% $current_value %></textarea>
+% if ($widget eq '/Widgets/Form/MultilineString' || $widget eq '/Widgets/Form/Code' || $widget eq '/Widgets/Form/JSON') {
+<% $row_start |n %><textarea disabled class="<% $format %> form-control" rows="6" cols="80"><% $current_value %></textarea>
 % } else {
 <% $row_start |n %><input type="text" disabled value="<% $current_value %>" class="form-control" />
 % }
diff --git a/share/html/Admin/Tools/EditConfig.html b/share/html/Admin/Tools/EditConfig.html
index 2a33c93348..ab20b903fb 100644
--- a/share/html/Admin/Tools/EditConfig.html
+++ b/share/html/Admin/Tools/EditConfig.html
@@ -53,8 +53,6 @@ unless ($session{'CurrentUser'}->HasRight( Object=> $RT::System, Right => 'Super
  Abort(loc('This feature is only available to system administrators'));
 }
 
-my $has_execute_code = $session{CurrentUser}->HasRight(Right => 'ExecuteCode', Object => RT->System);
-
 my $options = RT->Config->LoadSectionMap();
 my $active_context = {
     tab        => CSSClass( $ARGS{tab}        || $options->[0]->{Name}) ,
@@ -81,8 +79,8 @@ if (delete $ARGS{Update}) {
             next if !exists $ARGS{$key . '-Current'};
 
             my $meta = RT->Config->Meta( $key );
-            my $widget = $meta->{Widget} || '/Widgets/Form/Code';
-            my $is_code = $widget eq '/Widgets/Form/Code';
+            my $widget = $meta->{Widget} || '/Widgets/Form/JSON';
+            my $is_json = $widget eq '/Widgets/Form/JSON';
 
             my $val = $ARGS{$key};
             $val = '' if $val eq '__empty_value__';
@@ -100,38 +98,16 @@ if (delete $ARGS{Update}) {
                 next;
             }
 
-            if ($is_code) {
-                if (!$has_execute_code) {
-                    push @results, loc("Cannot change [_1]: Permission Denied", $key);
-                    $has_error++;
-                    next;
-                }
-
-                my $code = $val;
-                my $coderef;
-                # similar to RT::Scrip::CompileCheck
-                do {
-                    no strict 'vars';
-                    $coderef = eval "sub { $code \n }";
-                };
-                if ($@) {
+            if ($is_json) {
+                my $json = $val;
+                ($val) = RT::Configuration->new( $session{CurrentUser} )->_DeJSONContent($json);
+                if (!defined $val) {
                     my $error = $@;
-                    push @results, loc("Couldn't compile [_1] codeblock '[_2]': [_3]", $key, $code, $error);
+                    push @results, loc("Couldn't decode [_1] JSON '[_2]': [_3]", $key, $json, $error);
                     $has_error++;
                     next;
                 }
-
-                if ($coderef) {
-                    $val = eval { $coderef->() };
-                    if ($@) {
-                        my $error = $@;
-                        push @results, loc("Couldn't execute [_1] codeblock '[_2]': [_3]", $key, $code, $error);
-                        $has_error++;
-                        next;
-                    }
-                }
             }
-
             my $setting = RT::Configuration->new($session{CurrentUser});
             $setting->LoadByCols(Name => $key, Disabled => 0);
             if ($setting->Id) {
diff --git a/share/html/Widgets/Form/JSON b/share/html/Widgets/Form/JSON
new file mode 100644
index 0000000000..8f9d407c93
--- /dev/null
+++ b/share/html/Widgets/Form/JSON
@@ -0,0 +1,57 @@
+%# BEGIN BPS TAGGED BLOCK {{{
+%#
+%# COPYRIGHT:
+%#
+%# This software is Copyright (c) 1996-2020 Best Practical Solutions, LLC
+%#                                          <sales at bestpractical.com>
+%#
+%# (Except where explicitly superseded by other copyright notices)
+%#
+%#
+%# LICENSE:
+%#
+%# This work is made available to you under the terms of Version 2 of
+%# the GNU General Public License. A copy of that license should have
+%# been provided with this software, but in any event can be snarfed
+%# from www.gnu.org.
+%#
+%# This work is distributed in the hope that it will be useful, but
+%# WITHOUT ANY WARRANTY; without even the implied warranty of
+%# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+%# General Public License for more details.
+%#
+%# You should have received a copy of the GNU General Public License
+%# along with this program; if not, write to the Free Software
+%# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+%# 02110-1301 or visit their web page on the internet at
+%# http://www.gnu.org/licenses/old-licenses/gpl-2.0.html.
+%#
+%#
+%# CONTRIBUTION SUBMISSION POLICY:
+%#
+%# (The following paragraph is not intended to limit the rights granted
+%# to you to modify and distribute this software under the terms of
+%# the GNU General Public License and is only of importance to you if
+%# you choose to contribute your changes and enhancements to the
+%# community by submitting them to Best Practical Solutions, LLC.)
+%#
+%# By intentionally submitting any modifications, corrections or
+%# derivatives to this work, or any other work intended for use with
+%# Request Tracker, to Best Practical Solutions, LLC, you confirm that
+%# you are the copyright holder for those contributions and you grant
+%# Best Practical Solutions,  LLC a nonexclusive, worldwide, irrevocable,
+%# royalty-free, perpetual, license to use, copy, create derivative
+%# works based on those contributions, and sublicense and distribute
+%# those contributions and any derivatives thereof.
+%#
+%# END BPS TAGGED BLOCK }}}
+<%DOC>
+see docs/extending/using_forms_widgets.pod
+</%DOC>
+<& /Widgets/Form/MultilineString, Class => 'json', %ARGS &>
+<%METHOD InputOnly>
+<& /Widgets/Form/MultilineString:InputOnly, %ARGS &>
+</%METHOD>
+<%METHOD Process>
+<& /Widgets/Form/MultilineString:Process, %ARGS &>
+</%METHOD>

commit cbe5408ed6ce6123cedfdc35a5a91cec711f39ae
Author: Dianne Skoll <dianne at bestpractical.com>
Date:   Wed Aug 19 15:19:43 2020 -0400

    Update unit test for JSON entry of hashes/arrays rather than Perl.

diff --git a/t/web/admin_tools_editconfig.t b/t/web/admin_tools_editconfig.t
index f23d2fdb04..2182411a4b 100644
--- a/t/web/admin_tools_editconfig.t
+++ b/t/web/admin_tools_editconfig.t
@@ -24,6 +24,10 @@ $m->follow_link_ok( { text => 'System Configuration' }, 'followed link to "Syste
 $m->follow_link_ok( { text => 'History' }, 'followed link to History page' );
 $m->follow_link_ok( { text => 'Edit' }, 'followed link to Edit page' );
 
+# In the tests below, new_value is *always* the string we feed
+# into the Web form.  For compound objects such as hashes and arrays,
+# we have a separate expected member that is the Perl data structure
+# resulting from feeding new_value into the Web interface.
 my $tests = [
     {
         name      => 'change a string value',
@@ -41,13 +45,15 @@ my $tests = [
         name      => 'change an arrayref value',
         form_id   => 'form-System-Extra_security',
         setting   => 'ReferrerWhitelist',
-        new_value => ['www.example.com:443', 'www3.example.com:80'],
+        new_value => '["www.example.com:443", "www3.example.com:80"]',
+        expected  => ['www.example.com:443', 'www3.example.com:80'],
     },
     {
         name      => 'change a hashref value',
         form_id   => 'form-System-Outgoing_mail',
         setting   => 'OverrideOutgoingMailFrom',
-        new_value => { 1 => 'new-outgoing-from at example.com' },
+        new_value => '{"1":"new-outgoing-from at example.com"}',
+        expected  => {1 => 'new-outgoing-from at example.com'},
     },
 ];
 
@@ -80,7 +86,7 @@ sub run_test {
         {
             form_id => $args{form_id},
             fields  => {
-                $args{setting} => stringify( $args{new_value} ),
+                $args{setting} => $args{new_value},
             },
         },
         'form was submitted successfully'
@@ -100,22 +106,26 @@ sub run_test {
 
     my $rt_config_value = RT->Config->Get( $args{setting} );
 
-    is( $rt_configuration_value, stringify($args{new_value}), 'value from RT::Configuration->Load matches new value' );
-    cmp_deeply( $rt_config_value, $args{new_value}, 'value from RT->Config->Get matches new value' );
+    is( $rt_configuration_value, stringify($args{expected}) || $args{new_value}, 'value from RT::Configuration->Load matches new value' );
+    cmp_deeply( $rt_config_value, $args{expected} || $args{new_value}, 'value from RT->Config->Get matches new value' );
 }
 
 sub check_transaction {
     my ($tx, $change) = @_;
     is($tx->ObjectType, 'RT::Configuration', 'tx is config change');
     is($tx->Field, $change->{setting}, 'tx matches field changed');
-    is($tx->NewValue, stringify($change->{new_value}), 'tx value matches');
+    is($tx->NewValue, stringify($change->{expected}) || $change->{new_value}, 'tx value matches');
 }
 
 sub check_history_page_item {
     my ($tx, $change) = @_;
     my $link = sprintf('ConfigHistory.html?id=%d#txn-%d', $tx->ObjectId, $tx->id);
     ok($m->find_link(url => $link), 'found tx link in history');
-    $m->text_contains(compactify($change->{new_value}), 'fetched tx has new value');
+    if ($change->{expected}) {
+        $m->text_contains(compactify($change->{expected}), 'fetched tx has new value');
+    } else {
+        $m->text_contains($change->{new_value});
+    }
     $m->text_contains("$change->{setting} changed", 'fetched tx has changed field');
 }
 

-----------------------------------------------------------------------


More information about the rt-commit mailing list