net/upnp: Improve ACL validation, update changelog - #5677
Self-Hosting-Group wants to merge 1 commit into
Conversation
|
@fichtner Ping |
0100b2d to
b121b14
Compare
| Plugin Changelog | ||
| ================ | ||
|
|
||
| 1.9_2??? |
There was a problem hiding this comment.
revisions are not new versions so keep adding 1.9 updates to 1.9 or add 1.10 (I'd avoid 2.0 until MVC rewrite happens)
There was a problem hiding this comment.
I just changed it to 1.10. 2.0 was also my idea for the MVC rewrite.
| /* user permissions validation */ | ||
| foreach (miniupnpd_permuser_list() as $i => $permuser) { | ||
| if (!empty($pconfig[$permuser])) { | ||
| $perm = explode(' ', $pconfig[$permuser]); |
There was a problem hiding this comment.
what's wrong with this? adding trim() is not the best idea to validation, because it can obscure issues and force premature normalization.
if you accept the trim but don't store it trimmed you'll always have two different ways to address the same value. it it much easier to discard wrong values even if they just have an extra space so the user can remote that.
in mvc there is also getValues() which can normalize and filter value lists much better
There was a problem hiding this comment.
if you accept the trim but don't store it trimmed you'll always have two different ways to address the same value. it it much easier to discard wrong values even if they just have an extra space so the user can remote that.
Yes, I was aware that this was a hack. It should be cleaner this way, normalised string is directly used now. I like that it removes also multiple spaces between the fields, which can happen when you copy-paste an IP address to an existing entry.
| } | ||
| foreach (miniupnpd_permuser_list() as $fieldname) { | ||
| $upnp[$fieldname] = $pconfig[$fieldname]; | ||
| $upnp[$fieldname] = implode(' ', preg_split('/\s+/', trim($pconfig[$fieldname]))); |
There was a problem hiding this comment.
same here, you're going on a goose chase due to the above change already
b121b14 to
2e63b75
Compare
|
If this PR is better now, and if there isn't a new OPNsense release this week, I’d prefer not to merge this PR just yet, and to keep it open. |
fichtner
left a comment
fichtner
left a comment
There was a problem hiding this comment.
leaving open as requested -- but be aware I may grab this next week in order to finish the release (if I don't forget)
2e63b75 to
e3d59dc
Compare
2 participants
Disable IPv6 mappingoption to better place and update changelogFixes: #5371
Important notices
Before you submit a pull request, we ask you kindly to acknowledge the following: