Escape user name in Airbrake notify command #144

Merged
merged 1 commit into from Jun 21, 2013

Projects

None yet

2 participants

@jasonmp85

If a user has manually overridden this variable to be a full name, it
may contain a space which will break the command. Escaping the name
solves this issue.

This is an attempt to solve #103, which has been bothering me for almost a year. The main issue is that I don't want to use simple usernames because at a decentralized-enough company, those are basically user-chosen. I like using full, real names, but those break. This fixes it (at least in Unix).

@jasonmp85

I see recipes all over the place that don't properly escape user input in their shell commands. This could be problematic if someone's name is "; rm -rf /". In Ruby 1.9 and higher, there is the Shellwords module, but it doesn't apply in Ruby 1.8 or Windows.

This solution at least works on Ruby 1.8: perhaps someone can figure out what needs to be done to have it work in Windows, too.

@jasonmp85

Also, I followed all the directions in the TESTING document to no avail: after vendoring all the rails versions, it bails out installing therubyracer. It appeared there was a problem with libv8, so I ran brew install v8 and cleared the vendored gem cache. It installed this time, but testing just hangs forever. If someone could tell me what to do to get the tests running properly (or could pull this and test themselves or on the CI server or whatever) that would be wonderful.

@shime

@jasonmp85 sorry for not updating you sooner. Looks good to me!

Some tests would be wonderful. We've had some issues with the test suite, but everything is working like charm now.

You can run the unit tests with

rake test

Or the complete suite with

rake appraisal:install && rake

I'll understand if you don't want to do this anymore, but please let me know.

@jasonmp85 jasonmp85 Escape user name in Airbrake notify command
If a user has manually overridden this variable to be a full name, it
may contain a space which will break the command. Escaping the name
solves this issue.

Added a unit test for a name needing escaping.
be729f9
@jasonmp85

Got the tests running (they only run on master and I had rebased off the latest release tag because I wanted to have a stable base, heh) so I wrote a simple test to look for the escaped values in the capistrano output.

Having whitebox knowledge of the codebase, I know that the command echoed is the command run, but I guess to really be certain we should turn off --dry-run and stub calls to Kernel's backticks method but that is probably going overboard and is just a whitebox test in different fashion.

The shellescape method itself does not need testing as it is a copy-paste of a well-tested backport version of Ruby 1.9's shellescape method.

@jasonmp85

Anyways, tests all pass and I've been using this patch for several months now to put real names in deployment notifications (I use the full git user name if present, so it shows up as "Jason Petersen", which is descriptive, unlike my system username which is just "jason").

Tell me if anything else is required for merge.

@jasonmp85

Any update? All the tests pass and I've added one to test the escaping functionality.

@shime shime merged commit d26ea52 into airbrake:master Jun 21, 2013
@shime

Nicely done. Thanks Jason!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment