Aufteilen von Funktionen = doppelte Validierung

Für ein kleines Nebenprojekt bin ich am Werkeln mit codeigniter. Soweit so gut. Ich brauchte eine Funktion, die den Bildupload und anschließendes resizing erledigte. Kein Problem, dank Upload-Klasse und Image Manipulation-Klasse. Okay, das hier soll aber keine Lobhymne auf ein Framework werden, sondern ein allgemeines Thema ansprechen.

Ich hab also meine Funktion, die upload und resize erledigt. Wens interessiert:

public function process_image($cat_id)
{
	//----------------------------------
	//Upload
	
	if (intval($cat_id) <= 0)
		throw new Exception("Invalid Video ID");
	
	$upload_config = array
	(
		'upload_path' => IMAGES_PATH,
		'allowed_types' => 'gif|jpg|png|jpeg',
		'overwrite' => true
	);
	
	if (!is_dir(IMAGES_PATH))
		throw new Exception("Could not find Upload directory");

	$this->load->library('upload', $upload_config);

	if (!$this->upload->do_upload())
		throw new Exception("Upload Error: " . $this->upload->display_errors()); 

	$upload_info = $this->upload->data();

	//----------------------------------
	//Resize
	$thumbname = IMAGES_PATH . 'thumb_' . $cat_id . '_' . uniqid() . "." . $upload_info['image_type'];
				
	$resize_config = array
	(
		'source_image' => $upload_info['full_path'],
		'maintain_ratio' => false,
		'width' => 65,
		'height' => 65,
		'new_image' => $thumbname
	);
	
	$this->load->library('image_lib', $resize_config); 
	
	if (!$this->image_lib->resize())
	{
		@unlink($upload_info['full_path']);
		
		throw new Exception("Resize Error: " . $this->image_lib->display_errors());
	}
	
	@unlink($upload_info['full_path']);
	
	return true;
}

Nunja, als Mensch der die OOP Grundprinzipien hochhalten möchte, schmerzt es natürlich, dass die Funktion sich um Upload und Resize kümmert. Ich trenne also die Funktionen auf, Wiederverwendbarkeit und so…

Der Haken an der Sache

Dabei passiert nun das, was immer passiert: Ich fühle mich getrieben, den ganzen Kram neu zu validieren.

Ist die $cat_id noch gültig? Gibt es das Verzeichnis IMAGE_PATH noch? Existiert mein zu resizendes Bild und ist es wirklich ein Bild?

Toll. Hätte ich die Funktionen nicht aufgetrennt, könnte mich auf all das verlassen. Weil ich jetzt genau weiß, dass ich die Resize-Methode alleine nie in diesem Projekt brauchen werde, ist der Refactoring-Aufwand also komplett für die Katz, weil er mir keinen Vorteil bringt. Man kann argumentieren, dass man später ja irgendwann nochmal einen isolierten Resize brauchen werde, ja…

Naja, ich werde wohl zukünftig diesbezüglich meine Prinzipien ab und an über den Haufen werfen, wenn ich in so einem kleinen Projekt gewiss sein kann, dass dadurch kein Nachteil entsteht. Wie löst ihr solche Fragen?

Weitere Posts:

Dieser Beitrag wurde unter php, Software Engineering, webdev veröffentlicht. Setze ein Lesezeichen auf den Permalink.

