You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
In other words, if you can only tell at runtime if a usage is correct, should the test fail, warn, or pass?
For instance, TestSessionRegenFalse checks if the function session_regenerate_id() is used. It uses Node::isFunction() to determine this. The call to isFunction() returns true if a function is called via a variable. Therefore, the test fails if a variable function is used and the other criteria are not met. In other words, $func(), $func(false) and $func(1) all fail the test.
In the same test, if the parameter to session_regenerate_id() is not the literal value true, the test fails. Even if the variable will always be true.
In the first case, I'd argue that the best behavior would be to pass the test; session_regenerate_id() will not likely be called via a variable. It could also be argued that this should issue a warning since it is possible that the variable will point to session_regenerate_id(). I don't believe it should always fail as it does now. It seems to me that false positives would be far too common.
In the second case, the behavior for a variable will almost always be incorrect, or at least potentially incorrect. Even if the parameter is a variable, it should always be passed true and only true. A variable will be wrong more often that right.
The text was updated successfully, but these errors were encountered:
Well, that's one of the major downfalls of static scanners. They can only test the code statically and at rest, not as it is executed. I could see how if it was something like $func = 'session_generate_id'; $func(); this kind of checking could be useful, but I think it's an edge case that may not be worth it.
Maybe the isFunction check needs to be a bit smarter to evaluate if the function call is variable or not...
I'm just wondering if there might be a way to mitigate the weakness somewhat with separate "this is definitely bad" and "this is potentially bad" buckets.
As for isFunction(), I think it would be pretty easy to add that check. Right now, I think it just bails with true if it's a variable function.
In other words, if you can only tell at runtime if a usage is correct, should the test fail, warn, or pass?
For instance,
TestSessionRegenFalse
checks if the functionsession_regenerate_id()
is used. It usesNode::isFunction()
to determine this. The call toisFunction()
returns true if a function is called via a variable. Therefore, the test fails if a variable function is used and the other criteria are not met. In other words,$func()
,$func(false)
and$func(1)
all fail the test.In the same test, if the parameter to
session_regenerate_id()
is not the literal valuetrue
, the test fails. Even if the variable will always be true.In the first case, I'd argue that the best behavior would be to pass the test;
session_regenerate_id()
will not likely be called via a variable. It could also be argued that this should issue a warning since it is possible that the variable will point tosession_regenerate_id()
. I don't believe it should always fail as it does now. It seems to me that false positives would be far too common.In the second case, the behavior for a variable will almost always be incorrect, or at least potentially incorrect. Even if the parameter is a variable, it should always be passed
true
and onlytrue
. A variable will be wrong more often that right.The text was updated successfully, but these errors were encountered: