From mboxrd@z Thu Jan 1 00:00:00 1970 From: Leo Hofmann To: development@lists.ipfire.org Subject: Re: [PATCH 5/7] pakfire.cgi: Implement Post/Redirect/Get pattern Date: Sat, 04 Jun 2022 16:43:13 +0200 Message-ID: <5f3f67c9-ead6-0600-9223-a05b13d1d33f@leo-andres.de> In-Reply-To: <096BA664-343E-4677-92AD-887CBB00E9F7@ipfire.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1884385256408419431==" List-Id: --===============1884385256408419431== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi Michael, Am 31.05.2022 um 14:10 schrieb Michael Tremer: > Hello, > >> On 20 May 2022, at 10:47, hofmann(a)leo-andres.de wrote: >> >> Hi, >> Please excuse my delayed reply, I wanted to test core 168 first. >> >> 12. Mai 2022 11:30, "Michael Tremer" schrieb: >> >>> Hello, >>> >>>> On 8 May 2022, at 13:09, Leo-Andres Hofmann wr= ote: >>>> >>>> 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 >>>> --- >>>> 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 =3D (); >>>> my %pakfiresettings =3D (); >>>> my %mainsettings =3D (); >>>> >>>> +# The page mode is used to explictly switch between user interface func= tions: >>>> +my $PM_DEFAULT =3D 'default'; # Default user interface with command pro= cessing >>>> +my $PM_LOGREAD =3D 'logread'; # Log messages viewer (ignores all comman= ds) >>>> +my $pagemode =3D $PM_DEFAULT; >>>> + >>>> # Load general settings >>>> &General::readhash("${General::swroot}/main/settings", \%mainsettings); >>>> &General::readhash("${General::swroot}/pakfire/settings", \%pakfiresetti= ngs); >>>> &General::readhash("/srv/web/ipfire/html/themes/ipfire/include/colors.tx= t", \%color); >>>> >>>> -# Get CGI request data >>>> +# Get CGI POST request data >>>> $cgiparams{'ACTION'} =3D ''; >>>> $cgiparams{'FORCE'} =3D ''; >>>> >>>> @@ -51,6 +57,17 @@ $cgiparams{'DELPAKS'} =3D ''; >>>> >>>> &Header::getcgihash(\%cgiparams); >>>> >>>> +# Get CGI GET request data (if available) >>>> +if($ENV{'QUERY_STRING'}) { >>>> + my $uri =3D URI->new($ENV{'REQUEST_URI'}); >>>> + my %query =3D $uri->query_form; >>>> + >>>> + my $mode =3D lc($query{'mode'} // ''); >>>> + if(($mode eq $PM_DEFAULT) || ($mode eq $PM_LOGREAD)) { >>>> + $pagemode =3D $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 =3D $Lang::tr{'pakfire already busy'}; >>>> + $pagemode =3D $PM_LOGREAD; # Running Pakfire instance found, switch to= log viewer mode >>>> } elsif(($cgiparams{'ACTION'} eq 'install') && ($cgiparams{'FORCE'} eq '= on')) { >>>> my @pkgs =3D 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 'o= n')) { >>>> my @pkgs =3D split(/\|/, $cgiparams{'DELPAKS'}); >>>> &General::system_background("/usr/local/bin/pakfire", "remove", "--non-i= nteractive", "--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"} =3D $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 <>>> @@ -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 =3D 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_DEFAUL= T)) { >>>> &Header::openbox("100%", "center", $Lang::tr{'request'}); >>>> >>>> my @pkgs =3D 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 ("$pagem= ode") is also updated >>>> immediately. >>>> +sub _http_pagemode_redirect { >>>> + my ($mode, $switch_mode) =3D @_; >>>> + $mode //=3D $PM_DEFAULT; >>>> + $switch_mode //=3D 0; >>>> + >>>> + # Send HTTP redirect with GET parameter >>>> + my $location =3D "https://$ENV{'SERVER_NAME'}:$ENV{'SERVER_PORT'}$ENV{= 'SCRIPT_NAME'}?mode=3D${mode}"; >>>> + print "Status: 303 See Other\n"; >>>> + print "Location: $location\n"; >>> I believe that technically you would want another newline at the end of t= he header. >> Yes the second newline would terminate the header. I want Header::showhttp= headers() to be able to >> print it's headers later on, so I don't close the header yet. > Oh, you intend to send a page? > > Why? That would never be shown when browsers follow the redirect. I'll answer below, I think this is all related. > >>> Would you also not want to call =E2=80=9Cexit(0)=E2=80=9D here to finish = processing the script? What else is there >>> to do after you have redirected the user? >> I found that sometimes Perl did not close the connection for a long time, = probably until the forked >> Pakfire process terminated. > When the main CGI script terminates, the connection should be closed (unles= s you are using HTTP/1.1 with keep-alive). > >> If this happened, the browser waited and did not immediately follow the re= direct. However, it was able to start rendering the page received so far. > Yes, you are sending some content there. I don=E2=80=99t quite unterstand w= hy. > >> That's why I decided to send a redirect header and additionally generate t= he log viewer before >> exiting. This way the user hopefully never gets to see a blank page. > I am not sure what the problem is, but I thought the exit(0) would fix it :) Strangely enough, this did not work for me. I used two test systems for this:= Virtualbox on Windows and a VM on a Proxmox host. With both systems I had the problem that the page sometimes blocked the conne= ction. I tried exit(0), closing STDIN/STDOUT, without success. I used htop to monitor the running processes: The connection remained open until the Pakfire process terminated. My browser= did not follow the redirect while the connection was still open. To avoid having to wait on a blank page, I have chosen to send a page along w= ith the redirect. You're right, if the redirect would always work immediately, all this would b= e unnecessary. But honestly it's beyond my knowledge to figure out what exactly is happening= here. Do you know if this has been worked on? If this does not occur any more, I'd be happy to remove my workaround. Best Leo > >> With core 168 installed on my test system I noticed that it happens much l= ess often now. >> Personally I found this very difficult to reproduce and would like to leav= e my solution it as it is. >> >> Best regards >> Leo >> >>> -Michael >>> >>>> + >>>> + # Change global page mode >>>> + if($switch_mode) { >>>> + $pagemode =3D $mode; >>>> + } >>>> +} >>>> -- >>>> 2.27.0.windows.1 --===============1884385256408419431==--