Quoting Jesse Becker <[EMAIL PROTECTED]>:
> Attached is a bunch of variable sanitation checking. It isn't done,
> but I wanted to throw it out for comments before I go too far down
> some hole and can't dig myself out.
>
> This is not a patch, since are only two "include_once" lines (in
> index.php and graph.php) for existing files. The rest of the patch
> would be to add this file. Just imagine that this file gets run
> after conf.php is sourced, be before get_context.php is read.
>
> The idea is to take $_GET (and later $_COOKIE), and check to make sure
> that their contents have valid information. Invalid information is
> *discarded*, and cannot be used by the rest of the code. Thus, if
> $_GET['st'] has bogus data (non-integral data, to be precise), then it
> is deleted from the array. This is pretty harsh, but should make
> problems obvious very quickly.
>
> There are two main sections to the code. The first is the large array
> near the top of the file. This defines what parameters we care about,
> and how they should be used. Anything not in this array, or doens't
> match the datatype requested, will be discarded. Any new parameters
> that are added (for example, to indicated which metric groups should
> be "collapsed") should be added to this array.
>
> The second part is the foreach{} loop at the end. It runs through all
> variables in $_GET (and later, $_COOKIE, etc), and checks if we care
> about it at all (e.g. it is in the large array I just mentioned).
> Keys that we want, and are valid will be kept, but everything else
> will be pitched.
>
> As I said, it isn't done yet, although it is basically functional.
> I'm looking for general comments, not specific bug reports yet.
>
> So, comments and suggestion welcome.
I think this looks like a good approach. Centralizing all the input
validation is a great idea. You said you hadn't finished with it yet,
so I may be commenting on stuff that you're still working on. Please
disregard if so. :)
You said the only patching would be to include this script in
get_context.php and graph.php. So, I take that to mean that
get_context.php and graph.php would still access values in $_GET like
they currently do, but these would be modified by santize.php.
I think it'd be clearer to do everything in one place. Then, the only
script that touches $_GET/$_COOKIE is sanitize.php, and all other
scripts use variables that it makes available. That way you have only
1 place where user input is touched, and $_GET continues to be what
you'd expect: raw user input.
In the foreach() loop, if a $_GET value is found to be valid (the if()
condition is true), put the value in a $clean array, using the same
key as $_GET/$_COOKIE. All other code should use these values from
the $clean array rather than the current local variables. When
reading other scripts, this convention makes it clear that a variable
is user input, and that it has been put through appropriate checks.
If a value is invalid, maybe create the key in $clean with a sane
default value, or a null if no sensible default is possible. Code
that wanted to use something in $clean then wouldn't need to do lots
of isset() checking. This is pretty much what get_context.php and
graph.php already do.
Won't the INT, FLOAT, and NUMBER checks in valid_parameter() always be
true? (float)$value would always be a float.
The BOOL check is allowing a 1 or 0, not an actual boolean. If I saw
a value being validated as BOOL, I'd expect it to be a boolean, not a
1 or a 0. Mostly it doesn't matter since PHP is so loose with types,
but the broken 'Show Hosts' bug in 3.0.6 shows that the difference
does matter in some cases.
error_log calls should identify where they are coming from.
Alternately, you could trigger_error( "message", E_USER_WARNING ).
That automatically adds information about file and line number where
the error occurred.
Hope that's helpful,
alex
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
Ganglia-developers mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ganglia-developers
