-
Notifications
You must be signed in to change notification settings - Fork 255
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
Issue: Trusted strings, shell escaping and backwards compatibility. #333
Comments
Hmm, I am not familiar with taint/untaint in Ruby, so I don't know what is involved. Could this be done like HTML escaping in Rails views, where strings are assumed to be unsafe unless The purist in me says that SSHKit should do no escaping at all, and then it is up to users to explicitly escape what needs to be, but I realize that this is an unrealistic (and unsafe) expectation. Like you say, the best compromise is probably to do the best automatic escaping we can, and give users the ability to opt-out on a per-string basis if they know what they're doing. |
I believe this was once implemented (or, may still be) in terms of tainted/untained. There are some simple rules, for example if you load something from a file ( Unfortunately in our case it would be difference, since it's all in If the default were "no value" we could compare it against the shellwords escaped and trust/untrust it if they differ. If the user has specified a value ahead of time then we Another option would be to use refinements, and add something like tainted/untained to Strings within SSHKit ourselves. |
I looked into this, |
Simple test-cases: require 'test_helper'
class Shellwords::Escape::RefinementTest < Minitest::Test
using Shellwords::Escape::Refinement
def test_trusting_a_string_that_is_equal_to_it_s_escaped_value
assert "helloworld".escaped?
end
def test_not_trusting_a_string_that_is_equal_to_it_s_escaped_value
refute "hello & world".escaped?
end
def test_trusting_a_string_that_is_not_equal_to_it_s_escaped_value_but_force_trusted
assert "hello & world".escaped!.escaped?
end
end And the implementation... module Shellwords
module Escape
module Refinement
refine String do
def escaped!
@_sw_escaped = true
self
end
def escaped?
case @_sw_escaped
when nil then self == Shellwords.shellescape(self)
else @_sw_escaped
end
end
end
end
end
end Refinements might be the wrong hammer here, I'm still trying to work out how the API would look. We would have to call I think someone who wanted to override these would have to pull in the refinement via |
I like where this is going! Couple more ideas:
|
Probably both wise decisions, in the end we can choose (once) Freezing the string does seem like a decent idea incase of naming it safe. |
Agreed. /cc @websi |
@ncreuschling @mattbrictson /cc
Continuing from the discussion in #283 I'd like to raise the general issue of shell escaping (
shellwords
) and what we mean/expect about "escaping strings" and so forth, whilst closing #283 it came to my mind that Rails usingObject#taint
,Object#tainted?
and family works mostly pretty well.I want to unify behavior.
I would propose the following:
test()
,capture()
,execute()
etc (via the commonexecute()
function)with(){}
default_env
tainted
or not by default in the Capistrano context.I'm afraid that strings coming from the same file that is running will be trusted by default, so the whole plan falls apart, I'd have to look more closely at the API to see if the default taint value for a string is tainted, untainted, or (ideally) none.
Ideally, this would mean, I could do something like this:
This would lead to us calling a literal
export $MY_LITTLE_PONY=i should however be escaped
, for better or worse.The text was updated successfully, but these errors were encountered: