Skip to content

Conversation

@algethamy
Copy link
Contributor

@algethamy algethamy commented Jan 20, 2026

trying to fix this #781

What changed
• Switched to using bash with a quoted heredoc (<<'EOF') when executing commands as another user.
• Updated the related test case to assert the new heredoc-based command structure.
• Removed unnecessary base64 encoding/decoding from the execution flow.

Why this change
• Improves readability of executed commands.
• Avoids complex shell quoting and nested bash -c calls.
• Prevents unintended variable expansion by using a quoted heredoc.
• Makes the execution logic more robust and easier to reason about in tests.

Notes
• No functional behavior change in command execution logic, only the execution method.
• Tests were updated accordingly to reflect the new expected command format.

@algethamy algethamy changed the base branch from 2.x to 3.x January 20, 2026 06:13
@algethamy algethamy marked this pull request as draft January 20, 2026 10:29
@algethamy algethamy marked this pull request as ready for review January 20, 2026 10:29
@saeedvaziry
Copy link
Member

To test this I literally need to test the entire vito. I don't have time for it unfortunately at the moment

@algethamy
Copy link
Contributor Author

Hey @saeedvaziry 👋
Many existing test cases in the repo already use this function with the new approach and all of them pass, so the behavior is well-covered and stable. 

Can you review / merge when you get a chance? Thanks! 👍

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR changes how SSH commands are wrapped when executed as another user, moving from a base64+sudo su approach to a heredoc-based sudo -u wrapper, and adds a unit test to pin the new behavior.

Changes:

  • Update SSH::exec to wrap commands for asUser using sudo -u <user> bash <<'EOF' instead of base64-encoding and nested bash -c calls.
  • Add a unit test on the server model to verify the exact SSH command emitted when using a custom user.
  • Remove base64 encoding/decoding from the SSH command execution path.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
app/Helpers/SSH.php Reworks the exec method’s asUser path to use a quoted heredoc via sudo -u instead of a base64-encoded sudo su - ... -c command.
tests/Unit/Models/ServerModelTest.php Adds a unit test that asserts the new heredoc-based SSH command wrapper is used when executing as a custom user.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +174 to +177
sudo -u {$this->asUser} bash <<'EOF'
{$command}
EOF
BASH;
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The heredoc assigned to $command is indented inside the method, so the generated string will contain leading spaces before sudo -u ..., the embedded command, and EOF. In contrast, the new unit test in ServerModelTest expects those lines to start at column 1 with no leading spaces, so this mismatch will cause the assertion to fail and also changes the exact command sent over SSH. Consider left-aligning the heredoc contents (or otherwise normalizing whitespace) so that the produced command matches the expected format.

Suggested change
sudo -u {$this->asUser} bash <<'EOF'
{$command}
EOF
BASH;
sudo -u {$this->asUser} bash <<'EOF'
{$command}
EOF
BASH;

Copilot uses AI. Check for mistakes.
Comment on lines +173 to +177
$command = <<<BASH
sudo -u {$this->asUser} bash <<'EOF'
{$command}
EOF
BASH;
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new wrapper switches from sudo su - {user} -c ... to sudo -u {user} bash <<'EOF' ..., which changes how the target user's environment and working directory are initialized (login shell vs non-login shell semantics). This can impact commands that rely on the user's login profile, environment variables, or home-directory-relative paths, which conflicts with the claim that there is no functional behavior change in command execution; consider either preserving the previous login-shell behavior or documenting and verifying this behavioral change with additional tests.

Copilot uses AI. Check for mistakes.
@saeedvaziry saeedvaziry merged commit fbb595a into vitodeploy:3.x Jan 27, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants