Was ein Senior Code Review wirklich findet: Ein Praxisbeispiel für Engineering Leader

Engineering Leader erzählen mir oft, dass ihr Team Code Reviews macht. Wenn ich frage, was diese Reviews tatsächlich abdecken, lautet die Antwort meist in irgendeiner Form: "Funktioniert es? Ist die Formatierung in Ordnung? Hält es sich an die Konventionen?"

Das ist kein Senior Code Review. Das ist ein Plausibilitätscheck mit einem Stempel am Ende.

Ein echtes Senior Code Review, also die Art, die Produktionsvorfälle, Sicherheitslücken und schmerzhafte Rewrites sechs Monate später verhindert, sieht völlig anders aus. Es findet Dinge, nach denen Junior- und Mid-Level-Reviewer schlicht nicht suchen, weil man erst dann weiß, worauf man achten muss, wenn man sich einmal daran verbrannt hat.

Dieser Beitrag geht ein echtes Praxisbeispiel durch. Der Code ist ein Komposit aus tatsächlichen Code-Quality-Consulting-Projekten, anonymisiert und zur Klarheit vereinfacht. Die Befunde sind real. Wenn du ein Engineering-Team führst, sollte dir das ein konkretes Bild davon geben, was du bekommst und was dir entgeht, je nachdem, wer deinen Code reviewt.

Das Szenario: Ein Endpoint für die Nutzersuche

Das Feature ist unkompliziert: Eine B2B-SaaS-Anwendung braucht einen Admin-Endpoint, über den Support-Mitarbeitende Nutzer nach Name oder E-Mail suchen können. Der Junior-Entwickler hat ihn geschrieben, ein Mid-Level-Entwickler hat ihn freigegeben, und er ist bereit zum Mergen.

Hier ist die Controller-Methode, leicht vereinfacht:

public function searchUsers(Request $request): JsonResponse
{
    $query = $request->get('q');
    $role = $request->get('role', 'all');

    $sql = "SELECT id, name, email, role, created_at
            FROM users
            WHERE (name LIKE '%" . $query . "%'
            OR email LIKE '%" . $query . "%')";

    if ($role !== 'all') {
        $sql .= " AND role = '" . $role . "'";
    }

    $results = $this->db->executeQuery($sql)->fetchAllAssociative();

    $enriched = [];
    foreach ($results as $user) {
        $enriched[] = array_merge($user, [
            'subscription' => $this->subscriptionService->getForUser($user['id']),
            'lastActivity' => $this->activityService->getLastActivity($user['id']),
        ]);
    }

    return new JsonResponse($enriched);
}

Der Mid-Level-Reviewer hinterließ einen Kommentar: "Schön, sieht gut aus. Vielleicht noch einen Kommentar zur LIKE-Syntax für spätere Entwickler."

Ein Senior Reviewer würde das nicht freigeben. Hier ist, was er tatsächlich sieht.

Befund 1: SQL-Injection, direkt ausnutzbar

Die String-Konkatenation im SQL-Query ist eine lehrbuchmäßige SQL-Injection-Schwachstelle. Jeder Nutzer mit Zugriff auf diesen Endpoint, und denk daran, das ist ein Admin-Panel, der Angreifer muss also nur einen einzigen Support-Account kompromittieren, kann beliebiges SQL einschleusen.

Ein Query-String wie ' OR '1'='1 im Parameter q gibt die komplette Users-Tabelle aus. Eine raffiniertere Injection kann Tabellen löschen, Daten auf einen externen Server exfiltrieren oder Rechte eskalieren. Da auch der Parameter role nicht parametrisiert ist, verdoppelt sich die Angriffsfläche.

Die Lösung sind parametrisierte Queries. Das ist keine Stilfrage. Das ist eine harte Sicherheitsanforderung, und kein Reviewer mit Produktionserfahrung sollte das durchgehen lassen.

$sql = "SELECT id, name, email, role, created_at
        FROM users
        WHERE (name LIKE :search OR email LIKE :search)";

$params = ['search' => '%' . $query . '%'];

if ($role !== 'all') {
    $sql .= " AND role = :role";
    $params['role'] = $role;
}

$results = $this->db->executeQuery($sql, $params)->fetchAllAssociative();

Ein Junior-Reviewer hat SQL-Injection vielleicht noch nie im Feld ausgenutzt gesehen. Ein Senior Reviewer schon, oder er hat hinterher aufgeräumt. Die Kosten eines einzigen Vorfalls, also Meldepflichten, DSGVO-Bußgelder, Kundenabwanderung, Engineering-Zeit, übersteigen die Kosten einer Korrektur jetzt bei Weitem.

