feat(security): add cacti_dispatch helper for action table authorisation#7063
Merged
TheWitness merged 11 commits intoMay 12, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
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.phpto 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. |
Contributor
Author
|
Addressed the Copilot review in 7d46ccd:
|
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>
b45be1d to
522b0a7
Compare
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
approved these changes
May 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
No call sites migrated in this PR. Adoption is opt-in and can be done one controller at a time in follow-ups.