From 37084bec5919dd1573df2e025cf4f3bab4cbcff0 Mon Sep 17 00:00:00 2001 From: Petr Mironychev <9195189+Palm1r@users.noreply.github.com> Date: Fri, 13 Mar 2026 00:34:20 +0100 Subject: [PATCH] feat: Improve execute terminal command tool --- settings/SettingsConstants.hpp | 1 + settings/ToolsSettings.cpp | 10 ++ settings/ToolsSettings.hpp | 1 + tools/ExecuteTerminalCommandTool.cpp | 163 +++++++++++++-------------- tools/ExecuteTerminalCommandTool.hpp | 4 +- 5 files changed, 93 insertions(+), 86 deletions(-) diff --git a/settings/SettingsConstants.hpp b/settings/SettingsConstants.hpp index 9c5bf79..f035648 100644 --- a/settings/SettingsConstants.hpp +++ b/settings/SettingsConstants.hpp @@ -116,6 +116,7 @@ const char CA_ALLOWED_TERMINAL_COMMANDS[] = "QodeAssist.caAllowedTerminalCommand const char CA_ALLOWED_TERMINAL_COMMANDS_LINUX[] = "QodeAssist.caAllowedTerminalCommandsLinux"; const char CA_ALLOWED_TERMINAL_COMMANDS_MACOS[] = "QodeAssist.caAllowedTerminalCommandsMacOS"; const char CA_ALLOWED_TERMINAL_COMMANDS_WINDOWS[] = "QodeAssist.caAllowedTerminalCommandsWindows"; +const char CA_TERMINAL_COMMAND_TIMEOUT[] = "QodeAssist.caTerminalCommandTimeout"; const char QODE_ASSIST_GENERAL_OPTIONS_ID[] = "QodeAssist.GeneralOptions"; const char QODE_ASSIST_GENERAL_SETTINGS_PAGE_ID[] = "QodeAssist.1GeneralSettingsPageId"; diff --git a/settings/ToolsSettings.cpp b/settings/ToolsSettings.cpp index afd9d6e..eb5923a 100644 --- a/settings/ToolsSettings.cpp +++ b/settings/ToolsSettings.cpp @@ -128,6 +128,14 @@ ToolsSettings::ToolsSettings() allowedTerminalCommandsWindows.setDisplayStyle(Utils::StringAspect::LineEditDisplay); allowedTerminalCommandsWindows.setDefaultValue("git, dir, type, findstr, where"); + terminalCommandTimeout.setSettingsKey(Constants::CA_TERMINAL_COMMAND_TIMEOUT); + terminalCommandTimeout.setLabelText(Tr::tr("Command Timeout (seconds)")); + terminalCommandTimeout.setToolTip( + Tr::tr("Maximum time in seconds to wait for a terminal command to complete. " + "Increase for long-running commands like builds.")); + terminalCommandTimeout.setRange(5, 3600); + terminalCommandTimeout.setDefaultValue(30); + resetToDefaults.m_buttonText = Tr::tr("Reset Page to Defaults"); readSettings(); @@ -167,6 +175,7 @@ ToolsSettings::ToolsSettings() enableTerminalCommandTool, enableTodoTool, currentOsCommands, + terminalCommandTimeout, autoApplyFileEdits}}, Stretch{1}}; }); @@ -203,6 +212,7 @@ void ToolsSettings::resetSettingsToDefaults() resetAspect(allowedTerminalCommandsLinux); resetAspect(allowedTerminalCommandsMacOS); resetAspect(allowedTerminalCommandsWindows); + resetAspect(terminalCommandTimeout); writeSettings(); } } diff --git a/settings/ToolsSettings.hpp b/settings/ToolsSettings.hpp index e96990a..a447a64 100644 --- a/settings/ToolsSettings.hpp +++ b/settings/ToolsSettings.hpp @@ -45,6 +45,7 @@ public: Utils::StringAspect allowedTerminalCommandsLinux{this}; Utils::StringAspect allowedTerminalCommandsMacOS{this}; Utils::StringAspect allowedTerminalCommandsWindows{this}; + Utils::IntegerAspect terminalCommandTimeout{this}; Utils::BoolAspect autoApplyFileEdits{this}; private: diff --git a/tools/ExecuteTerminalCommandTool.cpp b/tools/ExecuteTerminalCommandTool.cpp index e01561d..d114500 100644 --- a/tools/ExecuteTerminalCommandTool.cpp +++ b/tools/ExecuteTerminalCommandTool.cpp @@ -32,6 +32,8 @@ #include #include +#include + namespace QodeAssist::Tools { ExecuteTerminalCommandTool::ExecuteTerminalCommandTool(QObject *parent) @@ -188,54 +190,66 @@ QFuture ExecuteTerminalCommandTool::executeAsync(const QJsonObject &inp QFuture future = promise->future(); promise->start(); + auto resolved = std::make_shared>(false); + QProcess *process = new QProcess(); process->setWorkingDirectory(workingDir); process->setProcessChannelMode(QProcess::MergedChannels); - + process->setReadChannel(QProcess::StandardOutput); + const int timeoutMs = commandTimeoutMs(); + QTimer *timeoutTimer = new QTimer(); timeoutTimer->setSingleShot(true); - timeoutTimer->setInterval(COMMAND_TIMEOUT_MS); - - auto outputSize = QSharedPointer::create(0); + timeoutTimer->setInterval(timeoutMs); + + QObject::connect(timeoutTimer, &QTimer::timeout, [process, promise, resolved, command, args, timeoutTimer, timeoutMs]() { + if (*resolved) + return; + *resolved = true; - QObject::connect(timeoutTimer, &QTimer::timeout, [process, promise, command, args, timeoutTimer]() { LOG_MESSAGE(QString("ExecuteTerminalCommandTool: Command '%1 %2' timed out after %3ms") .arg(command) .arg(args) - .arg(COMMAND_TIMEOUT_MS)); - + .arg(timeoutMs)); + process->terminate(); - + QTimer::singleShot(1000, process, [process]() { - if (process->state() == QProcess::Running) { + if (process->state() != QProcess::NotRunning) { LOG_MESSAGE("ExecuteTerminalCommandTool: Forcefully killing process after timeout"); process->kill(); } + process->deleteLater(); }); - + promise->addResult(QString("Error: Command '%1 %2' timed out after %3 seconds. " "The process has been terminated.") .arg(command) .arg(args.isEmpty() ? "" : args) - .arg(COMMAND_TIMEOUT_MS / 1000)); + .arg(timeoutMs / 1000)); promise->finish(); - process->deleteLater(); timeoutTimer->deleteLater(); }); QObject::connect( process, QOverload::of(&QProcess::finished), - [this, process, promise, command, args, timeoutTimer, outputSize]( + [this, process, promise, resolved, command, args, timeoutTimer]( int exitCode, QProcess::ExitStatus exitStatus) { + if (*resolved) { + process->deleteLater(); + return; + } + *resolved = true; + timeoutTimer->stop(); timeoutTimer->deleteLater(); const QByteArray rawOutput = process->readAll(); - *outputSize += rawOutput.size(); - const QString output = sanitizeOutput(QString::fromUtf8(rawOutput), *outputSize); + const qint64 outputSize = rawOutput.size(); + const QString output = sanitizeOutput(QString::fromUtf8(rawOutput), outputSize); const QString fullCommand = args.isEmpty() ? command : QString("%1 %2").arg(command).arg(args); @@ -244,7 +258,7 @@ QFuture ExecuteTerminalCommandTool::executeAsync(const QJsonObject &inp LOG_MESSAGE(QString("ExecuteTerminalCommandTool: Command '%1' completed " "successfully (output size: %2 bytes)") .arg(fullCommand) - .arg(*outputSize)); + .arg(outputSize)); promise->addResult( QString("Command '%1' executed successfully.\n\nOutput:\n%2") .arg(fullCommand) @@ -254,7 +268,7 @@ QFuture ExecuteTerminalCommandTool::executeAsync(const QJsonObject &inp "exit code %2 (output size: %3 bytes)") .arg(fullCommand) .arg(exitCode) - .arg(*outputSize)); + .arg(outputSize)); promise->addResult( QString("Command '%1' failed with exit code %2.\n\nOutput:\n%3") .arg(fullCommand) @@ -265,7 +279,7 @@ QFuture ExecuteTerminalCommandTool::executeAsync(const QJsonObject &inp LOG_MESSAGE(QString("ExecuteTerminalCommandTool: Command '%1' crashed or was " "terminated (output size: %2 bytes)") .arg(fullCommand) - .arg(*outputSize)); + .arg(outputSize)); const QString error = process->errorString(); promise->addResult( QString("Command '%1' crashed or was terminated.\n\nError: %2\n\nOutput:\n%3") @@ -278,11 +292,13 @@ QFuture ExecuteTerminalCommandTool::executeAsync(const QJsonObject &inp process->deleteLater(); }); - QObject::connect(process, &QProcess::errorOccurred, [process, promise, command, args, timeoutTimer]( + QObject::connect(process, &QProcess::errorOccurred, [process, promise, resolved, command, args, timeoutTimer]( QProcess::ProcessError error) { - if (promise->future().isFinished()) { + if (*resolved) { + process->deleteLater(); return; } + *resolved = true; timeoutTimer->stop(); timeoutTimer->deleteLater(); @@ -292,7 +308,7 @@ QFuture ExecuteTerminalCommandTool::executeAsync(const QJsonObject &inp .arg(fullCommand) .arg(error) .arg(process->errorString())); - + QString errorMessage; switch (error) { case QProcess::FailedToStart: @@ -318,71 +334,46 @@ QFuture ExecuteTerminalCommandTool::executeAsync(const QJsonObject &inp .arg(process->errorString()); break; } - + promise->addResult(QString("Error: %1").arg(errorMessage)); promise->finish(); process->deleteLater(); }); - QString fullCommand = command; + QStringList argsList; if (!args.isEmpty()) { - fullCommand += " " + args; + argsList = QProcess::splitCommand(args); } #ifdef Q_OS_WIN static const QStringList windowsBuiltinCommands = { - "dir", "type", "del", "copy", "move", "ren", "rename", + "dir", "type", "del", "copy", "move", "ren", "rename", "md", "mkdir", "rd", "rmdir", "cd", "chdir", "cls", "echo", "set", "path", "prompt", "ver", "vol", "date", "time" }; - + const QString lowerCommand = command.toLower(); const bool isBuiltin = windowsBuiltinCommands.contains(lowerCommand); - + if (isBuiltin) { LOG_MESSAGE(QString("ExecuteTerminalCommandTool: Executing Windows builtin command '%1' via cmd.exe") .arg(command)); - process->start("cmd.exe", QStringList() << "/c" << fullCommand); + QStringList cmdArgs; + cmdArgs << "/c" << command; + cmdArgs.append(argsList); + process->start("cmd.exe", cmdArgs); } else { -#endif - QStringList splitCommand = QProcess::splitCommand(fullCommand); - if (splitCommand.isEmpty()) { - LOG_MESSAGE("ExecuteTerminalCommandTool: Failed to parse command"); - promise->addResult(QString("Error: Failed to parse command '%1'").arg(fullCommand)); - promise->finish(); - process->deleteLater(); - timeoutTimer->deleteLater(); - return future; - } - - const QString program = splitCommand.takeFirst(); - process->start(program, splitCommand); -#ifdef Q_OS_WIN + process->start(command, argsList); } +#else + process->start(command, argsList); #endif - if (!process->waitForStarted(PROCESS_START_TIMEOUT_MS)) { - LOG_MESSAGE(QString("ExecuteTerminalCommandTool: Failed to start command '%1' within %2ms") - .arg(fullCommand) - .arg(PROCESS_START_TIMEOUT_MS)); - const QString errorString = process->errorString(); - promise->addResult(QString("Error: Failed to start command '%1': %2\n\n" - "Possible reasons:\n" - "- Command not found in PATH\n" - "- Insufficient permissions\n" - "- Invalid command syntax") - .arg(fullCommand) - .arg(errorString)); - promise->finish(); - process->deleteLater(); - timeoutTimer->deleteLater(); - return future; - } - - LOG_MESSAGE(QString("ExecuteTerminalCommandTool: Process started successfully (PID: %1)") - .arg(process->processId())); - timeoutTimer->start(); + + LOG_MESSAGE(QString("ExecuteTerminalCommandTool: Process start requested for '%1'") + .arg(command)); + return future; } @@ -414,19 +405,27 @@ bool ExecuteTerminalCommandTool::areArgumentsSafe(const QString &args) const return true; } + // Check for null bytes + if (args.contains(QChar('\0'))) { + LOG_MESSAGE("ExecuteTerminalCommandTool: Null byte found in args"); + return false; + } + static const QStringList dangerousPatterns = { ";", // Command separator - "&&", // AND operator - "||", // OR operator + "&", // Command separator / background execution "|", // Pipe operator ">", // Output redirection - ">>", // Append redirection "<", // Input redirection "`", // Command substitution "$(", // Command substitution - "$()", // Command substitution - "\\n", // Newline (could start new command) - "\\r" // Carriage return + "${", // Variable expansion + "\n", // Newline (could start new command) + "\r", // Carriage return +#ifdef Q_OS_WIN + "^", // Escape character in cmd.exe (can bypass other checks) + "%", // Environment variable expansion on Windows +#endif }; for (const QString &pattern : dangerousPatterns) { @@ -456,9 +455,6 @@ QString ExecuteTerminalCommandTool::sanitizeOutput(const QString &output, qint64 QStringList ExecuteTerminalCommandTool::getAllowedCommands() const { - static QString cachedCommandsStr; - static QStringList cachedCommands; - QString commandsStr; #ifdef Q_OS_LINUX @@ -471,28 +467,27 @@ QStringList ExecuteTerminalCommandTool::getAllowedCommands() const commandsStr = Settings::toolsSettings().allowedTerminalCommandsLinux().trimmed(); // fallback #endif - if (commandsStr == cachedCommandsStr && !cachedCommands.isEmpty()) { - return cachedCommands; - } - - cachedCommandsStr = commandsStr; - cachedCommands.clear(); - if (commandsStr.isEmpty()) { return QStringList(); } + QStringList result; const QStringList rawCommands = commandsStr.split(',', Qt::SkipEmptyParts); - cachedCommands.reserve(rawCommands.size()); - + result.reserve(rawCommands.size()); + for (const QString &cmd : rawCommands) { const QString trimmed = cmd.trimmed(); if (!trimmed.isEmpty()) { - cachedCommands.append(trimmed); + result.append(trimmed); } } - return cachedCommands; + return result; +} + +int ExecuteTerminalCommandTool::commandTimeoutMs() const +{ + return Settings::toolsSettings().terminalCommandTimeout() * 1000; } QString ExecuteTerminalCommandTool::getCommandDescription() const @@ -518,7 +513,7 @@ QString ExecuteTerminalCommandTool::getCommandDescription() const "Commands have a %2 second timeout. " "Returns the command output (stdout and stderr) or an error message if the command fails.%3") .arg(allowedList) - .arg(COMMAND_TIMEOUT_MS / 1000) + .arg(commandTimeoutMs() / 1000) .arg(osInfo); } diff --git a/tools/ExecuteTerminalCommandTool.hpp b/tools/ExecuteTerminalCommandTool.hpp index 75fdc92..27b04be 100644 --- a/tools/ExecuteTerminalCommandTool.hpp +++ b/tools/ExecuteTerminalCommandTool.hpp @@ -46,12 +46,12 @@ private: QString getCommandDescription() const; QString sanitizeOutput(const QString &output, qint64 maxSize) const; + int commandTimeoutMs() const; + // Constants for production safety - static constexpr int COMMAND_TIMEOUT_MS = 30000; // 30 seconds static constexpr qint64 MAX_OUTPUT_SIZE = 10 * 1024 * 1024; // 10 MB static constexpr int MAX_COMMAND_LENGTH = 1024; static constexpr int MAX_ARGS_LENGTH = 4096; - static constexpr int PROCESS_START_TIMEOUT_MS = 3000; }; } // namespace QodeAssist::Tools