Befund 2: Ein N+1-Query, das erst irgendwann wehtut

Die Schleife, die jedes Suchergebnis anreichert, feuert pro Zeile zwei zusätzliche Datenbank-Queries ab, eines für die Abo-Daten, eines für die letzte Aktivität. Liefert die Suche 10 Nutzer, sind das 21 Datenbank-Queries für einen einzigen Request. Bei 100 Nutzern sind es 201.

Aktuell, mit kleinem Datenbestand und wenig Traffic, ist das unsichtbar. Der Endpoint antwortet in 80 ms und niemand merkt etwas. In sechs Monaten, wenn der Kundenstamm gewachsen ist und ein Support-Mitarbeiter eine breite Suche mit 400 Treffern startet, hängt der Endpoint, der Connection Pool der Datenbank läuft voll, und andere Teile der Anwendung fangen an, in Timeouts zu laufen.

Der Senior Reviewer markiert das nicht, weil es heute kaputt ist, sondern weil es kaputt gehen wird. Und es im Nachhinein zu reparieren bedeutet eine Datenbankmigration, eine Cache-Invalidierungsstrategie und wahrscheinlich eine Woche Debugging eines Produktionsvorfalls um zwei Uhr nachts.

Die Lösung: alle Nutzer-IDs aus dem ersten Query sammeln, Abos und Aktivitätsdatensätze in zwei Bulk-Queries laden und im Speicher zusammenführen:

$userIds = array_column($results, 'id');

$subscriptions = $this->subscriptionService->getForUsers($userIds);
$activities = $this->activityService->getLastActivities($userIds);

$enriched = array_map(function ($user) use ($subscriptions, $activities) {
    return array_merge($user, [
        'subscription' => $subscriptions[$user['id']] ?? null,
        'lastActivity' => $activities[$user['id']] ?? null,
    ]);
}, $results);

Drei Queries, unabhängig von der Ergebnisgröße. Das ist ein Muster, das jeder Senior-Entwickler intuitiv kennt, weil er genug N+1-Probleme debuggt hat, um die Form eines solchen Problems zu erkennen, bevor es wehtut.

Befund 3: Kein Ergebnis-Limit, keine Pagination

Das Query hat keine LIMIT-Klausel. Sucht jemand nach einem leeren String oder nach einem einzelnen häufigen Buchstaben wie "a", versucht das Query, jeden Nutzer der Datenbank in einer einzigen Response zurückzugeben.

Bei 10.000 Nutzern wird das langsam. Bei 100.000 Nutzern läuft es in einen Timeout oder stürzt ab. In jeder Größenordnung ist es unnötige Last für Datenbank, Applikationsserver und Client, Zehntausende angereicherter Nutzerobjekte in eine JSON-Response zu serialisieren und über die Leitung zu schicken.

Die Lösung ist verpflichtende Pagination mit sinnvollem Default und hartem Deckel:

$page = max(1, (int) $request->get('page', 1));
$perPage = min(50, max(1, (int) $request->get('per_page', 20)));
$offset = ($page - 1) * $perPage;

$sql .= " LIMIT :limit OFFSET :offset";
$params['limit'] = $perPage;
$params['offset'] = $offset;

Ein Mid-Level-Reviewer würde Pagination ergänzen, wenn das Ticket es ausdrücklich verlangt. Ein Senior Reviewer ergänzt sie, weil er versteht, dass unbegrenzte Queries ein Denial-of-Service-Vektor und eine Performance-Zeitbombe sind, auch wenn das Ticket nichts davon erwähnt.

Befund 4: Fehlende Authentifizierungs- und Autorisierungsprüfungen

Die Controller-Methode enthält keine explizite Autorisierungsprüfung. Sie verlässt sich vollständig auf die Middleware oder Firewall, die auf Router-Ebene konfiguriert ist.

Das ist ein Coupling-Problem, das gleichzeitig ein Sicherheitsproblem ist. Wenn ein späteres Refactoring diesen Endpoint verschiebt, die Routing-Konfiguration ändert oder einen neuen Authentifizierungs-Bypass für Tests einbaut, wird der Endpoint stillschweigend ohne korrekte Autorisierung erreichbar. Defense in Depth bedeutet, Autorisierung auch auf Controller-Ebene zu prüfen, statt darauf zu vertrauen, dass der Router dich immer schützt.

