DEV Community

Cover image for How a Five Line Architecture Test Caught a Data Leak a Code Review Missed
Sheikh Shahzaman
Sheikh Shahzaman

Posted on

How a Five Line Architecture Test Caught a Data Leak a Code Review Missed

TL;DR: Pest PHP can test the structure of your code, not just its behavior. Write your team rules as architecture tests and CI enforces them on every commit. One such test caught a multi-tenant data leak that a human review had missed.


We had a rule. Every model holding tenant-specific data must use our BelongsToTenant trait. That trait adds the global scope that keeps one clinic from seeing another clinic's data.

The rule was in onboarding. It was in the code review checklist. Everyone knew it.

A developer joined the team. Three weeks in they added a new model and forgot the trait. The reviewer was focused on the business logic, which was genuinely well written, and did not notice the missing trait. The model shipped.

For two days one clinic could see fragments of another clinic's data in one specific report. A support ticket caught it. Our tests did not.

That was the day architecture tests went into the project.

What an Architecture Test Is

Most tests check behavior. Given this input the function returns that output. An architecture test checks structure instead. It asserts things about how the code is organized rather than what it computes.

Pest has an arch function for exactly this.

// tests/Architecture/ArchTest.php

arch('tenant models must use the BelongsToTenant trait')
    ->expect('App\Models')
    ->toUseTrait('App\Traits\BelongsToTenant')
    ->ignoring('App\Models\SystemSetting');

arch('controllers may not touch the DB facade directly')
    ->expect('App\Http\Controllers')
    ->not->toUse('Illuminate\Support\Facades\DB');

arch('services may not depend on the HTTP request')
    ->expect('App\Services')
    ->not->toUse('Illuminate\Http\Request');

arch('no env calls outside config files')
    ->expect('App')
    ->not->toUse('env');
Enter fullscreen mode Exit fullscreen mode

These run in CI on every commit. Break a rule and the build fails with a message naming the rule and the file that broke it.

The Tests That Earned Their Keep

The tenant trait test caught four more models over the following months that were missing the trait. Each one was a potential data leak. That one test paid for the entire effort by itself.

The controller test catches the "I will just add a quick query here" move. It happens more than people admit, usually under deadline pressure.

// This fails the architecture test
class ReportController extends Controller
{
    public function index()
    {
        $total = DB::table('orders')->sum('total');

        return response()->json($total);
    }
}
Enter fullscreen mode Exit fullscreen mode

The env test is one I wish we had added on day one. Calling env directly outside config files works in development and returns null in production once the config is cached. It is a classic Laravel trap. The test makes it impossible to fall into again.

Introducing This to an Existing Codebase

If you add strict architecture tests to a large old codebase you will get hundreds of failures at once. The team will resent it and the effort stalls. Here is the rollout that actually worked for us.

Start with one rule. The single most important one. For us that was the tenant trait. Fix its handful of violations and merge just that.

Add one rule per week after that. Slow enough that the team can absorb each one and fix the existing violations it surfaces.

For a rule with many legacy violations use the ignoring method to exclude the old code.

arch('controllers may not use the DB facade')
    ->expect('App\Http\Controllers')
    ->not->toUse('Illuminate\Support\Facades\DB')
    ->ignoring('App\Http\Controllers\Legacy');
Enter fullscreen mode Exit fullscreen mode

This enforces the rule for all new code while you refactor the old code on your own schedule. Then track the count of ignoring exceptions. That number should fall every sprint. If it rises, new code is being written into the legacy exception and you have a problem.

The Mistake I Made

I tried to add ten rules in one pull request. The build lit up with violations across the whole codebase. It was overwhelming and the team pushed back hard. I almost abandoned the idea.

Adding rules one at a time is not a nice-to-have. It is the only way this works on a real project. A wall of red failures makes people want to delete the tests. A single new failing rule makes people want to fix one thing.

What I Would Do Differently

I would add architecture tests on the first commit of every new project. Adding them to an existing codebase is all catch-up work. Starting fresh means the rules are enforced from the beginning and there is never a backlog of violations.

I would also build a small shared package of the common tests. We wrote nearly identical architecture tests across three projects. A reusable package would have saved that duplication.

My Honest Take

Architecture tests are not about being strict for its own sake. They are about making the codebase predictable.

When every tenant model provably has the trait, isolation is not a hope. It is a fact the build verifies. When no controller can touch the database, the service layer is genuinely the single path. These guarantees hold even as the team changes and the codebase grows, because the machine checks them on every commit and a human never has to remember.

Your team rules belong in your test suite. A rule in a wiki is a suggestion. A rule in CI is real.

What rule would you turn into a test first if you could?

Top comments (0)