Skip to content
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

Support custom mypy path #95

Open
achimnol opened this issue Nov 21, 2024 · 8 comments
Open

Support custom mypy path #95

achimnol opened this issue Nov 21, 2024 · 8 comments

Comments

@achimnol
Copy link

achimnol commented Nov 21, 2024

In a monorepo setting using Pantsbuild, various Python tools are installed in separate virtaulenvs like:

  • dist/export/python/virtualenvs/mypy/3.12.6/bin/mypy
  • dist/export/python/virtualenvs/ruff/3.12.6/bin/ruff

while the main source tree looks at (while it is also possible to have per-subdir lock files):

  • dist/export/python/virtualenvs/python-default/3.12.6/bin/python

I can override the --python-executable argument passed to mypy for the main virtualenv, but I cannot override the mypy executable path itself.
It seems to be hardcoded here:

if shutil.which("mypy"):
# mypy exists on path
# -> use mypy on path
log.info("executing mypy args = %s on path", args)
completed_process = subprocess.run(
["mypy", *args], capture_output=True, **windows_flag, encoding="utf-8"
)

Could we make it something configurable?

@Richardk2n
Copy link
Member

This is in no way hardcoded. These lines just mean "pick whatever mypy is on PATH", which can easily be changed by changing PATH.

If you can propose a way to make it configurable, that does not present a significant security risk, I am open to it.
Simply providing a config option would mean, that a malicious repo could make you execute malicious code by opening its source files in your editor.

@achimnol
Copy link
Author

I have multiple Python projects to work on, where each project have distinct configurations:

  • mypy is embedded in the virtualenv
  • mypy should be taken from a special location like the above case

Technically it is possible to switch PATH using something like direnv, but it becomes quickly cumbersome to do so in Zed, Neovim, VSCode, etc.

@achimnol
Copy link
Author

Also, the sibling ruff plugin support the custom executable path as mentioned in the PR. Why not adding this? 😉

@Richardk2n
Copy link
Member

Another plugin having a security issue does not compel me to include one in mine.

The newest commit contains an option how you can risk your computer without endangering others.

@achimnol
Copy link
Author

achimnol commented Nov 26, 2024

@Richardk2n Hm... Could you elaborate concrete scenarios that having a customizable Python tool executable path becomes a security issue?

For instance, are you concerned with CI executions in public repositories where people can send malicious PRs that modifies the configuration? If so, I think the same could still happen with the PATH environment variable by modifying CI configurations or adding additional scripts, etc. in the PRs. I don't get the difference between PATH env-var based customization and the config-based customization in the perspective of security.

AFAIK, GitHub offers an option to "approve the run of CI" for PRs from unforeseen people as a mitigation and I'm pretty sure other open-source collaboration tools provide similar options. Wouldn't this be sufficient?

@Richardk2n
Copy link
Member

I would not expect anyone to use this in CI. Still a PR attack is theoretically possible. But modifying the CI config is probably a lot more suspicious than a config file. This is not the scenario I had in mind.

What I am thinking about is the scenario of you just cloned a repo and have opened it in your IDE to look at the code. With this feature, you have just given the repo execute permission on your system. In case of an untrustworthy repo (or a malicious maintainer), merely looking at the code can lead to malicious code being executed.

@achimnol
Copy link
Author

achimnol commented Nov 26, 2024

Thanks for the detailed explanation. Now I get the point.

What I'm trying to achieve is to configure my shiny new Zed editor with pylsp working with a mypy in custom location like dist/export/python/virtualenvs/mypy/3.12.6/bin/mypy generated by the Pantsbuild system for a mono-repo.

The problem is:

  • There is no way to inject the PATH env-var variable in the Zed editor configuration just for mypy execution by pylsp.
  • It is not convenient to change the PATH env-var for the entire Zed editor app because I would open multiple workspace folders with a single app instance, which may require different PATH configurations.
  • The main target scenario for me is to use the Zed editor with remote SSH development mode, so setting a local PATH variable would & should not propagate to the remote server.
  • Since I could open multiple projects, making a symbolic link like /usr/local/bin/mypy (or ~/.loca/bin/mypy) or setting a global PATH env-var in the remote server would not work as well.
  • I could just install mypy in a global Python setup or pyenv environments, but I'm using a special mypyc-based mypy build for linux-aarch64 to speed up typechecking with the 800+ files mono-repo. That mypy build is automatically populated to the dist/export/... location via Pantsbuild. It is also important to pin an exact mypy version for this project, while other Python projects may need different mypy versions.

I thought the simplest solution is just to set a specific location of the mypy executable in the per-project configuration in .zed/settings.json (which is part of .gitignore in the repo setting; it's not there yet because I'm testing the configurability). Actually, I didn't intend that people would place such configuration in a repository but as a private configuration. But I now understand your concern about such case by malicious repositories.

e.g., https://docs.backend.ai/en/latest/dev/daily-workflows.html#vim-neovim
→ I'd like to make a Zed version of this guide.
For my Zed case, the only missing piece needed to complete the guide is the custom mypy executable path configurability of the pylsp plugin.

It seems that other LSP-capable editors like VSCode and NeoVim have similar issues and often just offers a way to add a local private configuration to customize Python tool executable locations in LSP configurations. I think that's why VSCode enters the restricted mode when opening a new, unforeseen filesystem location. (If all LSP-capable editors provides this feature, then would you agree with my addition? Maybe, this restricted-mode behavior should be included in the LSP specification....)

Could you suggest me a good secure alternative in this case? How would you configure your IDE/editor in such case?

@achimnol
Copy link
Author

The challenge here is that it would be ideal if all LSP-capable editors have the restricted mode for the concerned scenario. Unfortunately, not all editors in the reality have such feature and it prevents me from trying those new editors even at my own risk and supervision. 😞

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 a pull request may close this issue.

2 participants