Ein Senior Reviewer fragt: Was passiert, wenn diese Route versehentlich ohne die ROLE_ADMIN-Firewall registriert wird? Die Antwort sollte nicht lauten "Nutzer bekommen vollen Zugriff auf den Such-Endpoint". Sie sollte eine Berechtigungsprüfung in der Methode selbst sein:

if (!$this->security->isGranted('ROLE_ADMIN')) {
    throw new AccessDeniedException();
}

Eine Zeile. Null Performance-Kosten. Eliminiert eine ganze Klasse von Privilege-Escalation-Bugs.

Befund 5: Enges Coupling an Infrastruktur-Services

Der Controller greift direkt auf $this->subscriptionService und $this->activityService zu. Beide machen vermutlich Datenbankaufrufe oder externe API-Calls. Keiner ist in Fehlerbehandlung eingebettet.

Wirft der Subscription-Service eine Exception, sei es wegen eines Datenbank-Timeouts, eines API-Rate-Limits oder eines transienten Netzwerkfehlers, scheitert der gesamte Such-Request mit einem 500er. Der Nutzer bekommt keine Ergebnisse. Der Support kann seine Arbeit nicht machen.

Ein Senior Reviewer denkt über Teilausfälle nach. Die Nutzersuche selbst hat funktioniert. Die Anreicherung ist gescheitert. Das sind trennbare Belange. Eine robuste Implementierung fängt Anreicherungsfehler pro Nutzer ab und liefert Teildaten statt gar nichts:

try {
    $subscription = $this->subscriptionService->getForUser($user['id']);
} catch (\Exception $e) {
    $this->logger->warning('Failed to load subscription for user', [
        'user_id' => $user['id'],
        'error' => $e->getMessage(),
    ]);
    $subscription = null;
}

Das ist kein Over-Engineering. Das ist der Unterschied zwischen einer eingeschränkten und einer kaputten Nutzererfahrung, wenn ein nachgelagerter Service einen schlechten Tag hat.

Was das Mid-Level-Review übersehen hat

Um es klar zu sagen: Der Mid-Level-Reviewer ist nicht inkompetent. Er hat gefunden, wonach er gesucht hat: logische Korrektheit, Namenskonventionen, Code-Stil. Der Endpoint tut, was das Ticket beschrieben hat.

Was er nicht gefunden hat: eine direkt ausnutzbare Sicherheitslücke, ein Query-Muster, das in der Produktion Performance-Probleme verursachen wird, ein unbegrenztes Query als Denial-of-Service-Risiko, eine fehlende Autorisierungsabsicherung und brüchiges Coupling an nachgelagerte Services.

Nichts davon taucht in Unit-Tests auf, es sei denn, man schreibt gezielt Tests dafür. Nichts davon verursacht sichtbare Fehler in Entwicklungs- oder Staging-Umgebungen. Es sind genau die Probleme, die in der Produktion auftauchen, unter Last, zu ungünstigen Zeiten.

Was das für Engineering Leader bedeutet

Wenn der Code-Review-Prozess deines Teams lautet "funktioniert es, sieht es okay aus, approved", sammelst du mit jedem Merge Risiko an. Die Bugs, die Ausfälle und Sicherheitsvorfälle verursachen, sind fast nie Logikfehler. Es sind die strukturellen Probleme, die daraus entstehen, dass man nicht weiß, wonach man suchen muss.

Einen Senior Reviewer hinzuzuziehen, sei es durch Einstellung, ein anteiliges Arrangement oder Code-Quality-Consulting für regelmäßige Audits, ersetzt dein Team nicht. Es schärft dessen Blick. Es findet die Probleme, die erst für jemanden sichtbar werden, der ausgeliefert, debuggt und gelegentlich Rufbereitschaft für die Konsequenzen gehabt hat.

Die fünf Befunde in diesem Praxisbeispiel brauchten in einem echten Review etwa 15 Minuten. Die Kosten, sie vor dem Merge zu beheben: ein paar Stunden konzentrierte Arbeit. Die Kosten, sie nach einem Produktionsvorfall zu beheben: irgendwo zwischen einem Wochenende und einem Monat, plus Reputationsschaden und ein Gespräch mit der Aufsichtsbehörde, das du nie führen wolltest.

Wenn dich interessiert, was ein solches Review in deiner Codebasis zutage fördern würde, melde dich unter hello@wolf-tech.io oder besuche wolf-tech.io. Das erste Gespräch ist immer kostenlos.