only use arguments on suda#executable if it's equal to "sudo" #60
Conversation
|
Just tested, it doesn't pass the password to doas, and 2 failed attempts it somehow counted as 3 |
|
The password is passed through stdin, but This will only work with |
|
One tool that does work with this change is |
|
The difference is that |
This is true, but as I've already said, unless you can discover a way to pass the password to doas, we can only support any tools that accept the password through stdin (like, |
|
My apologies for the delay. I completely overlooked this matter. Could you please resolve the conflicts first so we can proceed with the review? @aarondill |
lambdalisue
requested review from
lambdalisue
and removed request for
lambdalisue
This ensures that the sudo options work correctly, and creates *rudimentary* support for other commands (such as `doas`, see lambdalisue#40)
This helps avoid the printf('%s %s') which is repetative and unclear. get_command now returns the *full* command to run.
WalkthroughThis update introduces a more sophisticated approach to command construction and execution in Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- autoload/suda.vim (1 hunks)
Additional comments: 2
autoload/suda.vim (2)
- 11-45: In the
suda#systemfunction, there's a significant change in how commands are constructed and executed, particularly with the introduction of conditional logic to handle password prompts interactively. This change is crucial for supporting different privilege escalation commands beyondsudo, such asdoasand potentiallysu.However, there are a few points to consider:
- The logic to determine whether to ask for a password non-interactively (lines 23-37) is specific to
sudoand relies on thesudotimestamp feature. This is a clever use ofsudo's behavior but may not be transparent to users unfamiliar withsudo's configuration nuances.- The handling of passwords (lines 38-45) assumes that the command can accept a password via stdin, which, as noted in the PR comments, does not work with
doasin its default configuration. This limitation should be clearly documented to manage user expectations.Consider adding detailed comments or documentation to explain the specific behaviors and limitations of the
suda#systemfunction, especially regarding the non-interactive password logic and its compatibility with different privilege escalation tools. This will help users understand how to configure their systems for optimal compatibility withsuda.vim.
- 1-48: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [46-118]
The changes in the
suda#read,suda#write,suda#BufReadCmd,suda#FileReadCmd,suda#BufWriteCmd, andsuda#FileWriteCmdfunctions seem to be primarily focused on integrating the new command construction and execution logic introduced earlier. These changes are consistent with the PR's objectives to refine the behavior of the plugin when using different command executables for privilege escalation.However, it's important to ensure that these changes do not introduce any regressions or unexpected behaviors, especially in edge cases such as handling file paths with special characters, dealing with large files, or operating in environments with strict security policies.
Ensure thorough testing of these functions in various scenarios, including with file paths containing special characters, large files, and different operating system environments. Consider adding automated tests to cover these cases if not already present.
| function! s:get_command(opts, cmd) | ||
| " TODO: should we pass '--' between a:opts and a:cmd? | ||
| " TODO: should we change this api to use lists? system() allows either | ||
| " strings or lists. We don't need a intermediate shell for anything though. | ||
| " TODO: Should we move shell escaping to the responsibility of | ||
| " suda#system/s:get_command to avoid forgetting it at the call site? | ||
| return g:suda#executable ==# "sudo" && len(a:opts) > 0 | ||
| \ ? printf('%s %s %s', g:suda#executable, a:opts, a:cmd) | ||
| \ : printf('%s %s', g:suda#executable, a:cmd) | ||
| endfunction |
There was a problem hiding this comment.
The new s:get_command function introduces a flexible way to construct commands based on the g:suda#executable variable. However, there are several TODO comments that raise important questions about the function's design and implementation:
- The consideration of passing
'--'betweena:optsanda:cmdto signify the end of command options. This is a best practice for security, ensuring that subsequent strings are not interpreted as options. - The suggestion to change the API to use lists instead of strings for
system()calls. This would indeed avoid the need for an intermediate shell, which can enhance security and performance. - The idea of moving shell escaping to the responsibility of
suda#system/s:get_commandto avoid forgetting it at the call site. This could significantly reduce the risk of shell injection vulnerabilities.
Each of these points merits careful consideration. The first point is particularly important for security, as it prevents potential command-line argument injection. The second and third points could improve the maintainability and security of the code by reducing complexity and the risk of errors.
Consider implementing these TODO items, especially the use of '--' to separate options from commands and the transition to list-based APIs for system calls. Additionally, centralizing shell escaping would enhance security and reduce the risk of injection vulnerabilities.
There was a problem hiding this comment.
While I personally think we should pass -- (at least for sudo), this isn't present in the original source, so I'll leave this up to you, @lambdalisue.
As to the other 2 TODOs that I left, I'd like to implement these, if you'd allow me to; however, they are both out of scope of this PR. I'll implement them after this PR is merged, if you'd merge it.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
@lambdalisue I've rebased this onto HEAD. |
|
Thanks a lot 🎉 |
This ensures that the sudo options work correctly, and creates rudimentary support for other commands (such as
doas, see #40)This isn't perfect, but likely the best we can do (see my comment).
This change allows any command which accepts no password, or else a password from stdin to work. Any command set to
g:suda#executablewill be executed with no further arguments (so the user may need to provide any needed), and will be run, first with an empty stdin, then with the user's provided password as stdin. Further support would likely need to be added on a case-by-case basis and may quickly spiral out of control.Summary by CodeRabbit