Im ersten Teil dieser Geschichte führte uns eine SQL Injection zu zwei Statistics-Helpern, zwei buildSelects()-Funktionen und schließlich zu zwei Funktionen, die nichts weiter tun, als eine Zahl zu quadrieren.
function square($number)
{
if ($number == 0) {
$squarenumber = 0;
} else {
$squarenumber = $number * $number;
}
return $squarenumber;
}
Zweimal.
Die Git-Historie führte zurück ins Jahr 2016. Für die Public Statistics wurde damals ein großer Teil der bestehenden Statistics-Implementierung abgespalten. Seitdem entwickelten sich Admin Statistics und Public Statistics getrennt weiter – bis eine SQL Injection zehn Jahre später beide Zweige wieder einholte.
Damit war die Geschichte eigentlich erzählt. Oder?
Denn sobald man eine solche technische Schuld erkannt hat, stellt sich zwangsläufig die nächste Frage: Wie wird man sie wieder los?
Die Antwort scheint zunächst offensichtlich. Und genau damit beginnt das nächste Problem.
Ein Hinweis vorab: Anders als im ersten Teil, in dem ich vor allem berichtet und belegt habe, bewege ich mich hier stärker auf dem Feld der Einschätzung. Was folgt, ist mein Vorschlag, wie ich an dieses Problem herangehen würde – keine dokumentierte Faktenlage, sondern eine Meinung, die ich so gut wie möglich an konkreten Fundstellen im Code festmache.
Dann führen wir die beiden Dateien eben wieder zusammen
Wir haben zwei große Helper mit gemeinsamer Herkunft. Viele Funktionen existieren zweimal. Einige davon sind nahezu identisch. Also könnte man die gemeinsamen Teile extrahieren. Vielleicht so:
abstract class AbstractStatisticsHelper
{
// gemeinsamer Code
}
class statistics_helper extends AbstractStatisticsHelper
{
// Admin
}
class userstatistics_helper extends AbstractStatisticsHelper
{
// Public
}
Problem gelöst – möchte man meinen.
Aus zwei God Classes hätten wir dann aber nur eine gemeinsame God Class und zwei kleinere God Classes gemacht. Die Duplizierung wäre geringer. Die Architektur wäre deswegen noch lange nicht besser.
Schlimmer noch: Nach zehn Jahren getrennter Entwicklung wissen wir nicht einmal mehr, ob Code, der ähnlich aussieht, tatsächlich dasselbe bedeuten soll.
Damit verändert sich die Frage. Nicht: Wie bekommen wir aus zwei Implementierungen wieder eine? Sondern: Dürfen wir aus diesen beiden Implementierungen überhaupt wieder eine machen?
Zehn Jahre hinterlassen Spuren
Eine Kopie bleibt nur in dem Moment identisch, in dem sie entsteht. Danach beginnt Evolution.
Ein Bug wird in der einen Variante gefunden und möglicherweise auch in der anderen behoben. Ein neues Feature betrifft nur den Admin-Bereich. Eine Änderung für eine neue PHP-Version landet vielleicht zunächst nur auf einer Seite. Public Statistics benötigen anderes Rendering. Eine Sicherheitskorrektur verändert eine Query. Eine andere Änderung wird schlicht vergessen.
Nach zehn Jahren sieht das ungefähr so aus:
ursprünglicher Code
│
┌──────┴──────┐
▼ ▼
Admin Public
│ │
Fix A Fix A
│ │
Feature B │
│ │
Fix C │
│ Änderung D
│ │
Refactoring E │
│ │
└──────┬──────┘
▼
heute
Ein Diff kann uns sagen, dass sich beide Varianten unterscheiden. Er kann uns nicht sagen, warum. Genau das ist der entscheidende Unterschied zwischen dupliziertem Code und dupliziertem Wissen: Duplizierten Code kann man mit einem Diff finden. Dupliziertes Wissen muss man rekonstruieren.
Unterschiedlich bedeutet nicht falsch
Ein Beispiel dafür lässt sich im tatsächlichen LimeSurvey-Code nachvollziehen: die Berechnung von Quartilen.
Admin Statistics und Public Statistics besitzen beide eine Methode getQuartile(). Historisch haben sie denselben Ursprung und dienen demselben fachlichen Zweck: der Berechnung eines Quartils. Heute unterscheiden sich ihre Implementierungen tatsächlich: Die Admin-Variante arbeitet mit CDbCriteria und baut ihre Bedingungen darüber auf. Die Public-Variante verzichtet darauf und setzt die entsprechende SQL-Bedingung direkt als String zusammen.
// Admin (statistics_helper.php)
$criteria->addCondition(Yii::app()->db->quoteColumnName($fielddata['fieldname']) . " IS NOT null");
// Public (userstatistics_helper.php)
$query = " FROM {{responses_$surveyid}} WHERE " . Yii::app()->db->quoteColumnName($fieldname) . " IS NOT null";
Zwei Wege zum selben fachlichen Ziel, technisch unterschiedlich gelöst.
Was bedeutet das? Vielleicht wurde die Admin-Version irgendwann auf CDbCriteria umgestellt und die Änderung nie auf Public Statistics übertragen. Vielleicht gab es dafür einen Grund, der mit den unterschiedlichen Einsatzkontexten zusammenhängt. Vielleicht ist die eine Variante schlicht die ältere, unveränderte Fassung. Aus dem Diff allein wissen wir es nicht.
Das ist ein gefährlicher Moment beim Refactoring von Legacy-Code. Der Wunsch nach Vereinheitlichung verführt zu einer einfachen Annahme: Das sieht ähnlich aus, also sollte es gleich sein. Nach zehn Jahren Evolution ist diese Annahme für mich nicht mehr zulässig.
Die erste Regel des Refactorings lautet deshalb, so paradox das klingt: noch nichts refactoren.
Verhalten einfrieren, bevor man es verändert
Bevor eine gemeinsame Implementierung entsteht, müsste zunächst geklärt werden, welches Verhalten überhaupt existiert. Hier würde ich auf Characterization Tests setzen.
Sie unterscheiden sich in ihrer Fragestellung von klassischen Tests. Bei neuem Code fragen wir: Was soll diese Komponente tun? Bei Legacy-Code lautet die erste Frage eher: Was tut diese Komponente heute tatsächlich? Das ist nicht dasselbe. Ein Characterization Test dokumentiert zunächst vorhandenes Verhalten – selbst wenn man dieses Verhalten später für merkwürdig hält.
Für die beiden Statistics-Implementierungen könnte man dieselben Response-Daten durch Admin und Public Statistics laufen lassen und die Ergebnisse vergleichen. Wo sie übereinstimmen, hat man einen ersten Hinweis auf gemeinsames Verhalten. Wo sie abweichen, hat man noch keinen Bug gefunden – man hat eine Frage gefunden, die sich in eine von mehreren Kategorien einordnen lässt: fachlich notwendig, Feature nur einer Variante, technische Verbesserung, Bugfix nur einer Variante, Regression, oder historische Drift ohne erkennbaren Grund.
Diese letzte Kategorie ist vermutlich die unangenehmste. Aber ein bewusstes „unbekannt" ist beim Refactoring ein besserer Zustand als eine selbstbewusst falsche Annahme.
Die versteckte Abhängigkeit
Schaut man sich buildSelects() konzeptionell an, sieht der heutige Datenfluss etwa so aus: Request kommt herein, wird interpretiert, trifft auf Survey- und Fragetyp-Wissen, wird validiert, mündet in SQL. Alles in derselben Funktion.
Das eigentliche Problem ist damit größer als die Tatsache, dass die Funktion zweimal existiert: HTTP, Domain und Persistence besitzen keine klare Grenze.
Der erste sinnvolle Schritt wäre deshalb unspektakulär: buildSelects() sollte nicht selbst auf $_POST zugreifen. Statt die Filterwerte implizit aus dem globalen Request-Zustand zu lesen, würde ich sie explizit als Parameter übergeben. Keine neue Architektur, kein Pattern, kein neues Framework – nur eine versteckte Abhängigkeit weniger.
Das klingt banal. Es verändert aber etwas Entscheidendes: Zum ersten Mal ließe sich die Filterlogik mit beliebigen Eingaben aufrufen, ohne dafür einen HTTP-Request simulieren zu müssen. Und damit würde sie testbar.
Die explizite Übergabe der Request-Daten wäre dabei erst der Anfang. Im nächsten Schritt müsste aus den untrusted Rohdaten eine validierte interne Repräsentation entstehen. Erst hinter dieser Grenze dürfte der nachfolgende Code darauf vertrauen, mit gültigen Response-Feldern zu arbeiten.
Eine Trust Boundary entsteht
Bei sicherheitsrelevantem Code ist diese Grenze besonders wichtig. Ein HTTP-Request ist grundsätzlich untrusted input. Ein intern repräsentiertes, validiertes Response-Feld sollte es nicht mehr sein.
Heute muss tief in buildSelects() immer wieder mitgedacht werden: Dieser String stammt möglicherweise aus einem Request. Nach einer sauberen Grenze dürfte der dahinterliegende Code voraussetzen: Dieses Feld wurde bereits gegen das Survey-Schema validiert.
Das ist mehr als Code-Aufräumen. Eine solche Grenze würde beginnen auszudrücken, welchen Daten man vertraut und welchen nicht – genau die Art von Grenze, deren Bedeutung PR #5225 aus Teil 1 besonders deutlich gemacht hat.
Was das für den nächsten Schritt bedeutet
Damit ergibt sich eine Reihenfolge, die ich vor jeder konkreten Architekturentscheidung für richtig halte: erst verstehen, welches Verhalten heute tatsächlich existiert. Erst danach entscheiden, was gemeinsam sein sollte. Die Frage “Sind diese beiden Funktionen gleich?” ist die falsche Frage für den Anfang. Die richtige lautet: Sollten sie gleich sein?
Diese zweite Frage lässt sich nicht mit einem Diff beantworten. Man braucht Wissen über die Domäne, die Historie und die unterschiedlichen Einsatzkontexte – Wissen, das sich über zehn Jahre eben nicht von selbst mitkopiert hat, als square() zum zweiten Mal entstand.
Wie eine konkrete Architektur aussehen könnte, die aus diesem Verständnis heraus entsteht, und wie man sie einführt, ohne den bestehenden Betrieb zu gefährden, ist Thema des nächsten Teils: Wir haben verstanden, was gemeinsam sein sollte. Aber wie verhindert man, dass der gemeinsame Kern wieder von Yii, HTTP, Session, SQL und Rendering durchwachsen wird?