Security-Code-Review: Worauf Reviewer wirklich achten sollten

1. Warum Security-Code-Review Teil jedes normalen PR-Reviews sein muss

Ein Security-Code-Review als separate Prüfstufe kurz vor dem Deployment kommt strukturell zu spät. Taucht die Sicherheitsfrage erst in einem eigenen Gate nach dem regulären Code-Review auf, ist der Kontext der Änderung beim Entwickler bereits verblasst, und jede Rückfrage kostet einen vollständigen Zyklus. Effektiver ist es, Sicherheit als festen Bestandteil des normalen Pull-Request-Reviews zu behandeln, gleichrangig mit Codequalität und Testabdeckung. Reviewer prüfen dann nicht zusätzlich, sondern mit einer zweiten Blickrichtung auf denselben Diff.

Der Grund ist ökonomisch: Eine im PR-Review erkannte Lücke kostet einen Kommentar und einen erneuten Commit. Dieselbe Lücke nach dem Merge kostet Incident-Response, im schlimmsten Fall einen Datenabfluss mit Meldepflicht nach DSGVO. Teams mit einem spät angesetzten, separaten Security-Gate verschieben die teuerste Variante der Fehlerkorrektur systematisch nach hinten. Die folgende Checkliste ist deshalb so aufgebaut, dass sie in wenigen Minuten pro Pull-Request anwendbar ist, nicht als eigenständiges Audit.

2. Die Kern-Checkliste: Input, Auth, Output, gefährliche Funktionen

Eine praxistaugliche Security-Checkliste für Code-Reviews reduziert sich auf vier Kernfragen, die sich auf fast jeden Diff anwenden lassen. Erstens: Woher kommt jeder Wert, der in diesem Code verarbeitet wird, und wurde er validiert, bevor er verwendet wird? Zweitens: Prüft dieser Endpunkt, ob der aufrufende Benutzer die nötige Berechtigung hat? Drittens: Wird jede Ausgabe in den passenden Kontext escaped, bevor sie im Browser landet? Viertens: Verwendet der Code eine Funktion aus der Liste bekannt gefährlicher PHP-Funktionen ohne zusätzliche Absicherung?

Diese vier Fragen lassen sich in der Diff-Ansicht direkt an den geänderten Zeilen beantworten, ohne die gesamte Codebasis zu kennen. Wichtig ist die Reihenfolge: Input zuerst, weil er die Quelle jeder nachgelagerten Lücke ist. Ein Reviewer, der diese vier Punkte konsequent an jedem PR mit Nutzereingabe, Datenbankzugriff oder Ausgabe prüft, deckt einen Großteil der OWASP-Top-10-Kategorien ab, ohne eine separate Sicherheitsschulung zu benötigen.

3. Input-Handling: Validierung und Sanitizing korrekt prüfen

Der häufigste Fehler in Magento-Controllern ist ungeprüfte Übernahme von Request-Parametern in Geschäftslogik oder Datenbankzugriffe. $this->getRequest()->getParam('id') liefert immer einen String oder null, niemals garantiert eine Zahl, auch wenn das Feld im Frontend ein Zahlenfeld ist. Wird dieser Wert ungeprüft an ein Repository, eine Collection oder eine Raw-Query weitergereicht, kann ein Angreifer Typen manipulieren, Arrays statt Skalare senden oder SQL-Fragmente einschleusen. Reviewer sollten bei jedem getParam-Aufruf fragen, ob ein expliziter Cast oder eine Validierung folgt.

Das folgende Beispiel zeigt den Unterschied zwischen einem Controller, der Parameter blind weiterreicht, und einem, der Typ und Wertebereich vor der Verarbeitung prüft. Entscheidend ist, dass Validierung nicht nur den Typ, sondern auch die fachliche Gültigkeit sicherstellt, etwa ob eine Bestell-ID tatsächlich zum eingeloggten Kunden gehört. Eine rein technische Typprüfung ohne fachliche Eigentümerprüfung verhindert SQL-Injection, aber keine Insecure Direct Object References.

<?php
declare(strict_types=1);

namespace Mironsoft\Customer\Controller\Account;

use Magento\Framework\App\Action\HttpGetActionInterface;
use Magento\Framework\App\RequestInterface;
use Magento\Framework\Controller\Result\JsonFactory;
use Magento\Customer\Model\Session as CustomerSession;

/**
 * WRONG: parameter is passed straight into the repository without any
 * type check or ownership check. Any logged-in customer can read any
 * other customer's order by simply changing the "id" query parameter.
 */
final class OrderUnsafe implements HttpGetActionInterface
{
    public function __construct(
        private readonly RequestInterface $request,
        private readonly \Magento\Sales\Api\OrderRepositoryInterface $orderRepository,
        private readonly JsonFactory $jsonFactory
    ) {
    }

    public function execute(): \Magento\Framework\Controller\Result\Json
    {
        // UNSAFE: raw param, no type cast, no ownership check
        $orderId = $this->request->getParam('id');
        $order = $this->orderRepository->get($orderId);

        return $this->jsonFactory->create()->setData($order->getData());
    }
}

/**
 * RIGHT: id is cast to int, validated against zero/negative values,
 * and the order's customer_id is compared against the current session.
 */
final class OrderSafe implements HttpGetActionInterface
{
    public function __construct(
        private readonly RequestInterface $request,
        private readonly \Magento\Sales\Api\OrderRepositoryInterface $orderRepository,
        private readonly CustomerSession $customerSession,
        private readonly JsonFactory $jsonFactory
    ) {
    }

    public function execute(): \Magento\Framework\Controller\Result\Json
    {
        $orderId = (int) $this->request->getParam('id', 0);
        $resultJson = $this->jsonFactory->create();

        if ($orderId <= 0) {
            return $resultJson->setHttpResponseCode(400)->setData(['error' => 'Invalid order id']);
        }

        $order = $this->orderRepository->get($orderId);

        // Ownership check: prevent Insecure Direct Object Reference (IDOR)
        if ((int) $order->getCustomerId() !== (int) $this->customerSession->getCustomerId()) {
            return $resultJson->setHttpResponseCode(403)->setData(['error' => 'Access denied']);
        }

        return $resultJson->setData($order->getData());
    }
}

4. Auth- und Berechtigungsprüfungen: ACL, Session, CSRF

Admin-Controller in Magento prüfen Berechtigungen nicht automatisch. Jede von \Magento\Backend\App\Action abgeleitete Klasse muss die Konstante ADMIN_RESOURCE auf eine spezifische ACL-Ressource setzen. Fehlt diese Konstante oder wird versehentlich die generische Ressource Magento_Backend::admin übernommen, kann jeder Backend-Benutzer mit minimalen Rechten den Controller aufrufen, unabhängig davon, welche Rolle ihm eigentlich zugewiesen wurde. Dieser Fehler entsteht typischerweise durch Copy-Paste eines bestehenden Controllers, bei dem die ACL-Ressource nicht an das neue Modul angepasst wird.

Reviewer sollten bei jedem neuen oder geänderten Admin-Controller drei Dinge prüfen: Ist ADMIN_RESOURCE gesetzt und spezifisch genug? Existiert der passende Eintrag in acl.xml? Und wird die Berechtigung nicht nur beim Aufruf der Seite, sondern auch bei jeder AJAX-Aktion innerhalb desselben Controllers geprüft? Magento ruft _isAllowed() zwar automatisch vor execute() auf, aber zusätzliche interne Weiterleitungen oder Massenaktionen können diese Prüfung umgehen, wenn sie nicht sauber durch dieselbe Controller-Hierarchie laufen.

<?php
declare(strict_types=1);

namespace Mironsoft\SeoSuite\Controller\Adminhtml\Redirect;

use Magento\Backend\App\Action;
use Magento\Framework\Controller\ResultFactory;

/**
 * WRONG: no ADMIN_RESOURCE override. Falls back to the generic
 * Magento_Backend::admin resource, which almost every backend user
 * holds. Any admin user, regardless of assigned role, can delete
 * redirects even without the "Mironsoft_SeoSuite::redirect" permission.
 */
