Skip to content

feat(security): add cacti_dispatch helper for action table authorisation#7063

Merged
TheWitness merged 11 commits into
Cacti:developfrom
somethingwithproof:feat/cacti-dispatch-helper
May 12, 2026
Merged

feat(security): add cacti_dispatch helper for action table authorisation#7063
TheWitness merged 11 commits into
Cacti:developfrom
somethingwithproof:feat/cacti-dispatch-helper

Conversation

@somethingwithproof

Copy link
Copy Markdown
Contributor

Extracts the cacti_dispatch() helper from #7055 (which is being closed due to unrelated scope drift) and ships it as a small, focused change.

What it does

Declarative action dispatcher that every Cacti controller can opt into:

```php
cacti_dispatch(array(
'edit' => array('callback' => 'report_edit', 'method' => 'GET', 'realm' => 21),
'save' => array('callback' => 'report_save', 'method' => 'POST', 'realm' => 21,
'object_acl' => function() { return reports_user_may_mutate(grv('id')); }),
));
```

Guards run in method -> realm -> object-ACL order. Each one logs with a WEBUI category before the helper returns 405 or 403. No existing code is touched by this PR; helpers only.

Why

The repeated `switch (get_request_var('action')) { case 'save': form_save(); ... }` pattern scattered across every controller is where realm checks most often get dropped or duplicated inconsistently. Centralising the guards means new controllers adopt the three checks by construction rather than copy-pasting a switch from a neighbouring file.

Scope

  • `lib/cacti_dispatch.php` (121 lines, single function + a fallback `raise_ajax_permission_denied`)
  • `tests/Unit/CactiDispatchTest.php` (9 source-scan tests locking in the guard ordering)

No call sites migrated in this PR. Adoption is opt-in and can be done one controller at a time in follow-ups.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a reusable, declarative action dispatcher helper to centralize per-action HTTP-method and authorization checks in Cacti controllers, along with unit tests that lock in guard ordering.

Changes:

  • Introduces cacti_dispatch() helper to dispatch actions from a declarative action table with method/realm/object-ACL guard sequence.
  • Adds a permission-denied fallback helper intended for early-load contexts.
  • Adds Pest unit tests that source-scan lib/cacti_dispatch.php to enforce guard ordering and expected denial behaviors.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.

File Description
lib/cacti_dispatch.php New dispatcher helper implementing guard checks and dispatch to action callbacks.
tests/Unit/CactiDispatchTest.php New Pest tests that source-scan the dispatcher to pin guard ordering and logging/denial calls.

Comment thread lib/cacti_dispatch.php Outdated
Comment thread lib/cacti_dispatch.php Outdated
Comment thread lib/cacti_dispatch.php Outdated
Comment thread lib/cacti_dispatch.php Outdated
Comment thread tests/Unit/CactiDispatchTest.php
Comment thread tests/Unit/CactiDispatchTest.php
Comment thread lib/cacti_dispatch.php Outdated
Comment thread lib/cacti_dispatch.php Outdated
@somethingwithproof

Copy link
Copy Markdown
Contributor Author

Addressed the Copilot review in 7d46ccd:

  • String-guard on $action before offset access so ?action[]=x cannot produce a TypeError.
  • cacti_strtoupper() + defaulted REQUEST_METHOD so method comparison is locale-independent and CLI tests work.
  • Non-callable object_acl now fails closed with an ERROR log and 403 instead of silently skipping the ACL.
  • Renamed the fallback denial emitter to cacti_dispatch_deny() so it cannot collide with the unconditionally-loaded raise_ajax_permission_denied() in lib/functions.php.
  • cacti_dispatch_deny() now always emits an explicit http_response_code() after the AJAX helper so denied non-AJAX requests do not fall through to a 200 render.
  • Non-callable callback now returns 500 with an ERROR log.
  • Dropped the redundant docblock and tightened parameter types on cacti_dispatch().
  • Tests extended to cover each new branch (non-string action, non-callable ACL denial, 500 for bad callback, status clamp in cacti_dispatch_deny).

cacti_dispatch() wraps the repeated switch/case on get_request_var('action')
that every Cacti controller writes. Each action is declared with the
HTTP method it accepts, the realm id that must be allowed, and an
optional object-level ACL callback. The guards run in method then
realm then object-ACL order and each one logs with a WEBUI category
before returning a 405 or 403, so review across controllers no longer
depends on every author remembering to add the same three checks in
the same order.

The test file source-scans the helper for each guard and for the guard
ordering, so a future refactor that silently drops the realm or ACL
branch fails CI rather than regressing authorisation for every
controller that opts in.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Reject non-string action inputs before offset access so ?action[]=x
cannot produce a TypeError. Default REQUEST_METHOD to GET for CLI
and tests, and normalise both sides with cacti_strtoupper so the
comparison is locale-independent. Treat a declared-but-non-callable
object_acl as a misdeclaration and deny (log ERROR + 403) rather
than silently skipping the check. Split the denial emitter into
cacti_dispatch_deny() under a prefixed name to avoid colliding with
the existing raise_ajax_permission_denied() in lib/functions.php,
and always follow the AJAX helper with an explicit http_response_code()
so a denied non-AJAX request cannot fall through to a 200 render.
Reject non-callable callbacks with a 500 and ERROR log. Drop the
redundant docblock on the function and tighten the parameter types.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…level 6

PHPStan level 6 flagged two is_string() calls as always true:
- $default was already type-hinted as string via PHPDoc; the runtime
  cast to '' was redundant.
- The second is_string($action) at the unknown-action log happens
  after the earlier is_string guard already narrowed the type.

Tighten the signature to string $default so the first cast drops,
and remove the second is_string from the log interpolation since
$action is always a string by the time that branch runs.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof somethingwithproof force-pushed the feat/cacti-dispatch-helper branch from b45be1d to 522b0a7 Compare April 28, 2026 06:18
strspn whitelist on action; explicit AJAX detection for 403 vs redirect;
file_get_contents guard in tests.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@TheWitness TheWitness merged commit 6cbe01b into Cacti:develop May 12, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants