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

Escape user name in Airbrake notify command #144

Merged
merged 1 commit into from Jun 21, 2013
Merged

Escape user name in Airbrake notify command #144

merged 1 commit into from Jun 21, 2013

Conversation

jasonmp85
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

shime commented May 21, 2013

@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.

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.
@jasonmp85
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

shime added a commit that referenced this pull request Jun 21, 2013
Escape user name in Airbrake notify command
@shime shime merged commit d26ea52 into airbrake:master Jun 21, 2013
@shime
Copy link
Contributor

shime commented Jun 21, 2013

Nicely done. Thanks Jason!

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.

None yet

2 participants