final class MassDeleteUnsafe extends Action
{
    // Missing: const ADMIN_RESOURCE = 'Mironsoft_SeoSuite::redirect_delete';

    public function execute(): \Magento\Framework\Controller\ResultInterface
    {
        $ids = (array) $this->getRequest()->getParam('selected', []);
        foreach ($ids as $id) {
            $this->redirectRepository->deleteById((int) $id);
        }

        $resultRedirect = $this->resultFactory->create(ResultFactory::TYPE_REDIRECT);
        return $resultRedirect->setPath('*/*/');
    }
}

/**
 * RIGHT: explicit, narrow ACL resource. Magento checks this
 * automatically via _isAllowed() before execute() runs.
 */
final class MassDeleteSafe extends Action
{
    public const ADMIN_RESOURCE = 'Mironsoft_SeoSuite::redirect_delete';

    public function execute(): \Magento\Framework\Controller\ResultInterface
    {
        $ids = (array) $this->getRequest()->getParam('selected', []);
        foreach ($ids as $id) {
            $this->redirectRepository->deleteById((int) $id);
        }

        $resultRedirect = $this->resultFactory->create(ResultFactory::TYPE_REDIRECT);
        return $resultRedirect->setPath('*/*/');
    }
}

5. Output-Encoding: XSS in Hyvä-Templates verhindern

Hyvä-Templates stellen den $escaper-Service in jeder phtml-Datei bereit, trotzdem landet unescapter Output regelmäßig im Diff. Der Grund ist, dass Escaping kontextabhängig ist: Text zwischen HTML-Tags braucht escapeHtml(), ein Attributwert braucht escapeHtmlAttr(), eine URL in einem href braucht escapeUrl(), und ein Wert innerhalb eines Alpine.js x-data-Attributs braucht escapeJs() zusätzlich zu escapeHtmlAttr(). Wer den falschen Escaper für den Kontext verwendet, öffnet trotz vorhandenem Escaping eine Cross-Site-Scripting-Lücke.

Besonders leicht übersehen werden dynamische Attribute, die aus CMS-Blöcken oder Produktattributen stammen, weil sie im Backend als vertrauenswürdig gelten. Ein Produkttitel, der über den Admin gepflegt wird, ist trotzdem Nutzereingabe, sobald ein zweiter Redakteur mit eingeschränkten Rechten ihn bearbeiten kann. Reviewer sollten jede Stelle markieren, an der <?= $var ?> ohne einen der vier Escaper-Methoden auftaucht, und explizit nach dem Ausgabekontext fragen.

<?php
/** @var \Magento\Framework\View\Element\Template $block */
/** @var \Magento\Framework\Escaper $escaper */
?>

<!-- WRONG: raw output, no escaping at all -->
<div class="product-badge">
    <?= $block->getProductLabel() ?>
</div>
<a href="<?= $block->getReviewUrl() ?>">Reviews</a>

<!-- WRONG: escapeHtml used for an attribute context (wrong escaper) -->
<img src="<?= $escaper->escapeHtml($block->getImageUrl()) ?>" alt="Product">

<!-- RIGHT: context-specific escaping for each output location -->
<div class="product-badge">
    <?= $escaper->escapeHtml($block->getProductLabel()) ?>
</div>
<a href="<?= $escaper->escapeUrl($block->getReviewUrl()) ?>">Reviews</a>
<img src="<?= $escaper->escapeHtmlAttr($block->getImageUrl()) ?>" alt="Product">

<!-- RIGHT: value injected into Alpine.js x-data needs escapeJs + escapeHtmlAttr -->
<div x-data="{ price: <?= /* @noEscape */ $escaper->escapeJs($block->getProductPriceJson()) ?> }">
    <span x-text="price"></span>
</div>

6. Gefährliche Funktionen und Anti-Patterns erkennen

