Adjust SSH heredoc wrapper#998
Conversation
|
To test this I literally need to test the entire vito. I don't have time for it unfortunately at the moment |
|
Hey @saeedvaziry 👋 Can you review / merge when you get a chance? Thanks! 👍 |
There was a problem hiding this comment.
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::execto wrap commands forasUserusingsudo -u <user> bash <<'EOF'instead of base64-encoding and nestedbash -ccalls. - 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.
| sudo -u {$this->asUser} bash <<'EOF' | ||
| {$command} | ||
| EOF | ||
| BASH; |
There was a problem hiding this comment.
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.
| sudo -u {$this->asUser} bash <<'EOF' | |
| {$command} | |
| EOF | |
| BASH; | |
| sudo -u {$this->asUser} bash <<'EOF' | |
| {$command} | |
| EOF | |
| BASH; |
| $command = <<<BASH | ||
| sudo -u {$this->asUser} bash <<'EOF' | ||
| {$command} | ||
| EOF | ||
| BASH; |
There was a problem hiding this comment.
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.
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.