Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cache rendering with namespaced resources #874

Merged
merged 1 commit into from
May 24, 2023
+66 −36
Merged

Fix cache rendering with namespaced resources #874

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 3 additions & 17 deletions app/controllers/apipie/apipies_controller.rb
View file
Original file line number Diff line number Diff line change
@@ -154,26 +154,12 @@ def get_format

def render_from_cache
path = Apipie.configuration.doc_base_url.dup
# some params can contain dot, but only one in row
if [:resource, :method, :format, :version].any? { |p| params[p].to_s.gsub(".", "") =~ /\W/ || params[p].to_s.include?('..') }
head :bad_request and return
end

path << "/" << params[:version] if params[:version].present?
path << "/" << params[:resource] if params[:resource].present?
path << "/" << params[:method] if params[:method].present?
if params[:format].present?
path << ".#{params[:format]}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but we are not adding this anymore?

else
path << ".html"
end

# we sanitize the params before so in ideal case, this condition
# will be never satisfied. It's here for cases somebody adds new
# param into the path later and forgets about sanitation.
if path.include?('..')
head :bad_request and return
end
# Sanitize path against directory traversal attacks (e.g. ../../foo)
# by turning path into an absolute path before appending it to the cache dir
path = File.expand_path("#{path}.#{request.format.symbol}", '/')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you are taking the format from the request, but can it be different?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Sorry I didn't respond earlier - yesterday was really busy.

Since Rails/Rack assemble the request format for us, I figured it was safest to use request.format rather than us needing to sanitize params[:format] ourselves. It's one small security item we don't have to do.

If we had a large variety request formats it might be possible for request.format.symbol to be different from the file extension on the file system, but we only have html and json which are pretty standard. I don't think it can be different.

I'm more than willing to change this in a new PR if you think we should. Thank you for merging it! (And all my other recent PRs - you are super responsive which is not often found 🎉 )


cache_file = File.join(Apipie.configuration.cache_dir, path)
if File.exist?(cache_file)
82 changes: 63 additions & 19 deletions spec/lib/apipie/apipies_controller_spec.rb
View file
Original file line number Diff line number Diff line change
@@ -258,12 +258,12 @@

before do
FileUtils.rm_r(cache_dir) if File.exist?(cache_dir)
FileUtils.mkdir_p(File.join(cache_dir, "apidoc", "v1", "resource"))
FileUtils.mkdir_p(File.join(cache_dir, "apidoc", "v1", "resource-with-namespace"))
File.open(File.join(cache_dir, "apidoc", "v1.html"), "w") { |f| f << "apidoc.html cache v1" }
File.open(File.join(cache_dir, "apidoc", "v2.html"), "w") { |f| f << "apidoc.html cache v2" }
File.open(File.join(cache_dir, "apidoc", "v1.json"), "w") { |f| f << "apidoc.json cache" }
File.open(File.join(cache_dir, "apidoc", "v1", "resource.html"), "w") { |f| f << "resource.html cache" }
File.open(File.join(cache_dir, "apidoc", "v1", "resource", "method.html"), "w") { |f| f << "method.html cache" }
File.open(File.join(cache_dir, "apidoc", "v1", "resource-with-namespace.html"), "w") { |f| f << "resource-with-namespace.html cache" }
File.open(File.join(cache_dir, "apidoc", "v1", "resource-with-namespace", "method.html"), "w") { |f| f << "method.html cache" }

Apipie.configuration.use_cache = true
@orig_cache_dir = Apipie.configuration.cache_dir
@@ -279,24 +279,68 @@
# FileUtils.rm_r(cache_dir) if File.exist?(cache_dir)
end

it "uses the file in cache dir instead of generating the content on runtime" do
get :index
expect(response.body).to eq("apidoc.html cache v1")
get :index, :params => { :version => 'v1' }
expect(response.body).to eq("apidoc.html cache v1")
get :index, :params => { :version => 'v2' }
expect(response.body).to eq("apidoc.html cache v2")
get :index, :params => { :version => 'v1', :format => "html" }
expect(response.body).to eq("apidoc.html cache v1")
get :index, :params => { :version => 'v1', :format => "json" }
expect(response.body).to eq("apidoc.json cache")
get :index, :params => { :version => 'v1', :format => "html", :resource => "resource" }
expect(response.body).to eq("resource.html cache")
get :index, :params => { :version => 'v1', :format => "html", :resource => "resource", :method => "method" }
expect(response.body).to eq("method.html cache")
context 'when the file exists' do
it "uses the file in cache dir instead of generating the content on runtime" do
get :index
expect(response.body).to eq("apidoc.html cache v1")

get :index, :params => { :version => 'v1' }
expect(response.body).to eq("apidoc.html cache v1")

get :index, :params => { :version => 'v2' }
expect(response.body).to eq("apidoc.html cache v2")

get :index, :params => { :version => 'v1', :format => "html" }
expect(response.body).to eq("apidoc.html cache v1")

get :index, :params => { :version => 'v1', :format => "json" }
expect(response.body).to eq("apidoc.json cache")

get :index, :params => { :version => 'v1', :format => "html", :resource => "resource-with-namespace" }
expect(response.body).to eq("resource-with-namespace.html cache")

get :index, :params => { :version => 'v1', :format => "html", :resource => "resource-with-namespace", :method => "method" }
expect(response.body).to eq("method.html cache")
end
end

end
context 'when the file does not exist' do
it 'returns a not found' do
get :index, :params => { :version => 'v3-does-not-exist' }
expect(response).to have_http_status(:not_found)
end
end

context 'preventing path traversal' do
context 'when resource contains ..' do
it "returns a not found" do
get :index, :params => { :version => 'v1', :format => "html", :resource => "../resource-with-namespace", :method => "method" }
expect(response).to have_http_status(:not_found)
end
end

context 'when method contains ..' do
it "returns a not found" do
get :index, :params => { :version => 'v1', :format => "html", :resource => "resource-with-namespace", :method => "../method" }
expect(response).to have_http_status(:not_found)
end
end

context 'when version contains ..' do
it "returns a not found" do
get :index, :params => { :version => '../v1', :format => "html", :resource => "resource-with-namespace", :method => "method" }
expect(response).to have_http_status(:not_found)
end
end

context 'when format contains ..' do
it "returns a not found" do
get :index, :params => { :version => 'v1', :format => "../html", :resource => "resource-with-namespace", :method => "method" }
expect(response).to have_http_status(:not_found)
end
end
end

end

end