Bestimmte PHP-Funktionen sind in Magento-Code fast nie legitim und sollten bei jedem Fund im Diff eine sofortige Rückfrage auslösen: eval(), unserialize() ohne allowed_classes, extract() auf Nutzereingaben, create_function(), sowie exec(), shell_exec() und system() mit interpolierten Variablen. Jede dieser Funktionen kann, kombiniert mit unzureichend geprüftem Input, zu Remote Code Execution oder Command Injection führen. Auch file_get_contents() oder fopen() mit einer aus dem Request stammenden URL ist riskant, weil sie Server-Side Request Forgery gegen interne Systeme ermöglicht.

Ein Reviewer muss diese Funktionen nicht auswendig kennen, wenn ein statischer Analyzer sie automatisch markiert. Wichtig ist trotzdem der Kontext: unserialize($data, ['allowed_classes' => false]) ist deutlich sicherer als der Aufruf ohne Optionen, und ein exec()-Aufruf mit vollständig hartcodierten Argumenten ist unkritisch. Die pauschale Regel lautet: jeder Fund braucht eine explizite Begründung im PR, warum die Funktion hier sicher ist, nicht ein automatisches Veto.

{
  "forbiddenFunctions": {
    "description": "Static analysis rule set: flag dangerous PHP functions in app/code",
    "severity": "error",
    "rules": [
      { "function": "eval", "reason": "Arbitrary code execution", "allowException": false },
      { "function": "unserialize", "reason": "Object injection / RCE via magic methods", "allowException": true, "exceptionRequires": "allowed_classes option set explicitly" },
      { "function": "extract", "reason": "Variable overwrite from untrusted array", "allowException": false },
      { "function": "create_function", "reason": "Deprecated eval() wrapper", "allowException": false },
      { "function": "exec", "reason": "Command injection", "allowException": true, "exceptionRequires": "all arguments hardcoded, no user input" },
      { "function": "shell_exec", "reason": "Command injection", "allowException": false },
      { "function": "system", "reason": "Command injection", "allowException": false },
      { "function": "assert", "reason": "String argument executed as code in PHP < 8", "allowException": false }
    ],
    "excludePaths": [
      "vendor/*",
      "dev/tests/*"
    ]
  }
}

7. Blinde Flecken: Warum Reviewer internen Code zu sehr vertrauen

Der größte blinde Fleck im Security-Review ist die Annahme, dass Code, der nur von Administratoren ausgeführt wird, keine Eingabevalidierung braucht. Diese Annahme verwechselt die Frage, wer eine Aktion auslöst, mit der Frage, woher die verarbeiteten Daten stammen. Ein Admin-Benutzer, der einen CSV-Import startet, ist vertrauenswürdig, aber die CSV-Datei selbst kann aus einer kompromittierten Lieferanten-Quelle stammen, per E-Mail-Anhang weitergereicht worden sein oder absichtlich präparierte Formel-Injektionen enthalten, die beim Öffnen in Excel Code ausführen.

Dasselbe Muster gilt für CLI-Kommandos, Cron-Jobs und Webhook-Handler, die scheinbar außerhalb der Reichweite externer Angreifer laufen. Ein Webhook-Endpunkt eines Zahlungsanbieters wird oft als internes System behandelt, ist aber öffentlich erreichbar und muss die Signatur jeder eingehenden Anfrage prüfen. Reviewer sollten bei jedem als admin-only oder intern markierten Code explizit fragen, woher die verarbeiteten Rohdaten ursprünglich stammen, nicht nur, wer den Prozess auslöst. Diese Unterscheidung deckt einen überproportionalen Anteil übersehener Lücken auf.

8. Security-Review in den normalen PR-Workflow integrieren

Die nachhaltigste Integration von Security-Review in den PR-Prozess ist eine kurze, feste Checkliste direkt in der Pull-Request-Vorlage, nicht ein separates Ticket in einem anderen Tool. Wenn die vier Kernfragen aus Abschnitt 2 als Checkbox-Liste im PR-Template stehen, beantwortet sie der Autor bereits beim Erstellen des PRs, und der Reviewer bekommt eine Selbstauskunft, die er gezielt gegenprüfen kann, statt bei null anzufangen. Das reduziert die Reviewzeit, weil offensichtliche Fragen schon vorab geklärt sind.

