Validate queue_id is a positive integer before passing to the BAO #14355
Conversation
|
(Standard links)
|
extern/open.php
Outdated
| @@ -11,6 +12,11 @@ | |||
| exit(); | |||
| } | |||
|
|
|||
| if (!CRM_Utils_Rule::positiveInteger($queue_id)) { | |||
There was a problem hiding this comment.
yeah - or use retrieveValue rather that $_GET directly
There was a problem hiding this comment.
yeh maybe i dunno however this just felt sensible way of managing it
There was a problem hiding this comment.
The reason I prefer to go with a retrieve fn variant is we should be able to grep our code base for instances of $_GET & find very few - so eliminating them makes future audits easier
There was a problem hiding this comment.
have switched it to that now hopefully @eileenmcnaughton
seamuslee001
force-pushed
the
harden_extern_open
branch
from
3066c7d to
bbc3875
Compare
| @@ -2,10 +2,12 @@ | |||
| require_once '../civicrm.config.php'; | |||
| require_once 'CRM/Core/Config.php'; | |||
| require_once 'CRM/Core/Error.php'; | |||
| require_once 'CRM/Utils/Array.php'; | |||
| require_once 'CRM/Utils/Type.php'; | |||
There was a problem hiding this comment.
@seamuslee001 do we need Type & Rule here now?
There was a problem hiding this comment.
They are used by the Request function and given that we aren't actually loading the class loader here we can't be certain they are loaded at this point. I don't think Array is used now for this
extern/open.php
Outdated
|
|
||
| $config = CRM_Core_Config::singleton(); | ||
| $queue_id = CRM_Utils_Array::value('q', $_GET); | ||
| $queue_id = CRM_Utils_Request::retrieveValue('q', 'positive', NULL, FALSE, 'GET'); |
There was a problem hiding this comment.
@seamuslee001 I tested this locally but it needs to be 'Positive' or it fails
There was a problem hiding this comment.
thanks @eileenmcnaughton have updated now
There was a problem hiding this comment.
Looks good to go now - I note that when you pass a string it drops it & then declares 'missing input parameters' rather than the validation error bubbling up. I don't really have an opinion on that though - at the margins it could cause a debugger to take a fee more minutes debugging
seamuslee001
force-pushed
the
harden_extern_open
branch
from
bbc3875 to
5cc61b0
Compare
Switch to using retrieveValue as per Eileen
seamuslee001
force-pushed
the
harden_extern_open
branch
from
5cc61b0 to
f3e9be3
Compare
|
Jenkins re test this please |
|
Test fail unrelated merging |
Overview
Just a small hardening ping @eileenmcnaughton @totten