Skip to content

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.

Sign up for GitHub

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

Merged
eldy merged 6 commits into from
Oct 7, 2024
Merged

Fix 20.0 Action buttion url autocomplete (button dropdown part04) #31266

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4d8085a
Fix dol get action buttion url autocomplette
thersane-john Oct 4, 2024
b631646
change mailto
thersane-john Oct 4, 2024
2a9dd94
change mailto
thersane-john Oct 4, 2024
31dc249
Remove anonymous function
thersane-john Oct 7, 2024
dc7f01d
Remove anonymous function
thersane-john Oct 7, 2024
1f64ba4
Update functions.lib.php
eldy Oct 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 43 additions & 7 deletions htdocs/core/lib/functions.lib.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12263,6 +12263,7 @@ function dolGetStatus($statusLabel = '', $statusLabelShort = '', $html = '', $st
* $arrayforbutaction = array(
* 10 => array('attr' => array('class'=>''), 'lang'=>'propal', 'enabled'=>isModEnabled("propal"), 'perm'=>$user->hasRight('propal', 'creer'), 'label' => 'AddProp', 'url'=>'/comm/propal/card.php?action=create&projectid='.$object->id.'&socid='.$object->socid),
* 20 => array('attr' => array('class'=>''), 'lang'=>'mymodule', 'enabled'=>isModEnabled("mymodule"), 'perm'=>$user->hasRight('mymodule', 'write'), 'label' => 'MyModuleAction', 'urlroot'=>dol_build_patch('/mymodule/mypage.php?action=create')),
* 30 => array('attr' => array('class'=>''), 'lang'=>'mymodule', 'enabled'=>isModEnabled("mymodule"), 'perm'=>$user->hasRight('mymodule', 'write'), 'label' => 'MyModuleOtherAction', 'urlraw' => '# || external Url || javascript: || tel: || mailto:' ),
* ); );
* @param string $id Attribute id of action button. Example 'action-delete'. This can be used for full ajax confirm if this code is reused into the ->formconfirm() method.
* @param int|boolean $userRight User action right
Expand Down Expand Up @@ -12292,6 +12293,39 @@ function dolGetButtonAction($label, $text = '', $actionType = 'default', $url =

// If $url is an array, we must build a dropdown button or recursively iterate over each value
if (is_array($url)) {

/**
* An anonymous function to complete dropdown url
* @param string $url
* @return string
*/
$completeUrl = function ($url, $addDolUrlRoot = true) use ($params) {
Copy link
Member

@eldy eldy Oct 4, 2024

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 ?

Copy link
Contributor Author

@thersane-john thersane-john Oct 4, 2024 •
edited
Loading

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

Copy link
Contributor Author

@thersane-john thersane-john Oct 4, 2024

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.

Copy link
Member

@eldy eldy Oct 6, 2024 •
edited
Loading

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.

Copy link
Contributor Author

@thersane-john thersane-john Oct 7, 2024

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.

Copy link
Member

@eldy eldy Oct 7, 2024 •
edited
Loading

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.

if (empty($url)) {
return '';
}

$parsedUrl = parse_url($url);
if ((isset($parsedUrl['scheme']) && in_array($parsedUrl['scheme'], ['javascript', 'mailto', 'tel'])) || strpos($url, '#') === 0) {
return $url;
}

if (!empty($parsedUrl['query'])) {
// Use parse_str() function to parse the string passed via URL
parse_str($parsedUrl['query'], $urlQuery);
if (!isset($urlQuery['backtopage']) && isset($params['backtopage'])) {
$url.= '&backtopage='.urlencode($params['backtopage']);
}
}

if (!isset($parsedUrl['scheme']) && $addDolUrlRoot) {
$url = DOL_URL_ROOT.$url;
}

return $url;
};



// Loop on $url array to remove entries of disabled modules
foreach ($url as $key => $subbutton) {
if (isset($subbutton['enabled']) && empty($subbutton['enabled'])) {
Expand Down Expand Up @@ -12328,17 +12362,18 @@ function dolGetButtonAction($label, $text = '', $actionType = 'default', $url =
$langs->load($subbutton['lang']);
}

if (!empty($subbutton['urlroot'])) {
$tmpurl = $subbutton['urlroot'].(empty($params['backtopage']) ? '' : '&backtopage='.urlencode($params['backtopage']));
if (!empty($subbutton['urlraw'])) {
$tmpurl = $subbutton['urlraw']; // Use raw url, no url completion, use only what developer send
} else {
$tmpurl = DOL_URL_ROOT.$subbutton['url'].(empty($params['backtopage']) ? '' : '&backtopage='.urlencode($params['backtopage']));
$tmpurl = !empty($subbutton['urlroot']) ? $subbutton['urlroot'] : $subbutton['url'];
$tmpurl = $completeUrl($tmpurl, empty($subbutton['urlroot']));
}

$subbuttonparam = array();
if (!empty($subbutton['attr'])) {
$subbuttonparam['attr'] = $subbutton['attr'];
}
$subbuttonparam['isDropDown'] = (empty($params['isDropDown']) ? $subbutton['isDropDown'] : $params['isDropDown']);
$subbuttonparam['isDropDown'] = (empty($params['isDropDown']) ? ($subbutton['isDropDown']??false) : $params['isDropDown']);

$out .= dolGetButtonAction('', $langs->trans($subbutton['label']), 'default', $tmpurl, $subbutton['id'] ?? '', $subbutton['perm'], $subbuttonparam);
}
Expand All @@ -12350,10 +12385,11 @@ function dolGetButtonAction($label, $text = '', $actionType = 'default', $url =
$langs->load($subbutton['lang']);
}

if (!empty($subbutton['urlroot'])) {
$tmpurl = $subbutton['urlroot'].(empty($params['backtopage']) ? '' : '&backtopage='.urlencode($params['backtopage']));
if (!empty($subbutton['urlraw'])) {
$tmpurl = $subbutton['urlraw']; // Use raw url, no url completion, use only what developer send
} else {
$tmpurl = DOL_URL_ROOT.$subbutton['url'].(empty($params['backtopage']) ? '' : '&backtopage='.urlencode($params['backtopage']));
$tmpurl = !empty($subbutton['urlroot']) ? $subbutton['urlroot'] : $subbutton['url'];
$tmpurl = $completeUrl($tmpurl, empty($subbutton['urlroot']));
}

$out .= dolGetButtonAction('', $langs->trans($subbutton['label']), 'default', $tmpurl, '', $subbutton['perm'], $params);
Expand Down