Skip to content

Conversation

@favincen
Copy link

@favincen favincen commented Oct 30, 2025

Base information

Question Answer
Related to a SourceForge thead / Another PR / Combodo ticket? bug #8838
Type of change? Bug fix

Symptom (bug)

Regression: testing of a correct custom path to mysqldump fails with error 'mysqldump not found in ...'.
image

Reproduction procedure (bug)

  1. On iTop 3.2.2-1
  2. With PHP 8.1.33 or 8.3.12
  3. Set a valid custom path to mysqldump command in parameter 'mysql_bindir' of 'itop-backup' module configuration
'itop-backup' => array (
		'mysql_bindir' => '/opt/local/bin/',
  1. access backup status page (${ITOP-app_root_url}/pages/exec.php?exec_module=itop-backup&exec_page=status.php)
  2. iTop fails to find mysqldump command even if it is present and with appropriate rx rights on path and file.

Cause (bug)

commit "N°8379 - fix backup issue" introduced a security test on content of parameter 'mysql_bindir' of 'itop-backup' module configuration, but with incorrect quoting : double quote are added to the command before testing it exist, but file_exists() doesn't need and doesn't interpret these quotes and fails to find the file.

Still, quoting is needed before returing the command for system call to properly handle cases where the path contains spaces or other characters shell would interpret as separators.

Proposed solution (bug and enhancement)

postpone quoting the command so that file_exist() can have a unquotted command to test

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have tested all changes I made on an iTop instance (linux and MacOS darwin)
  • I have added a unit test, otherwise I have explained why I couldn't
  • Is the PR clear and detailed enough so anyone can understand digging in the code?

Checklist of things to do before PR is ready to merge

  • test on a iTop instance running on Windows
  • consider rebase to support/3.2
  • consider adding unit tests

@favincen
Copy link
Author

My first PR ever ! 😅🤞

@favincen favincen changed the title 🐛 fix testing MySQLBinDir 🐛 N°8838 - fix testing MySQLBinDir Oct 30, 2025
@favincen favincen changed the title 🐛 N°8838 - fix testing MySQLBinDir 🐛 N°8838 - fix testing mysqldump custom path Oct 30, 2025
@jf-cbd
Copy link
Member

jf-cbd commented Oct 30, 2025

Youhou, right in time for Hacktoberfest 😄
The functional review occurred the previous week, but since a ticket and a bug already exist, I'll check if it can be validated "functionally" before the next technical review on Friday 7

@jf-cbd jf-cbd moved this from Hacktoberfest to Pending technical review in Combodo PRs dashboard Nov 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Pending technical review

Development

Successfully merging this pull request may close these issues.

2 participants