Skip to content

gate-50: the guard window crosses method boundaries — a guard in the NEXT method clears an unguarded read #429

Description

@rubenvdlinde

Summary

gate-50 (security-config-fail-mode) builds its guard window as eleven lines of code following the config read, counted over the file, with no notion of where the method ends. A guard belonging to a different method therefore clears an unguarded read in the method above it.

Pre-existing — identical on fa555a2 and on a316aa5. #420 changed the window's budget (comment lines no longer spend it) but not its extent, so the fix moved this defect closer rather than away: blanking comments means real code lines from the next method now arrive inside the window sooner.

Reproduced — one file, one variable

<?php
namespace OCA\C50\Controller;
class WindowController {
    // UNGUARDED: this read has no fail-mode handling of its own.
    public function readToken() {
        $tok = $this->config->getValueString($this->appName, 'api_token', '');
        return $this->render($tok);
    }

    // A DIFFERENT METHOD. Its guard is 5 code lines below the read above.
    public function readRegister() {
        $reg = $this->config->getValueString($this->appName, 'register_key', '');
        if ($reg === '') {
            return $this->render('');
        }
        return $this->lookup($reg);
    }
}
[gate-50] security-config-fail-mode: PASS          <- the defect

Positive control — delete readRegister() and nothing else:

[gate-50] security-config-fail-mode: FAIL — 1 unsafe security-config read(s)
lib/Controller/WindowController.php:6: security-relevant config read of "api_token" has no fail-mode guard within 10 lines

api_token is read, is never compared, is never defaulted, and reaches the response. The only thing standing between that and a finding is an unrelated method that happens to be written below it.

Why it matters

This is a false negative on a gate whose subject is a defense silently deactivating. It is also order-dependent in a way nobody can see while reading: moving a method, or adding one, changes the verdict for a method whose body did not change. Two of the four current opencatalogi findings sit in a file where the next read is 1,697 lines away, so the fleet count is not obviously wrong today — but the shape is there and nothing forbids it.

There is a second, milder consequence in the same mechanism: the last read in a file has a short window by accident (the file ends), so it is judged more strictly than its neighbours.

Suggested fix

Clip the window at the enclosing method's closing brace — the brace walk in gate-49's checker already does exactly this over php_masked text and could be shared. A read with fewer than eleven code lines left in its method should be judged against what its method actually contains, not against the next method's opening.

Acceptance

Both arms above, driven through bin/hydra-gates: the two-method arm must FAIL naming readToken(), and the control must keep FAILing. Add a third arm where the guard is genuinely in the same method eleven code lines down, which must still PASS — otherwise the clip becomes a new false positive.

Measured 2026-08-13 while re-measuring gates 7/25/48/49/50 across the eighteen core apps.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions