-
-
Notifications
You must be signed in to change notification settings - Fork 645
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cider-connect
improvements
#3410
Conversation
(if (eq system-type 'windows-nt) | ||
port-string | ||
(when (not (equal "" | ||
(shell-command-to-string (concat "lsof -i:" port-string)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know the possibility of not having lsof
installed on a Linux machine is low (maybe it comes by default in OSX, but that is not the case in every Linux distribution). But if it isn't installed, shell-command-to-string
will return a non-empty string with an error similar to:
"/bin/bash: line 1: lsof: command not found
"
which will make the above code assume that port-string
is a "alive", even if we couldn't check that it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review!
I actually considered that.
If there's no lsof
installed, the only reasonable option we have is to allow the port, instead of excluding it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't splicing arbitrary file contents into a shell command in this manner introduce some sort of vulnerability?
Not that I have a specific threat model in mind, but it might at least help to sanitize the input (eg. matching against a basic "^\\d+$"
regexp)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do that, thanks!
Most of all it would seem a good way to ensure the command being run is reasonable (e.g. not an empty string or non-numeric contents)
The changes look good, but your branch has to be rebased on top of the current |
9de86cf
to
4b49255
Compare
Thanks 🍻 |
.nrepl-port
-like files for livenesscider-connect
improvements #3408cider-locate-running-nrepl-ports
Cheers - V