Skip to content

G
gitlab-ce
Commit f82d5dca authored by GitLab Bot's avatar GitLab Bot
Browse files
Options

Add latest changes from gitlab-org/gitlab@master

parent 619d0b69
Expand all Hide whitespace changes
Inline Side-by-side
Showing with 884 additions and 628 deletions
app/assets/javascripts/main.js
View file @ f82d5dca
......@@ -69,7 +69,7 @@ if (gon && gon.disable_animations) {
// inject test utilities if necessary
if (process.env.NODE_ENV !== 'production' && gon && gon.test_env) {
disableJQueryAnimations();
import(/* webpackMode: "eager" */ './test_utils/');
import(/* webpackMode: "eager" */ './test_utils/'); // eslint-disable-line no-unused-expressions
}
document.addEventListener('beforeunload', () => {
......
babel.config.js
View file @ f82d5dca
......@@ -18,12 +18,10 @@ const presets = [
// include stage 3 proposals
const plugins = [
'@babel/plugin-syntax-dynamic-import',
'@babel/plugin-syntax-import-meta',
'@babel/plugin-proposal-class-properties',
'@babel/plugin-proposal-json-strings',
'@babel/plugin-proposal-private-methods',
'@babel/plugin-proposal-optional-chaining',
'lodash',
];
......
changelogs/unreleased/file-path-validator.yml 0 → 100644
View file @ f82d5dca
---
title: Add custom validator for validating file path
merge_request: 24223
author: Rajendra Kadam
type: added
doc/development/merge_request_performance_guidelines.md
View file @ f82d5dca
......@@ -397,3 +397,108 @@ changes.
Read more about when and how feature flags should be used in
[Feature flags in GitLab development](feature_flags/process.md#feature-flags-in-gitlab-development).
## Storage
We can consider the following types of storages:
- **Local temporary storage** (very-very short-term storage) This type of storage is system-provided storage, ex. `/tmp` folder.
This is the type of storage that you should ideally use for all your temporary tasks.
The fact that each node has its own temporary storage makes scaling significantly easier.
This storage is also very often SSD-based, thus is significantly faster.
The local storage can easily be configured for the application with
the usage of `TMPDIR` variable.
- **Shared temporary storage** (short-term storage) This type of storage is network-based temporary storage,
usually run with a common NFS server. As of Feb 2020, we still use this type of storage
for most of our implementations. Even though this allows the above limit to be significantly larger,
it does not really mean that you can use more. The shared temporary storage is shared by
all nodes. Thus, the job that uses significant amount of that space or performs a lot
of operations will create a contention on execution of all other jobs and request
across the whole application, this can easily impact stability of the whole GitLab.
Be respectful of that.
- **Shared persistent storage** (long-term storage) This type of storage uses
shared network-based storage (ex. NFS). This solution is mostly used by customers running small
installations consisting of a few nodes. The files on shared storage are easily accessible,
but any job that is uploading or downloading data can create a serious contention to all other jobs.
This is also an approach by default used by Omnibus.
- **Object-based persistent storage** (long term storage) this type of storage uses external
services like [AWS S3](https://en.wikipedia.org/wiki/Amazon_S3). The Object Storage
can be treated as infinitely scalable and redundant. Accessing this storage usually requires
downloading the file in order to manipulate it. The Object Storage can be considered as an ultimate
solution, as by definition it can be assumed that it can handle unlimited concurrent uploads
and downloads of files. This is also ultimate solution required to ensure that application can
run in containerized deployments (Kubernetes) at ease.
### Temporary storage
The storage on production nodes is really sparse. The application should be built
in a way that accomodates running under very limited temporary storage.
You can expect the system on which your code runs has a total of `1G-10G`
of temporary storage. However, this storage is really shared across all
jobs being run. If your job requires to use more than `100MB` of that space
you should reconsider the approach you have taken.
Whatever your needs are, you should clearly document if you need to process files.
If you require more than `100MB`, consider asking for help from a maintainer
to work with you to possibly discover a better solution.
#### Local temporary storage
The usage of local storage is a desired solution to use,
especially since we work on deploying applications to Kubernetes clusters.
When you would like to use `Dir.mktmpdir`? In a case when you want for example
to extract/create archives, perform extensive manipulation of existing data, etc.
```ruby
Dir.mktmpdir('designs') do |path|
# do manipulation on path
# the path will be removed once
# we go out of the block
end
```
#### Shared temporary storage
The usage of shared temporary storage is required if your intent
is to persistent file for a disk-based storage, and not Object Storage.
[Workhorse direct_upload](./uploads.md#direct-upload) when accepting file
can write it to shared storage, and later GitLab Rails can perform a move operation.
The move operation on the same destination is instantaneous.
The system instead of performing `copy` operation just re-attaches file into a new place.
Since this introduces extra complexity into application, you should only try
to re-use well established patterns (ex.: `ObjectStorage` concern) instead of re-implementing it.
The usage of shared temporary storage is otherwise deprecated for all other usages.
### Persistent storage
#### Object Storage
It is required that all features holding persistent files support saving data
to Object Storage. Having a persistent storage in the form of shared volume across nodes
is not scalable, as it creates a contention on data access all nodes.
GitLab offers the [ObjectStorage concern](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/uploaders/object_storage.rb)
that implements a seamless support for Shared and Object Storage-based persistent storage.
#### Data access
Each feature that accepts data uploads or allows to download them needs to use
[Workhorse direct_upload](./uploads.md#direct-upload). It means that uploads needs to be
saved directly to Object Storage by Workhorse, and all downloads needs to be served
by Workhorse.
Performing uploads/downloads via Unicorn/Puma is an expensive operation,
as it blocks the whole processing slot (worker or thread) for the duration of the upload.
Performing uploads/downloads via Unicorn/Puma also has a problem where the operation
can time out, which is especially problematic for slow clients. If clients take a long time
to upload/download the processing slot might be killed due to request processing
timeout (usually between 30s-60s).
For the above reasons it is required that [Workhorse direct_upload](./uploads.md#direct-upload) is implemented
for all file uploads and downloads.
doc/user/admin_area/settings/account_and_limit_settings.md
View file @ f82d5dca
......@@ -15,7 +15,7 @@ If you choose a size larger than what is currently configured for the web server
you will likely get errors. See the [troubleshooting section](#troubleshooting) for more
details.
## Repository size limit **(STARTER)**
## Repository size limit **(STARTER ONLY)**
> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/740) in [GitLab Enterprise Edition 8.12](https://about.gitlab.com/blog/2016/09/22/gitlab-8-12-released/#limit-project-size-ee).
> Available in [GitLab Starter](https://about.gitlab.com/pricing/).
......
lib/api/files.rb
View file @ f82d5dca
......@@ -61,7 +61,7 @@ module API
end
params :simple_file_params do
requires :file_path, type: String, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
requires :file_path, type: String, file_path: true, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
requires :branch, type: String, desc: 'Name of the branch to commit into. To create a new branch, also provide `start_branch`.', allow_blank: false
requires :commit_message, type: String, allow_blank: false, desc: 'Commit message'
optional :start_branch, type: String, desc: 'Name of the branch to start the new commit from'
......@@ -85,7 +85,7 @@ module API
desc 'Get blame file metadata from repository'
params do
requires :file_path, type: String, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
requires :file_path, type: String, file_path: true, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
requires :ref, type: String, desc: 'The name of branch, tag or commit', allow_blank: false
end
head ":id/repository/files/:file_path/blame", requirements: FILE_ENDPOINT_REQUIREMENTS do
......@@ -96,7 +96,7 @@ module API
desc 'Get blame file from the repository'
params do
requires :file_path, type: String, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
requires :file_path, type: String, file_path: true, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
requires :ref, type: String, desc: 'The name of branch, tag or commit', allow_blank: false
end
get ":id/repository/files/:file_path/blame", requirements: FILE_ENDPOINT_REQUIREMENTS do
......@@ -110,7 +110,7 @@ module API
desc 'Get raw file metadata from repository'
params do
requires :file_path, type: String, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
requires :file_path, type: String, file_path: true, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
requires :ref, type: String, desc: 'The name of branch, tag or commit', allow_blank: false
end
head ":id/repository/files/:file_path/raw", requirements: FILE_ENDPOINT_REQUIREMENTS do
......@@ -121,7 +121,7 @@ module API
desc 'Get raw file contents from the repository'
params do
requires :file_path, type: String, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
requires :file_path, type: String, file_path: true, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
requires :ref, type: String, desc: 'The name of branch, tag commit', allow_blank: false
end
get ":id/repository/files/:file_path/raw", requirements: FILE_ENDPOINT_REQUIREMENTS do
......@@ -135,7 +135,7 @@ module API
desc 'Get file metadata from repository'
params do
requires :file_path, type: String, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
requires :file_path, type: String, file_path: true, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
requires :ref, type: String, desc: 'The name of branch, tag or commit', allow_blank: false
end
head ":id/repository/files/:file_path", requirements: FILE_ENDPOINT_REQUIREMENTS do
......@@ -146,7 +146,7 @@ module API
desc 'Get a file from the repository'
params do
requires :file_path, type: String, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
requires :file_path, type: String, file_path: true, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
requires :ref, type: String, desc: 'The name of branch, tag or commit', allow_blank: false
end
get ":id/repository/files/:file_path", requirements: FILE_ENDPOINT_REQUIREMENTS do
......
lib/api/helpers/custom_validators.rb
View file @ f82d5dca
......@@ -3,6 +3,17 @@
module API
module Helpers
module CustomValidators
class FilePath < Grape::Validations::Base
def validate_param!(attr_name, params)
path = params[attr_name]
Gitlab::Utils.check_path_traversal!(path)
rescue StandardError
raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)],
message: "should be a valid file path"
end
end
class Absence < Grape::Validations::Base
def validate_param!(attr_name, params)
return if params.respond_to?(:key?) && !params.key?(attr_name)
......@@ -38,6 +49,7 @@ module API
end
end
Grape::Validations.register_validator(:file_path, ::API::Helpers::CustomValidators::FilePath)
Grape::Validations.register_validator(:absence, ::API::Helpers::CustomValidators::Absence)
Grape::Validations.register_validator(:integer_none_any, ::API::Helpers::CustomValidators::IntegerNoneAny)
Grape::Validations.register_validator(:array_none_any, ::API::Helpers::CustomValidators::ArrayNoneAny)
lib/gitlab/utils.rb
View file @ f82d5dca
......@@ -5,10 +5,20 @@ module Gitlab
extend self
# Ensure that the relative path will not traverse outside the base directory
def check_path_traversal!(path)
raise StandardError.new("Invalid path") if path.start_with?("..#{File::SEPARATOR}") ||
# We url decode the path to avoid passing invalid paths forward in url encoded format.
# We are ok to pass some double encoded paths to File.open since they won't resolve.
# Also see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/24223#note_284122580
# It also checks for ALT_SEPARATOR aka '\' (forward slash)
def check_path_traversal!(path, allowed_absolute: false)
path = CGI.unescape(path)
if path.start_with?("..#{File::SEPARATOR}", "..#{File::ALT_SEPARATOR}") ||
path.include?("#{File::SEPARATOR}..#{File::SEPARATOR}") ||
path.end_with?("#{File::SEPARATOR}..")
path.end_with?("#{File::SEPARATOR}..") ||
(!allowed_absolute && Pathname.new(path).absolute?)
raise StandardError.new("Invalid path")
end
path
end
......
locale/gitlab.pot
View file @ f82d5dca
......@@ -21817,6 +21817,9 @@ msgstr ""
msgid "Vulnerability|Links"
msgstr ""
msgid "Vulnerability|Method"
msgstr ""
msgid "Vulnerability|Namespace"
msgstr ""
......@@ -22935,9 +22938,6 @@ msgstr ""
msgid "ciReport|Base pipeline codequality artifact not found"
msgstr ""
msgid "ciReport|Class"
msgstr ""
msgid "ciReport|Code quality"
msgstr ""
......@@ -22968,9 +22968,6 @@ msgstr ""
msgid "ciReport|Dependency scanning"
msgstr ""
msgid "ciReport|Description"
msgstr ""
msgid "ciReport|Download patch to resolve"
msgstr ""
......@@ -22983,42 +22980,21 @@ msgstr ""
msgid "ciReport|Failed to load %{reportName} report"
msgstr ""
msgid "ciReport|File"
msgstr ""
msgid "ciReport|Fixed:"
msgstr ""
msgid "ciReport|Identifiers"
msgstr ""
msgid "ciReport|Image"
msgstr ""
msgid "ciReport|Instances"
msgstr ""
msgid "ciReport|Investigate this vulnerability by creating an issue"
msgstr ""
msgid "ciReport|Learn more about interacting with security reports"
msgstr ""
msgid "ciReport|Links"
msgstr ""
msgid "ciReport|Loading %{reportName} report"
msgstr ""
msgid "ciReport|Manage licenses"
msgstr ""
msgid "ciReport|Method"
msgstr ""
msgid "ciReport|Namespace"
msgstr ""
msgid "ciReport|No changes to code quality"
msgstr ""
......@@ -23040,9 +23016,6 @@ msgstr ""
msgid "ciReport|Security scanning failed loading any results"
msgstr ""
msgid "ciReport|Severity"
msgstr ""
msgid "ciReport|Solution"
msgstr ""
......
spec/lib/api/helpers/custom_validators_spec.rb
View file @ f82d5dca
......@@ -24,7 +24,38 @@ describe API::Helpers::CustomValidators do
context 'invalid parameters' do
it 'raises a validation error' do
expect_validation_error({ 'test' => 'some_value' })
expect_validation_error('test' => 'some_value')
end
end
end
describe API::Helpers::CustomValidators::FilePath do
subject do
described_class.new(['test'], {}, false, scope.new)
end
context 'valid file path' do
it 'does not raise a validation error' do
expect_no_validation_error('test' => './foo')
expect_no_validation_error('test' => './bar.rb')
expect_no_validation_error('test' => 'foo%2Fbar%2Fnew%2Ffile.rb')
expect_no_validation_error('test' => 'foo%2Fbar%2Fnew')
expect_no_validation_error('test' => 'foo%252Fbar%252Fnew%252Ffile.rb')
end
end
context 'invalid file path' do
it 'raise a validation error' do
expect_validation_error('test' => '../foo')
expect_validation_error('test' => '../')
expect_validation_error('test' => 'foo/../../bar')
expect_validation_error('test' => 'foo/../')
expect_validation_error('test' => 'foo/..')
expect_validation_error('test' => '../')
expect_validation_error('test' => '..\\')
expect_validation_error('test' => '..\/')
expect_validation_error('test' => '%2e%2e%2f')
expect_validation_error('test' => '/etc/passwd')
end
end
end
......@@ -36,12 +67,12 @@ describe API::Helpers::CustomValidators do
context 'valid parameters' do
it 'does not raise a validation error' do
expect_no_validation_error({ 'test' => 2 })
expect_no_validation_error({ 'test' => 100 })
expect_no_validation_error({ 'test' => 'None' })
expect_no_validation_error({ 'test' => 'Any' })
expect_no_validation_error({ 'test' => 'none' })
expect_no_validation_error({ 'test' => 'any' })
expect_no_validation_error('test' => 2)
expect_no_validation_error('test' => 100)
expect_no_validation_error('test' => 'None')
expect_no_validation_error('test' => 'Any')
expect_no_validation_error('test' => 'none')
expect_no_validation_error('test' => 'any')
end
end
......@@ -59,18 +90,18 @@ describe API::Helpers::CustomValidators do
context 'valid parameters' do
it 'does not raise a validation error' do
expect_no_validation_error({ 'test' => [] })
expect_no_validation_error({ 'test' => [1, 2, 3] })
expect_no_validation_error({ 'test' => 'None' })
expect_no_validation_error({ 'test' => 'Any' })
expect_no_validation_error({ 'test' => 'none' })
expect_no_validation_error({ 'test' => 'any' })
expect_no_validation_error('test' => [])
expect_no_validation_error('test' => [1, 2, 3])
expect_no_validation_error('test' => 'None')
expect_no_validation_error('test' => 'Any')
expect_no_validation_error('test' => 'none')
expect_no_validation_error('test' => 'any')
end
end
context 'invalid parameters' do
it 'raises a validation error' do
expect_validation_error({ 'test' => 'some_other_string' })
expect_validation_error('test' => 'some_other_string')
end
end
end
......
spec/lib/gitlab/utils_spec.rb
View file @ f82d5dca
......@@ -31,6 +31,14 @@ describe Gitlab::Utils do
it 'does nothing for a safe string' do
expect(check_path_traversal!('./foo')).to eq('./foo')
end
it 'does nothing if an absolute path is allowed' do
expect(check_path_traversal!('/etc/folder/path', allowed_absolute: true)). to eq('/etc/folder/path')
end
it 'raises exception if an absolute path is not allowed' do
expect { check_path_traversal!('/etc/folder/path') }.to raise_error(/Invalid path/)
end
end
describe '.slugify' do
......
spec/requests/api/files_spec.rb
View file @ f82d5dca
......@@ -7,6 +7,8 @@ describe API::Files do
let!(:project) { create(:project, :repository, namespace: user.namespace ) }
let(:guest) { create(:user) { |u| project.add_guest(u) } }
let(:file_path) { "files%2Fruby%2Fpopen%2Erb" }
let(:rouge_file_path) { "%2e%2e%2f" }
let(:invalid_file_message) { 'file_path should be a valid file path' }
let(:params) do
{
ref: 'master'
......@@ -55,6 +57,12 @@ describe API::Files do
describe "HEAD /projects/:id/repository/files/:file_path" do
shared_examples_for 'repository files' do
it 'returns 400 when file path is invalid' do
head api(route(rouge_file_path), current_user), params: params
expect(response).to have_gitlab_http_status(:bad_request)
end
it 'returns file attributes in headers' do
head api(route(file_path), current_user), params: params
......@@ -145,6 +153,13 @@ describe API::Files do
describe "GET /projects/:id/repository/files/:file_path" do
shared_examples_for 'repository files' do
it 'returns 400 for invalid file path' do
get api(route(rouge_file_path), current_user), params: params
expect(response).to have_gitlab_http_status(:bad_request)
expect(json_response['error']).to eq(invalid_file_message)
end
it 'returns file attributes as json' do
get api(route(file_path), current_user), params: params
......@@ -302,6 +317,13 @@ describe API::Files do
.to eq('c440cd09bae50c4632cc58638ad33c6aa375b6109d811e76a9cc3a613c1e8887')
end
it 'returns 400 when file path is invalid' do
get api(route(rouge_file_path) + '/blame', current_user), params: params
expect(response).to have_gitlab_http_status(:bad_request)
expect(json_response['error']).to eq(invalid_file_message)
end
it 'returns blame file attributes as json' do
get api(route(file_path) + '/blame', current_user), params: params
......@@ -418,6 +440,13 @@ describe API::Files do
describe "GET /projects/:id/repository/files/:file_path/raw" do
shared_examples_for 'repository raw files' do
it 'returns 400 when file path is invalid' do
get api(route(rouge_file_path) + "/raw", current_user), params: params
expect(response).to have_gitlab_http_status(:bad_request)
expect(json_response['error']).to eq(invalid_file_message)
end
it 'returns raw file info' do
url = route(file_path) + "/raw"
expect(Gitlab::Workhorse).to receive(:send_git_blob)
......@@ -535,6 +564,13 @@ describe API::Files do
}
end
it 'returns 400 when file path is invalid' do
post api(route(rouge_file_path), user), params: params
expect(response).to have_gitlab_http_status(:bad_request)
expect(json_response['error']).to eq(invalid_file_message)
end
it "creates a new file in project repo" do
post api(route(file_path), user), params: params
......@@ -662,6 +698,17 @@ describe API::Files do
expect(response).to have_gitlab_http_status(:ok)
end
it "returns 400 when file path is invalid" do
last_commit = Gitlab::Git::Commit
.last_for_path(project.repository, 'master', URI.unescape(file_path))
params_with_correct_id = params.merge(last_commit_id: last_commit.id)
put api(route(rouge_file_path), user), params: params_with_correct_id
expect(response).to have_gitlab_http_status(:bad_request)
expect(json_response['error']).to eq(invalid_file_message)
end
it "returns a 400 bad request if no params given" do
put api(route(file_path), user)
......@@ -690,6 +737,13 @@ describe API::Files do
}
end
it 'returns 400 when file path is invalid' do
delete api(route(rouge_file_path), user), params: params
expect(response).to have_gitlab_http_status(:bad_request)
expect(json_response['error']).to eq(invalid_file_message)
end
it "deletes existing file in project repo" do
delete api(route(file_path), user), params: params
......
yarn.lock
View file @ f82d5dca
This diff is collapsed.
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
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7