Commit 5c6cbfa6 authored by Robert Speicher's avatar Robert Speicher

Merge branch 'atom-routes' into 'master'

Allow browsing branches that end with '.atom'

## What does this MR do?

1. Simplify the regex capture in the routing for the CommitsController
   to not exclude the '.atom' suffix. That's a perfectly valid git
   branch name, so we shouldn't blow up if we get it.
2. Because Rails now can't automatically detect the request format, add
   some code to do so in `ExtractPath` when there is no path. This means
   that, given branches 'foo' and 'foo.atom', the Atom feed for the
   former is unroutable. To fix this: don't do that! Give the branches
   different names!

## Why was this MR needed?

Creating a branch or tag name ending in '.atom' would cause some 500s on that repo.

## What are the relevant issue numbers?

Closes #21955. Related to !5994.

See merge request !6750
parents e28623cc 1022456b
...@@ -20,6 +20,7 @@ v 8.13.0 (unreleased) ...@@ -20,6 +20,7 @@ v 8.13.0 (unreleased)
- Don't include archived projects when creating group milestones. !4940 (Jeroen Jacobs) - Don't include archived projects when creating group milestones. !4940 (Jeroen Jacobs)
- Add tag shortcut from the Commit page. !6543 - Add tag shortcut from the Commit page. !6543
- Keep refs for each deployment - Keep refs for each deployment
- Allow browsing branches that end with '.atom'
- Log LDAP lookup errors and don't swallow unrelated exceptions. !6103 (Markus Koller) - Log LDAP lookup errors and don't swallow unrelated exceptions. !6103 (Markus Koller)
- Add more tests for calendar contribution (ClemMakesApps) - Add more tests for calendar contribution (ClemMakesApps)
- Update Gitlab Shell to fix some problems with moving projects between storages - Update Gitlab Shell to fix some problems with moving projects between storages
......
...@@ -159,7 +159,7 @@ resources :namespaces, path: '/', constraints: { id: /[a-zA-Z.0-9_\-]+/ }, only: ...@@ -159,7 +159,7 @@ resources :namespaces, path: '/', constraints: { id: /[a-zA-Z.0-9_\-]+/ }, only:
get( get(
'/commits/*id', '/commits/*id',
to: 'commits#show', to: 'commits#show',
constraints: { id: /(?:[^.]|\.(?!atom$))+/, format: /atom/ }, constraints: { id: /.+/, format: false },
as: :commits as: :commits
) )
end end
......
...@@ -52,8 +52,7 @@ module ExtractsPath ...@@ -52,8 +52,7 @@ module ExtractsPath
# Append a trailing slash if we only get a ref and no file path # Append a trailing slash if we only get a ref and no file path
id += '/' unless id.ends_with?('/') id += '/' unless id.ends_with?('/')
valid_refs = @project.repository.ref_names valid_refs = ref_names.select { |v| id.start_with?("#{v}/") }
valid_refs.select! { |v| id.start_with?("#{v}/") }
if valid_refs.length == 0 if valid_refs.length == 0
# No exact ref match, so just try our best # No exact ref match, so just try our best
...@@ -74,6 +73,19 @@ module ExtractsPath ...@@ -74,6 +73,19 @@ module ExtractsPath
pair pair
end end
# If we have an ID of 'foo.atom', and the controller provides Atom and HTML
# formats, then we have to check if the request was for the Atom version of
# the ID without the '.atom' suffix, or the HTML version of the ID including
# the suffix. We only check this if the version including the suffix doesn't
# match, so it is possible to create a branch which has an unroutable Atom
# feed.
def extract_ref_without_atom(id)
id_without_atom = id.sub(/\.atom$/, '')
valid_refs = ref_names.select { |v| "#{id_without_atom}/".start_with?("#{v}/") }
valid_refs.max_by(&:length)
end
# Assigns common instance variables for views working with Git tree-ish objects # Assigns common instance variables for views working with Git tree-ish objects
# #
# Assignments are: # Assignments are:
...@@ -86,6 +98,10 @@ module ExtractsPath ...@@ -86,6 +98,10 @@ module ExtractsPath
# If the :id parameter appears to be requesting a specific response format, # If the :id parameter appears to be requesting a specific response format,
# that will be handled as well. # that will be handled as well.
# #
# If there is no path and the ref doesn't exist in the repo, try to resolve
# the ref without an '.atom' suffix. If _that_ ref is found, set the request's
# format to Atom manually.
#
# Automatically renders `not_found!` if a valid tree path could not be # Automatically renders `not_found!` if a valid tree path could not be
# resolved (e.g., when a user inserts an invalid path or ref). # resolved (e.g., when a user inserts an invalid path or ref).
def assign_ref_vars def assign_ref_vars
...@@ -103,6 +119,13 @@ module ExtractsPath ...@@ -103,6 +119,13 @@ module ExtractsPath
@commit = @repo.commit(@options[:extended_sha1]) @commit = @repo.commit(@options[:extended_sha1])
end end
if @path.empty? && !@commit
@id = @ref = extract_ref_without_atom(@id)
@commit = @repo.commit(@ref)
request.format = :atom if @commit
end
raise InvalidPathError unless @commit raise InvalidPathError unless @commit
@hex_path = Digest::SHA1.hexdigest(@path) @hex_path = Digest::SHA1.hexdigest(@path)
...@@ -125,4 +148,10 @@ module ExtractsPath ...@@ -125,4 +148,10 @@ module ExtractsPath
id += "/" + params[:path] unless params[:path].blank? id += "/" + params[:path] unless params[:path].blank?
id id
end end
def ref_names
return [] unless @project
@ref_names ||= @project.repository.ref_names
end
end end
...@@ -10,16 +10,39 @@ describe Projects::CommitsController do ...@@ -10,16 +10,39 @@ describe Projects::CommitsController do
end end
describe "GET show" do describe "GET show" do
context "as atom feed" do context "when the ref name ends in .atom" do
render_views
context "when the ref does not exist with the suffix" do
it "renders as atom" do it "renders as atom" do
get(:show, get(:show,
namespace_id: project.namespace.to_param, namespace_id: project.namespace.to_param,
project_id: project.to_param, project_id: project.to_param,
id: "master", id: "master.atom")
format: "atom")
expect(response).to be_success expect(response).to be_success
expect(response.content_type).to eq('application/atom+xml') expect(response.content_type).to eq('application/atom+xml')
end end
end end
context "when the ref exists with the suffix" do
before do
commit = project.repository.commit('master')
allow_any_instance_of(Repository).to receive(:commit).and_call_original
allow_any_instance_of(Repository).to receive(:commit).with('master.atom').and_return(commit)
get(:show,
namespace_id: project.namespace.to_param,
project_id: project.to_param,
id: "master.atom")
end
it "renders as HTML" do
expect(response).to be_success
expect(response.content_type).to eq('text/html')
end
end
end
end end
end end
...@@ -6,6 +6,7 @@ describe ExtractsPath, lib: true do ...@@ -6,6 +6,7 @@ describe ExtractsPath, lib: true do
include Gitlab::Routing.url_helpers include Gitlab::Routing.url_helpers
let(:project) { double('project') } let(:project) { double('project') }
let(:request) { double('request') }
before do before do
@project = project @project = project
...@@ -15,9 +16,10 @@ describe ExtractsPath, lib: true do ...@@ -15,9 +16,10 @@ describe ExtractsPath, lib: true do
allow(project).to receive(:repository).and_return(repo) allow(project).to receive(:repository).and_return(repo)
allow(project).to receive(:path_with_namespace). allow(project).to receive(:path_with_namespace).
and_return('gitlab/gitlab-ci') and_return('gitlab/gitlab-ci')
allow(request).to receive(:format=)
end end
describe '#assign_ref' do describe '#assign_ref_vars' do
let(:ref) { sample_commit[:id] } let(:ref) { sample_commit[:id] }
let(:params) { { path: sample_commit[:line_code_path], ref: ref } } let(:params) { { path: sample_commit[:line_code_path], ref: ref } }
...@@ -61,6 +63,75 @@ describe ExtractsPath, lib: true do ...@@ -61,6 +63,75 @@ describe ExtractsPath, lib: true do
expect(@id).to eq(get_id) expect(@id).to eq(get_id)
end end
end end
context 'ref only exists without .atom suffix' do
context 'with a path' do
let(:params) { { ref: 'v1.0.0.atom', path: 'README.md' } }
it 'renders a 404' do
expect(self).to receive(:render_404)
assign_ref_vars
end
end
context 'without a path' do
let(:params) { { ref: 'v1.0.0.atom' } }
before { assign_ref_vars }
it 'sets the un-suffixed version as @ref' do
expect(@ref).to eq('v1.0.0')
end
it 'sets the request format to Atom' do
expect(request).to have_received(:format=).with(:atom)
end
end
end
context 'ref exists with .atom suffix' do
context 'with a path' do
let(:params) { { ref: 'master.atom', path: 'README.md' } }
before do
repository = @project.repository
allow(repository).to receive(:commit).and_call_original
allow(repository).to receive(:commit).with('master.atom').and_return(repository.commit('master'))
assign_ref_vars
end
it 'sets the suffixed version as @ref' do
expect(@ref).to eq('master.atom')
end
it 'does not change the request format' do
expect(request).not_to have_received(:format=)
end
end
context 'without a path' do
let(:params) { { ref: 'master.atom' } }
before do
repository = @project.repository
allow(repository).to receive(:commit).and_call_original
allow(repository).to receive(:commit).with('master.atom').and_return(repository.commit('master'))
end
it 'sets the suffixed version as @ref' do
assign_ref_vars
expect(@ref).to eq('master.atom')
end
it 'does not change the request format' do
expect(request).not_to receive(:format=)
assign_ref_vars
end
end
end
end end
describe '#extract_ref' do describe '#extract_ref' do
...@@ -115,4 +186,18 @@ describe ExtractsPath, lib: true do ...@@ -115,4 +186,18 @@ describe ExtractsPath, lib: true do
end end
end end
end end
describe '#extract_ref_without_atom' do
it 'ignores any matching refs suffixed with atom' do
expect(extract_ref_without_atom('master.atom')).to eq('master')
end
it 'returns the longest matching ref' do
expect(extract_ref_without_atom('release/app/v1.0.0.atom')).to eq('release/app/v1.0.0')
end
it 'returns nil if there are no matching refs' do
expect(extract_ref_without_atom('foo.atom')).to eq(nil)
end
end
end end
...@@ -337,7 +337,7 @@ describe Projects::CommitsController, 'routing' do ...@@ -337,7 +337,7 @@ describe Projects::CommitsController, 'routing' do
end end
it 'to #show' do it 'to #show' do
expect(get('/gitlab/gitlabhq/commits/master.atom')).to route_to('projects/commits#show', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'master', format: 'atom') expect(get('/gitlab/gitlabhq/commits/master.atom')).to route_to('projects/commits#show', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'master.atom')
end end
end 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