Bug 1052724: Use JSON::XS instead of Data::Dumper to store parameters into data/params

Bugzilla.pm

@@ -122,8 +122,8 @@ sub init_page {
     #
     # This code must go here. It cannot go anywhere in Bugzilla::CGI, because
     # it uses Template, and that causes various dependency loops.
-    if (Bugzilla->params->{"shutdownhtml"}
-        && !grep { $_ eq $script } SHUTDOWNHTML_EXEMPT)
+    if (!grep { $_ eq $script } SHUTDOWNHTML_EXEMPT
+        and Bugzilla->params->{'shutdownhtml'})
     {
         # Allow non-cgi scripts to exit silently (without displaying any
         # message), if desired. At this point, no DBI call has been made
@@ -939,9 +939,9 @@ Change the database object to refer to the main database.
 
 =item C<params>
 
-The current Parameters of Bugzilla, as a hashref. If C<data/params>
-does not exist, then we return an empty hashref. If C<data/params>
-is unreadable or is not valid perl, we C<die>.
+The current Parameters of Bugzilla, as a hashref. If C<data/params.js>
+does not exist, then we return an empty hashref. If C<data/params.js>
+is unreadable or is not valid, we C<die>.
 
 =item C<local_timezone>
 

Bugzilla/Config.pm

@@ -12,11 +12,16 @@ use strict;
 use warnings;
 
 use parent qw(Exporter);
+use autodie qw(:default);
 
 use Bugzilla::Constants;
 use Bugzilla::Hook;
-use Data::Dumper;
+use Bugzilla::Util qw(trick_taint);
+
+use JSON::XS;
+use File::Slurp;
 use File::Temp;
+use File::Basename;
 
 # Don't export localvars by default - people should have to explicitly
 # ask for it, as a (probably futile) attempt to stop code using it
@@ -93,8 +98,30 @@ sub SetParam {
 sub update_params {
     my ($params) = @_;
     my $answer = Bugzilla->installation_answers;
+    my $datadir = bz_locations()->{'datadir'};
+    my $param;
+
+    # If the old data/params file using Data::Dumper output still exists,
+    # read it. It will be deleted once the parameters are stored in the new
+    # data/params.js file.
+    my $old_file = "$datadir/params";
+
+    if (-e $old_file) {
+        require Safe;
+        my $s = new Safe;
+
+        $s->rdo($old_file);
+        die "Error reading $old_file: $!" if $!;
+        die "Error evaluating $old_file: $@" if $@;
+
+        # Now read the param back out from the sandbox.
+        $param = \%{ $s->varglob('param') };
+    }
+    else {
+        # Read the new data/params.js file.
+        $param = read_param_file();
+    }
 
-    my $param = read_param_file();
     my %new_params;
 
     # If we didn't return any param values, then this is a new installation.
@@ -212,7 +239,6 @@ sub update_params {
     }
 
     # Write any old parameters to old-params.txt
-    my $datadir = bz_locations()->{'datadir'};
     my $old_param_file = "$datadir/old-params.txt";
     if (scalar(keys %oldparams)) {
         my $op_file = new IO::File($old_param_file, '>>', 0600)
@@ -222,12 +248,9 @@ sub update_params {
               " and so have been\nmoved from your parameters file into",
               " $old_param_file:\n";
 
-        local $Data::Dumper::Terse  = 1;
-        local $Data::Dumper::Indent = 0;
-
         my $comma = "";
         foreach my $item (keys %oldparams) {
-            print $op_file "\n\n$item:\n" . Data::Dumper->Dump([$oldparams{$item}]) . "\n";
+            print $op_file "\n\n$item:\n" . $oldparams{$item} . "\n";
             print "${comma}$item";
             $comma = ", ";
         }
@@ -258,6 +281,11 @@ sub update_params {
 
     write_params($param);
 
+    if (-e $old_file) {
+        unlink $old_file;
+        say "$old_file has been converted into $old_file.js, using the JSON format.";
+    }
+
     # Return deleted params and values so that checksetup.pl has a chance
     # to convert old params to new data.
     return %oldparams;
@@ -266,22 +294,10 @@ sub update_params {
 sub write_params {
     my ($param_data) = @_;
     $param_data ||= Bugzilla->params;
+    my $param_file = bz_locations()->{'datadir'} . '/params.js';
 
-    my $datadir    = bz_locations()->{'datadir'};
-    my $param_file = "$datadir/params";
-
-    local $Data::Dumper::Sortkeys = 1;
-
-    my ($fh, $tmpname) = File::Temp::tempfile('params.XXXXX',
-                                              DIR => $datadir );
-
-    print $fh (Data::Dumper->Dump([$param_data], ['*param']))
-      || die "Can't write param file: $!";
-
-    close $fh;
-
-    rename $tmpname, $param_file
-      or die "Can't rename $tmpname to $param_file: $!";
+    my $json_data = JSON::XS->new->canonical->pretty->encode($param_data);
+    write_file($param_file, { binmode => ':utf8', atomic => 1 }, \$json_data);
 
     # It's not common to edit parameters and loading
     # Bugzilla::Install::Filesystem is slow.
@@ -295,21 +311,23 @@ sub write_params {
 
 sub read_param_file {
     my %params;
-    my $datadir = bz_locations()->{'datadir'};
-    if (-e "$datadir/params") {
-        # Note that checksetup.pl sets file permissions on '$datadir/params'
+    my $file = bz_locations()->{'datadir'} . '/params.js';
 
-        # Using Safe mode is _not_ a guarantee of safety if someone does
-        # manage to write to the file. However, it won't hurt...
-        # See bug 165144 for not needing to eval this at all
-        my $s = new Safe;
-
-        $s->rdo("$datadir/params");
-        die "Error reading $datadir/params: $!" if $!;
-        die "Error evaluating $datadir/params: $@" if $@;
+    if (-e $file) {
+        my $data;
+        read_file($file, binmode => ':utf8', buf_ref => \$data);
 
-        # Now read the param back out from the sandbox
-        %params = %{$s->varglob('param')};
+        # If params.js has been manually edited and e.g. some quotes are
+        # missing, we don't want JSON::XS to leak the content of the file
+        # to all users in its error message, so we have to eval'uate it.
+        %params = eval { %{JSON::XS->new->decode($data)} };
+        if ($@) {
+            my $error_msg = (basename($0) eq 'checksetup.pl') ?
+                $@ : 'run checksetup.pl to see the details.';
+            die "Error parsing $file: $error_msg";
+        }
+        # JSON::XS doesn't detaint data for us.
+        trick_taint($params{$_}) foreach keys %params;
     }
     elsif ($ENV{'SERVER_SOFTWARE'}) {
        # We're in a CGI, but the params file doesn't exist. We can't
@@ -319,7 +337,7 @@ sub read_param_file {
        # so that the user sees the error.
        require CGI::Carp;
        CGI::Carp->import('fatalsToBrowser');
-       die "The $datadir/params file does not exist."
+       die "The $file file does not exist."
            . ' You probably need to run checksetup.pl.',
     }
     return \%params;
@@ -375,7 +393,7 @@ specified.
 Description: Writes the parameters to disk.
 
 Params:      C<$params> (optional) - A hashref to write to the disk
-               instead of C<Bugzilla->params>. Used only by
+               instead of C<Bugzilla-E<gt>params>. Used only by
                C<update_params>.
 
 Returns:     nothing
@@ -383,12 +401,12 @@ Returns:     nothing
 =item C<read_param_file()>
 
 Description: Most callers should never need this. This is used
-             by C<Bugzilla->params> to directly read C<$datadir/params>
-             and load it into memory. Use C<Bugzilla->params> instead.
+             by C<Bugzilla-E<gt>params> to directly read C<$datadir/params.js>
+             and load it into memory. Use C<Bugzilla-E<gt>params> instead.
 
 Params:      none
 
-Returns:     A hashref containing the current params in C<$datadir/params>.
+Returns:     A hashref containing the current params in C<$datadir/params.js>.
 
 =item C<param_panels()>
 

Bugzilla/Install/DB.pm

@@ -2561,7 +2561,7 @@ sub _fix_whine_queries_title_and_op_sys_value {
                  undef, "Other", "other");
         if (Bugzilla->params->{'defaultopsys'} eq 'other') {
             # We can't actually fix the param here, because WriteParams() will
-            # make $datadir/params unwriteable to the webservergroup.
+            # make $datadir/params.js unwriteable to the webservergroup.
             # It's too much of an ugly hack to copy the permission-fixing code
             # down to here. (It would create more potential future bugs than
             # it would solve problems.)

Bugzilla/Install/Filesystem.pm

@@ -171,7 +171,7 @@ sub FILESYSTEM {
         'docs/style.css'       => { perms => WS_SERVE },
         'docs/*/rel_notes.txt' => { perms => WS_SERVE },
         'docs/*/README.docs'   => { perms => OWNER_WRITE },
-        "$datadir/params"      => { perms => CGI_WRITE },
+        "$datadir/params.js"   => { perms => CGI_WRITE },
         "$datadir/old-params.txt"  => { perms => OWNER_WRITE },
         "$extensionsdir/create.pl" => { perms => OWNER_EXECUTE },
         "$extensionsdir/*/*.pl"    => { perms => WS_EXECUTE },

Bugzilla/Install/Requirements.pm

@@ -166,6 +166,12 @@ sub REQUIRED_MODULES {
         module  => 'File::Slurp',
         version => '9999.13',
     },
+    {
+        package => 'JSON-XS',
+        module  => 'JSON::XS',
+        # 2.0 is the first version that will work with JSON::RPC.
+        version => '2.01',
+    },
     );
 
     if (ON_WINDOWS) {
@@ -299,13 +305,6 @@ sub OPTIONAL_MODULES {
         feature => ['jsonrpc', 'rest'],
     },
     {
-        package => 'JSON-XS',
-        module  => 'JSON::XS',
-        # 2.0 is the first version that will work with JSON::RPC.
-        version => '2.0',
-        feature => ['jsonrpc_faster'],
-    },
-    {
         package => 'Test-Taint',
         module  => 'Test::Taint',
         # 1.06 no longer throws warnings with Perl 5.10+.

checksetup.pl

@@ -109,7 +109,7 @@ my $lc_hash = Bugzilla->localconfig;
 
 # At this point, localconfig is defined and is readable. So we know
 # everything we need to create the DB. We have to create it early,
-# because some data required to populate data/params is stored in the DB.
+# because some data required to populate data/params.js is stored in the DB.
 
 Bugzilla::DB::bz_check_requirements(!$silent);
 Bugzilla::DB::bz_create_database() if $lc_hash->{'db_check'};
@@ -364,7 +364,7 @@ L<Bugzilla::Install::Filesystem/create_htaccess>.
 
 =item 9
 
-Updates the system parameters (stored in F<data/params>), using
+Updates the system parameters (stored in F<data/params.js>), using
 L<Bugzilla::Config/update_params>.
 
 =item 10

docs/en/rst/administration.rst

@@ -346,7 +346,7 @@ user_verify_class
     well, you may otherwise not be able to log back in to Bugzilla once
     you log out.
     If this happens to you, you will need to manually edit
-    :file:`data/params` and set user_verify_class to
+    :file:`data/params.js` and set user_verify_class to
     ``DB``.
 
 LDAPserver
@@ -414,7 +414,7 @@ user_verify_class
     well, you may otherwise not be able to log back in to Bugzilla once
     you log out.
     If this happens to you, you will need to manually edit
-    :file:`data/params` and set user_verify_class to
+    :file:`data/params.js` and set user_verify_class to
     ``DB``.
 
 RADIUS_server

template/en/default/setup/strings.txt.pl

@@ -88,7 +88,6 @@ END
     feature_inbound_email     => 'Inbound Email',
     feature_jobqueue          => 'Mail Queueing',
     feature_jsonrpc           => 'JSON-RPC Interface',
-    feature_jsonrpc_faster    => 'Make JSON-RPC Faster',
     feature_new_charts        => 'New Charts',
     feature_old_charts        => 'Old Charts',
     feature_memcached         => 'Memcached Support',