Don't mess up our parent controller

2 files changed, 28 insertions, 30 deletions

diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb
index 817727d7868..74150ad606b 100644
--- a/app/controllers/projects/application_controller.rb
+++ b/app/controllers/projects/application_controller.rb
@@ -10,6 +10,9 @@ class Projects::ApplicationController < ApplicationController
 
   def project
     unless @project
+      namespace = params[:namespace_id]
+      id = params[:project_id] || params[:id]
+
       # Redirect from
       #   localhost/group/project.git
       # to
@@ -20,11 +23,12 @@ class Projects::ApplicationController < ApplicationController
         return
       end
 
-      @project = find_project
+      project_path = "#{namespace}/#{id}"
+      @project = Project.find_with_namespace(project_path)
 
       if @project && can?(current_user, :read_project, @project)
-        if @project.path_with_namespace != path_with_namespace
-          redirect_to request.original_url.gsub(path_with_namespace, @project.path_with_namespace)
+        if @project.path_with_namespace != project_path
+          redirect_to request.original_url.gsub(project_path, @project.path_with_namespace)
         end
       else
         @project = nil
@@ -40,22 +44,6 @@ class Projects::ApplicationController < ApplicationController
     @project
   end
 
-  def id
-    params[:project_id] || params[:id]
-  end
-  
-  def namespace
-    params[:namespace_id]
-  end
-  
-  def path_with_namespace
-    "#{namespace}/#{id}"
-  end
-  
-  def find_project
-    Project.find_with_namespace(path_with_namespace)
-  end
-
   def repository
     @repository ||= project.repository
   end
diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index cd8dd610bcd..e38552218ec 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -119,27 +119,37 @@ class Projects::GitHttpController < Projects::ApplicationController
 
   def project
     return @project if defined?(@project)
-    @project = find_project
+
+    project_id, _ = project_id_with_suffix
+    if project_id.blank?
+      @project = nil
+    else
+      @project = Project.find_with_namespace("#{params[:namespace_id]}/#{project_id}")
+    end
   end
 
-  def id
-    id = params[:project_id]
-    return if id.nil?
+  # This method returns two values so that we can parse
+  # params[:project_id] (untrusted input!) in exactly one place.
+  def project_id_with_suffix
+    id = params[:project_id] || ''
 
     %w{.wiki.git .git}.each do |suffix|
-      # Be careful to only remove the suffix from the end of 'id'.
-      # Accidentally removing it from the middle is how security
-      # vulnerabilities happen!
-      return id.slice(0, id.length - suffix.length) if id.end_with?(suffix)
+      if id.end_with?(suffix)
+        # Be careful to only remove the suffix from the end of 'id'.
+        # Accidentally removing it from the middle is how security
+        # vulnerabilities happen!
+        return [id.slice(0, id.length - suffix.length), suffix]
+      end
     end
 
-    # No valid id was found.
-    nil
+    # Something is wrong with params[:project_id]; do not pass it on.
+    [nil, nil]
   end
 
   def repository
     @repository ||= begin
-      if params[:project_id].end_with?('.wiki.git')
+      _, suffix = project_id_with_suffix
+      if suffix == '.wiki.git'
         project.wiki.repository
       else
         project.repository