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

Add only trusted projects' bin directory to $PATH #191

Closed
wants to merge 1 commit into from

Conversation

croaky
Copy link
Contributor

@croaky croaky commented Sep 11, 2013

Assuming the binstubs for a project are in the local bin/ directory, you can
even go a step further to add the directory to shell $PATH so that rspec can
be invoked without the bin/ prefix:

export PATH="./bin:$PATH"

However, doing so on a system that other people have write access to
(such as a shared host) is a security risk.

rbenv/rbenv#309

@JoelQ
Copy link
Contributor

JoelQ commented Sep 11, 2013

@joshuaclayton Do you know if this breaks the Spring binstub?

@joshuaclayton
Copy link
Contributor

@JoelQ this commit specifically does not, no, since it's only adding another directory to search when looking for binaries.

The original change stems from https://twitter.com/tpope/status/165631968996900865 and requires you call mkdir .git/safe in every repo (gem or Rails app) you trust. The path change finds .git/safe, traverses up two directories, and then adds bin/ from there.

@mike-burns
Copy link
Member

I'm not opposed to this, but it will make setup more cumbersome. Perhaps something like this should go in bin/setup scripts:

print "Do you trust the ./bin directory for this project? [y/N] "
trusted = gets
if trusted.start_with?('y')
  puts "FileUtils.mkdir_p('.git/safe')"
end

(Maybe a check to make sure that .git/safe doesn't already exist before asking, too.)

@jferris
Copy link
Member

jferris commented Nov 20, 2013

If we add it to bin/setup, I think it would be fine to not prompt and just create the directory. You'd need to have ./.git/safe/../../bin on your PATH and run bin/setup before scripts were executable.

@croaky croaky mentioned this pull request Dec 31, 2013
@@ -21,3 +21,6 @@ export PS1='$(git_prompt_info)[${SSH_CONNECTION+"%{$fg_bold[green]%}%n@%m:"}%{$f

# load thoughtbot/dotfiles scripts
export PATH="$HOME/.bin:$PATH"

# mkdir .git/safe in the root of repositories you trust
export PATH=".git/safe/../../bin:$PATH"
Copy link
Member

Choose a reason for hiding this comment

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

Should this be in ~/.zshrc instead? Do we want this to run for non-login shells? @pbrisbin @mike-burns

Copy link
Member

Choose a reason for hiding this comment

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

To quote http://zsh.sourceforge.net/Intro/intro_3.html

.zlogin is not the place for alias definitions, options, environment variable settings, etc.; as a general rule, it should not change the shell environment at all. Rather, it should be used to set the terminal type and run a series of external commands (fortune, msgs, etc).

So no, don't change $PATH in here.

Copy link

Choose a reason for hiding this comment

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

I do it in ~/.zprofile.

Upstream's intentions are vague (IMO), but because Arch (and Fedora/Debian) puts PATH manipulation in /etc/zsh/zprofile, which is sourced after ~/.zshenv (my preference), I have to do it in ~/.zprofile or ~/.zshrc. I chose ~/.zprofile because I was sick of getting command-not-found errors in non-interactive settings.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we want this to be available in vim then we need to put it in ~/.zshenv, I believe.

Copy link
Member

Choose a reason for hiding this comment

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

I think that's true of MacVim on OS X, because of the way the environment is initialized. I'm not sure if that's specific to OS X or MacVim. Running vim from Terminal picks up the environment exported from the shell, so any config file is fine for terminal vim.

Copy link

Choose a reason for hiding this comment

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

From Mike's link:

`.zshenv' is sourced on all invocations of the shell, unless the -f option is set. It should contain commands to set the command search path...

I mentioned ~/.zprofile because OSX may have the same feature/bug as Arch. If the system-level zprofile (/etc/zsh/zprofile) sets a PATH, it will clobber anything you do in ~/.zshenv since it's sourced after. I also like that ~/.zprofile is sourced less frequently so moving what you can there will speed up terminal launch, by however small an amount.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neither zprofile nor zshenv seem to automatically work in OS X. exporting from the command line directly works. Any thoughts on how to get it to automatically work?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have this in zshrc on OS X and it works fine from vim for me. Note that I have also applied this zsh-fix

@croaky
Copy link
Contributor Author

croaky commented Jan 17, 2014

I moved this to zshenv. Ready for re-review. /cc @jferris @pbrisbin @mike-burns

Assuming the binstubs for a project are in the local bin/ directory, you
can even go a step further to add the directory to shell $PATH so that
rspec can be invoked without the bin/ prefix:

    export PATH="./bin:$PATH"

Doing so on a system that other people have write access to
(such as a shared host) is a security risk:

rbenv/rbenv#309

The `.git/safe` convention addresses the security problem:

https://twitter.com/tpope/status/165631968996900865

Put this in `zshenv` because:

http://zsh.sourceforge.net/Intro/intro_3.html

> `.zshenv' is sourced on all invocations of the shell, unless the -f
> option is set. It should contain commands to set the command search
> path.

Load `zshenv.local` config at the end of the file so that users can
extend their `zshenv` needs in their personal dotfiles using `rcup`.
@croaky
Copy link
Contributor Author

croaky commented Jan 23, 2014

I'm trying a different approach and including rbenv in the commit in #215. It's exactly @derekprior's approach and is working for me with no zshenv or zshenv.local files. I'm able to run specs from vim as well.

@mike-burns
Copy link
Member

@croaky I don't think I understand #215. It seems to be just like #191 (this PR), but limited only to rbenv?

@croaky
Copy link
Contributor Author

croaky commented Jan 23, 2014

@mike-burns In #191:

  • Loading .git/safe stuff is in zshenv
  • No rbenv

In #215:

  • Loading .git/safe bit is in zshrc
  • Adds rbenv
  • both .git/safe and rbenv bits wrapped in an "if rbenv exists" conditional

The rbenv part seemed related enough to try as a separate pull while keeping this version around to compare.

@croaky croaky closed this Jan 23, 2014
@croaky croaky deleted the dc-bin-path branch January 23, 2014 22:26
@croaky
Copy link
Contributor Author

croaky commented Jan 23, 2014

Merged #215 instead.

ericboehs added a commit to brightbit/bin_setup that referenced this pull request Jan 29, 2014
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.

None yet

8 participants