Skip to content

Commit d35e9cc

Browse files
feat(validator): add settings constraint validation pilot
Routes saved settings through Symfony Validator so include/global_settings.php can declare per-setting constraints inline. The save handler runs one validator pass and surfaces violations via the existing raise_message() pattern, replacing scattered ad-hoc checks. Pilot covers 12 high-value settings; the rest stay on the existing implicit form-render checks. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
1 parent ce31e1e commit d35e9cc

6 files changed

Lines changed: 736 additions & 9 deletions

File tree

composer.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,8 @@
5555
"phpseclib/phpseclib": "^3.0",
5656
"slim/slim": "^4.14",
5757
"stevenmaguire/oauth2-keycloak": "^6.1.0",
58-
"stevenmaguire/oauth2-microsoft": "^2.2"
58+
"stevenmaguire/oauth2-microsoft": "^2.2",
59+
"symfony/validator": "^6.4"
5960
},
6061
"require-dev": {
6162
"friendsofphp/php-cs-fixer": "^3.86",
Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
# Settings Validation
2+
3+
## Why this exists
4+
5+
Cacti settings are written from several places: the web UI (`settings.php`),
6+
CLI utilities under `cli/`, and (planned) JSON-RPC endpoints. Each of those
7+
paths previously carried its own ad-hoc checks for the same setting, and the
8+
checks drifted apart as code was added. The intent of `CactiSettings::validate()`
9+
is to put the constraint definition next to the setting itself so every write
10+
path uses the same rules.
11+
12+
A setting in `include/global_settings.php` declares its constraints alongside
13+
the existing keys (`method`, `default`, `max_length`, ...). The validator runs
14+
the entire posted set in one pass and returns a `{name => message}` map. An
15+
empty map means the input is valid.
16+
17+
### Why constraints are declared as closures
18+
19+
`include/global_settings.php` is loaded from `include/global.php` *before*
20+
`include/vendor/autoload.php`. Eagerly instantiating `new Assert\NotBlank()`
21+
inside the array literal would fatal at file-load time because the Symfony
22+
Validator classes are not yet loadable. Wrapping each constraint in an arrow
23+
function (`fn() => new Assert\NotBlank()`) defers the instantiation until
24+
`CactiSettings::validate()` runs the closures. The `use Symfony\Component\Validator\Constraints as Assert;`
25+
import at the top of the file is a compile-time alias and does not trigger
26+
autoloading.
27+
28+
## How `CactiSettings::validate` is used in `settings.php`
29+
30+
`save_settings()` builds a snapshot of posted values via Cacti's request-var
31+
helpers (`gnrv`) and calls the validator before writing any rows:
32+
33+
```php
34+
require_once(CACTI_PATH_LIBRARY . '/CactiSettings.php');
35+
36+
$snapshot = [];
37+
foreach ($settings[grv('tab')] as $field_name => $field_array) {
38+
$snapshot[$field_name] = gnrv($field_name);
39+
}
40+
41+
$violations = CactiSettings::validate($snapshot, $settings);
42+
43+
if (cacti_sizeof($violations) > 0) {
44+
foreach ($violations as $name => $message) {
45+
$_SESSION['sess_error_fields'][$name] = $name;
46+
$_SESSION['sess_field_values'][$name] = $snapshot[$name] ?? '';
47+
raise_message('cacti_settings_' . $name, ..., MESSAGE_LEVEL_ERROR);
48+
}
49+
header('Location: settings.php?tab=...');
50+
exit;
51+
}
52+
```
53+
54+
When violations are present the request is rejected before any `REPLACE INTO settings`
55+
runs. The user is redirected back to the same tab with the error highlighted.
56+
57+
## How to add constraints to a setting
58+
59+
1. Open `include/global_settings.php` and find the setting definition.
60+
2. Add a `'constraints'` key holding an array of closures, each returning a
61+
Symfony constraint instance.
62+
3. The `use Symfony\Component\Validator\Constraints as Assert;` alias is
63+
already imported at the top of the file, so constraints read as
64+
`fn() => new Assert\Range(...)`.
65+
4. Wrap any custom `message:` argument in `__()` so the message participates
66+
in Cacti i18n.
67+
68+
Example:
69+
70+
```php
71+
'snmp_timeout' => [
72+
'friendly_name' => __('Timeout'),
73+
'method' => 'textbox',
74+
'default' => '500',
75+
'max_length' => '10',
76+
'constraints' => [
77+
fn() => new Assert\Regex(pattern: '/^\d+$/', message: __('must be a positive integer (milliseconds).')),
78+
fn() => new Assert\Range(min: 1, max: 600000),
79+
],
80+
],
81+
```
82+
83+
When a `Choice` list duplicates a canonical array from `global_arrays.php`,
84+
derive the choices from that array inside the closure rather than restating
85+
the values:
86+
87+
```php
88+
'constraints' => [
89+
fn() => new Assert\Choice(choices: array_merge(
90+
array_keys($GLOBALS['poller_intervals']),
91+
array_map('strval', array_keys($GLOBALS['poller_intervals']))
92+
)),
93+
],
94+
```
95+
96+
Both string and integer forms of the keys are accepted because `$_POST`
97+
values arrive as strings while the canonical keys are integers.
98+
99+
## Constraint types used in the pilot
100+
101+
See the [Symfony Validator reference](https://symfony.com/doc/6.4/validation.html)
102+
for the full catalog. The pilot uses:
103+
104+
- `Assert\NotBlank` -- value is present and non-empty.
105+
- `Assert\Length` -- string length within `min`/`max`.
106+
- `Assert\Range` -- numeric value within `min`/`max`.
107+
- `Assert\Choice` -- value is one of an enumerated list. Use this for any
108+
setting that is rendered as a `drop_array`.
109+
- `Assert\Regex` -- value matches a pattern. Useful for "is this an integer
110+
string" since `$_POST` values arrive as strings.
111+
- `Assert\Positive` -- value is strictly greater than zero.
112+
113+
## Migration template for the next batch
114+
115+
For each setting you want to constrain:
116+
117+
1. Identify the setting's existing implicit rule (e.g. `method` is `drop_array`
118+
with a fixed key set, or `max_length` is `'255'`).
119+
2. Translate that rule to a constraint object. `drop_array` becomes
120+
`Assert\Choice` over the array keys. `max_length` becomes `Assert\Length`.
121+
Numeric textboxes typically need `Assert\Regex` plus `Assert\Range`.
122+
3. Add a unit test in `tests/Unit/CactiSettingsTest.php` that exercises the
123+
new constraint with a synthetic definition (do not depend on
124+
`include/global_settings.php` from the test).
125+
4. Run `composer test` and confirm the new case passes.
126+
127+
## Translator wiring (known limitation)
128+
129+
`CactiSettings::validator()` builds the validator with
130+
`Validation::createValidator()` and does not pass a `TranslatorInterface`.
131+
Symfony's default English messages render regardless of the active Cacti
132+
locale. Custom messages declared in `global_settings.php` should be wrapped
133+
in `__()` so the operator-facing text is translatable through Cacti's own
134+
i18n stack. Wiring a `TranslatorInterface` so that the framework's built-in
135+
constraint messages also translate is a planned follow-up.
136+
137+
## Defense in depth
138+
139+
The constraint layer supplements existing checks rather than replacing them.
140+
141+
- The form-render method (`drop_array`, `dirpath`, `filepath`, `textbox_password`)
142+
still enforces its implicit rules in `save_settings()`. For example,
143+
`dirpath` continues to verify the directory exists before storing.
144+
- Any post-save handler (cache-clear, poller restart, log rotation) keeps
145+
whatever validation it already performs.
146+
- The constraint check runs first and short-circuits the write, so downstream
147+
handlers see only values that already cleared the declared rules.

include/global_settings.php

Lines changed: 65 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@
2222
+-------------------------------------------------------------------------+
2323
*/
2424

25+
use Symfony\Component\Validator\Constraints as Assert;
26+
2527
$dir = dir(CACTI_PATH_INCLUDE . '/themes/');
2628

2729
// Work around issue where phpstan is not detecting globals
@@ -184,14 +186,20 @@
184186
'description' => __('The path to your snmpwalk binary.'),
185187
'method' => 'filepath',
186188
'file_type' => 'binary',
187-
'max_length' => '255'
189+
'max_length' => '255',
190+
'constraints' => [
191+
fn () => new Assert\Length(min: 1, max: 255),
192+
],
188193
],
189194
'path_snmpget' => [
190195
'friendly_name' => __('snmpget Binary Path'),
191196
'description' => __('The path to your snmpget binary.'),
192197
'method' => 'filepath',
193198
'file_type' => 'binary',
194-
'max_length' => '255'
199+
'max_length' => '255',
200+
'constraints' => [
201+
fn () => new Assert\Length(min: 1, max: 255),
202+
],
195203
],
196204
'path_snmpbulkwalk' => [
197205
'friendly_name' => __('snmpbulkwalk Binary Path'),
@@ -219,14 +227,22 @@
219227
'description' => __('The path to the rrdtool binary.'),
220228
'method' => 'filepath',
221229
'file_type' => 'binary',
222-
'max_length' => '255'
230+
'max_length' => '255',
231+
'constraints' => [
232+
fn () => new Assert\NotBlank(),
233+
fn () => new Assert\Length(min: 1, max: 255),
234+
],
223235
],
224236
'path_php_binary' => [
225237
'friendly_name' => __('PHP Binary Path'),
226238
'description' => __('The path to your PHP binary file (may require a php recompile to get this file).'),
227239
'method' => 'filepath',
228240
'file_type' => 'binary',
229-
'max_length' => '255'
241+
'max_length' => '255',
242+
'constraints' => [
243+
fn () => new Assert\NotBlank(),
244+
fn () => new Assert\Length(min: 1, max: 255),
245+
],
230246
],
231247
'path_fping' => [
232248
'friendly_name' => __('FPing Binary Path'),
@@ -1001,15 +1017,23 @@
10011017
'method' => 'textbox',
10021018
'default' => '500',
10031019
'max_length' => '10',
1004-
'size' => '5'
1020+
'size' => '5',
1021+
'constraints' => [
1022+
fn () => new Assert\Regex(pattern: '/^\d+$/', message: __('must be a positive integer (milliseconds).')),
1023+
fn () => new Assert\Range(min: 1, max: 600000),
1024+
],
10051025
],
10061026
'snmp_retries' => [
10071027
'friendly_name' => __('Retries'),
10081028
'description' => __('Default SNMP retries for all new Devices.'),
10091029
'method' => 'textbox',
10101030
'default' => '3',
10111031
'max_length' => '10',
1012-
'size' => '5'
1032+
'size' => '5',
1033+
'constraints' => [
1034+
fn () => new Assert\Regex(pattern: '/^\d+$/', message: __('must be a non-negative integer.')),
1035+
fn () => new Assert\Range(min: 0, max: 100),
1036+
],
10131037
],
10141038
'snmp_bulk_walk_size' => [
10151039
'friendly_name' => __('Bulkwalk Fetch Size'),
@@ -1388,6 +1412,11 @@
13881412
'default' => CACTI_PATH_CACHE . '/realtime/',
13891413
'max_length' => 255,
13901414
'size' => 40,
1415+
'constraints' => [
1416+
fn () => new Assert\NotBlank(),
1417+
fn () => new Assert\Regex(pattern: '#^([A-Za-z]:\\\\|/)#', message: __('must be an absolute path.')),
1418+
fn () => new Assert\Length(max: 255),
1419+
],
13911420
],
13921421
'rrdtool_header' => [
13931422
'friendly_name' => __('RRDtool Graph Options'),
@@ -1527,13 +1556,29 @@
15271556
'method' => 'drop_array',
15281557
'default' => 300,
15291558
'array' => $poller_intervals,
1559+
// Choices derive from $poller_intervals (keys are interval seconds).
1560+
// Both string and int forms are accepted because $_POST values arrive
1561+
// as strings while the canonical keys are int.
1562+
'constraints' => [
1563+
fn () => new Assert\Choice(choices: array_merge(
1564+
array_keys($GLOBALS['poller_intervals']),
1565+
array_map('strval', array_keys($GLOBALS['poller_intervals']))
1566+
)),
1567+
],
15301568
],
15311569
'cron_interval' => [
15321570
'friendly_name' => __('Cron/Daemon Interval'),
15331571
'description' => __('The frequency that the Cacti data collector will be started. You can use either crontab, a scheduled task (for windows), or the cactid systemd service to control launching the Cacti data collector. For instructions on using the cactid daemon, review the README.md file in the service directory.'),
15341572
'method' => 'drop_array',
15351573
'default' => 300,
15361574
'array' => $cron_intervals,
1575+
// Choices derive from $cron_intervals (keys are interval seconds).
1576+
'constraints' => [
1577+
fn () => new Assert\Choice(choices: array_merge(
1578+
array_keys($GLOBALS['cron_intervals']),
1579+
array_map('strval', array_keys($GLOBALS['cron_intervals']))
1580+
)),
1581+
],
15371582
],
15381583
'process_leveling' => [
15391584
'friendly_name' => __('Balance Process Load'),
@@ -2268,14 +2313,22 @@
22682313
'method' => 'textbox',
22692314
'default' => 'localhost',
22702315
'max_length' => 255,
2316+
'constraints' => [
2317+
fn () => new Assert\NotBlank(),
2318+
fn () => new Assert\Length(max: 255),
2319+
],
22712320
],
22722321
'settings_smtp_port' => [
22732322
'friendly_name' => __('SMTP Port'),
22742323
'description' => __('The port on the SMTP Server to use.'),
22752324
'method' => 'textbox',
22762325
'max_length' => 255,
22772326
'default' => 25,
2278-
'size' => 5
2327+
'size' => 5,
2328+
'constraints' => [
2329+
fn () => new Assert\Regex(pattern: '/^\d+$/', message: __('must be a positive integer.')),
2330+
fn () => new Assert\Range(min: 1, max: 65535),
2331+
],
22792332
],
22802333
'settings_smtp_username' => [
22812334
'friendly_name' => __('SMTP Username'),
@@ -3130,7 +3183,11 @@
31303183
'description' => __('The default RRA to use in rare occasions.'),
31313184
'method' => 'drop_sql',
31323185
'sql' => 'SELECT id, name FROM data_source_profiles_rra ORDER BY steps',
3133-
'default' => '1'
3186+
'default' => '1',
3187+
'constraints' => [
3188+
fn () => new Assert\Regex(pattern: '/^\d+$/', message: __('must be a positive integer id.')),
3189+
fn () => new Assert\Positive(),
3190+
],
31343191
],
31353192
'default_timespan' => [
31363193
'friendly_name' => __('Default Timespan'),

0 commit comments

Comments
 (0)