FreeRADIUS - Add input validation (Bug #7263) #308
Conversation
Improve a couple of descriptions while here.
Improve a couple of descriptions while here.
Improve a couple of descriptions while here.
Input validation part pfsense#1 - EAP, SQL, LDAP
Input validation part pfsense#2 - Interfaces, Settings
Improve a couple of descriptions while here.
Improve a couple of descriptions while here.
Improve a couple of descriptions while here.
Input validation part pfsense#3 - MACs, NAS/Clients
Improve a couple of descriptions while here.
Input validation part pfsense#4 (final) - Users.
|
Wasn't done on purpose, because it'd result in conflicts with #306. |
|
Yeah, I imagined :) |
There was a problem hiding this comment.
Looks good, just have a few suggestions.
Also looking things over, print_input_errors() doesn't run the individual input errors through gettext() so it might be a good idea to use gettext() on all these validation messages now while you're making changes. Though that could be a lot of work in some places since you'd have to use sprintf in many of the strings.
It's OK if you don't want to do that now though.
| // Time Offset | ||
| if ($post['varusersmotpoffset']) { | ||
| if (!preg_match('/^-?[0-9]{2,4}$/', $post['varusersmotpoffset'])) { | ||
| $input_errors[] = "The 'Time Offset' field may only contain a valid TZ offset in minutes, with optional leading minus character."; |
There was a problem hiding this comment.
The requirement in the test below checks if it is divisible by 15, so that requirement should be mentioned in this error and in the one below.
| // Possible Login Times | ||
| // TODO: Produce some regex or better check here | ||
| if ($post['varuserslogintime'] && !preg_match("/^[a-zA-Z0-9,|-]*$/", $post['varuserslogintime'])) { | ||
| $input_errors[] = "The 'Possible Login Times' field may only contain a-z, A-Z, 0-9, comma, vertical bar and hyphen (regex /^[a-zA-Z0-9,|-]*$/)"; |
There was a problem hiding this comment.
You aren't going to split that up and validate each time entry in the list? ;-)
There was a problem hiding this comment.
The FreeRADIUS config syntax for similar things drives me mad, frankly... (Ditto the account expiry format, WTF is wrong with format like YYYY/MM/DD.)
|
|
||
| // Accounting Interim Interval | ||
| if ($post['varmacsacctinteriminterval'] != '') { | ||
| if (!is_numericint($post['varmacsacctinteriminterval'])) { |
There was a problem hiding this comment.
I'd combine both of these tests and print the same message for either case, such as "The 'Accounting Interim Interval' field must contain an integer with a value larger than 60 (seconds)"
|
|
||
| // OTP Lifetime | ||
| if ($post['varsettingsmotpenable'] == 'on') { | ||
| if ($post['varsettingsmotptimespan'] === 0) { |
There was a problem hiding this comment.
If you are going to test using ===, you should cast to (int) or similar on the variable since it would be a string.
There was a problem hiding this comment.
It doesn't work anyway, I guess it's impossible in PHP to distinguish between 0 and empty string in input. Will replace it with a single test for empty().
| } elseif (!is_numericint($post['varsettingsmotptimespan'])) { | ||
| $input_errors[] = "The 'OTP Lifetime' field must contain an integer value."; | ||
| } elseif ($post['varsettingsmotptimespan'] > 12) { | ||
| $input_errors[] = "The 'OTP Lifetime' field should contain only sane secure values. Values higher than 12 (~120 seconds) are not allowed."; |
There was a problem hiding this comment.
I'd remove "sane" here, since "secure" gets the point across on its own.
|
|
||
| // EAP-TLS Fragment Size | ||
| if ($post['vareapconffragmentsize'] != '') { | ||
| if (!is_numericint($post['vareapconffragmentsize'])) { |
There was a problem hiding this comment.
I'd combine the test and print the same error for both, mentioning the type and limit.
| if ($post['varsqlconfincludeenable'] == 'on') { | ||
| if (empty($post['varsqlconfserver'])) { | ||
| $input_errors[] = "The 'Server Address' field for SQL Server 1 must not be empty when 'SQL Support' for Server 1 is enabled."; | ||
| } elseif (!is_ipaddr($post['varsqlconfserver']) && !is_hostname($post['varsqlconfserver']) && !is_domain($post['varsqlconfserver'])) { |
There was a problem hiding this comment.
is_hostname() also returns true for domain names (it uses is_domain() internally as part of its validation), so the is_domain() test isn't needed, just is_hostname() will catch short hostnames, FQDNs, and domain names.
| if ($post['varsqlconf2includeenable'] == 'on') { | ||
| if (empty($post['varsqlconf2server'])) { | ||
| $input_errors[] = "The 'Server Address' field for SQL Server 2 must not be empty when 'SQL Support' for Server 2 is enabled."; | ||
| } elseif (!is_ipaddr($post['varsqlconf2server']) && !is_hostname($post['varsqlconf2server']) && !is_domain($post['varsqlconf2server'])) { |
There was a problem hiding this comment.
See above, the is_domain() test can be removed
| if ($post['varmodulesldapenableauthorize'] == 'on') { | ||
| if (empty($post['varmodulesldapserver'])) { | ||
| $input_errors[] = "The 'Server Address' field for LDAP Server 1 must not be empty when 'LDAP Authorization Support' for Server 1 is enabled."; | ||
| } elseif (!is_ipaddr($post['varmodulesldapserver']) && !is_hostname($post['varmodulesldapserver']) && !is_domain($post['varmodulesldapserver'])) { |
There was a problem hiding this comment.
See above, re: is_domain() test
| if ($post['varmodulesldap2enableauthorize'] == 'on') { | ||
| if (empty($post['varmodulesldap2server'])) { | ||
| $input_errors[] = "The 'Server Address' field for LDAP Server 2 must not be empty when 'LDAP Authorization Support' for Server 2 is enabled."; | ||
| } elseif (!is_ipaddr($post['varmodulesldap2server']) && !is_hostname($post['varmodulesldap2server']) && !is_domain($post['varmodulesldap2server'])) { |
There was a problem hiding this comment.
See above, re: is_domain() test
|
@jim-p - Should be done. (No, I didn't split the login times string 🤣 ) As for gettext(), are packages getting translated now as well? Anyway, this should be done in the whole package, not just for the validation part, so yeah - lots of work and largely unrelated to this; leaving that for next time. |
|
I'm not sure what the current plan of action is for translation in packages, but it seemed like low-hanging fruit since it was being modified anyhow. I'll look the changes over shortly. |
Changes since 0.3.0: We reached v1.0.0 - fix!: Replace limit flag with paginate by @ankitpokhrel in #359 - fix!: Append components on edit instead of overriding by @ankitpokhrel in #368 - feat!: Append label to an issue, show labels at issue list view by @stchar in #300 - refactor!: Move boards and project list to subcommand by @ankitpokhrel in #314 - feat: Support custom fields on issue create by @ankitpokhrel in #319 - feat: Add support to read from .netrc by @adolsalamanca in #329 - feat: Add support for OS keyrings/-chains by @boyvanamstel in #348 - feat: Support auth with personal access tokens by @marek-veber / @ankitpokhrel in #327 - feat: Allow to set fixVersions on issue creation by @ankitpokhrel in #276 - feat: Allow insecure TLS by @ankitpokhrel in #305 - feat: Add --no-browser option to open cmd by @ankitpokhrel in #308 - feat: Add search option for boards on jira init by @ankitpokhrel in #322 - feat: Add issues unlink command by @sushilkg in #347 - feat: Support refresh for issues list by @GZLiew in #325 - feat: Ability to delete issue by @ankitpokhrel in #336 - feat: Allow to set custom fields on epic create by @ankitpokhrel in #364 - feat: Allow to edit release-info/fixVersions by @ankitpokhrel in #365 - feat: Allow removing labels on edit by @ankitpokhrel in #371 - feat: Support creating issues with custom subtask type by @danobi in #372 - feat: Allow removing component on edit by @ankitpokhrel in #374 - feat: Allow removing fixVersions on edit by @ankitpokhrel in #376 - feat: Support custom fields on issue edit by @ankitpokhrel in #377 - feat: Jira init non-interactive by @ankitpokhrel in #381 - feat: Show subtasks in issue view by @ankitpokhrel in #382 - feat: Allow project filter in raw jql by @ankitpokhrel in #395 - fix: Makefile compatiblity with Make 3.81 by @danmichaelo in #252 - fix: Config generation issue by @ankitpokhrel in #275 - fix(cfg): Strip trailing slash on server name by @ankitpokhrel in #295 - fix: Jira client should respect timeout opt by @ankitpokhrel in #304 - fix: Respect GLAMOUR_STYLE env on issue view by @ankitpokhrel in #317 - fix: Get subtask handle from config by @ankitpokhrel in #296 - fix: Jira wiki parser by @ankitpokhrel in #326 - fix: Display correctly columns in list sprint command help by @adolsalamanca in #320 - fix: Panic on empty sub-list by @ankitpokhrel in #330 - fix: Issue with assigning user by @ankitpokhrel in #321 - fix: OOM bug on issue view by @ankitpokhrel in #350 - fix: Assign parent key as is on edit by @ankitpokhrel in #351 - fix: Add additional check for total boards returned by @ankitpokhrel in #360 - fix: Issue with query param in user assignment by @ankitpokhrel in #380 - fix: Subtask clone by @ankitpokhrel in #383 - fix: editing issue with custom field in non interactive mode by @DrudgeRajen in #391 - dep: Upgrade charmbracelet/glamour to 0.5.0 by @ankitpokhrel in #309 - dep: Upgrade rivo/tview to latest by @ankitpokhrel in #310 - dep: Upgrade outdated packages by @ankitpokhrel in #311 - dep: Upgrade cobra to 1.4.0 by @ankitpokhrel in #373 - Use md ext for tmp file to trigger vim syntax by @ElementalWarrior in #318 Full Changelog: ankitpokhrel/jira-cli@v0.3.0...v1.0.0
This adds input validation to FreeRADIUS package (Bug #7263).