Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Escape user name in Airbrake notify command #144

Merged
merged 1 commit into from

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

Nicely done. Thanks Jason!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 14, 2013
  1. @jasonmp85

    Escape user name in Airbrake notify command

    jasonmp85 authored Jason Petersen committed
    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.
This page is out of date. Refresh to see the latest.
Showing with 65 additions and 1 deletion.
  1. +57 −1 lib/airbrake/capistrano.rb
  2. +8 −0 test/capistrano_test.rb
View
58 lib/airbrake/capistrano.rb
@@ -3,6 +3,62 @@
module Airbrake
module Capistrano
+ # What follows is a copy-paste backport of the shellescape method
+ # included in Ruby 1.9 and greater. The FSF's guidance on a snippet
+ # of this size indicates that such a small function is not subject
+ # to copyright and as such there is no risk of a license conflict:
+ # See www.gnu.org/prep/maintain/maintain.html#Legally-Significant
+ #
+ # Escapes a string so that it can be safely used in a Bourne shell
+ # command line. +str+ can be a non-string object that responds to
+ # +to_s+.
+ #
+ # Note that a resulted string should be used unquoted and is not
+ # intended for use in double quotes nor in single quotes.
+ #
+ # argv = Shellwords.escape("It's better to give than to receive")
+ # argv #=> "It\\'s\\ better\\ to\\ give\\ than\\ to\\ receive"
+ #
+ # String#shellescape is a shorthand for this function.
+ #
+ # argv = "It's better to give than to receive".shellescape
+ # argv #=> "It\\'s\\ better\\ to\\ give\\ than\\ to\\ receive"
+ #
+ # # Search files in lib for method definitions
+ # pattern = "^[ \t]*def "
+ # open("| grep -Ern #{pattern.shellescape} lib") { |grep|
+ # grep.each_line { |line|
+ # file, lineno, matched_line = line.split(':', 3)
+ # # ...
+ # }
+ # }
+ #
+ # It is the caller's responsibility to encode the string in the right
+ # encoding for the shell environment where this string is used.
+ #
+ # Multibyte characters are treated as multibyte characters, not bytes.
+ #
+ # Returns an empty quoted String if +str+ has a length of zero.
+ def self.shellescape(str)
+ str = str.to_s
+
+ # An empty argument will be skipped, so return empty quotes.
+ return "''" if str.empty?
+
+ str = str.dup
+
+ # Treat multibyte characters as is. It is caller's responsibility
+ # to encode the string in the right encoding for the shell
+ # environment.
+ str.gsub!(/([^A-Za-z0-9_\-.,:\/@\n])/, "\\\\\\1")
+
+ # A LF cannot be escaped with a backslash because a backslash + LF
+ # combo is regarded as line continuation and simply ignored.
+ str.gsub!(/\n/, "'\n'")
+
+ return str
+ end
+
def self.load_into(configuration)
configuration.load do
after "deploy", "airbrake:deploy"
@@ -20,7 +76,7 @@ def self.load_into(configuration)
local_user = ENV['USER'] || ENV['USERNAME']
executable = RUBY_PLATFORM.downcase.include?('mswin') ? fetch(:rake, 'rake.bat') : fetch(:rake, 'bundle exec rake ')
directory = configuration.release_path
- notify_command = "cd #{directory}; #{executable} RAILS_ENV=#{rails_env} airbrake:deploy TO=#{airbrake_env} REVISION=#{current_revision} REPO=#{repository} USER=#{local_user}"
+ notify_command = "cd #{directory}; #{executable} RAILS_ENV=#{rails_env} airbrake:deploy TO=#{airbrake_env} REVISION=#{current_revision} REPO=#{repository} USER=#{Airbrake::Capistrano::shellescape(local_user)}"
notify_command << " DRY_RUN=true" if dry_run
notify_command << " API_KEY=#{ENV['API_KEY']}" if ENV['API_KEY']
logger.info "Notifying Airbrake of Deploy (#{notify_command})"
View
8 test/capistrano_test.rb
@@ -8,6 +8,10 @@ def setup
super
reset_config
+ # Save value to avoid polluting ENV for future tests
+ @old_user = ENV['USER']
+ ENV['USER'] = %q[D'Angelo "D" Barksdale]
+
@configuration = Capistrano::Configuration.new
Airbrake::Capistrano.load_into(@configuration)
@configuration.dry_run = true
@@ -30,5 +34,9 @@ def setup
assert io.string.include?('** Notifying Airbrake of Deploy')
assert io.string.include?('** Airbrake Notification Complete')
+ assert io.string.include?(%q[D\'Angelo\ \"D\"\ Barksdale])
end
+
+ # Return ENV['USER'] to its original value
+ def teardown; ENV['USER'] = @old_user end
end
Something went wrong with that request. Please try again.