Security Code Review: What Reviewers Should Actually Look For

1. Why security code review belongs in every normal PR review

A security code review run as a separate stage right before deployment arrives structurally too late. When the security question only shows up in its own gate after the regular code review, the context of the change has already faded for the developer, and every follow up question costs a full review cycle. It is far more effective to treat security as a fixed part of the normal pull request review, on equal footing with code quality and test coverage. Reviewers then do not review twice, they simply add a second angle to the same diff.

The reason is economic: a gap caught during PR review costs a comment and a follow up commit. The same gap after the merge costs an incident response process, in the worst case a data breach with mandatory reporting obligations under GDPR. Teams that organize security review as a separate, late stage process step systematically defer the most expensive variant of the fix. The checklist below is therefore built to be applicable in a few minutes per pull request, not as a standalone audit.

2. The core checklist: input, auth, output, dangerous functions

A practical security checklist for code review boils down to four core questions that apply to almost every diff. First: where does every value processed in this code come from, and was it validated before use? Second: does this endpoint check whether the calling user actually holds the required permission? Third: is every output escaped for the correct context before it reaches the browser? Fourth: does the code call a function from the list of known dangerous PHP functions without additional safeguards?

These four questions can be answered directly against the changed lines in the diff view, without knowing the entire codebase. The order matters: input first, because it is the source of nearly every downstream vulnerability. A reviewer who consistently checks these four points on every PR touching user input, database access, or output covers a large share of the OWASP Top 10 categories without needing a separate security course.

3. Input handling: checking validation and sanitizing correctly

The most common mistake in Magento controllers is passing request parameters into business logic or database access without checking them. $this->getRequest()->getParam('id') always returns a string or null, never a guaranteed number, even when the frontend field is numeric. If that value is passed unchecked into a repository, a collection, or a raw query, an attacker can manipulate types, send arrays instead of scalars, or inject SQL fragments. Reviewers should ask, for every getParam call, whether an explicit cast or validation follows it.

The example below shows the difference between a controller that passes parameters through blindly and one that checks type and value range before processing. What matters is that validation ensures not just the type but also the business validity, for example whether an order id actually belongs to the logged in customer. A purely technical type check without an ownership check prevents SQL injection but not 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 and authorization checks: ACL, session, CSRF

Admin controllers in Magento do not check permissions automatically. Every class extending \Magento\Backend\App\Action must set the ADMIN_RESOURCE constant to a specific ACL resource. If this constant is missing, or if the generic resource Magento_Backend::admin is used by mistake, any backend user with minimal rights can call the controller, regardless of which role they were actually assigned. This mistake typically happens through copy pasting an existing controller where the ACL resource was never adjusted for the new module.

Reviewers should check three things on every new or changed admin controller: is ADMIN_RESOURCE set and specific enough? Does a matching entry exist in acl.xml? And is the permission re-checked not only when the page loads, but also for every AJAX action inside the same controller? Magento does call _isAllowed() automatically before execute(), but additional internal forwards or mass actions can bypass that check if they do not run cleanly through the same controller hierarchy.

<?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: preventing XSS in Hyva templates

Hyva templates make the $escaper service available in every phtml file, yet unescaped output still shows up regularly in diffs. The reason is that escaping is context dependent: text between HTML tags needs escapeHtml(), an attribute value needs escapeHtmlAttr(), a URL inside an href needs escapeUrl(), and a value inside an Alpine.js x-data attribute needs escapeJs() in addition to escapeHtmlAttr(). Using the wrong escaper for the context opens a cross site scripting hole even though escaping technically happened.

Dynamic attributes coming from CMS blocks or product attributes are especially easy to overlook, because they are treated as trustworthy simply for being in the backend. A product title maintained through the admin panel is still user input the moment a second editor with restricted rights can edit it. Reviewers should flag every place where <?= $var ?> appears without one of the four escaper methods, and explicitly ask about the output context.

<?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. Spotting dangerous functions and anti patterns

Certain PHP functions are almost never legitimate in Magento code and should trigger an immediate question the moment they show up in a diff: eval(), unserialize() without allowed_classes, extract() on user input, create_function(), and exec(), shell_exec() or system() with interpolated variables. Each of these, combined with insufficiently checked input, can lead to remote code execution or command injection. file_get_contents() or fopen() with a URL sourced from the request is risky too, since it enables server side request forgery against internal systems.

A reviewer does not need to memorize this list if a static analyzer flags these functions automatically. Context still matters: unserialize($data, ['allowed_classes' => false]) is significantly safer than the call without options, and an exec() call with fully hardcoded arguments is uncritical. The blanket rule is: every finding needs an explicit justification in the PR explaining why the function is safe here, not an automatic 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. Blind spots: why reviewers trust internal code too much

The biggest blind spot in security review is assuming that code only ever executed by administrators needs no input validation. That assumption confuses who triggers an action with the separate question of where the processed data originates. An admin user starting a CSV import is trustworthy, but the CSV file itself can come from a compromised supplier source, be forwarded as an email attachment, or intentionally contain formula injection payloads that execute code when opened in Excel.

The same pattern applies to CLI commands, cron jobs, and webhook handlers that appear to sit outside the reach of external attackers. A payment provider's webhook endpoint is often treated as an internal system, yet it is publicly reachable and must verify the signature of every incoming request. Reviewers should explicitly ask, for any code marked admin only or internal, where the processed raw data originally came from, not merely who triggers the process. That distinction accounts for a disproportionate share of overlooked vulnerabilities.

8. Integrating security review into the normal PR workflow

The most durable way to integrate security review into the PR process is a short, fixed checklist embedded directly in the pull request template, not a separate ticket in a different tool. When the four core questions from section two appear as a checkbox list in the PR template, the author already answers them while creating the PR, and the reviewer gets a self declaration they can specifically verify instead of starting from zero. That reduces review time because the obvious questions are already addressed upfront.

This responsibility should rest with every reviewer, not exclusively with a dedicated security team that cannot realistically see every PR anyway. For genuinely complex changes such as custom cryptography, payment processing, or the authentication core, an explicit escalation to a senior colleague before merging is still worthwhile. The checklist filters these cases out automatically if it includes a question like "Does this change touch auth, payments, or cryptography?"

# .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. Manual review versus automated tooling compared

Manual review and automated tooling solve different problems and do not substitute for one another. Static analyzers like PHPStan, Psalm, or a SAST tool like Semgrep reliably find known patterns: dangerous function calls, missing type checks, unescaped output in phtml files. What they cannot recognize is business context: whether an authorization check verifies the right resource, whether an ownership check is missing, or whether an exception marked as safe is actually justified. Those questions require a human review that reads the diff in the context of the business logic.

The most economically sound approach combines both in the right order: automated tooling runs on every push in the CI pipeline and blocks obvious violations before a human even opens the PR. Manual review then focuses exclusively on the questions tools structurally cannot answer, without wasting time on things a machine checks faster and more reliably.

10. Summary

An effective security code review needs no separate review stage and no deep cryptography knowledge, just a fixed four-point checklist applied on every pull request: where does the input come from and was it validated? Does the endpoint check authorization correctly? Is every output escaped for its context? Does the diff contain a known dangerous function without justification? These four questions cover a large share of the real vulnerabilities found in Magento projects, from IDOR to missing ACL resources to XSS in Hyva templates.

The biggest lever is refusing to treat admin only and internal code paths as automatically safe, and always asking where the processed raw data originally came from. Automated tooling like PHPStan, Psalm, or Semgrep reliably takes over the mechanical search for dangerous functions and missing escaping, freeing the human reviewer to focus fully on authorization logic and business context.

11. FAQ: Security Code Review