Kodus Caught Two Authorization Bugs in a Report Handoff

Two checks before signing the report token

A reporting change gave me a more specific example of the review work. An internal application was being updated to issue short-lived signed tokens for a separate reporting service. Kodus found two ways it could issue a valid token without the appropriate authorization.

The excerpts below use generalized identifiers and shortened messages. They show the checks involved, not a complete authentication or token-signing implementation. The review record shows fixes for both findings; I am not presenting this as an independent penetration test.

The method name promised more than the check did

The original code checked a report object before signing the handoff:

if (!is_object($report) || !$report->userCanView()) {
    http_response_code(403);
    exit('Access denied.');
}

A comment assumed that only reports granted to the user could reach that point. But the request handler could construct the report named in the request, and userCanView() only checked that the report was not deprecated. Neither established that the signed-in user was allowed to open it.

The fix checked the user’s explicit directory-backed report grants before issuing a token:

$identity = $_SESSION['identity'] ?? null;
$grantedReports = is_object($identity)
    ? array_map('strtolower', (array) $identity->getReportGrants())
    : [];

if (
    !in_array(strtolower($reportName), $grantedReports, true)
    || !is_object($report)
    || !$report->userCanView()
) {
    http_response_code(403);
    exit('Access denied.');
}

This is why I want a reviewer to follow a permission check into its implementation. A reassuring method name and a comment about earlier validation are not evidence that authorization happened.

The permissions and the token could refer to different environments

A later revision added environment information to the token. Its target came from the current session, but the user’s cached grants still came from the environment used at login.

An older request-handling path in another file could change that session target without refreshing the identity or its grants. As a result, the new report-permission check could pass for one environment while the application signed a token for another.

The implemented fix refused the handoff when the selected environment no longer matched the login environment:

function reportEnvironment($databaseTarget): string
{
    return $databaseTarget === DEVELOPMENT_DATABASE
        ? 'development'
        : 'production';
}

$loginTarget = $_SESSION['identity']->getLoginDatabase();

if (
    reportEnvironment($_SESSION['databaseTarget'])
    !== reportEnvironment($loginTarget)
) {
    http_response_code(403);
    exit('The environment changed. Sign in again.');
}

This excerpt preserves the two-environment classification used by the supplied fix: every target other than the recognized development database is classified as production. It is not an allowlist of valid targets or a check that two database hosts are identical. A system with more environments or independent targets needs those distinctions represented explicitly.

Kodus connected the new token code to the older session-changing path elsewhere in the application. An earlier revision had received human approval; the second issue appeared in a subsequent revision. That chronology matters. It does not show that a human approved the exact vulnerable revision, or establish which reviewer would have caught it next.

In both findings, the application could produce a correctly signed token. The missing work was deciding whether it should issue that token for this user, report, and environment.

Turn the findings into tests

I would keep these cases in the regression suite. They are proposed checks, not results I am claiming to have run:

• A signed-in user without the requested report grant receives a denial and no token.
• A user granted the report can receive a token when the authenticated and selected environments agree.
• Changing the selected environment after login prevents token issuance, even when the cached report grant passes.
• A deprecated report remains denied after adding the explicit grant check.

The environment test should exercise the request path that changes session state as well as the handoff. A unit test of the comparison alone would miss the interaction that caused the bug.

Run a small reproduction locally

This standalone PHP example models the original check, the added report-grant check, and the environment check. It runs without dependencies on PHP 8.0 or later. Save the linked text file, inspect it, then run it from the directory where you saved it:

php report-handoff-regression.php.txt

The example passed 18 checks on PHP 8.2.33. I reran the same example on October 10 with PHP 8.2.34; all 18 checks passed again. It first reproduces issuance without a report grant, then changes the selected target while retaining the login grants to reproduce the second flaw. The fixed paths deny those requests and record no issuance. Allowed requests, deprecated reports, and switching in the opposite direction are covered too.

Two checks document a limitation rather than a safe outcome: the supplied two-environment classification treats unknown targets and other production targets as production. Passing this example does not make that classification appropriate for another system.

No real tokens are signed, and nothing connects to a database, directory, or private application. These are tests of a synthetic model, not proof of the production fix. To turn them into application regression tests, drive the real login and target-changing request paths, replace the signer with a test spy, and assert both the denial and zero signer calls. Also test invalid targets and revoked grants against the policy your application is meant to enforce.

The firewall migration raised the same review question

The migration was away from FireHOL, into a custom firewall solution already used at work for years. That solution now supports FireHOL ipsets, leaves Docker’s rules alone, and uses a calculated hash to make changes to the rules evident.

There is plenty to get wrong in that combination. A conversion can look reasonable in a diff while changing what traffic gets through. Those are the findings I want a reviewer to spend its time on.

The rules have to mean the same thing

Moving between firewall systems requires more than translating their configuration syntax. The existing rules express decisions about which systems can talk to each other. Unless the migration deliberately changes those decisions, the replacement needs to preserve them.

Kodus was able to identify subtle differences in the conversion and flag their security implications. I am not publishing the internal rules or claiming a measured detection rate. What made the reviews useful was that the comments concerned behavior I needed to examine.

For someone planning a similar move, I would make the intended behavior explicit before asking an agent to convert anything. Which sources should reach each service? Which should remain blocked? What does each address set represent? Which rules belong to another service?

That last question matters here because Docker manages rules too. Leaving them alone is a requirement of this replacement. A firewall implementation that produces its own intended rules but interferes with a neighboring system still has a problem.

These requirements also give a reviewer something concrete to work against. “Review this firewall migration” is a much weaker instruction than providing the expected traffic policy and identifying the parts the migration must not touch.

A hash makes changes visible

The custom solution also uses a calculated hash to detect rules being changed, deleted, or added. That provides evidence that the rules differ from the expected state.

There are separate questions worth asking about any implementation of that idea: what is included in the hash, where the expected value is kept, and how legitimate changes are handled. A hash does not establish that the original policy was correct. Nor does detecting a change prevent it.

I would review those properties separately from the migration itself. Preserving the old policy and recognizing later changes are different jobs, even when the same tooling handles both.

Judge the review by the finding

I have been getting better reviews from Kodus with the models I have been using, but these examples do not establish a detection rate or a model ranking. They show two findings I could trace to code, explain, and fix.

For cost, I would record the actual charge for each review alongside findings accepted, findings dismissed, and time spent investigating them. Count reruns and generated output as well as input. A cache discount is useful only when the requests actually reuse eligible context; it is not a discount on the entire review.

The regression tests should remain after the review comment is resolved. For the report handoff, the important assertion is not merely that the response says access denied: the signer must not be called.