Merge branch 'sanitize-snippet-file-name' into 'master'

Validate and sanitize snippet file name

Fixes #1816

See merge request !1322

4 files changed, 16 insertions, 6 deletions

diff --git a/app/controllers/projects/snippets_controller.rb b/app/controllers/projects/snippets_controller.rb
index 9d5dd8a95cc..25c887deafa 100644
--- a/app/controllers/projects/snippets_controller.rb
+++ b/app/controllers/projects/snippets_controller.rb
@@ -68,7 +68,7 @@ class Projects::SnippetsController < Projects::ApplicationController
       @snippet.content,
       type: 'text/plain; charset=utf-8',
       disposition: 'inline',
-      filename: @snippet.file_name
+      filename: @snippet.sanitized_file_name
     )
   end
 
diff --git a/app/controllers/snippets_controller.rb b/app/controllers/snippets_controller.rb
index bf3312fedc8..312e561b522 100644
--- a/app/controllers/snippets_controller.rb
+++ b/app/controllers/snippets_controller.rb
@@ -79,7 +79,7 @@ class SnippetsController < ApplicationController
       @snippet.content,
       type: 'text/plain; charset=utf-8',
       disposition: 'inline',
-      filename: @snippet.file_name
+      filename: @snippet.sanitized_file_name
     )
   end
 
diff --git a/app/models/snippet.rb b/app/models/snippet.rb
index a47fbca3260..9aba42a0622 100644
--- a/app/models/snippet.rb
+++ b/app/models/snippet.rb
@@ -29,7 +29,9 @@ class Snippet < ActiveRecord::Base
 
   validates :author, presence: true
   validates :title, presence: true, length: { within: 0..255 }
-  validates :file_name, presence: true, length: { within: 0..255 }
+  validates :file_name, presence: true, length: { within: 0..255 },
+            format: { with: Gitlab::Regex.path_regex,
+                      message: Gitlab::Regex.path_regex_message }
   validates :content, presence: true
   validates :visibility_level, inclusion: { in: Gitlab::VisibilityLevel.values }
 
@@ -62,6 +64,10 @@ class Snippet < ActiveRecord::Base
     file_name
   end
 
+  def sanitized_file_name
+    file_name.gsub(/[^a-zA-Z0-9_\-\.]+/, '')
+  end
+
   def mode
     nil
   end
@@ -72,7 +78,7 @@ class Snippet < ActiveRecord::Base
 
   def visibility_level_field
     visibility_level
-  end 
+  end
 
   class << self
     def search(query)
diff --git a/spec/factories.rb b/spec/factories.rb
index 58060131638..50580cd1334 100644
--- a/spec/factories.rb
+++ b/spec/factories.rb
@@ -5,10 +5,14 @@ FactoryGirl.define do
     Faker::Lorem.sentence
   end
 
-  sequence :name, aliases: [:file_name] do
+  sequence :name do
     Faker::Name.name
   end
 
+  sequence :file_name do
+    Faker::Internet.user_name
+  end
+
   sequence(:url) { Faker::Internet.uri('http') }
 
   factory :user, aliases: [:author, :assignee, :owner, :creator] do
@@ -18,7 +22,7 @@ FactoryGirl.define do
     password "12345678"
     password_confirmation { password }
     confirmed_at { Time.now }
-    confirmation_token { nil }    
+    confirmation_token { nil }
 
     trait :admin do
       admin true