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

Postgres rake fixes #52

Merged
merged 4 commits into from
Oct 6, 2013
Merged

Conversation

nirvdrum
Copy link

@nirvdrum nirvdrum commented Oct 5, 2013

No description provided.

…buntu where pg_wrapper doesn't work properly.
… handler so we invoke the commands with the correct arguments.
@JonathanTron
Copy link
Member

Hi @nirvdrum, thanks for the pull-request. The problem with using backticks and joining the commands strings is that the result is not escaped correctly. In Ruby > 1.9 there is a Shellwords.shellescape method, but it's not present in Ruby 1.8.

It's great that it works with backticks, but if we're going to use it we need to be sure the commands are correctly escaped. Granted we want to keep the Ruby 1.8.x compatibility, the best way to do it would probably to vendor https://github.com/akr/escape (which does not exists as a gem, at least with a recent version) and create a wrapper to use it under Ruby 1.8.x and use the built Shellwords.shellescape when run under Ruby 1.9+.

What do you think?

@nirvdrum
Copy link
Author

nirvdrum commented Oct 5, 2013

That's fair. I assumed since this was all configured input rather than generated from random 3rd parties that the security concerns would be minimal. An alternative proposal is to just try to load Shellwords and if it fails, call system, otherwise escape and use backticks. That would retain 1.8 compatibility without introducing a new dependency. Obviously this would still present the pg_wrapper problem on 1.8, but that'd be no worse a situation than you have today.

@JonathanTron
Copy link
Member

Yes the reason is not so much about security risk than problem related to database or user/owner names.

…ted value between backticks and :system calls.
@nirvdrum
Copy link
Author

nirvdrum commented Oct 5, 2013

@JonathanTron Let me know what you think of the latest commit.

@JonathanTron
Copy link
Member

That's excellent! Thanks!

JonathanTron added a commit that referenced this pull request Oct 6, 2013
@JonathanTron JonathanTron merged commit 1e86e26 into TalentBox:master Oct 6, 2013
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.

2 participants