9 Antworten auf Aufteilen von Funktionen = doppelte Validierung

  1. maTTes sagt:

    Mal zum von dir genannten PHP-Framework: Ist das ganz brauchbar, leicht verständlich und gut geordnet? Ich hatte nämlich auch vor, mich mal mit einem Framework zu beschäftigen, da bin ich natürlich schon auf viele gestoßen, die ich mir anschauen wollte: Zend, symfony und doctrine. Da fällt mir die Wahl natürlich schwer.
    Hast du da evtl. Tipps und Anregungen für mich?

    1. david sagt:

      Servus! Zum Einsteigen kann CodeIgniter auf jeden Fall nix schaden. Es lässt dir viele Freiheiten und ist leicht zu lernen. Der user guide ist exzellent, zwei kleine Video Tutorials gibts dazu auch. Mach dir mal ein Bild, ich finds ganz okay. Ob ich jetzt microsoft.com darauf aufbauen würde, müsste ich mir allerdings noch überlegen ;)

  2. Oliver sagt:

    Erstmal zum Framework. Viele Sachen, die da drauf aufbauen sind löchriger als ein Schweizer Käse. Bei PyroCMS hab ich gut ein Dutzend Sicherheitslücken gefunden, ohne wirklich gesucht zu haben u. a. auch ne tolle Lücke, mit der man alle Daten der Foren User sehen und ändern konnte. Neben der Tatsache, dass ich nicht mal ein Danke dafür bekommen habe (drauf gesch*ssen), haben sie es bis heute nicht geschafft alle Lücken zu beseitigen. Die schlimmsten haben sie erst entfernt, nachdem ich massiv genervt hab.

    Das meiste hängt an dem XSS Modul von Codeigniter, was einfach nur Mist ist. Ich hab ihnen z. B. auch ne Demo gemacht, was das Problem demonstriert => http://is.gd/DukFPG (tippt was in das Suchfeld) aber offensichtlich hing es da am Können oder am Wollen – Feedback gab es dann einfach nicht. Seitdem rate ich jedem von Codeigniter ab, weil ich sowas im Jahr 2011 schon für grob fahrlässig halte.

    Zum Problem – wer sagt denn, dass Funktionen nicht wiederverwendbar sind? So lange Du im gleichen Framework bist, kannst Du auch Funktionen wiederverwenden. Ich bin davon weg, päpstlicher als der Papst zu sein. Wenn ich weiß, dass ich es eh nicht noch mal brauche, dann ist mir das egal, ob es 100% akurater Code ist. Auch weil es, wie Du ja schon bemerkt hast, keinen wirklichen Vorteil bringt, alles neu zu schreiben.

    Kernfunktionen wie meine E-Mail Klasse, Worttrennung oder meine Klasse zum Generieren von PDFs warte ich seit Jahren fast pedantisch, weil die viele einzigartige Features haben und daher für mich sehr wertvoll sind. Auch mit Erweiterungen für mein Lieblingsframework bin ich recht ordentlich, wenn Kernfunktion (Session, DB, Userdaten, usw.) betroffen sind. Dafür nehm ich mir lieber Zeit, erweitere die Klassen und strukturiere alles ordentlich. Da nutze ich aber auch teilweise Funktionen, z. B. um Schreibaufwand zu sparen oder wenn es um Sachen geht, die PHP in meinen Augen eh längst können sollte. Ich käme also nie auf die Idee, sowas zu benutzen SuperUserFunktionen::model()->userinfo()->getRealIp(), wenn ich auch einfach getRealIp() nehmen kann.

    Wenn ich spezielle Funktionen hab, die ich eh nie wieder brauche, dann sollen die einfach nur funktionieren und nicht hübsch aussehen. Mir hat auch noch nie einer gesagt … dein Code ist so ordentlich … aber schon öfter … wow, das Feature ist geil …

    1. david sagt:

      Danke für deine Antwort! Dass jetzt das PyroCMS abkackt würde ich allerdings nicht als Gegenargument zu CodeIgniter sehen. Expression Engine basiert auch, und da hab ich nun bisher noch keine Horrormeldungen drüber gehört. Kommt immer drauf an, wie die Leute mit dem Framework umgehen können. Da ich die eingebauten Anti-XSS-Features sowieso nicht nutze, habe ich mich bisher also noch nicht daran gestört.

      Zumindest beim eigentlichen Thema habe ich in dir wohl einen ähnlichen Pragmatiker gefunden ;)

      1. Oliver sagt:

        Dass jetzt das PyroCMS abkackt würde ich allerdings nicht als Gegenargument zu CodeIgniter sehen.

        Die Schnittmenge zwischen Codeigniter und PyroCMS Entwicklern ist recht hoch. Wenn die nicht damit umgehen können, wer dann? Das Problem, das ich sehe ist, dass dort ein Modul als „Sicherheitsfeature“ angeboten wird, dass generell untauglich ist. Die gleichen XSS Attacken kann man nämlich auch gegen andere Codeigniter Seiten fahren. Aber ich mach da nicht den Robin Hood. Wenn es die Entwickler nicht interessiert, warum dann mich?

        Um bei Yii eine SQL Injection zu bauen, muss ich erst alle Sicherheitsfunktionen bewusst ausschalten. Bisher hab ich das noch nicht (unabsichtlich) geschafft. XSS Lücken sind auch nur dann möglich, wenn ich wirklich bewusst gegen Regeln verstoße. Und genau das will ich, ein Framework, dass mich aktiv daran hindert, Bockmist zu machen.

        1. david sagt:

          Ich argumentiere mal frech dagegen und sage „Ich will nicht, dass mir ein Framework ins Handwerk fuscht und selbst entscheiden dürfen, ob und wie ich die Validierung von Userinput vornehme“. Deswegen finde ichs auch gut, dass so kritische Einstellungen default off sind. Woher soll das Framework wissen, ob nicht gewisse Usereingaben in meiner Applikation sinnvoll sind, die zufällig ins XSS-Raster fallen. Magic Quotes geht in die gleiche Richtung. Ich unterstelle dem durchschnittlichen Framework-User so viel Kompetenz, dass er schonmal von htmlspecialchars gehört hat.

          1. Oliver sagt:

            Ich unterstelle dem durchschnittlichen Framework-User so viel Kompetenz, dass er schonmal von htmlspecialchars gehört hat.

            Und ich unterstelle dem durchschnittlichem Framework-User inkl. mir, dass er nicht immer an alles denkt. Wenn ich html brauche, kann ich es ja zurück codieren. Vom Rausfiltern von kritischen Zeichen halte ich nix, denn erstens erfasst man niemals jedes kranke Konstrukt (siehe Pyrocms) und zweitens will ich nicht die Usereingaben verändern. Entweder der Input ist richtig oder nicht. Im Zweiten Fall ist mir ein Fehler allemal lieber als mit „gesicherten“ Eingaben weiter zu arbeiten.

            So lange ich von „normalen Eingaben“ ausgehe, fällt ein Fehlen von htmlspecialchars nicht auf. Das Problem ist nur, dass XSS Angriffe nicht mit normalen Userinput funktioniert. Dann ist es mir lieber alles zu kodieren, und im Zweifel zurück zu kodieren. Das macht Yii auch nicht per default, aber man es anweisen, es zu tun. :-)

  3. Wenn du so ein Zitat geben kannst:

    „[…] dass ich die Resize-Methode alleine nie in diesem Projekt brauchen werde […]“

    dann erinnere dich an YAGNI ( http://de.wikipedia.org/wiki/YAGNI ).
    Soviel zum aktuellen Problem.

    Generell validiere ich alles nochmal, der Nutzen ist doch höher als die (nur sehr leicht) erhöhte Laufzeit der Anwendung. _Ich_ jedenfalls traue keiner Eingabe von aussen mehr über den Weg, auch nicht in der 12ten Tiefenebene einer private-Funktion, die ja „niemals einen unvalidierten Wert erhalten kann“…

    1. david sagt:

      Hi Sascha, danke für deine Antwort. Klar, der gute YAGNI… Mir tat in gegebenem Beispiel halt die offensichtliche Validierungsredudanz weh, weil ich 1:1 die selben Checks vornehme wie in der Funktion obendrüber und ich das „Refactoring“ eher als Alibi für mich selbst empfunden habe, weil man das eben so macht. Nichtsdestotrotz pflichte ich deiner Antwort auch bei, das ist ja genau der angesprochene Zwiespalt ;)

Schreibe einen Kommentar zu Oliver Antworten abbrechen

Deine E-Mail-Adresse wird nicht veröffentlicht. Erforderliche Felder sind mit * markiert