Skip to content

Conversation

@alokmenghrajani
Copy link

Kudos to Blake Burkhart for pointing this out.

Kudos to Blake Burkhart for pointing this out.
@longboardcat
Copy link
Contributor

LGTM

longboardcat added a commit that referenced this pull request Dec 12, 2015
@longboardcat longboardcat merged commit 14198fe into square:master Dec 12, 2015
@bburky
Copy link

bburky commented Dec 13, 2015

I'd suggest instead setting the 'GIT_ALLOW_PROTOCOL' environment variable to 'file:git:http:https:ssh' which should reuse git's newly introduced protocol whitelisting, as done in git/git@33cfccb. And if the user happens to have GIT_ALLOW_PROTOCOL already set in their environment try to preserve it if you can.

You should be able to do that with either Cocaine::CommandLine.environment['GIT_ALLOW_PROTOCOL'] = 'file:git:http:https:ssh' or globally with ENV['GIT_ALLOW_PROTOCOL'] = 'file:git:http:https:ssh'.

The main reason to use a whitelist is that it's more future proof, what if git introduces another strangely behaving protocol in the future? Or what if the user has some other locally installed custom remote helpers installed that can be abused?

Also git fastclone ext::foo should still work. All you should be preventing is use of the ext protocol in submodules. That's the new behavior of git-submodule and you probably seem to be trying to match it's behavior.

@alokmenghrajani
Copy link
Author

Agree. See #3.

Note: git fastclone ext::foo will only work if ext is in the whitelist of protocols. Differentiating between the module being cloned and the submodule might require a larger code change.

@bburky
Copy link

bburky commented Dec 14, 2015

Thanks, I just commented on #3 to say it looks good.

Yes, it seems it might require a significant code change to preserve git fastclone ext::foo working. I think this is a very seldom used feature so that's probably fine. I just thought I'd point out that you've diverged from Git's normal behavior a bit.

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.

3 participants