From mboxrd@z Thu Jan  1 00:00:00 1970
From: hofmann@leo-andres.de
To: development@lists.ipfire.org
Subject: Re: [PATCH 5/7] pakfire.cgi: Implement Post/Redirect/Get pattern
Date: Fri, 20 May 2022 09:47:40 +0000
Message-ID: <174029b36accce4549b0ffad84c6d26f@leo-andres.de>
In-Reply-To: <A35564E9-4E2E-4A45-B7A6-06B5BD128A2C@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============1808546691309007845=="
List-Id: <development.lists.ipfire.org>

--===============1808546691309007845==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

Hi,
Please excuse my delayed reply, I wanted to test core 168 first.

12. Mai 2022 11:30, "Michael Tremer" <michael.tremer(a)ipfire.org> schrieb:

> Hello,
>=20
>> On 8 May 2022, at 13:09, Leo-Andres Hofmann <hofmann(a)leo-andres.de> wrot=
e:
>>=20
>> Refreshing the Pakfire page may cause a command to be
>> executed multiple times and induce odd errors.
>>=20
>> 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.
>>=20
>> Fixes: #12781
>>=20
>> Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
>> ---
>> html/cgi-bin/pakfire.cgi | 56 +++++++++++++++++++++++++++++++++++-----
>> 1 file changed, 50 insertions(+), 6 deletions(-)
>>=20
>> 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 @@
>>=20
>> use strict;
>> use List::Util qw(any);
>> +use URI;
>>=20
>> # enable only the following on debugging purpose
>> #use warnings;
>> @@ -37,12 +38,17 @@ my %color =3D ();
>> my %pakfiresettings =3D ();
>> my %mainsettings =3D ();
>>=20
>> +# The page mode is used to explictly switch between user interface functi=
ons:
>> +my $PM_DEFAULT =3D 'default'; # Default user interface with command proce=
ssing
>> +my $PM_LOGREAD =3D 'logread'; # Log messages viewer (ignores all commands)
>> +my $pagemode =3D $PM_DEFAULT;
>> +
>> # Load general settings
>> &General::readhash("${General::swroot}/main/settings", \%mainsettings);
>> &General::readhash("${General::swroot}/pakfire/settings", \%pakfiresetting=
s);
>> &General::readhash("/srv/web/ipfire/html/themes/ipfire/include/colors.txt"=
, \%color);
>>=20
>> -# Get CGI request data
>> +# Get CGI POST request data
>> $cgiparams{'ACTION'} =3D '';
>> $cgiparams{'FORCE'} =3D '';
>>=20
>> @@ -51,6 +57,17 @@ $cgiparams{'DELPAKS'} =3D '';
>>=20
>> &Header::getcgihash(\%cgiparams);
>>=20
>> +# 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') {
>> }
>>=20
>> ### 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 l=
og 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-in=
teractive",
>> "--no-colors", @pkgs);
>> + &_http_pagemode_redirect($PM_LOGREAD, 1);
>> } elsif(($cgiparams{'ACTION'} eq 'remove') && ($cgiparams{'FORCE'} eq 'on'=
)) {
>> my @pkgs =3D split(/\|/, $cgiparams{'DELPAKS'});
>> &General::system_background("/usr/local/bin/pakfire", "remove", "--non-int=
eractive", "--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"};
>>=20
>> @@ -122,6 +144,7 @@ if($cgiparams{'ACTION'} ne '') {
>>=20
>> # 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();
>> }
>>=20
>> -# 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");
>>=20
>> print <<END
>> @@ -253,7 +276,8 @@ END
>> }
>>=20
>> # 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'});
>>=20
>> my @pkgs =3D split(/\|/, $cgiparams{'INSPAKS'});
>> @@ -291,7 +315,7 @@ END
>> &Header::closepage();
>> exit;
>>=20
>> -} elsif (($cgiparams{'ACTION'} eq 'remove') && (! &_is_pakfire_busy())) {
>> +} elsif (($cgiparams{'ACTION'} eq 'remove') && ($pagemode eq $PM_DEFAULT)=
) {
>> &Header::openbox("100%", "center", $Lang::tr{'request'});
>>=20
>> 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 al=
ready 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 ("$pagemod=
e") 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{'S=
CRIPT_NAME'}?mode=3D${mode}";
>> + print "Status: 303 See Other\n";
>> + print "Location: $location\n";
>=20
> I believe that technically you would want another newline at the end of the=
 header.

Yes the second newline would terminate the header. I want Header::showhttphea=
ders() to be able to
print it's headers later on, so I don't close the header yet.

> Would you also not want to call =E2=80=9Cexit(0)=E2=80=9D here to finish pr=
ocessing 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, pro=
bably until the forked
Pakfire process terminated.
If this happened, the browser waited and did not immediately follow the redir=
ect. However, it was able to start rendering the page received so far.

That's why I decided to send a redirect header and additionally generate the =
log viewer before
exiting. This way the user hopefully never gets to see a blank page.

With core 168 installed on my test system I noticed that it happens much less=
 often now.
Personally I found this very difficult to reproduce and would like to leave m=
y solution it as it is.

Best regards
Leo

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

--===============1808546691309007845==--