Use string interpolation for environment variables to avoid escaping issues with sprintf

[Sinjo]

@leehambley Following on from the discussion in #264.

user also runs the environment variables through sprintf, so I've applied the same fix there.

[Sinjo]

I've not been able to run the functional tests. Vagrant is estimating 2h30 to download hashicorp/precise64. I'm on a good connection, so I assume something's up at the host.

[Sinjo]

Tried again this morning - much faster download. Functional tests are 👍.

[Sinjo]

@leehambley Sorry to nudge - any thoughts on this? I'll bring it up to date if you're 👍 :)

[leehambley]

There's a bit of disagreement about how to do shell escaping. Check the issue list, someone is holding up the example that we escape parts of the env vars, and therefore we should do the same in command names and arguments. The problem is though there's no way to do it right, so I'm tempted to just not do it at all, and have people use the lower level APIs and call shell words escape themselves.

[Sinjo]

That's fair. I suspect trying to escape entire commands would just result in pain (e.g. how do you know if a user wants a character to be interpreted literally or not).

In the case of this PR, I felt it was justified as it's escaping for an internal use of printf, rather than trying to second-guess the command the user wants to run.

---

Going back to environment variables - the reason I wrote that #250 is that default_env is the only way I could find to ensure the environment was set correctly for deployment commands. Other things I'd considered were:

• Use a login shell - unsupported in Capistrano 3
• SSH's PermitUserEnvironment, which is subject to security issues

I've been doing some thinking since then, and maybe there's a third option which gets SSHKit out of this situation of trying to escape/quote things correctly.

How would you feel about an option like default_env, but which let you specify a file to be sourced on the remote server? I get that it forces people to do a little more in their config management system, but it would avoid SSHKit having to understand how to correctly format environment variables.

Would love to hear your thoughts.

[leehambley]

Use a login shell - unsupported in Capistrano 3

I'll start here, since this could be the crux of the problem! Actually Cap2 allowed you to define the shell, that included sh -l or similar, I'd actually think the sane way to implement this would be extending the command map to support prefixes and suffixes equally, and then expanding upon that to allow "wrapping" certain commands, which may include a default wrapper that is a bash -l -c '....' s shell, which would effectively be a prefix of bash -l -c ' and a suffix of ', of course the shell escaping requirements here shoot through the roof, and it's HARD to get right (actually you can't escape single quotes in bash this way, you have to end the string, and concatenate it with differently quoted strings, yikes.)

This, though I believe would solve all the issues, as you could wrap Cap in a login shell, wrap cap in a source ./my-thing command, or anything else, really!

Your thoughts?

[Sinjo]

Sounds spot on!

As you point out, there's really subtle escaping to get right with that approach, but it seems flexible enough to cover a lot of use-cases people have. If it's doable, it feels like the right thing(™).

I'd love to help out if you can excuse my free time being quite sporadic. Is a wiki page the place to start? Would be nice to have the feature written out and start coming up with a list of problems/edge-cases to tackle.

[leehambley]

I'd love to help out if you can excuse my free time being quite sporadic. Is a wiki page the place to start? Would be nice to have the feature written out and start coming up with a list of problems/edge-cases to tackle.

No worries, mine too. You'll find cloning SSHKit and running rake is a decent place to start, our test suite is wicked fast (at least, the unit tests)... Beyond that the command map tests themselves are really, really simple and could easily extended to do what you want https://github.com/capistrano/sshkit/blob/master/test/unit/test_command_map.rb

I could imagine a new part of the command map, not prefix/suffix but "Around" maybe which would take two args and then internally call prefix/suffix, if that was done, it would be quite easy to extend Cap (let's see about SSHKit first) to treat an option like :default_shell as calling into SSHKit to setup a fallback prefix and suffix for everything.

Caveats I see:

• We don't have a concept of a suffix, but it'd be easy to add
• Maybe the around is unnecessary, and maybe you want to unshift a prefix, but push a suffix (wrapping?
• Race conditions, what if plugins from Cap are pushing/unshifting prefixes/paths
• What about commands that don't need/want a custom shell (I can't see a bash login shell being too harmful, actually)

Heads-up: The default hashes on the command map use this slightly opaque technique http://ruby-doc.org/core-1.9.3/Hash.html#method-c-new described in more detail in the README, but quoted here:

One can also override the command map completely, this may not be wise, but it would be possible, for example:
SSHKit.config.command_map = Hash.new do |hash, command|
hash[command] = "/usr/local/rbenv/shims/#{command}"
end

Finally, you'll also have to check the Command tests, but they're pretty damned trivial - https://github.com/capistrano/sshkit/blob/master/test/unit/test_command.rb#L7-L17

[Sinjo]

So apologies for the year's gap, but this just appeared on my todo list again as we're maintaining an internal monkey-patch that applies this change.

The code in this PR feels kinda orthogonal to fixing escaping of environment variables - it's escaping strings for reasons internal to the code, rather than for bash.

Do you have any thoughts on merging this in the interim? I'm happy to rebase and re-run tests.

[mattbrictson]

I'm late to the game on this issue, but perhaps a simpler solution would be to remove sprintf entirely. In other words, replace this:

sprintf("( export #{environment_string} ; %s )", yield)

with this:

"( export #{environment_string} ; #{yield} )"

Are there any subtle differences between sprintf("%s", ...) and "#{...}" that we need to worry about?

[leehambley]

None, I don't think.

(Sent from my Nexus 6, please forgive typos!)

On Oct 3, 2016 21:16, "Matt Brictson" notifications@github.com wrote:

I'm late to the game on this issue, but perhaps a simpler solution would
be to remove sprintf entirely. In other words, replace this:

sprintf("( export #{environment_string} ; %s )", yield)

with this:

"( export #{environment_string} ; #{yield} )"

Are there any subtle differences between sprintf("%s", ...) and "#{...}"
that we need to worry about?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#280 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABKCFXovRQo8CV5J2ic624P-WDXPQQVks5qwVSIgaJpZM4F_T_I
.

[mattbrictson]

@Sinjo What do you think about changing this PR to use the #{...} approach? I think your test case is still useful. With this change (and an appropriate change to the CHANGELOG), I would be happy to merge.

[Sinjo]

It sounds good to me! Updating the PR now.

[mattbrictson]

Thanks! ✨

[Sinjo]

All done, rebased, tests still passing. Would you like those commits squashed before merge?

[Sinjo]

Ah, never mind. :P

[mattbrictson]

Yep, I took care of the squashing. 👍