Skip to content

Commit 4a6b642

Browse files
somethingwithproofsomethingwithproof
committed
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 4a6b642

6 files changed

Lines changed: 609 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",

‎docs/security/settings-validation.md‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
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+
## How `CactiSettings::validate` is used in `settings.php`
18+
19+
`save_settings()` calls the validator before writing any rows:
20+
21+
```php
22+
require_once(CACTI_PATH_LIBRARY . '/CactiSettings.php');
23+
24+
$violations = CactiSettings::validate($_POST, $settings);
25+
26+
if (cacti_sizeof($violations) > 0) {
27+
foreach ($violations as $name => $message) {
28+
$_SESSION['sess_error_fields'][$name] = $name;
29+
$_SESSION['sess_field_values'][$name] = $_POST[$name] ?? '';
30+
raise_message('cacti_settings_' . $name, ..., MESSAGE_LEVEL_ERROR);
31+
}
32+
header('Location: settings.php?tab=...');
33+
exit;
34+
}
35+
```
36+
37+
When violations are present the request is rejected before any `REPLACE INTO settings`
38+
runs. The user is redirected back to the same tab with the error highlighted.
39+
40+
## How to add constraints to a setting
41+
42+
1. Open `include/global_settings.php` and find the setting definition.
43+
2. Add a `'constraints'` key holding an array of Symfony constraint instances.
44+
3. The `use Symfony\Component\Validator\Constraints as Assert;` alias is
45+
already imported at the top of the file, so constraints read as
46+
`new Assert\Range(...)`.
47+
48+
Example:
49+
50+
```php
51+
'snmp_timeout' => [
52+
'friendly_name' => __('Timeout'),
53+
'method' => 'textbox',
54+
'default' => '500',
55+
'max_length' => '10',
56+
'constraints' => [
57+
new Assert\Regex(pattern: '/^\d+$/'),
58+
new Assert\Range(min: 1, max: 600000),
59+
],
60+
],
61+
```
62+
63+
## Constraint types used in the pilot
64+
65+
See the [Symfony Validator reference](https://symfony.com/doc/6.4/validation.html)
66+
for the full catalog. The pilot uses:
67+
68+
- `Assert\NotBlank` -- value is present and non-empty.
69+
- `Assert\Length` -- string length within `min`/`max`.
70+
- `Assert\Range` -- numeric value within `min`/`max`.
71+
- `Assert\Choice` -- value is one of an enumerated list. Use this for any
72+
setting that is rendered as a `drop_array`.
73+
- `Assert\Regex` -- value matches a pattern. Useful for "is this an integer
74+
string" since `$_POST` values arrive as strings.
75+
- `Assert\Positive` -- value is strictly greater than zero.
76+
77+
## Migration template for the next batch
78+
79+
For each setting you want to constrain:
80+
81+
1. Identify the setting's existing implicit rule (e.g. `method` is `drop_array`
82+
with a fixed key set, or `max_length` is `'255'`).
83+
2. Translate that rule to a constraint object. `drop_array` becomes
84+
`Assert\Choice` over the array keys. `max_length` becomes `Assert\Length`.
85+
Numeric textboxes typically need `Assert\Regex` plus `Assert\Range`.
86+
3. Add a unit test in `tests/Unit/CactiSettingsTest.php` that exercises the
87+
new constraint with a synthetic definition (do not depend on
88+
`include/global_settings.php` from the test).
89+
4. Run `composer test` and confirm the new case passes.
90+
91+
## Defense in depth
92+
93+
The constraint layer supplements existing checks rather than replacing them.
94+
95+
- The form-render method (`drop_array`, `dirpath`, `filepath`, `textbox_password`)
96+
still enforces its implicit rules in `save_settings()`. For example,
97+
`dirpath` continues to verify the directory exists before storing.
98+
- Any post-save handler (cache-clear, poller restart, log rotation) keeps
99+
whatever validation it already performs.
100+
- The constraint check runs first and short-circuits the write, so downstream
101+
handlers see only values that already cleared the declared rules.

‎include/global_settings.php‎

Lines changed: 55 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+
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+
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+
new Assert\NotBlank(),
233+
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+
new Assert\NotBlank(),
244+
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+
new Assert\Regex(pattern: '/^\d+$/', message: 'must be a positive integer (milliseconds).'),
1023+
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+
new Assert\Regex(pattern: '/^\d+$/', message: 'must be a non-negative integer.'),
1035+
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+
new Assert\NotBlank(),
1417+
new Assert\Regex(pattern: '#^([A-Za-z]:\\\\|/)#', message: 'must be an absolute path.'),
1418+
new Assert\Length(max: 255),
1419+
],
13911420
],
13921421
'rrdtool_header' => [
13931422
'friendly_name' => __('RRDtool Graph Options'),
@@ -1527,13 +1556,19 @@
15271556
'method' => 'drop_array',
15281557
'default' => 300,
15291558
'array' => $poller_intervals,
1559+
'constraints' => [
1560+
new Assert\Choice(choices: ['10', '15', '20', '30', '60', '300', 10, 15, 20, 30, 60, 300]),
1561+
],
15301562
],
15311563
'cron_interval' => [
15321564
'friendly_name' => __('Cron/Daemon Interval'),
15331565
'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.'),
15341566
'method' => 'drop_array',
15351567
'default' => 300,
15361568
'array' => $cron_intervals,
1569+
'constraints' => [
1570+
new Assert\Choice(choices: ['60', '300', 60, 300]),
1571+
],
15371572
],
15381573
'process_leveling' => [
15391574
'friendly_name' => __('Balance Process Load'),
@@ -2268,14 +2303,22 @@
22682303
'method' => 'textbox',
22692304
'default' => 'localhost',
22702305
'max_length' => 255,
2306+
'constraints' => [
2307+
new Assert\NotBlank(),
2308+
new Assert\Length(max: 255),
2309+
],
22712310
],
22722311
'settings_smtp_port' => [
22732312
'friendly_name' => __('SMTP Port'),
22742313
'description' => __('The port on the SMTP Server to use.'),
22752314
'method' => 'textbox',
22762315
'max_length' => 255,
22772316
'default' => 25,
2278-
'size' => 5
2317+
'size' => 5,
2318+
'constraints' => [
2319+
new Assert\Regex(pattern: '/^\d+$/', message: 'must be a positive integer.'),
2320+
new Assert\Range(min: 1, max: 65535),
2321+
],
22792322
],
22802323
'settings_smtp_username' => [
22812324
'friendly_name' => __('SMTP Username'),
@@ -3130,7 +3173,11 @@
31303173
'description' => __('The default RRA to use in rare occasions.'),
31313174
'method' => 'drop_sql',
31323175
'sql' => 'SELECT id, name FROM data_source_profiles_rra ORDER BY steps',
3133-
'default' => '1'
3176+
'default' => '1',
3177+
'constraints' => [
3178+
new Assert\Regex(pattern: '/^\d+$/', message: 'must be a positive integer id.'),
3179+
new Assert\Positive(),
3180+
],
31343181
],
31353182
'default_timespan' => [
31363183
'friendly_name' => __('Default Timespan'),

0 commit comments

Comments
 (0)