pantheon-deprecated / terminus-deprecated

Deprecated CLI for Pantheon. Do not install. Current project:
https://github.com/pantheon-systems/cli
82 stars 24 forks source link

Drush 6.4 passes COLUMNS environment var, is rejected by Pantheon #174

Closed damienmckenna closed 10 years ago

damienmckenna commented 10 years ago

I've noticed that my terminal, being 120 chars wide, forces the COLUMNS=120 environmental variable to be passed along with the Drush command via Drush aliases, which then causes the remote Drush command to fail with the message:

Command not supported as typed.

If I copy the ssh command and re-run it without the COLUMNS=120 value it can work just fine (excluding some other issues)

damienmckenna commented 10 years ago

Is there any way to assign this value to either FALSE or 0 via an alias fil?

joshkoenig commented 10 years ago

The real question is why this is being passed along at all. It shouldn't be. Can you gist or paste in the SSH command that's being generated which breaks?

Also, nice seeing you in Amherst. I'm sorry we didn't get a chance to actually chat; coulda/shoulda hacked on this stuff in person!

damienmckenna commented 10 years ago

Yeah, sorry we didn't really get to meet - next time!

The command I'm running is: drush @pantheon.example.dev st -v

That results in the following:

Loaded alias @pantheon.example.dev from file /Users/me/.drush/pantheon.aliases.drushrc.php                   [notice]
Begin redispatch via drush_invoke_process().                      [notice]
Calling proc_open(ssh -p 2222 -o "AddressFamily inet" dev.account@appserver.dev.account.drush.in
 'COLUMNS=120 drush6  --verbose --uri=dev-example.gotpantheon.com --root=.  core-status 2>&1'
 2>&1);
Command not supported as typed.
End redispatch via drush_invoke_process().                  [notice]

and the actual ssh command ends up being:

ssh -p 2222 -o "AddressFamily inet" dev.account@appserver.dev.account.drush.in
 'COLUMNS=120 drush6  --verbose --uri=dev-example.gotpantheon.com --root=.
 core-status 2>&1'
damienmckenna commented 10 years ago

Strictly speaking, it's ok that the argument is being passed, the problem is that the Pantheon servers don't like it.

The problem only occurs if the terminal is wider than the default 80 chars, so to replicate the problem you just have to expand your terminal to something larger.

damienmckenna commented 10 years ago

FYI this is controlled by drush_build_drush_command() in environment.inc.

joshkoenig commented 10 years ago

Huh. I run a much larger terminal and have never seen this. Is this a new behavior in Drush?

It does make sense though: we filter commands through the drush.in gateway to prevent nefariousness, and starting a command with COLUMNS=120 will cause it to be rejected by the filter.

joshkoenig commented 10 years ago

Yeah looks like this was added recently.

https://github.com/drush-ops/drush/commit/1753643d1937bbba4ad5f87e1223adb67778ea0b

That's a bummer. I really would prefer not to have to add special exceptions to our command filter.

damienmckenna commented 10 years ago

Passing the COLUMNS argument along shouldn't cause too many problems, maybe just add filtering so it only fails if the argument is non-numeric?

joshkoenig commented 10 years ago

Passing COLUMNS is not a risk, but I am reluctant to make a complex filter. The old "I solved the problem with a regex, and now I have two problems."

I'll need to see if there are any libraries out there for parsing a command that can assist.

greg-1-anderson commented 10 years ago

Perhaps the COLUMNS thing should not have been committed on the 6.x branch. :( Even if COLUMNS are not passed, under certain circumstances, Drush may include other environment variables at the head of the command (e.g. PHP_OPTIONS).

This is just going to be lousy for 6.4, but perhaps we could make it better for 6.5 and later. My suggestion would be to add a global Drush security option that instructs Drush to never add environment variables to remote calls. That would require a little extra config on the part of your clients, but it would solve the problem.

Please open an issue in the Drush queue if you think this sort of thing would be helpful.

joshkoenig commented 10 years ago

I think we can work around it. I don't think it's common to have a custom SSH daemon screening the commands (we're an outlier there). Hopefully we'll have this nailed soon.

mikevanwinkle commented 10 years ago

FYI, I was able to successfully run commands adding this alias to my ~/.bashrc file: alias drush='export COLUMNS=80; drush'

For some reason drush doesn't send the environment var if the existing value is 80, presumably because this is the default anyway.

An alias isn't a fix but it might make your lives easier in the short term.

joshkoenig commented 10 years ago

This should now be fixed on the platform side. If we can get someone else to verify I'll close the issue.

damienmckenna commented 10 years ago

I've confirmed that the COLUMNS value is ignored, so commands which pass along the value will now work correctly. Thanks Josh & team!

That said: the COLUMNS value is ignored entirely - should it be, or would it be possible for it to be used?

joshkoenig commented 10 years ago

For now we are ignoring it; that was the quickest way to get things working. We'll take this into consideration in a forthcoming overhaul.

Thanks for confirming!