-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Fix 20.0 Action buttion url autocomplete (button dropdown part04) #31266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix 20.0 Action buttion url autocomplete (button dropdown part04) #31266
Conversation
htdocs/core/lib/functions.lib.php
Outdated
| * @param string $url | ||
| * @return string | ||
| */ | ||
| $completeUrl = function ($url, $addDolUrlRoot = true) use ($params) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the goal of using here an anonymous function instead of dolCompletUrlForDropdownButton() for example ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try to not pollute functions.lib.php with function only used in one context..
If we want to add this function to lib, we will have to think about something more generic. Create a new functions like :
/**
* A generic function to complete url with query
* @param string $url
* @param array $query ex: ['queryKey'=>'yourData']
* @param bool $replace deault false , set to true to replace $query values if $query keys already exist
* @return string
*/
function completeUrlQuery(string $url, array $query, bool $replace = false){
}/**
* A generic function to complete url with DOL_URL_ROOT if missing
* @param string $url
* @return string
*/
function dolCompleteUrlRoot(string $url){
}but in this case too, the anonymous function will be used to call completeUrlQuery because of the context to be prepared and passed on to the function.
And to add this 2 new functions, I will have to put them in develop branch not in fix in V20
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anonymous functions are very useful when you want to start factoring some of your code. They make it easier to extract and create generic functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pb is that anonymous function that are called on the base of a variable are very useful for hackers too. They can be used to bypass a lot of security protection. This also broke a lot of feature of static analysis tools or IDE, so if we can avoid it, we should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eldy I removed the anonymous function.
Pb is that anonymous function that are called on the base of a variable are very useful for hackers too. They can be used to bypass a lot of security protection.
I agree if it use a global variable (or if $completeUrl is convert as global) but this is not the case here.
I'm very sensitive to security issues, do you have a source or an example of a hack like this (without use a global var) ? Because I'd like to know how in this case.
This also broke a lot of feature of static analysis tools or IDE, so if we can avoid it, we should.
Tested on php storm ans visual studio code with no problem.
It works because I declare the anonymous function just above these 2 uses inside a function and I also add the definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not example yet in real life, but experience shows that theory always find its real life exemple one day when we are speaking about security.
For example, we can image an extrafield where user enter a $completeUrl="exec" as a condition or computed field (so executed) and the button is called somewhere in a hook of dolGetButtonAction (so after the first definition of the anonymous function).
So when anonymous function is called, it call the exec bypassing all protection to forbid this this function into extrafields or dynamic conditions.
I don't think this example works, this will probably need to bypass other protection or the solve a problem on the scope of the variable, but the fact that I got this "idea", without searching (first think i thought), means there is a risk we must avoid.
See #31217
FIX : Action buttion url autocomplete
In some case if href attribute of link start with javascript: because a link can have differrent sheme like
but in all cases the code force to add
&backtopage=to url. Also the actual dolibarr code doesn't check if query already contain backtopage param.