Van: Peter Fokker
Aan: Dirk Schouten
Karin Abma
Fred Stuurman
Datum: 2006-12-22
Betreft: Bevindingen mailpage-module Site@School
1. Inleiding
============
Medio september 2006 heb ik een aantal adviezen geformuleerd [1] om te
komen tot de beantwoording van de vraag
"Moet Site@School geheel vanaf nul worden herschreven of is het
mogelijk om de bestaande code zodanig aan te passen dat het
systeem veilig en robuust wordt".
De conclusie was dat het op dat moment onmogelijk was om deze vraag te
beantwoorden. Wel was het mogelijk om een aantal algemene
aanbevelingen te doen om de code leesbaarder te maken en de kwaliteit
van de code te verhogen.
Om te voorkomen dat het gehele systeem overhoop gehaald moest worden
zijn deze aanbevelingen in eerste instantie uitgewerkt in een enkele
'model-module'. Gekozen is voor de module 'mailpage'.
De aangepaste bestanden zijn:
- starnet/modules/mailpage/mailpage.php [2]
- starnet/modules/mailpage/admin.php [3]
- starnet/core/common.inc.php (sanitize function) [4]
Ik heb deze bestanden beoordeeld op de aanbevelingen uit de eerder
aangehaalde netpostbericht [1].
2. Bevindingen
==============
Kortheidshalve refereer ik in onderstaand overzicht aan de
paragraafnummers uit [1].
Referentie: 2.3.1 Opmaak van de code
------------------------------------
De code is iets minder onoverzichtelijk geworden, waarschijnlijk
dankzij een beautifier. Wel zie ik nog steeds veel gemixte code,
bijv. in mailpage.php rond regel 220; daar staat een heel stuk
JavaScript in moeilijk te lezen opmaak en niet-ingesprongen blokken.
Kennelijk kwam de beautifier hier ook niet uit.
Opvallend is dat de code nog steeds een zeer lange rij instructies is,
zonder functies (subroutines). Voorbeeld: een enkele switch/case van
ruim 225 regels (=bijna 4 kantjes A4) in mailpage.php ongeveer
vanaf regel 112. Zelfs bij het in landscape afdrukken op A3 is dit
zeer moeilijk te overzien.
Referentie: 2.3.2 Programmalogica, sub a: inputvalidatie
--------------------------------------------------------
Hier is wel een verbetering te zien in de vorm van de functie
sanitize(). Echter, deze functie op zich is wel enigszins
problematisch. Ik kom daar later nog op terug.
Referentie: 2.3.3 Documentatie
------------------------------
Ook het bestand admin.php (252 regels) is een lange rij van
instructies met op regel 108 het begin van een switch/case die
doorloopt tot regel 248. De in-line documentatie "start of switch" op
deze regel 108 zegt mij in het geheel niets. Ik kan de code lezen en
op regel 108 staat dit:
switch ($modoption) // start of switch
Tja. Ik blijf zitten met de vraag wat er nu eigenlijk gebeurt.
Referentie: 3.2 Herformatteer de code
-------------------------------------
Zoals hiervoor al gezegd is hier een verbetering op te merken. Dat wil
niet zeggen dat het niet beter zou kunnen. Met name de formattering
van de _output_ is nog niet optimaal. Immers, ook de gegenereerde HTML
heeft ook baat bij een systematische formattering, met name op het
gebied van de embedded javascript.
Referentie: 3.3 Splits de code in hanteerbare stukken
-----------------------------------------------------
Helaas moet ik constateren dat hier nauwelijks verbetering is te
zien. De gigantische switch/case-constructies zouden echt prima
opgesplitst kunnen worden zodanig dat het programma veel beter te
begrijpen en te onderhouden is. Ik zie in admin.php dit (met
regelnummers):
108: switch ($modoption) // start of switch
111: case save_config :
122: case delete_contact :
127: case save_contact :
147: case config :
166: case add_contact :
189: case edit_contact :
215: case list_contacts :
248: } //end of switch
Overzichtelijker zou zijn:
switch ($modoption)
{
case save_config :
do_save_config();
break;
case delete_contact :
do_delete_contact();
break;
case save_contact :
do_save_contact();
break;
case config :
do_config();
break;
case add_contact :
do_add_contact();
break;
case edit_contact :
do_edit_contact();
break;
case list_contacts :
do_list_contacts();
break;
default:
trigger_error("unknonw option '$modoption'");
do_list_contacts();
break;
} //end of switch
gevolgd door de definities van de verschillende do_xxx()
functies. Door deze werkwijze zie je in ongeveer 30 regels -- een half
kantje A4 -- precies de logica van het programma. De details zijn dan
wel te vinden in de individuele routines, maar voor de grote lijn
leidt dat alleen maar af.
Een bijkomend voordeel van het gebruiken van functies is dat elke
gebruikte globale variabele expliciet gedeclareerd MOET
worden. Daarmee wordt je als programmeur min of meer gedwongen om je
af te vragen welke variabele je eigenlijk binnen die functie gebruikt
en vervolgens of die globale variabelen eigenlijk wel 'sane' zijn (dus
niet dankzij een register_globals = true gemanipuleerd kunnen worden
door een indringer).
Referentie: 3.5 Defensief programmeren
--------------------------------------
Dit gaat al beter dan voorheen, o.a. wegens de functie sanitize().
Wel levert het toepassen van reguliere expressies soms onbedoelde
resultaten op die moeilijk te vinden zijn, vooral als er geen enkele
documentatie bij zit. Een voorbeeld is te vinden in mailpage.php (met
regelnummers):
229: function checkinput ( form )
230: {
231: var filter = /^([a-zA-Z0-9_\.\-])+\@(([a-zA-Z0-9\-])+\.)+([a-zA-Z0-9]{2,4})+$/;
232:
[...]
246: if (!filter.test(form.email.value))
247: {
248: alert( \"$sas_lang[email_module_noemail_error]\" );
249: return false ;
250: }
Mijn interpretatie van het reguliere expressie-object 'filter' is dat
hier een poging wordt gedaan om client-side een netpostadres te
valideren. Ruwweg moet een geldig adres voldoen aan 1 of meer
letters/cijfers/underscore/dot/dash gevolgd door 1 at gevolgd door 1
of meer reeksn van 1 of meer letters/cijfers/dash eindigend op een
dot, gevolgd door een string van 2 tot 4 letters/cijfers. Mmmmm, en
als ik nou het perfect legale email-adres
info@leiden.volkenkunde.museum wil invoeren? Anderzijds wordt het
apert onjuiste adres 123@456.789 wel geaccepteerd. Tja.
Referentie: 3.7 In-line documentatie
------------------------------------
In mailpage.php lees ik
220: