Hello, > On 4 Oct 2022, at 19:59, Robin Roevens wrote: > > Hi Michael > > Michael Tremer schreef op di 04-10-2022 om 13:49 [+0100]: >> Hello, >> >>> On 4 Oct 2022, at 11:33, Robin Roevens >>> wrote: >>> >>> Hi >>> >>> Michael Tremer schreef op di 04-10-2022 om 10:51 [+0100]: >>>> Hello, >>>> >>>> I am going to start with the second patch, because that is going >>>> to >>>> be shorter :) >>>> >>>>> On 3 Oct 2022, at 16:27, Robin Roevens >>>>> >>>>> wrote: >>>>> >>>>> * addonctrl's new functionality to control explicit addon >>>>> services >>>>> was >>>>> implemented. >>>>> * Change 'Addon' column header to 'Addon Service' to be clear >>>>> that >>>>> it's not addons but services listed here. >>>>> * Services not matching the name of the addon now display the >>>>> addon >>>>> name between parentheses, so the user knows where the service >>>>> comes >>>>> from. >>>>> * When no valid runlevel symlink is found by addonctrl for a >>>>> service, >>>>> the 'enable on boot' checkbox is replaced by a small >>>>> exclamation >>>>> point >>>>> with alt-text "No valid runlevel symlink was found for the >>>>> initscript of >>>>> this service." to inform user why a service can't be enabled. >>>>> * Added German and Dutch translation for above message. >>>>> >>>>> Fixes: Bug#12935 >>>>> Signed-off-by: Robin Roevens >>>>> --- >>>>> html/cgi-bin/services.cgi | 103 +++++++++++++++---------------- >>>>> ---- >>>>> --- >>>>> langs/de/cgi-bin/de.pl | 1 + >>>>> langs/en/cgi-bin/en.pl | 1 + >>>>> langs/nl/cgi-bin/nl.pl | 1 + >>>>> 4 files changed, 42 insertions(+), 64 deletions(-) >>>>> >>>>> diff --git a/html/cgi-bin/services.cgi b/html/cgi- >>>>> bin/services.cgi >>>>> index 29926ecc3..e91da884b 100644 >>>>> --- a/html/cgi-bin/services.cgi >>>>> +++ b/html/cgi-bin/services.cgi >>>>> @@ -142,14 +142,14 @@ END >>>>> my $paramstr=$ENV{QUERY_STRING}; >>>>> my @param=split(/!/, $paramstr); >>>>> if ($param[1] ne ''){ >>>>> - &General::system("/usr/local/bin/addonctrl", >>>>> "$param[0]", "$param[1]"); >>>>> + &General::system("/usr/local/bin/addonctrl", >>>>> "$param[0]", "$param[1]", "$param[2]"); >>>>> } >>>> >>>> Although this has been like this before, I am strongly against >>>> passing anything from the request to the setuid binary. >>>> >>>> This will open us up for any shell command injections and so on. >>>> I >>>> would prefer to check the second argument to be at least on some >>>> kind >>>> of list as there are only a few keywords allowed (start, stop, >>>> restart, …) and the other parameters should only contain ASCII >>>> letters, numbers and maybe dash and underscore. Nothing else >>>> should >>>> be passed through here. >>> >>> Good point. I was thinking this regexp: /[^a-zA-Z_0-9\-\.]/ or >>> should I >>> not include a '.'? I don't think there currently are any addons or >>> initscripts that contain a '.', but it could be in the future? >> >> I was thinking about the dot as well, and then thought it would not >> be required to include it since we do not have any services like >> this, and I do not remember any of them using a dot on any other >> distributions. > > I don't think I have indeed ever encountered an initscript with a dot > in it. I was only thinking, as it is a legit and common character for > filenames (in general) it could cause some confusion as to why the > webui would not want to work if we ever come across an initscript with > a dot in its name. But on second thought, the odds are probably very > very low that we will ever have that on IPFire. So I'l leave the dot > out. Thank you. The dot is a little bit concerning, because in a full path, it has special meanings (e.g. /etc/init.d/../../bin/bash). We don’t allow using / here, so we should be safe, but it is probably still a good idea to be as strict as possible. > >> >> The regular expression is slightly incorrect though. You want it to >> be like this: >> >> /^[A-Za-z0-9\-\_]+$/ >> >> The ^ and $ indicate that the entire string must match and not only a >> part. And the + after the brackets with the list of allowed >> characters says that we need to have at least one character, but >> could be more. However, the entire string must only contain those >> characters. > > I have to disagree in the regular expression being incorrect :-). > The ^ right after the [ is not a start of line assertion here, but > makes it a character class negation which will match as soon as a > single character is found that is _not_ included in the list. So I was > looking for a single illegal character in the string, while your regex > will check if every character in the string is an allowed character. Hmm, I thought that that could have been the case, but then wrote the whole thing again because that seemed to be easier for me :) I kind of prefer my version, because it allows certain things and that is it. It feels more strict - probably isn’t. Disallowing seems to be logically more complex. -Michael > > > Regards > Robin > >> >> -Michael >> >>> >>> Regards >>> Robin >>>> >>>>> >>>>> print <>>>>
>>>>> >>>>> >>>>> - >>>>> + >>>>> >>>>> >>>>> >>>>> @@ -170,33 +170,35 @@ END >>>>> foreach my $pak (keys %paklist) { >>>>> my %metadata = &Pakfire::getmetadata($pak, >>>>> "installed"); >>>>> >>>>> + my $service; >>>>> + >>>>> if ("$metadata{'Services'}") { >>>>> - foreach my $service (split(/ /, >>>>> "$metadata{'Services'}")) { >>>>> - push(@addon_services, $service); >>>>> - } >>>>> - } >>>>> - } >>>>> + foreach $service (split(/ /, >>>>> "$metadata{'Services'}")) { >>>>> + $lines++; >>>>> + if ($lines % 2) { >>>>> + print ""; >>>>> + $col="bgcolor='$color{'colo >>>>> r22'}'"; >>>>> + } else { >>>>> + print ""; >>>>> + $col="bgcolor='$color{'colo >>>>> r20'}'"; >>>>> + } >>>>> >>>>> - foreach (@addon_services) { >>>>> - $lines++; >>>>> - if ($lines % 2){ >>>>> - print ""; >>>>> - $col="bgcolor='$color{'color22'}'"; >>>>> - }else{ >>>>> - print ""; >>>>> - $col="bgcolor='$color{'color20'}'"; >>>>> + # Add addon name to displayname of >>>>> service if servicename differs from addon >>>>> + my $displayname = ($pak ne >>>>> $service) ? "$service ($pak)" : $service; >>>>> + print " "; >>>>> + >>>>> + my $status = >>>>> isautorun($pak,$service,$col); >>>>> + print "$status "; >>>>> + print ""; >>>>> + print " "; >>>>> + my $status = >>>>> isrunningaddon($pak,$service,$col); >>>>> + $status =~ s/\\[[0-1]\;[0-9]+m//g; >>>>> + >>>>> + chomp($status); >>>>> + print "$status"; >>>>> + print ""; >>>>> + } >>>>> } >>>>> - print " "; >>>>> - my $status = isautorun($_,$col); >>>>> - print "$status "; >>>>> - print ""; >>>>> - print " "; >>>>> - my $status = isrunningaddon($_,$col); >>>>> - $status =~ s/\\[[0-1]\;[0-9]+m//g; >>>>> - >>>>> - chomp($status); >>>>> - print "$status"; >>>>> - print ""; >>>>> } >>>>> >>>>> print "
AddonAddon $Lang::tr{service}Boot>>>> colspan=2>$Lang::tr{'action'}$Lang::tr{'status'}
>>>> width='31%'>$displayname>>>> width='8%'>>>>> alt='$Lang::tr{'start'}' title='$Lang::tr{'start'}' >>>>> src='/images/go-up.png' border='0' />>>>> width='8%'>>>>> alt='$Lang::tr{'stop'}' title='$Lang::tr{'stop'}' >>>>> src='/images/go- >>>>> down.png' border='0' />
$_>>>> href='services.cgi?$_!start'>$Lang::tr{>>>> title='$Lang::tr{'start'}' src='/images/go-up.png' border='0' >>>>> />>>>> href='services.cgi?$_!stop'>$Lang::tr{>>>> title='$Lang::tr{'stop'}' src='/images/go-down.png' border='0' >>>>> />
\n"; >>>>> @@ -215,51 +217,24 @@ END >>>>> } >>>>> >>>>> sub isautorun (@) { >>>>> - my ($cmd, $col) = @_; >>>>> - >>>>> - # Init directory. >>>>> - my $initdir = "/etc/rc.d/rc3.d/"; >>>>> - >>>>> - my $status = ""; >>>>> + my ($pak, $service, $col) = @_; >>>>> + my @testcmd = >>>>> &General::system_output("/usr/local/bin/addonctrl", "$pak", >>>>> "boot- >>>>> status", "$service"); >>>>> + my $testcmd = @testcmd[0]; >>>>> + my $status = ">>>> alt='$Lang::tr{'service boot setting unavailable'}' >>>>> title='$Lang::tr{'service boot setting unavailable'}' >>>>> src='/images/dialog-warning.png' border='0' width='16' >>>>> height='16' >>>>> />"; >>>>> >>>>> - # Check if autorun for the given cmd is enabled. >>>>> - if ( &find_init("$cmd", "$initdir") ) { >>>>> + # Check if autorun for the given service is enabled. >>>>> + if ( $testcmd =~ /enabled\ on\ boot/ ) { >>>>> # Adjust status. >>>>> - $status = ">>>> href='services.cgi?$_!disable'>>>>> alt='$Lang::tr{'deactivate'}' >>>>> title='$Lang::tr{'deactivate'}' src='/images/on.gif' border='0' >>>>> width='16' height='16' />"; >>>>> - } else { >>>>> + $status = ">>>> href='services.cgi?$pak!disable!$service'>>>>> alt='$Lang::tr{'deactivate'}' title='$Lang::tr{'deactivate'}' >>>>> src='/images/on.gif' border='0' width='16' height='16' >>>>> />"; >>>>> + } elsif ( $testcmd =~ /disabled\ on\ boot/ ) { >>>>> # Adjust status. >>>>> - $status = ">>>> href='services.cgi?$_!enable'>$Lang::tr{>>>> title='$Lang::tr{'activate'}' src='/images/off.gif' border='0' >>>>> width='16' height='16' />"; >>>>> + $status = ">>>> href='services.cgi?$pak!enable!$service'>>>>> alt='$Lang::tr{'activate'}' title='$Lang::tr{'activate'}' >>>>> src='/images/off.gif' border='0' width='16' height='16' >>>>> />"; >>>>> } >>>>> >>>>> # Return the status. >>>>> return $status; >>>>> } >>>>> >>>>> -sub find_init (@) { >>>>> - my ($cmd, $dir) = @_; >>>>> - >>>>> - # Open given init directory. >>>>> - opendir (INITDIR, "$dir") || die "Cannot opendir $dir: $!"; >>>>> - >>>>> - # Read-in init files from directory. >>>>> - my @inits = readdir(INITDIR); >>>>> - >>>>> - # Close directory handle. >>>>> - closedir(INITDIR); >>>>> - >>>>> - # Loop through the directory. >>>>> - foreach my $init (@inits) { >>>>> - # Check if the current processed file belongs to >>>>> the given command. >>>>> - if ($init =~ /S\d+\d+$cmd\z/) { >>>>> - # Found, return "1" - True. >>>>> - return "1"; >>>>> - } >>>>> - } >>>>> - >>>>> - # Nothing found, return nothing. >>>>> - return; >>>>> -} >>>>> - >>>>> sub isrunning (@) { >>>>> my ($cmd, $col) = @_; >>>>> my $status = ">>>> bgcolor='${Header::colourred}'>>>>> color='white'>$Lang::tr{'stopped'}>>>> colspan='2' $col>"; >>>>> @@ -313,7 +288,7 @@ sub isrunning (@) { >>>>> } >>>>> >>>>> sub isrunningaddon (@) { >>>>> - my ($cmd, $col) = @_; >>>>> + my ($pak, $service, $col) = @_; >>>>> >>>>> my $status = ">>>> bgcolor='${Header::colourred}'>>>>> color='white'>$Lang::tr{'stopped'}>>>> colspan='2' $col>"; >>>>> my $pid = ''; >>>>> @@ -321,7 +296,7 @@ sub isrunningaddon (@) { >>>>> my $exename; >>>>> my @memory; >>>>> >>>>> - my @testcmd = >>>>> &General::system_output("/usr/local/bin/addonctrl", "$cmd", >>>>> "status"); >>>>> + my @testcmd = >>>>> &General::system_output("/usr/local/bin/addonctrl", "$pak", >>>>> "status", "$service"); >>>>> my $testcmd = @testcmd[0]; >>>>> >>>>> if ( $testcmd =~ /is\ running/ && $testcmd !~ /is\ not\ >>>>> running/){ >>>>> diff --git a/langs/de/cgi-bin/de.pl b/langs/de/cgi-bin/de.pl >>>>> index 798abcffc..db7d117b0 100644 >>>>> --- a/langs/de/cgi-bin/de.pl >>>>> +++ b/langs/de/cgi-bin/de.pl >>>>> @@ -2251,6 +2251,7 @@ >>>>> 'server string' => 'Server String', >>>>> 'service' => 'Dienst', >>>>> 'service added' => 'Benutzerdefinierter Netzwerkdienst wurde >>>>> hinzugefügt', >>>>> +'service boot setting unavailable' => 'Für das Initscript >>>>> dieses >>>>> Dienstes wurde kein gültiger Runlevel-Symlink gefunden.', >>>>> 'service name' => 'Name des Dienstes:', >>>>> 'service removed' => 'Benutzerdefinierter Netzwerkdienst wurde >>>>> entfernt', >>>>> 'service updated' => 'Benutzerdefinierter Netzwerkdienst wurde >>>>> aktualisiert', >>>>> diff --git a/langs/en/cgi-bin/en.pl b/langs/en/cgi-bin/en.pl >>>>> index f770e7cd9..60dca5be4 100644 >>>>> --- a/langs/en/cgi-bin/en.pl >>>>> +++ b/langs/en/cgi-bin/en.pl >>>>> @@ -2306,6 +2306,7 @@ >>>>> 'server string' => 'Server String', >>>>> 'service' => 'Service', >>>>> 'service added' => 'Custom network service added', >>>>> +'service boot setting unavailable' => 'No valid runlevel >>>>> symlink >>>>> was found for the initscript of this service.', >>>>> 'service name' => 'Service name:', >>>>> 'service removed' => 'Custom network service removed', >>>>> 'service updated' => 'Custom network service updated', >>>>> diff --git a/langs/nl/cgi-bin/nl.pl b/langs/nl/cgi-bin/nl.pl >>>>> index 49dabec99..4fd6955cc 100644 >>>>> --- a/langs/nl/cgi-bin/nl.pl >>>>> +++ b/langs/nl/cgi-bin/nl.pl >>>>> @@ -1899,6 +1899,7 @@ >>>>> 'server string' => 'Server String', >>>>> 'service' => 'Dienst', >>>>> 'service added' => 'Aangepaste netwerkdienst toegevoegd', >>>>> +'service boot setting unavailable' => 'Er werd voor het >>>>> initscript >>>>> van deze service geen geldige runlevel symlink gevonden.', >>>>> 'service name' => 'Dienstennaam:', >>>>> 'service removed' => 'Aangepaste netwerkdienst verwijderd', >>>>> 'service updated' => 'Aangepaste netwerkdienst bijgewerkt', >>>>> -- >>>>> 2.37.3 >>>>> >>>>> >>>>> -- >>>>> Dit bericht is gescanned op virussen en andere gevaarlijke >>>>> inhoud door MailScanner en lijkt schoon te zijn. >>>>> >>>> >>>> >>> >>> -- >>> Dit bericht is gescanned op virussen en andere gevaarlijke >>> inhoud door MailScanner en lijkt schoon te zijn. >> >> > > -- > Dit bericht is gescanned op virussen en andere gevaarlijke > inhoud door MailScanner en lijkt schoon te zijn.