Re: [PATCH 5/7] pakfire.cgi: Implement Post/Redirect/Get pattern

[Michael Tremer <michael.tremer@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 6436 bytes --]

Hello,

> On 8 May 2022, at 13:09, Leo-Andres Hofmann <hofmann(a)leo-andres.de> wrote:
> 
> Refreshing the Pakfire page may cause a command to be
> executed multiple times and induce odd errors.
> 
> This patch implements a HTTP 303 redirect after form processing,
> which causes the browser to discard the POST form data.
> Navigating backward or reloading the page now does not trigger
> multiple executions anymore.
> 
> Fixes: #12781
> 
> Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
> ---
> html/cgi-bin/pakfire.cgi | 56 +++++++++++++++++++++++++++++++++++-----
> 1 file changed, 50 insertions(+), 6 deletions(-)
> 
> diff --git a/html/cgi-bin/pakfire.cgi b/html/cgi-bin/pakfire.cgi
> index ec3ee2cc6..6fade81bd 100644
> --- a/html/cgi-bin/pakfire.cgi
> +++ b/html/cgi-bin/pakfire.cgi
> @@ -21,6 +21,7 @@
> 
> use strict;
> use List::Util qw(any);
> +use URI;
> 
> # enable only the following on debugging purpose
> #use warnings;
> @@ -37,12 +38,17 @@ my %color = ();
> my %pakfiresettings = ();
> my %mainsettings = ();
> 
> +# The page mode is used to explictly switch between user interface functions:
> +my $PM_DEFAULT = 'default'; # Default user interface with command processing
> +my $PM_LOGREAD = 'logread'; # Log messages viewer (ignores all commands)
> +my $pagemode = $PM_DEFAULT;
> +
> # Load general settings
> &General::readhash("${General::swroot}/main/settings", \%mainsettings);
> &General::readhash("${General::swroot}/pakfire/settings", \%pakfiresettings);
> &General::readhash("/srv/web/ipfire/html/themes/ipfire/include/colors.txt", \%color);
> 
> -# Get CGI request data
> +# Get CGI POST request data
> $cgiparams{'ACTION'} = '';
> $cgiparams{'FORCE'} = '';
> 
> @@ -51,6 +57,17 @@ $cgiparams{'DELPAKS'} = '';
> 
> &Header::getcgihash(\%cgiparams);
> 
> +# Get CGI GET request data (if available)
> +if($ENV{'QUERY_STRING'}) {
> +	my $uri = URI->new($ENV{'REQUEST_URI'});
> +	my %query = $uri->query_form;
> +
> +	my $mode = lc($query{'mode'} // '');
> +	if(($mode eq $PM_DEFAULT) || ($mode eq $PM_LOGREAD)) {
> +		$pagemode = $mode; # Limit to existing modes
> +	}
> +}
> +
> ### Process AJAX/JSON request ###
> if($cgiparams{'ACTION'} eq 'json-getstatus') {
> 	# Send HTTP headers
> @@ -96,19 +113,24 @@ if($cgiparams{'ACTION'} eq 'json-getstatus') {
> }
> 
> ### Process Pakfire install/update commands ###
> -if($cgiparams{'ACTION'} ne '') {
> +if(($cgiparams{'ACTION'} ne '') && ($pagemode eq $PM_DEFAULT)) {
> 	if(&_is_pakfire_busy()) {
> 		$errormessage = $Lang::tr{'pakfire already busy'};
> +		$pagemode = $PM_LOGREAD; # Running Pakfire instance found, switch to log viewer mode
> 	} elsif(($cgiparams{'ACTION'} eq 'install') && ($cgiparams{'FORCE'} eq 'on')) {
> 		my @pkgs = split(/\|/, $cgiparams{'INSPAKS'});
> 		&General::system_background("/usr/local/bin/pakfire", "install", "--non-interactive", "--no-colors", @pkgs);
> +		&_http_pagemode_redirect($PM_LOGREAD, 1);
> 	} elsif(($cgiparams{'ACTION'} eq 'remove') && ($cgiparams{'FORCE'} eq 'on')) {
> 		my @pkgs = split(/\|/, $cgiparams{'DELPAKS'});
> 		&General::system_background("/usr/local/bin/pakfire", "remove", "--non-interactive", "--no-colors", @pkgs);
> +		&_http_pagemode_redirect($PM_LOGREAD, 1);
> 	} elsif($cgiparams{'ACTION'} eq 'update') {
> 		&General::system_background("/usr/local/bin/pakfire", "update", "--force", "--no-colors");
> +		&_http_pagemode_redirect($PM_LOGREAD, 1);
> 	} elsif($cgiparams{'ACTION'} eq 'upgrade') {
> 		&General::system_background("/usr/local/bin/pakfire", "upgrade", "-y", "--no-colors");
> +		&_http_pagemode_redirect($PM_LOGREAD, 1);
> 	} elsif($cgiparams{'ACTION'} eq $Lang::tr{'save'}) {
> 		$pakfiresettings{"TREE"} = $cgiparams{"TREE"};
> 
> @@ -122,6 +144,7 @@ if($cgiparams{'ACTION'} ne '') {
> 
> 			# Update lists
> 			&General::system_background("/usr/local/bin/pakfire", "update", "--force", "--no-colors");
> +			&_http_pagemode_redirect($PM_LOGREAD, 1);
> 		}
> 	}
> }
> @@ -221,8 +244,8 @@ if ($errormessage) {
> 	&Header::closebox();
> }
> 
> -# Show log output while Pakfire is running
> -if(&_is_pakfire_busy()) {
> +# Show only log output while Pakfire is running and stop afterwards
> +if(($pagemode eq $PM_LOGREAD) || (&_is_pakfire_busy())) {
> 	&Header::openbox("100%", "center", "Pakfire");
> 
> 	print <<END
> @@ -253,7 +276,8 @@ END
> }
> 
> # Show Pakfire install/remove dependencies and confirm form
> -if (($cgiparams{'ACTION'} eq 'install') && (! &_is_pakfire_busy())) {
> +# (_is_pakfire_busy status was checked before and can be omitted)
> +if (($cgiparams{'ACTION'} eq 'install') && ($pagemode eq $PM_DEFAULT)) {
> 	&Header::openbox("100%", "center", $Lang::tr{'request'});
> 
> 	my @pkgs = split(/\|/, $cgiparams{'INSPAKS'});
> @@ -291,7 +315,7 @@ END
> 	&Header::closepage();
> 	exit;
> 
> -} elsif (($cgiparams{'ACTION'} eq 'remove') && (! &_is_pakfire_busy())) {
> +} elsif (($cgiparams{'ACTION'} eq 'remove') && ($pagemode eq $PM_DEFAULT)) {
> 	&Header::openbox("100%", "center", $Lang::tr{'request'});
> 
> 	my @pkgs = split(/\|/, $cgiparams{'DELPAKS'});
> @@ -476,3 +500,23 @@ sub _start_json_output {
> 	print "Content-Type: application/json\n";
> 	print "\n"; # End of HTTP headers
> }
> +
> +# Send HTTP 303 redirect headers to change page mode
> +# GET is always used to display the redirected page, which will remove already processed POST form data.
> +# Note: Custom headers must be sent before the HTML output is started by &Header::showhttpheaders().
> +# If switch_mode is set to true, the global page mode variable ("$pagemode") is also updated immediately.
> +sub _http_pagemode_redirect {
> +	my ($mode, $switch_mode) = @_;
> +	$mode //= $PM_DEFAULT;
> +	$switch_mode //= 0;
> +
> +	# Send HTTP redirect with GET parameter
> +	my $location = "https://$ENV{'SERVER_NAME'}:$ENV{'SERVER_PORT'}$ENV{'SCRIPT_NAME'}?mode=${mode}";
> +	print "Status: 303 See Other\n";
> +	print "Location: $location\n";

I believe that technically you would want another newline at the end of the header.

Would you also not want to call “exit(0)” here to finish processing the script? What else is there to do after you have redirected the user?

-Michael

> +
> +	# Change global page mode
> +	if($switch_mode) {
> +		$pagemode = $mode;
> +	}
> +}
> -- 
> 2.27.0.windows.1
>