Download as
Browse Code »

Commit 4c746c2522aedd8e8bf4d8bf1a8042e3a2b59bf9

Authored by Jacob Vosmaer
1 parent 9f20580e
Exists in spb-stable and in 3 other branches 6-8-stable, 7-0-stable, 7-0-stable-spb

Add shell commands guide

Showing 2 changed files with 112 additions and 0 deletions   Show diff stats
CONTRIBUTING.md
  Diff comments   View file @4c746c2
... ... @@ -66,6 +66,7 @@ If you can, please submit a merge request with the fix or improvements including
66 66 1. If the MR changes the UI it should include before and after screenshots
67 67 1. Link relevant [issues](https://gitlab.com/gitlab-org/gitlab-ce/issues) and/or [feedback items](http://feedback.gitlab.com/) from the merge request description and leave a comment on them with a link back to the MR
68 68 1. Be prepared to answer questions and incorporate feedback even if requests for this arrive weeks or months after your MR submittion
  69 +1. If your MR touches code that executes shell commands, make sure it adheres to the [shell command guidelines]( doc/development/shell_commands.md).
69 70  
70 71 The **official merge window** is in the beginning of the month from the 1st to the 7th day of the month. The best time to submit a MR and get feedback fast. Before this time the GitLab.com team is still dealing with work that is created by the monthly release such as assisting subscribers with upgrade issues, the release of Enterprise Edition and the upgrade of GitLab Cloud. After the 7th it is already getting closer to the release date of the next version. This means there is less time to fix the issues created by merging large new features.
71 72  
... ...
doc/development/shell_commands.md 0 → 100644
  Diff comments   View file @4c746c2
... ... @@ -0,0 +1,111 @@
  1 +# Guidelines for shell commands in the GitLab codebase
  2 +
  3 +## Use File and FileUtils instead of shell commands
  4 +
  5 +Sometimes we invoke basic Unix commands via the shell when there is also a Ruby API for doing it.
  6 +Use the Ruby API if it exists.
  7 +http://www.ruby-doc.org/stdlib-2.0.0/libdoc/fileutils/rdoc/FileUtils.html#module-FileUtils-label-Module+Functions
  8 +
  9 +```ruby
  10 +# Wrong
  11 +system "mkdir -p tmp/special/directory"
  12 +# Better (separate tokens)
  13 +system *%W(mkdir -p tmp/special/directory)
  14 +# Best (do not use a shell command)
  15 +FileUtils.mkdir_p "tmp/special/directory"
  16 +
  17 +# Wrong
  18 +contents = `cat #{filename}`
  19 +# Correct
  20 +contents = File.read(filename)
  21 +```
  22 +
  23 +This coding style could have prevented CVE-2013-4490.
  24 +
  25 +## Bypass the shell by splitting commands into separate tokens
  26 +
  27 +When we pass shell commands as a single string to Ruby, Ruby will let `/bin/sh` evaluate the entire string.
  28 +Essentially, we are asking the shell to evaluate a one-line script.
  29 +This creates a risk for shell injection attacks.
  30 +It is better to split the shell command into tokens ourselves.
  31 +Sometimes we use the scripting capabilities of the shell to change the working directory or set environment variables.
  32 +All of this can also be achieved securely straight from Ruby
  33 +
  34 +```ruby
  35 +# Wrong
  36 +system "cd /home/git/gitlab && bundle exec rake db:#{something} RAILS_ENV=production"
  37 +# Correct
  38 +system({'RAILS_ENV' => 'production'}, *%W(bundle exec rake db:#{something}), chdir: '/home/git/gitlab')
  39 +
  40 +# Wrong
  41 +system "touch #{myfile}"
  42 +# Better
  43 +system "touch", myfile
  44 +# Best (do not run a shell command at all)
  45 +FileUtils.touch myfile
  46 +```
  47 +
  48 +This coding style could have prevented CVE-2013-4546.
  49 +
  50 +## Separate options from arguments with --
  51 +
  52 +Make the difference between options and arguments clear to the argument parsers of system commands with `--`.
  53 +This is supported by many but not all Unix commands.
  54 +
  55 +To understand what `--` does, consider the problem below.
  56 +
  57 +```
  58 +# Example
  59 +$ echo hello > -l
  60 +$ cat -l
  61 +cat: illegal option -- l
  62 +usage: cat [-benstuv] [file ...]
  63 +```
  64 +
  65 +In the example above, the argument parser of `cat` assumes that `-l` is an option.
  66 +The solution in the example above is to make it clear to `cat` that `-l` is really an argument, not an option.
  67 +Many Unix command line tools follow the convention of separating options from arguments with `--`.
  68 +
  69 +```
  70 +# Example (continued)
  71 +$ cat -- -l
  72 +hello
  73 +```
  74 +
  75 +In the GitLab codebase, we avoid the option/argument ambiguity by _always_ using `--`.
  76 +
  77 +```ruby
  78 +# Wrong
  79 +system(*%W(git branch -d #{branch_name}))
  80 +# Correct
  81 +system(*%W(git branch -d -- #{branch_name}))
  82 +```
  83 +
  84 +This coding style could have prevented CVE-2013-4582.
  85 +
  86 +## Do not use the backticks
  87 +
  88 +Capturing the output of shell commands with backticks reads nicely, but you are forced to pass the command as one string to the shell.
  89 +We explained above that this is unsafe.
  90 +In the main GitLab codebase, the solution is to use `Gitlab::Popen.popen` instead.
  91 +
  92 +```ruby
  93 +# Wrong
  94 +logs = `cd #{repo_dir} && git log`
  95 +# Correct
  96 +logs, exit_status = Gitlab::Popen.popen(%W(git log), repo_dir)
  97 +
  98 +# Wrong
  99 +user = `whoami`
  100 +# Correct
  101 +user, exit_status = Gitlab::Popen.popen(%W(whoami))
  102 +```
  103 +
  104 +In other repositories, such as gitlab-shell you can also use `IO.popen`.
  105 +
  106 +```ruby
  107 +# Safe IO.popen example
  108 +logs = IO.popen(%W(git log), chdir: repo_dir).read
  109 +```
  110 +
  111 +Note that unlike `Gitlab::Popen.popen`, `IO.popen` does not capture standard error.
... ...