Diese Verantwortung sollte bei jedem Reviewer liegen, nicht ausschließlich bei einem dedizierten Security-Team, das ohnehin nicht jeden PR sehen kann. Für wirklich komplexe Änderungen wie eigene Kryptografie, Zahlungsabwicklung oder Authentifizierungskern lohnt sich trotzdem eine explizite Eskalation an einen erfahrenen Kollegen, bevor gemerged wird. Die Checkliste filtert diese Fälle automatisch heraus, wenn sie eine Frage wie "Betrifft diese Änderung Auth, Zahlungen oder Kryptografie?" enthält.

# .github/pull_request_template.yml (excerpt)
# Security section embedded directly in the standard PR checklist
security_review_checklist:
  input_handling:
    - "Every request parameter is type-cast or validated before use"
    - "Ownership of referenced entities is checked (no IDOR)"
    - "File uploads are restricted by extension and MIME type"
  authorization:
    - "New admin controllers define a specific ADMIN_RESOURCE"
    - "acl.xml contains a matching, narrowly scoped entry"
    - "AJAX/mass-action endpoints re-check permissions, not just the main action"
  output_encoding:
    - "Every dynamic phtml output uses the escaper matching its context"
    - "No raw variable interpolation into inline script blocks"
  dangerous_functions:
    - "No eval, extract, create_function, or unserialize without allowed_classes"
    - "exec/shell_exec/system calls use hardcoded arguments only"
  escalation:
    - question: "Does this change touch auth, payments, or cryptography?"
      if_yes: "Request review from a senior engineer before merging"

9. Manuelles Review und automatisiertes Tooling im Vergleich

Manuelles Review und automatisiertes Tooling lösen unterschiedliche Probleme und ersetzen sich nicht gegenseitig. Statische Analyzer wie PHPStan, Psalm oder ein SAST-Tool wie Semgrep finden zuverlässig bekannte Muster: gefährliche Funktionsaufrufe, fehlende Typprüfungen, unescaped Output in phtml-Dateien. Was sie nicht erkennen, ist fachlicher Kontext: ob eine Berechtigungsprüfung die richtige Ressource prüft, ob eine Eigentümerprüfung fehlt, oder ob eine als sicher markierte Ausnahme tatsächlich gerechtfertigt ist. Diese Fragen brauchen ein menschliches Review, das den Diff im Kontext der Geschäftslogik liest.

Der wirtschaftlich sinnvollste Ansatz kombiniert beides in der richtigen Reihenfolge: automatisiertes Tooling läuft bei jedem Push in der CI-Pipeline und blockiert offensichtliche Verstöße, bevor überhaupt ein Mensch den PR öffnet. Das manuelle Review konzentriert sich dann ausschließlich auf die Fragen, die Werkzeuge strukturell nicht beantworten können, und verschwendet keine Zeit mit Dingen, die eine Maschine schneller und zuverlässiger prüft.

10. Zusammenfassung

Ein wirksames Security-Code-Review braucht keine separate Prüfstufe und kein tiefes Kryptografie-Wissen, sondern eine feste Vier-Punkte-Checkliste, die bei jedem Pull-Request angewendet wird: Woher kommt der Input und wurde er validiert? Prüft der Endpunkt die Berechtigung korrekt? Wird jede Ausgabe kontextgerecht escaped? Enthält der Diff eine bekannt gefährliche Funktion ohne Begründung? Diese vier Fragen decken einen Großteil der realen Schwachstellen in Magento-Projekten ab, von IDOR über fehlende ACL-Ressourcen bis zu XSS in Hyvä-Templates.

Der größte Hebel liegt darin, Admin-only und interne Code-Pfade nicht automatisch als sicher zu behandeln, sondern immer zu fragen, woher die verarbeiteten Rohdaten ursprünglich stammen. Automatisiertes Tooling wie PHPStan, Psalm oder Semgrep übernimmt die mechanische Suche nach gefährlichen Funktionen und fehlendem Escaping zuverlässig und entlastet den menschlichen Reviewer, der sich dadurch voll auf Berechtigungslogik und fachlichen Kontext konzentrieren kann.

11. FAQ: Security-Code-Review