Commit 5d5e6954 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'fix-issue-32506' into 'master'

Fix redirects modifying the host

Closes #32506

See merge request !11498
parents bdf62a19 49697bc8
......@@ -24,15 +24,15 @@ module RoutableActions
end
end
def ensure_canonical_path(routable, requested_path)
def ensure_canonical_path(routable, requested_full_path)
return unless request.get?
canonical_path = routable.full_path
if canonical_path != requested_path
if canonical_path.casecmp(requested_path) != 0
flash[:notice] = "#{routable.class.to_s.titleize} '#{requested_path}' was moved to '#{canonical_path}'. Please update any links and bookmarks that may still have the old path."
if canonical_path != requested_full_path
if canonical_path.casecmp(requested_full_path) != 0
flash[:notice] = "#{routable.class.to_s.titleize} '#{requested_full_path}' was moved to '#{canonical_path}'. Please update any links and bookmarks that may still have the old path."
end
redirect_to request.original_url.sub(requested_path, canonical_path)
redirect_to build_canonical_path(routable)
end
end
end
......@@ -31,4 +31,10 @@ class Groups::ApplicationController < ApplicationController
return render_403
end
end
def build_canonical_path(group)
params[:group_id] = group.to_param
url_for(params)
end
end
......@@ -169,4 +169,12 @@ class GroupsController < Groups::ApplicationController
@notification_setting = current_user.notification_settings_for(group)
end
end
def build_canonical_path(group)
return group_path(group) if action_name == 'show' # root group path
params[:id] = group.to_param
url_for(params)
end
end
......@@ -29,6 +29,13 @@ class Projects::ApplicationController < ApplicationController
@project = find_routable!(Project, path, extra_authorization_proc: auth_proc)
end
def build_canonical_path(project)
params[:namespace_id] = project.namespace.to_param
params[:project_id] = project.to_param
url_for(params)
end
def repository
@repository ||= project.repository
end
......
......@@ -365,4 +365,11 @@ class ProjectsController < Projects::ApplicationController
def project_view_files_allowed?
!project.empty_repo? && can?(current_user, :download_code, project)
end
def build_canonical_path(project)
params[:namespace_id] = project.namespace.to_param
params[:id] = project.to_param
url_for(params)
end
end
......@@ -138,4 +138,8 @@ class UsersController < ApplicationController
def projects_for_current_user
ProjectsFinder.new(current_user: current_user).execute
end
def build_canonical_path(user)
url_for(params.merge(username: user.to_param))
end
end
......@@ -21,7 +21,6 @@ describe Groups::MilestonesController do
sign_in(user)
group.add_owner(user)
project.team << [user, :master]
controller.instance_variable_set(:@group, group)
end
it_behaves_like 'milestone tabs'
......@@ -29,7 +28,7 @@ describe Groups::MilestonesController do
describe "#create" do
it "creates group milestone with Chinese title" do
post :create,
group_id: group.id,
group_id: group.to_param,
milestone: { project_ids: [project.id, project2.id], title: title }
expect(response).to redirect_to(group_milestone_path(group, title.to_slug.to_s, title: title))
......@@ -37,9 +36,139 @@ describe Groups::MilestonesController do
end
it "redirects to new when there are no project ids" do
post :create, group_id: group.id, milestone: { title: title, project_ids: [""] }
post :create, group_id: group.to_param, milestone: { title: title, project_ids: [""] }
expect(response).to render_template :new
expect(assigns(:milestone).errors).not_to be_nil
end
end
describe '#ensure_canonical_path' do
before do
sign_in(user)
end
context 'for a GET request' do
context 'when requesting the canonical path' do
context 'non-show path' do
context 'with exactly matching casing' do
it 'does not redirect' do
get :index, group_id: group.to_param
expect(response).not_to have_http_status(301)
end
end
context 'with different casing' do
it 'redirects to the correct casing' do
get :index, group_id: group.to_param.upcase
expect(response).to redirect_to(group_milestones_path(group.to_param))
expect(controller).not_to set_flash[:notice]
end
end
end
context 'show path' do
context 'with exactly matching casing' do
it 'does not redirect' do
get :show, group_id: group.to_param, id: title
expect(response).not_to have_http_status(301)
end
end
context 'with different casing' do
it 'redirects to the correct casing' do
get :show, group_id: group.to_param.upcase, id: title
expect(response).to redirect_to(group_milestone_path(group.to_param, title))
expect(controller).not_to set_flash[:notice]
end
end
end
end
context 'when requesting a redirected path' do
let(:redirect_route) { group.redirect_routes.create(path: 'old-path') }
it 'redirects to the canonical path' do
get :merge_requests, group_id: redirect_route.path, id: title
expect(response).to redirect_to(merge_requests_group_milestone_path(group.to_param, title))
expect(controller).to set_flash[:notice].to(group_moved_message(redirect_route, group))
end
context 'when the old group path is a substring of the scheme or host' do
let(:redirect_route) { group.redirect_routes.create(path: 'http') }
it 'does not modify the requested host' do
get :merge_requests, group_id: redirect_route.path, id: title
expect(response).to redirect_to(merge_requests_group_milestone_path(group.to_param, title))
expect(controller).to set_flash[:notice].to(group_moved_message(redirect_route, group))
end
end
context 'when the old group path is substring of groups' do
# I.e. /groups/oups should not become /grfoo/oups
let(:redirect_route) { group.redirect_routes.create(path: 'oups') }
it 'does not modify the /groups part of the path' do
get :merge_requests, group_id: redirect_route.path, id: title
expect(response).to redirect_to(merge_requests_group_milestone_path(group.to_param, title))
expect(controller).to set_flash[:notice].to(group_moved_message(redirect_route, group))
end
end
context 'when the old group path is substring of groups plus the new path' do
# I.e. /groups/oups/oup should not become /grfoos
let(:redirect_route) { group.redirect_routes.create(path: 'oups/oup') }
it 'does not modify the /groups part of the path' do
get :merge_requests, group_id: redirect_route.path, id: title
expect(response).to redirect_to(merge_requests_group_milestone_path(group.to_param, title))
expect(controller).to set_flash[:notice].to(group_moved_message(redirect_route, group))
end
end
end
end
end
context 'for a non-GET request' do
context 'when requesting the canonical path with different casing' do
it 'does not 404' do
post :create,
group_id: group.to_param,
milestone: { project_ids: [project.id, project2.id], title: title }
expect(response).not_to have_http_status(404)
end
it 'does not redirect to the correct casing' do
post :create,
group_id: group.to_param,
milestone: { project_ids: [project.id, project2.id], title: title }
expect(response).not_to have_http_status(301)
end
end
context 'when requesting a redirected path' do
let(:redirect_route) { group.redirect_routes.create(path: 'old-path') }
it 'returns not found' do
post :create,
group_id: redirect_route.path,
milestone: { project_ids: [project.id, project2.id], title: title }
expect(response).to have_http_status(404)
end
end
end
def group_moved_message(redirect_route, group)
"Group '#{redirect_route.path}' was moved to '#{group.full_path}'. Please update any links and bookmarks that may still have the old path."
end
end
This diff is collapsed.
......@@ -157,4 +157,74 @@ describe Projects::LabelsController do
end
end
end
describe '#ensure_canonical_path' do
before do
sign_in(user)
end
context 'for a GET request' do
context 'when requesting the canonical path' do
context 'non-show path' do
context 'with exactly matching casing' do
it 'does not redirect' do
get :index, namespace_id: project.namespace, project_id: project.to_param
expect(response).not_to have_http_status(301)
end
end
context 'with different casing' do
it 'redirects to the correct casing' do
get :index, namespace_id: project.namespace, project_id: project.to_param.upcase
expect(response).to redirect_to(namespace_project_labels_path(project.namespace, project))
expect(controller).not_to set_flash[:notice]
end
end
end
end
context 'when requesting a redirected path' do
let!(:redirect_route) { project.redirect_routes.create(path: project.full_path + 'old') }
it 'redirects to the canonical path' do
get :index, namespace_id: project.namespace, project_id: project.to_param + 'old'
expect(response).to redirect_to(namespace_project_labels_path(project.namespace, project))
expect(controller).to set_flash[:notice].to(project_moved_message(redirect_route, project))
end
end
end
end
context 'for a non-GET request' do
context 'when requesting the canonical path with different casing' do
it 'does not 404' do
post :generate, namespace_id: project.namespace, project_id: project
expect(response).not_to have_http_status(404)
end
it 'does not redirect to the correct casing' do
post :generate, namespace_id: project.namespace, project_id: project
expect(response).not_to have_http_status(301)
end
end
context 'when requesting a redirected path' do
let!(:redirect_route) { project.redirect_routes.create(path: project.full_path + 'old') }
it 'returns not found' do
post :generate, namespace_id: project.namespace, project_id: project.to_param + 'old'
expect(response).to have_http_status(404)
end
end
end
def project_moved_message(redirect_route, project)
"Project '#{redirect_route.path}' was moved to '#{project.full_path}'. Please update any links and bookmarks that may still have the old path."
end
end
......@@ -169,27 +169,6 @@ describe ProjectsController do
end
end
context "when requested with case sensitive namespace and project path" do
context "when there is a match with the same casing" do
it "loads the project" do
get :show, namespace_id: public_project.namespace, id: public_project
expect(assigns(:project)).to eq(public_project)
expect(response).to have_http_status(200)
end
end
context "when there is a match with different casing" do
it "redirects to the normalized path" do
get :show, namespace_id: public_project.namespace, id: public_project.path.upcase
expect(assigns(:project)).to eq(public_project)
expect(response).to redirect_to("/#{public_project.full_path}")
expect(controller).not_to set_flash[:notice]
end
end
end
context "when the url contains .atom" do
let(:public_project_with_dot_atom) { build(:empty_project, :public, name: 'my.atom', path: 'my.atom') }
......@@ -219,17 +198,6 @@ describe ProjectsController do
expect(response).to redirect_to(namespace_project_path)
end
end
context 'when requesting a redirected path' do
let!(:redirect_route) { public_project.redirect_routes.create!(path: "foo/bar") }
it 'redirects to the canonical path' do
get :show, namespace_id: 'foo', id: 'bar'
expect(response).to redirect_to(public_project)
expect(controller).to set_flash[:notice].to(project_moved_message(redirect_route, public_project))
end
end
end
describe "#update" do
......@@ -256,34 +224,6 @@ describe ProjectsController do
expect(assigns(:repository).path).to eq(project.repository.path)
expect(response).to have_http_status(302)
end
context 'when requesting the canonical path' do
it "is case-insensitive" do
controller.instance_variable_set(:@project, project)
put :update,
namespace_id: 'FOo',
id: 'baR',
project: project_params
expect(project.repository.path).to include(new_path)
expect(assigns(:repository).path).to eq(project.repository.path)
expect(response).to have_http_status(302)
end
end
context 'when requesting a redirected path' do
let!(:redirect_route) { project.redirect_routes.create!(path: "foo/bar") }
it 'returns not found' do
put :update,
namespace_id: 'foo',
id: 'bar',
project: project_params
expect(response).to have_http_status(404)
end
end
end
describe "#destroy" do
......@@ -319,31 +259,6 @@ describe ProjectsController do
expect(merge_request.reload.state).to eq('closed')
end
end
context 'when requesting the canonical path' do
it "is case-insensitive" do
controller.instance_variable_set(:@project, project)
sign_in(admin)
orig_id = project.id
delete :destroy, namespace_id: project.namespace, id: project.path.upcase
expect { Project.find(orig_id) }.to raise_error(ActiveRecord::RecordNotFound)
expect(response).to have_http_status(302)
expect(response).to redirect_to(dashboard_projects_path)
end
end
context 'when requesting a redirected path' do
let!(:redirect_route) { project.redirect_routes.create!(path: "foo/bar") }
it 'returns not found' do
sign_in(admin)
delete :destroy, namespace_id: 'foo', id: 'bar'
expect(response).to have_http_status(404)
end
end
end
describe 'PUT #new_issue_address' do
......@@ -465,17 +380,6 @@ describe ProjectsController do
expect(parsed_body["Tags"]).to include("v1.0.0")
expect(parsed_body["Commits"]).to include("123456")
end
context 'when requesting a redirected path' do
let!(:redirect_route) { public_project.redirect_routes.create!(path: "foo/bar") }
it 'redirects to the canonical path' do
get :refs, namespace_id: 'foo', id: 'bar'
expect(response).to redirect_to(refs_namespace_project_path(namespace_id: public_project.namespace, id: public_project))
expect(controller).to set_flash[:notice].to(project_moved_message(redirect_route, public_project))
end
end
end
describe 'POST #preview_markdown' do
......@@ -488,6 +392,109 @@ describe ProjectsController do
end
end
describe '#ensure_canonical_path' do
before do
sign_in(user)
end
context 'for a GET request' do
context 'when requesting the canonical path' do
context "with exactly matching casing" do
it "loads the project" do
get :show, namespace_id: public_project.namespace, id: public_project
expect(assigns(:project)).to eq(public_project)
expect(response).to have_http_status(200)
end
end
context "with different casing" do
it "redirects to the normalized path" do
get :show, namespace_id: public_project.namespace, id: public_project.path.upcase
expect(assigns(:project)).to eq(public_project)
expect(response).to redirect_to("/#{public_project.full_path}")
expect(controller).not_to set_flash[:notice]
end
end
end
context 'when requesting a redirected path' do
let!(:redirect_route) { public_project.redirect_routes.create!(path: "foo/bar") }
it 'redirects to the canonical path' do
get :show, namespace_id: 'foo', id: 'bar'
expect(response).to redirect_to(public_project)
expect(controller).to set_flash[:notice].to(project_moved_message(redirect_route, public_project))
end
it 'redirects to the canonical path (testing non-show action)' do
get :refs, namespace_id: 'foo', id: 'bar'
expect(response).to redirect_to(refs_namespace_project_path(namespace_id: public_project.namespace, id: public_project))
expect(controller).to set_flash[:notice].to(project_moved_message(redirect_route, public_project))
end
end
end
context 'for a POST request' do
context 'when requesting the canonical path with different casing' do
it 'does not 404' do
post :toggle_star, namespace_id: public_project.namespace, id: public_project.path.upcase
expect(response).not_to have_http_status(404)
end
it 'does not redirect to the correct casing' do
post :toggle_star, namespace_id: public_project.namespace, id: public_project.path.upcase
expect(response).not_to have_http_status(301)
end
end
context 'when requesting a redirected path' do
let!(:redirect_route) { public_project.redirect_routes.create!(path: "foo/bar") }
it 'returns not found' do
post :toggle_star, namespace_id: 'foo', id: 'bar'
expect(response).to have_http_status(404)
end
end
end
context 'for a DELETE request' do
before do
sign_in(create(:admin))
end
context 'when requesting the canonical path with different casing' do
it 'does not 404' do
delete :destroy, namespace_id: project.namespace, id: project.path.upcase
expect(response).not_to have_http_status(404)
end
it 'does not redirect to the correct casing' do
delete :destroy, namespace_id: project.namespace, id: project.path.upcase
expect(response).not_to have_http_status(301)
end
end
context 'when requesting a redirected path' do
let!(:redirect_route) { project.redirect_routes.create!(path: "foo/bar") }
it 'returns not found' do
delete :destroy, namespace_id: 'foo', id: 'bar'
expect(response).to have_http_status(404)
end
end
end
end
def project_moved_message(redirect_route, project)
"Project '#{redirect_route.path}' was moved to '#{project.full_path}'. Please update any links and bookmarks that may still have the old path."
end
......
......@@ -53,40 +53,6 @@ describe UsersController do
end
end
context 'when requesting the canonical path' do
let(:user) { create(:user, username: 'CamelCaseUser') }
before { sign_in(user) }
context 'with exactly matching casing' do
it 'responds with success' do
get :show, username: user.username
expect(response).to be_success
end
end
context 'with different casing' do
it 'redirects to the correct casing' do
get :show, username: user.username.downcase
expect(response).to redirect_to(user)
expect(controller).not_to set_flash[:notice]
end
end
end
context 'when requesting a redirected path' do
let(:redirect_route) { user.namespace.redirect_routes.create(path: 'old-username') }
it 'redirects to the canonical path' do
get :show, username: redirect_route.path
expect(response).to redirect_to(user)
expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user))
end
end
context 'when a user by that username does not exist' do
context 'when logged out' do
it 'redirects to login page' do
......@@ -131,40 +97,6 @@ describe UsersController do
expect(assigns(:contributions_calendar).projects.count).to eq(2)
end
end
context 'when requesting the canonical path' do
let(:user) { create(:user, username: 'CamelCaseUser') }
before { sign_in(user) }
context 'with exactly matching casing' do
it 'responds with success' do
get :calendar, username: user.username
expect(response).to be_success
end
end
context 'with different casing' do
it 'redirects to the correct casing' do
get :calendar, username: user.username.downcase
expect(response).to redirect_to(user_calendar_path(user))
expect(controller).not_to set_flash[:notice]
end
end
end
context 'when requesting a redirected path' do
let(:redirect_route) { user.namespace.redirect_routes.create(path: 'old-username') }
it 'redirects to the canonical path' do
get :calendar, username: redirect_route.path
expect(response).to redirect_to(user_calendar_path(user))
expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user))
end
end
end
describe 'GET #calendar_activities' do
......@@ -187,38 +119,6 @@ describe UsersController do
get :calendar_activities, username: user.username
expect(response).to render_template('calendar_activities')
end
context 'when requesting the canonical path' do
let(:user) { create(:user, username: 'CamelCaseUser') }
context 'with exactly matching casing' do
it 'responds with success' do
get :calendar_activities, username: user.username
expect(response).to be_success
end
end
context 'with different casing' do
it 'redirects to the correct casing' do
get :calendar_activities, username: user.username.downcase
expect(response).to redirect_to(user_calendar_activities_path(user))
expect(controller).not_to set_flash[:notice]
end
end
end
context 'when requesting a redirected path' do
let(:redirect_route) { user.namespace.redirect_routes.create(path: 'old-username') }
it 'redirects to the canonical path' do
get :calendar_activities, username: redirect_route.path
expect(response).to redirect_to(user_calendar_activities_path(user))
expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user))
end
end
end
describe 'GET #snippets' do
......@@ -241,38 +141,6 @@ describe UsersController do
expect(JSON.parse(response.body)).to have_key('html')
end
end
context 'when requesting the canonical path' do
let(:user) { create(:user, username: 'CamelCaseUser') }
context 'with exactly matching casing' do
it 'responds with success' do
get :snippets, username: user.username
expect(response).to be_success
end
end
context 'with different casing' do
it 'redirects to the correct casing' do
get :snippets, username: user.username.downcase
expect(response).to redirect_to(user_snippets_path(user))
expect(controller).not_to set_flash[:notice]
end
end
end
context 'when requesting a redirected path' do
let(:redirect_route) { user.namespace.redirect_routes.create(path: 'old-username') }
it 'redirects to the canonical path' do
get :snippets, username: redirect_route.path
expect(response).to redirect_to(user_snippets_path(user))
expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user))
end
end
end
describe 'GET #exists' do
......@@ -321,6 +189,127 @@ describe UsersController do
end
end
describe '#ensure_canonical_path' do
before do
sign_in(user)
end
context 'for a GET request' do
context 'when requesting users at the root path' do
context 'when requesting the canonical path' do
let(:user) { create(:user, username: 'CamelCaseUser') }
context 'with exactly matching casing' do
it 'responds with success' do
get :show, username: user.username
expect(response).to be_success
end
end
context 'with different casing' do
it 'redirects to the correct casing' do
get :show, username: user.username.downcase
expect(response).to redirect_to(user)
expect(controller).not_to set_flash[:notice]
end
end
end
context 'when requesting a redirected path' do
let(:redirect_route) { user.namespace.redirect_routes.create(path: 'old-path') }
it 'redirects to the canonical path' do
get :show, username: redirect_route.path
expect(response).to redirect_to(user)
expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user))
end
context 'when the old path is a substring of the scheme or host' do
let(:redirect_route) { user.namespace.redirect_routes.create(path: 'http') }
it 'does not modify the requested host' do
get :show, username: redirect_route.path
expect(response).to redirect_to(user)
expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user))
end
end
context 'when the old path is substring of users' do
let(:redirect_route) { user.namespace.redirect_routes.create(path: 'ser') }
it 'redirects to the canonical path' do
get :show, username: redirect_route.path
expect(response).to redirect_to(user)
expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user))
end
end
end
end
context 'when requesting users under the /users path' do
context 'when requesting the canonical path' do
let(:user) { create(:user, username: 'CamelCaseUser') }
context 'with exactly matching casing' do
it 'responds with success' do
get :projects, username: user.username
expect(response).to be_success
end
end
context 'with different casing' do
it 'redirects to the correct casing' do
get :projects, username: user.username.downcase
expect(response).to redirect_to(user_projects_path(user))
expect(controller).not_to set_flash[:notice]
end
end
end
context 'when requesting a redirected path' do
let(:redirect_route) { user.namespace.redirect_routes.create(path: 'old-path') }
it 'redirects to the canonical path' do
get :projects, username: redirect_route.path
expect(response).to redirect_to(user_projects_path(user))
expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user))
end
context 'when the old path is a substring of the scheme or host' do
let(:redirect_route) { user.namespace.redirect_routes.create(path: 'http') }
it 'does not modify the requested host' do
get :projects, username: redirect_route.path
expect(response).to redirect_to(user_projects_path(user))
expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user))
end
end
context 'when the old path is substring of users' do
let(:redirect_route) { user.namespace.redirect_routes.create(path: 'ser') }
# I.e. /users/ser should not become /ufoos/ser
it 'does not modify the /users part of the path' do
get :projects, username: redirect_route.path
expect(response).to redirect_to(user_projects_path(user))
expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user))
end
end
end
end
end
end
def user_moved_message(redirect_route, user)
"User '#{redirect_route.path}' was moved to '#{user.full_path}'. Please update any links and bookmarks that may still have the old path."
end
......
shared_examples 'milestone tabs' do
def go(path, extra_params = {})
params = if milestone.is_a?(GlobalMilestone)
{ group_id: group.id, id: milestone.safe_title, title: milestone.title }
{ group_id: group.to_param, id: milestone.safe_title, title: milestone.title }
else
{ namespace_id: project.namespace.to_param, project_id: project, id: milestone.iid }
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment