Skip to content

G
gitlab-ce
Commit 679b81e8 authored by Rémy Coutable's avatar Rémy Coutable
Browse files
Options

Merge branch '233895-remove-upload-middleware-jwt-params-handler-feature-flag' into 'master' 

Remove the `upload_middleware_jwt_params_handler` feature flag

See merge request gitlab-org/gitlab!50840
parents bf1e9fb8 c7035352
Hide whitespace changes
Inline Side-by-side
Showing with 60 additions and 664 deletions
config/feature_flags/development/upload_middleware_jwt_params_handler.yml deleted 100644 → 0
View file @ bf1e9fb8
---
name: upload_middleware_jwt_params_handler
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/33277
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/233895
milestone: '13.4'
type: development
group: group::package
default_enabled: true
doc/administration/uploads.md
View file @ 679b81e8
...@@ -8,35 +8,6 @@ info: To determine the technical writer assigned to the Stage/Group associated w ...@@ -8,35 +8,6 @@ info: To determine the technical writer assigned to the Stage/Group associated w
Uploads represent all user data that may be sent to GitLab as a single file. As an example, avatars and notes' attachments are uploads. Uploads are integral to GitLab functionality, and therefore cannot be disabled. Uploads represent all user data that may be sent to GitLab as a single file. As an example, avatars and notes' attachments are uploads. Uploads are integral to GitLab functionality, and therefore cannot be disabled.
## Upload parameters
> - [Changed](https://gitlab.com/gitlab-org/gitlab/-/issues/214785) in GitLab 13.5.
> - It's [deployed behind a feature flag](../user/feature_flags.md), enabled by default.
> - It's enabled on GitLab.com.
> - It's recommended for production use.
> - For GitLab self-managed instances, GitLab administrators can opt to disable it. **(CORE ONLY)**
In 13.5 and later, upload parameters are passed [between Workhorse and GitLab Rails](../development/architecture.md#simplified-component-overview) differently than they
were before.
This change is deployed behind a feature flag that is **enabled by default**.
If you experience any issues with upload,
[GitLab administrators with access to the GitLab Rails console](feature_flags.md)
can opt to disable it.
To enable it:
```ruby
Feature.enable(:upload_middleware_jwt_params_handler)
```
To disable it:
```ruby
Feature.disable(:upload_middleware_jwt_params_handler)
```
## Using local storage ## Using local storage
This is the default configuration. To change the location where the uploads are This is the default configuration. To change the location where the uploads are
......
lib/gitlab/middleware/multipart.rb
View file @ 679b81e8
...@@ -41,7 +41,7 @@ module Gitlab ...@@ -41,7 +41,7 @@ module Gitlab
end end
def with_open_files def with_open_files
@rewritten_fields.each do |field, tmp_path| @rewritten_fields.keys.each do |field|
raise "invalid field: #{field.inspect}" unless valid_field_name?(field) raise "invalid field: #{field.inspect}" unless valid_field_name?(field)
parsed_field = Rack::Utils.parse_nested_query(field) parsed_field = Rack::Utils.parse_nested_query(field)
...@@ -51,10 +51,10 @@ module Gitlab ...@@ -51,10 +51,10 @@ module Gitlab
if value.nil? # we have a top level param, eg. field = 'foo' and not 'foo[bar]' if value.nil? # we have a top level param, eg. field = 'foo' and not 'foo[bar]'
raise "invalid field: #{field.inspect}" if field != key raise "invalid field: #{field.inspect}" if field != key
value = open_file(@request.params, key, tmp_path.presence) value = open_file(extract_upload_params_from(@request.params, with_prefix: key))
@open_files << value @open_files << value
else else
value = decorate_params_value(value, @request.params[key], tmp_path.presence) value = decorate_params_value(value, @request.params[key])
end end
update_param(key, value) update_param(key, value)
...@@ -67,12 +67,12 @@ module Gitlab ...@@ -67,12 +67,12 @@ module Gitlab
end end
# This function calls itself recursively # This function calls itself recursively
def decorate_params_value(path_hash, value_hash, path_override = nil) def decorate_params_value(hash_path, value_hash)
unless path_hash.is_a?(Hash) && path_hash.count == 1 unless hash_path.is_a?(Hash) && hash_path.count == 1
raise "invalid path: #{path_hash.inspect}" raise "invalid path: #{hash_path.inspect}"
end end
path_key, path_value = path_hash.first path_key, path_value = hash_path.first
unless value_hash.is_a?(Hash) && value_hash[path_key] unless value_hash.is_a?(Hash) && value_hash[path_key]
raise "invalid value hash: #{value_hash.inspect}" raise "invalid value hash: #{value_hash.inspect}"
...@@ -80,19 +80,19 @@ module Gitlab ...@@ -80,19 +80,19 @@ module Gitlab
case path_value case path_value
when nil when nil
value_hash[path_key] = open_file(value_hash.dig(path_key), '', path_override) value_hash[path_key] = open_file(extract_upload_params_from(value_hash[path_key]))
@open_files << value_hash[path_key] @open_files << value_hash[path_key]
value_hash value_hash
when Hash when Hash
decorate_params_value(path_value, value_hash[path_key], path_override) decorate_params_value(path_value, value_hash[path_key])
value_hash value_hash
else else
raise "unexpected path value: #{path_value.inspect}" raise "unexpected path value: #{path_value.inspect}"
end end
end end
def open_file(params, key, path_override = nil) def open_file(params)
::UploadedFile.from_params(params, key, allowed_paths, path_override) ::UploadedFile.from_params(params, allowed_paths)
end end
# update_params ensures that both rails controllers and rack middleware can find # update_params ensures that both rails controllers and rack middleware can find
...@@ -111,6 +111,20 @@ module Gitlab ...@@ -111,6 +111,20 @@ module Gitlab
private private
def extract_upload_params_from(params, with_prefix: '')
param_key = "#{with_prefix}#{JWT_PARAM_SUFFIX}"
jwt_token = params[param_key]
raise "Empty JWT param: #{param_key}" if jwt_token.blank?
payload = Gitlab::Workhorse.decode_jwt(jwt_token).first
raise "Invalid JWT payload: not a Hash" unless payload.is_a?(Hash)
upload_params = payload.fetch(JWT_PARAM_FIXED_KEY, {})
raise "Empty params for: #{param_key}" if upload_params.empty?
upload_params
end
def valid_field_name?(name) def valid_field_name?(name)
# length validation # length validation
return false if name.size >= REWRITTEN_FIELD_NAME_MAX_LENGTH return false if name.size >= REWRITTEN_FIELD_NAME_MAX_LENGTH
...@@ -149,82 +163,6 @@ module Gitlab ...@@ -149,82 +163,6 @@ module Gitlab
end end
end end
# TODO this class is meant to replace Handler when the feature flag
# upload_middleware_jwt_params_handler is removed
# See https://gitlab.com/gitlab-org/gitlab/-/issues/233895#roll-out-steps
class HandlerForJWTParams < Handler
def with_open_files
@rewritten_fields.keys.each do |field|
raise "invalid field: #{field.inspect}" unless valid_field_name?(field)
parsed_field = Rack::Utils.parse_nested_query(field)
raise "unexpected field: #{field.inspect}" unless parsed_field.count == 1
key, value = parsed_field.first
if value.nil? # we have a top level param, eg. field = 'foo' and not 'foo[bar]'
raise "invalid field: #{field.inspect}" if field != key
value = open_file(extract_upload_params_from(@request.params, with_prefix: key))
@open_files << value
else
value = decorate_params_value(value, @request.params[key])
end
update_param(key, value)
end
yield
ensure
@open_files.compact
.each(&:close)
end
# This function calls itself recursively
def decorate_params_value(hash_path, value_hash)
unless hash_path.is_a?(Hash) && hash_path.count == 1
raise "invalid path: #{hash_path.inspect}"
end
path_key, path_value = hash_path.first
unless value_hash.is_a?(Hash) && value_hash[path_key]
raise "invalid value hash: #{value_hash.inspect}"
end
case path_value
when nil
value_hash[path_key] = open_file(extract_upload_params_from(value_hash[path_key]))
@open_files << value_hash[path_key]
value_hash
when Hash
decorate_params_value(path_value, value_hash[path_key])
value_hash
else
raise "unexpected path value: #{path_value.inspect}"
end
end
def open_file(params)
::UploadedFile.from_params_without_field(params, allowed_paths)
end
private
def extract_upload_params_from(params, with_prefix: '')
param_key = "#{with_prefix}#{JWT_PARAM_SUFFIX}"
jwt_token = params[param_key]
raise "Empty JWT param: #{param_key}" if jwt_token.blank?
payload = Gitlab::Workhorse.decode_jwt(jwt_token).first
raise "Invalid JWT payload: not a Hash" unless payload.is_a?(Hash)
upload_params = payload.fetch(JWT_PARAM_FIXED_KEY, {})
raise "Empty params for: #{param_key}" if upload_params.empty?
upload_params
end
end
def initialize(app) def initialize(app)
@app = app @app = app
end end
...@@ -235,22 +173,12 @@ module Gitlab ...@@ -235,22 +173,12 @@ module Gitlab
message = ::Gitlab::Workhorse.decode_jwt(encoded_message)[0] message = ::Gitlab::Workhorse.decode_jwt(encoded_message)[0]
handler_class.new(env, message).with_open_files do ::Gitlab::Middleware::Multipart::Handler.new(env, message).with_open_files do
@app.call(env) @app.call(env)
end end
rescue UploadedFile::InvalidPathError => e rescue UploadedFile::InvalidPathError => e
[400, { 'Content-Type' => 'text/plain' }, e.message] [400, { 'Content-Type' => 'text/plain' }, e.message]
end end
private
def handler_class
if Feature.enabled?(:upload_middleware_jwt_params_handler, default_enabled: true)
::Gitlab::Middleware::Multipart::HandlerForJWTParams
else
::Gitlab::Middleware::Multipart::Handler
end
end
end end
end end
end end
lib/uploaded_file.rb
View file @ 679b81e8
...@@ -42,10 +42,7 @@ class UploadedFile ...@@ -42,10 +42,7 @@ class UploadedFile
@remote_id = remote_id @remote_id = remote_id
end end
# TODO this function is meant to replace .from_params when the feature flag def self.from_params(params, upload_paths)
# upload_middleware_jwt_params_handler is removed
# See https://gitlab.com/gitlab-org/gitlab/-/issues/233895#roll-out-steps
def self.from_params_without_field(params, upload_paths)
path = params['path'] path = params['path']
remote_id = params['remote_id'] remote_id = params['remote_id']
return if path.blank? && remote_id.blank? return if path.blank? && remote_id.blank?
...@@ -71,33 +68,6 @@ class UploadedFile ...@@ -71,33 +68,6 @@ class UploadedFile
) )
end end
# Deprecated. Don't use it.
# .from_params_without_field will replace this one
# See .from_params_without_field and
# https://gitlab.com/gitlab-org/gitlab/-/issues/233895#roll-out-steps
def self.from_params(params, field, upload_paths, path_override = nil)
path = path_override || params["#{field}.path"]
remote_id = params["#{field}.remote_id"]
return if path.blank? && remote_id.blank?
if remote_id.present? # don't use file_path if remote_id is set
file_path = nil
elsif path.present?
file_path = File.realpath(path)
unless self.allowed_path?(file_path, Array(upload_paths).compact)
raise InvalidPathError, "insecure path used '#{file_path}'"
end
end
UploadedFile.new(file_path,
filename: params["#{field}.name"],
content_type: params["#{field}.type"] || 'application/octet-stream',
sha256: params["#{field}.sha256"],
remote_id: remote_id,
size: params["#{field}.size"])
end
def self.allowed_path?(file_path, paths) def self.allowed_path?(file_path, paths)
paths.any? do |path| paths.any? do |path|
File.exist?(path) && file_path.start_with?(File.realpath(path)) File.exist?(path) && file_path.start_with?(File.realpath(path))
......
spec/lib/gitlab/middleware/multipart/handler_for_jwt_params_spec.rb deleted 100644 → 0
View file @ bf1e9fb8
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Gitlab::Middleware::Multipart::HandlerForJWTParams do
using RSpec::Parameterized::TableSyntax
let_it_be(:env) { Rack::MockRequest.env_for('/', method: 'post', params: {}) }
let_it_be(:message) { { 'rewritten_fields' => {} } }
describe '#allowed_paths' do
let_it_be(:expected_allowed_paths) do
[
Dir.tmpdir,
::FileUploader.root,
::Gitlab.config.uploads.storage_path,
::JobArtifactUploader.workhorse_upload_path,
::LfsObjectUploader.workhorse_upload_path,
File.join(Rails.root, 'public/uploads/tmp')
]
end
let_it_be(:expected_with_packages_path) { expected_allowed_paths + [::Packages::PackageFileUploader.workhorse_upload_path] }
subject { described_class.new(env, message).send(:allowed_paths) }
where(:package_features_enabled, :object_storage_enabled, :direct_upload_enabled, :expected_paths) do
false | false | true | :expected_allowed_paths
false | false | false | :expected_allowed_paths
false | true | true | :expected_allowed_paths
false | true | false | :expected_allowed_paths
true | false | true | :expected_with_packages_path
true | false | false | :expected_with_packages_path
true | true | true | :expected_allowed_paths
true | true | false | :expected_with_packages_path
end
with_them do
before do
stub_config(packages: {
enabled: package_features_enabled,
object_store: {
enabled: object_storage_enabled,
direct_upload: direct_upload_enabled
},
storage_path: '/any/dir'
})
end
it { is_expected.to eq(send(expected_paths)) }
end
end
end
spec/lib/gitlab/middleware/multipart_with_handler_for_jwt_params_spec.rb → spec/lib/gitlab/middleware/multipart_spec.rb
View file @ 679b81e8
...@@ -21,10 +21,6 @@ RSpec.describe Gitlab::Middleware::Multipart do ...@@ -21,10 +21,6 @@ RSpec.describe Gitlab::Middleware::Multipart do
middleware.call(env) middleware.call(env)
end end
before do
stub_feature_flags(upload_middleware_jwt_params_handler: true)
end
context 'remote file mode' do context 'remote file mode' do
let(:mode) { :remote } let(:mode) { :remote }
...@@ -34,7 +30,7 @@ RSpec.describe Gitlab::Middleware::Multipart do ...@@ -34,7 +30,7 @@ RSpec.describe Gitlab::Middleware::Multipart do
include_context 'with one temporary file for multipart' include_context 'with one temporary file for multipart'
let(:rewritten_fields) { rewritten_fields_hash('file' => uploaded_filepath) } let(:rewritten_fields) { rewritten_fields_hash('file' => uploaded_filepath) }
let(:params) { upload_parameters_for(key: 'file', filename: filename, remote_id: remote_id).merge('file.path' => '/should/not/be/read') } let(:params) { upload_parameters_for(key: 'file', mode: mode, filename: filename, remote_id: remote_id).merge('file.path' => '/should/not/be/read') }
it 'builds an UploadedFile' do it 'builds an UploadedFile' do
expect_uploaded_files(original_filename: filename, remote_id: remote_id, size: uploaded_file.size, params_path: %w(file)) expect_uploaded_files(original_filename: filename, remote_id: remote_id, size: uploaded_file.size, params_path: %w(file))
...@@ -55,14 +51,14 @@ RSpec.describe Gitlab::Middleware::Multipart do ...@@ -55,14 +51,14 @@ RSpec.describe Gitlab::Middleware::Multipart do
let(:allowed_paths) { [Dir.tmpdir] } let(:allowed_paths) { [Dir.tmpdir] }
before do before do
expect_next_instance_of(::Gitlab::Middleware::Multipart::HandlerForJWTParams) do |handler| expect_next_instance_of(::Gitlab::Middleware::Multipart::Handler) do |handler|
expect(handler).to receive(:allowed_paths).and_return(allowed_paths) expect(handler).to receive(:allowed_paths).and_return(allowed_paths)
end end
end end
context 'in allowed paths' do context 'in allowed paths' do
let(:rewritten_fields) { rewritten_fields_hash('file' => uploaded_filepath) } let(:rewritten_fields) { rewritten_fields_hash('file' => uploaded_filepath) }
let(:params) { upload_parameters_for(filepath: uploaded_filepath, key: 'file', filename: filename) } let(:params) { upload_parameters_for(filepath: uploaded_filepath, key: 'file', mode: mode, filename: filename) }
it 'builds an UploadedFile' do it 'builds an UploadedFile' do
expect_uploaded_files(filepath: uploaded_filepath, original_filename: filename, size: uploaded_file.size, params_path: %w(file)) expect_uploaded_files(filepath: uploaded_filepath, original_filename: filename, size: uploaded_file.size, params_path: %w(file))
...@@ -75,7 +71,7 @@ RSpec.describe Gitlab::Middleware::Multipart do ...@@ -75,7 +71,7 @@ RSpec.describe Gitlab::Middleware::Multipart do
let(:allowed_paths) { [] } let(:allowed_paths) { [] }
let(:rewritten_fields) { rewritten_fields_hash('file' => uploaded_filepath) } let(:rewritten_fields) { rewritten_fields_hash('file' => uploaded_filepath) }
let(:params) { upload_parameters_for(filepath: uploaded_filepath, key: 'file') } let(:params) { upload_parameters_for(filepath: uploaded_filepath, key: 'file', mode: mode) }
it 'returns an error' do it 'returns an error' do
result = subject result = subject
...@@ -89,7 +85,7 @@ RSpec.describe Gitlab::Middleware::Multipart do ...@@ -89,7 +85,7 @@ RSpec.describe Gitlab::Middleware::Multipart do
context 'with dummy params in remote mode' do context 'with dummy params in remote mode' do
let(:rewritten_fields) { { 'file' => 'should/not/be/read' } } let(:rewritten_fields) { { 'file' => 'should/not/be/read' } }
let(:params) { upload_parameters_for(key: 'file') } let(:params) { upload_parameters_for(key: 'file', mode: mode) }
let(:mode) { :remote } let(:mode) { :remote }
context 'with an invalid secret' do context 'with an invalid secret' do
...@@ -128,7 +124,7 @@ RSpec.describe Gitlab::Middleware::Multipart do ...@@ -128,7 +124,7 @@ RSpec.describe Gitlab::Middleware::Multipart do
RSpec.shared_examples 'rejecting the invalid key' do |key_in_header:, key_in_upload_params:, error_message:| RSpec.shared_examples 'rejecting the invalid key' do |key_in_header:, key_in_upload_params:, error_message:|
let(:rewritten_fields) { rewritten_fields_hash(key_in_header => uploaded_filepath) } let(:rewritten_fields) { rewritten_fields_hash(key_in_header => uploaded_filepath) }
let(:params) { upload_parameters_for(filepath: uploaded_filepath, key: key_in_upload_params, filename: filename, remote_id: remote_id) } let(:params) { upload_parameters_for(filepath: uploaded_filepath, key: key_in_upload_params, mode: mode, filename: filename, remote_id: remote_id) }
it 'raises an error' do it 'raises an error' do
expect { subject }.to raise_error(RuntimeError, error_message) expect { subject }.to raise_error(RuntimeError, error_message)
...@@ -171,7 +167,7 @@ RSpec.describe Gitlab::Middleware::Multipart do ...@@ -171,7 +167,7 @@ RSpec.describe Gitlab::Middleware::Multipart do
let(:rewritten_fields) { rewritten_fields_hash('file' => uploaded_filepath) } let(:rewritten_fields) { rewritten_fields_hash('file' => uploaded_filepath) }
let(:crafted_payload) { Base64.urlsafe_encode64({ 'path' => 'test' }.to_json) } let(:crafted_payload) { Base64.urlsafe_encode64({ 'path' => 'test' }.to_json) }
let(:params) do let(:params) do
upload_parameters_for(filepath: uploaded_filepath, key: 'file', filename: filename, remote_id: remote_id).tap do |params| upload_parameters_for(filepath: uploaded_filepath, key: 'file', mode: mode, filename: filename, remote_id: remote_id).tap do |params|
header, _, sig = params['file.gitlab-workhorse-upload'].split('.') header, _, sig = params['file.gitlab-workhorse-upload'].split('.')
params['file.gitlab-workhorse-upload'] = [header, crafted_payload, sig].join('.') params['file.gitlab-workhorse-upload'] = [header, crafted_payload, sig].join('.')
end end
...@@ -187,7 +183,7 @@ RSpec.describe Gitlab::Middleware::Multipart do ...@@ -187,7 +183,7 @@ RSpec.describe Gitlab::Middleware::Multipart do
let(:rewritten_fields) { rewritten_fields_hash('file' => uploaded_filepath) } let(:rewritten_fields) { rewritten_fields_hash('file' => uploaded_filepath) }
let(:params) do let(:params) do
upload_parameters_for(filepath: uploaded_filepath, key: 'file', filename: filename, remote_id: remote_id).tap do |params| upload_parameters_for(filepath: uploaded_filepath, key: 'file', mode: mode, filename: filename, remote_id: remote_id).tap do |params|
header, payload, sig = params['file.gitlab-workhorse-upload'].split('.') header, payload, sig = params['file.gitlab-workhorse-upload'].split('.')
params['file.gitlab-workhorse-upload'] = [header, payload, "#{sig}modified"].join('.') params['file.gitlab-workhorse-upload'] = [header, payload, "#{sig}modified"].join('.')
end end
......
spec/lib/gitlab/middleware/multipart_with_handler_spec.rb deleted 100644 → 0
View file @ bf1e9fb8
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Gitlab::Middleware::Multipart do
include MultipartHelpers
describe '#call' do
let(:app) { double(:app) }
let(:middleware) { described_class.new(app) }
let(:secret) { Gitlab::Workhorse.secret }
let(:issuer) { 'gitlab-workhorse' }
subject do
env = post_env(
rewritten_fields: rewritten_fields,
params: params,
secret: secret,
issuer: issuer
)
middleware.call(env)
end
before do
stub_feature_flags(upload_middleware_jwt_params_handler: false)
end
context 'remote file mode' do
let(:mode) { :remote }
it_behaves_like 'handling all upload parameters conditions'
context 'and a path set' do
include_context 'with one temporary file for multipart'
let(:rewritten_fields) { rewritten_fields_hash('file' => uploaded_filepath) }
let(:params) { upload_parameters_for(key: 'file', filename: filename, remote_id: remote_id).merge('file.path' => '/should/not/be/read') }
it 'builds an UploadedFile' do
expect_uploaded_files(original_filename: filename, remote_id: remote_id, size: uploaded_file.size, params_path: %w(file))
subject
end
end
end
context 'local file mode' do
let(:mode) { :local }
it_behaves_like 'handling all upload parameters conditions'
context 'when file is' do
include_context 'with one temporary file for multipart'
let(:allowed_paths) { [Dir.tmpdir] }
before do
expect_next_instance_of(::Gitlab::Middleware::Multipart::Handler) do |handler|
expect(handler).to receive(:allowed_paths).and_return(allowed_paths)
end
end
context 'in allowed paths' do
let(:rewritten_fields) { rewritten_fields_hash('file' => uploaded_filepath) }
let(:params) { upload_parameters_for(filepath: uploaded_filepath, key: 'file', filename: filename) }
it 'builds an UploadedFile' do
expect_uploaded_files(filepath: uploaded_filepath, original_filename: filename, size: uploaded_file.size, params_path: %w(file))
subject
end
end
context 'not in allowed paths' do
let(:allowed_paths) { [] }
let(:rewritten_fields) { rewritten_fields_hash('file' => uploaded_filepath) }
let(:params) { upload_parameters_for(filepath: uploaded_filepath, key: 'file') }
it 'returns an error' do
result = subject
expect(result[0]).to eq(400)
expect(result[2]).to include('insecure path used')
end
end
end
end
context 'with dummy params in remote mode' do
let(:rewritten_fields) { { 'file' => 'should/not/be/read' } }
let(:params) { upload_parameters_for(key: 'file') }
let(:mode) { :remote }
context 'with an invalid secret' do
let(:secret) { 'INVALID_SECRET' }
it { expect { subject }.to raise_error(JWT::VerificationError) }
end
context 'with an invalid issuer' do
let(:issuer) { 'INVALID_ISSUER' }
it { expect { subject }.to raise_error(JWT::InvalidIssuerError) }
end
context 'with invalid rewritten field key' do
invalid_keys = [
'[file]',
';file',
'file]',
';file]',
'file]]',
'file;;'
]
invalid_keys.each do |invalid_key|
context invalid_key do
let(:rewritten_fields) { { invalid_key => 'should/not/be/read' } }
it { expect { subject }.to raise_error(RuntimeError, "invalid field: \"#{invalid_key}\"") }
end
end
end
context 'with invalid key in parameters' do
include_context 'with one temporary file for multipart'
let(:rewritten_fields) { rewritten_fields_hash('file' => uploaded_filepath) }
let(:params) { upload_parameters_for(filepath: uploaded_filepath, key: 'wrong_key', filename: filename, remote_id: remote_id) }
it 'builds no UploadedFile' do
expect(app).to receive(:call) do |env|
received_params = get_params(env)
expect(received_params['file']).to be_nil
expect(received_params['wrong_key']).to be_nil
end
subject
end
end
context 'with invalid key in header' do
include_context 'with one temporary file for multipart'
RSpec.shared_examples 'rejecting the invalid key' do |key_in_header:, key_in_upload_params:, error_message:|
let(:rewritten_fields) { rewritten_fields_hash(key_in_header => uploaded_filepath) }
let(:params) { upload_parameters_for(filepath: uploaded_filepath, key: key_in_upload_params, filename: filename, remote_id: remote_id) }
it 'raises an error' do
expect { subject }.to raise_error(RuntimeError, error_message)
end
end
it_behaves_like 'rejecting the invalid key',
key_in_header: 'user[avatar',
key_in_upload_params: 'user[avatar]',
error_message: 'invalid field: "user[avatar"'
it_behaves_like 'rejecting the invalid key',
key_in_header: '[user]avatar',
key_in_upload_params: 'user[avatar]',
error_message: 'invalid field: "[user]avatar"'
it_behaves_like 'rejecting the invalid key',
key_in_header: 'user[]avatar',
key_in_upload_params: 'user[avatar]',
error_message: 'invalid field: "user[]avatar"'
it_behaves_like 'rejecting the invalid key',
key_in_header: 'user[avatar[image[url]]]',
key_in_upload_params: 'user[avatar]',
error_message: 'invalid field: "user[avatar[image[url]]]"'
it_behaves_like 'rejecting the invalid key',
key_in_header: '[]',
key_in_upload_params: 'user[avatar]',
error_message: 'invalid field: "[]"'
it_behaves_like 'rejecting the invalid key',
key_in_header: 'x' * 11000,
key_in_upload_params: 'user[avatar]',
error_message: "invalid field: \"#{'x' * 11000}\""
end
context 'with key with unbalanced brackets in header' do
include_context 'with one temporary file for multipart'
let(:invalid_key) { 'user[avatar' }
let(:rewritten_fields) { rewritten_fields_hash( invalid_key => uploaded_filepath) }
let(:params) { upload_parameters_for(filepath: uploaded_filepath, key: 'user[avatar]', filename: filename, remote_id: remote_id) }
it 'builds no UploadedFile' do
expect(app).not_to receive(:call)
expect { subject }.to raise_error(RuntimeError, "invalid field: \"#{invalid_key}\"")
end
end
end
end
end
spec/lib/uploaded_file_spec.rb
View file @ 679b81e8
...@@ -27,12 +27,12 @@ RSpec.describe UploadedFile do ...@@ -27,12 +27,12 @@ RSpec.describe UploadedFile do
end end
it 'handles a blank path' do it 'handles a blank path' do
params['file.path'] = '' params['path'] = ''
# Not a real file, so can't determine size itself # Not a real file, so can't determine size itself
params['file.size'] = 1.byte params['size'] = 1.byte
expect { described_class.from_params(params, :file, upload_path) } expect { described_class.from_params(params, upload_path) }
.not_to raise_error .not_to raise_error
end end
end end
...@@ -50,7 +50,7 @@ RSpec.describe UploadedFile do ...@@ -50,7 +50,7 @@ RSpec.describe UploadedFile do
end end
end end
describe '.from_params_without_field' do describe '.from_params' do
let(:upload_path) { nil } let(:upload_path) { nil }
after do after do
...@@ -58,7 +58,7 @@ RSpec.describe UploadedFile do ...@@ -58,7 +58,7 @@ RSpec.describe UploadedFile do
end end
subject do subject do
described_class.from_params_without_field(params, [upload_path, Dir.tmpdir]) described_class.from_params(params, [upload_path, Dir.tmpdir])
end end
context 'when valid file is specified' do context 'when valid file is specified' do
...@@ -170,190 +170,6 @@ RSpec.describe UploadedFile do ...@@ -170,190 +170,6 @@ RSpec.describe UploadedFile do
end end
end end
end end
describe '.from_params' do
let(:upload_path) { nil }
let(:file_path_override) { nil }
after do
FileUtils.rm_r(upload_path) if upload_path
end
subject do
described_class.from_params(params, :file, [upload_path, Dir.tmpdir], file_path_override)
end
RSpec.shared_context 'filepath override' do
let(:temp_file_override) { Tempfile.new(%w[override override], temp_dir) }
let(:file_path_override) { temp_file_override.path }
before do
FileUtils.touch(temp_file_override)
end
after do
FileUtils.rm_f(temp_file_override)
end
end
context 'when valid file is specified' do
context 'only local path is specified' do
let(:params) do
{ 'file.path' => temp_file.path }
end
it { is_expected.not_to be_nil }
it "generates filename from path" do
expect(subject.original_filename).to eq(::File.basename(temp_file.path))
end
end
context 'all parameters are specified' do
context 'with a filepath' do
let(:params) do
{ 'file.path' => temp_file.path,
'file.name' => 'dir/my file&.txt',
'file.type' => 'my/type',
'file.sha256' => 'sha256' }
end
it_behaves_like 'using the file path',
filename: 'my_file_.txt',
content_type: 'my/type',
sha256: 'sha256',
path_suffix: 'test'
end
context 'with a filepath override' do
include_context 'filepath override'
let(:params) do
{ 'file.path' => temp_file.path,
'file.name' => 'dir/my file&.txt',
'file.type' => 'my/type',
'file.sha256' => 'sha256' }
end
it_behaves_like 'using the file path',
filename: 'my_file_.txt',
content_type: 'my/type',
sha256: 'sha256',
path_suffix: 'override'
end
context 'with a remote id' do
let(:params) do
{
'file.name' => 'dir/my file&.txt',
'file.sha256' => 'sha256',
'file.remote_url' => 'http://localhost/file',
'file.remote_id' => '1234567890',
'file.etag' => 'etag1234567890',
'file.size' => '123456'
}
end
it_behaves_like 'using the remote id',
filename: 'my_file_.txt',
content_type: 'application/octet-stream',
sha256: 'sha256',
size: 123456,
remote_id: '1234567890'
end
context 'with a path and a remote id' do
let(:params) do
{
'file.path' => temp_file.path,
'file.name' => 'dir/my file&.txt',
'file.sha256' => 'sha256',
'file.remote_url' => 'http://localhost/file',
'file.remote_id' => '1234567890',
'file.etag' => 'etag1234567890',
'file.size' => '123456'
}
end
it_behaves_like 'using the remote id',
filename: 'my_file_.txt',
content_type: 'application/octet-stream',
sha256: 'sha256',
size: 123456,
remote_id: '1234567890'
end
context 'with a path override and a remote id' do
include_context 'filepath override'
let(:params) do
{
'file.name' => 'dir/my file&.txt',
'file.sha256' => 'sha256',
'file.remote_url' => 'http://localhost/file',
'file.remote_id' => '1234567890',
'file.etag' => 'etag1234567890',
'file.size' => '123456'
}
end
it_behaves_like 'using the remote id',
filename: 'my_file_.txt',
content_type: 'application/octet-stream',
sha256: 'sha256',
size: 123456,
remote_id: '1234567890'
end
end
end
context 'when no params are specified' do
let(:params) do
{}
end
it "does not return an object" do
is_expected.to be_nil
end
end
context 'when verifying allowed paths' do
let(:params) do
{ 'file.path' => temp_file.path }
end
context 'when file is stored in system temporary folder' do
let(:temp_dir) { Dir.tmpdir }
it "succeeds" do
is_expected.not_to be_nil
end
end
context 'when file is stored in user provided upload path' do
let(:upload_path) { Dir.mktmpdir }
let(:temp_dir) { upload_path }
it "succeeds" do
is_expected.not_to be_nil
end
end
context 'when file is stored outside of user provided upload path' do
let!(:generated_dir) { Dir.mktmpdir }
let!(:temp_dir) { Dir.mktmpdir }
before do
# We overwrite default temporary path
allow(Dir).to receive(:tmpdir).and_return(generated_dir)
end
it "raises an error" do
expect { subject }.to raise_error(UploadedFile::InvalidPathError, /insecure path used/)
end
end
end
end
end end
describe '.initialize' do describe '.initialize' do
......
spec/support/helpers/multipart_helpers.rb
View file @ 679b81e8
...@@ -13,29 +13,23 @@ module MultipartHelpers ...@@ -13,29 +13,23 @@ module MultipartHelpers
) )
end end
# This function assumes a `mode` variable to be set def upload_parameters_for(filepath: nil, key: nil, mode: nil, filename: 'filename', remote_id: 'remote_id')
def upload_parameters_for(filepath: nil, key: nil, filename: 'filename', remote_id: 'remote_id')
result = { result = {
"#{key}.name" => filename, "name" => filename,
"#{key}.type" => "application/octet-stream", "type" => "application/octet-stream",
"#{key}.sha256" => "1234567890" "sha256" => "1234567890"
} }
case mode case mode
when :local when :local
result["#{key}.path"] = filepath result["path"] = filepath
when :remote when :remote
result["#{key}.remote_id"] = remote_id result["remote_id"] = remote_id
result["#{key}.size"] = 3.megabytes result["size"] = 3.megabytes
else else
raise ArgumentError, "can't handle #{mode} mode" raise ArgumentError, "can't handle #{mode} mode"
end end
return result if ::Feature.disabled?(:upload_middleware_jwt_params_handler, default_enabled: true)
# the HandlerForJWTParams expects a jwt token with the upload parameters
# *without* the "#{key}." prefix
result.deep_transform_keys! { |k| k.remove("#{key}.") }
{ {
"#{key}.gitlab-workhorse-upload" => jwt_token(data: { 'upload' => result }) "#{key}.gitlab-workhorse-upload" => jwt_token(data: { 'upload' => result })
} }
......
spec/support/shared_examples/features/file_uploads_shared_examples.rb
View file @ 679b81e8
...@@ -2,28 +2,6 @@ ...@@ -2,28 +2,6 @@
RSpec.shared_examples 'handling file uploads' do |shared_examples_name| RSpec.shared_examples 'handling file uploads' do |shared_examples_name|
context 'with object storage disabled' do context 'with object storage disabled' do
context 'with upload_middleware_jwt_params_handler disabled' do it_behaves_like shared_examples_name
before do
stub_feature_flags(upload_middleware_jwt_params_handler: false)
expect_next_instance_of(Gitlab::Middleware::Multipart::Handler) do |handler|
expect(handler).to receive(:with_open_files).and_call_original
end
end
it_behaves_like shared_examples_name
end
context 'with upload_middleware_jwt_params_handler enabled' do
before do
stub_feature_flags(upload_middleware_jwt_params_handler: true)
expect_next_instance_of(Gitlab::Middleware::Multipart::HandlerForJWTParams) do |handler|
expect(handler).to receive(:with_open_files).and_call_original
end
end
it_behaves_like shared_examples_name
end
end end
end end
spec/support/shared_examples/lib/gitlab/middleware/multipart_shared_examples.rb
View file @ 679b81e8
...@@ -5,7 +5,7 @@ RSpec.shared_examples 'handling all upload parameters conditions' do ...@@ -5,7 +5,7 @@ RSpec.shared_examples 'handling all upload parameters conditions' do
include_context 'with one temporary file for multipart' include_context 'with one temporary file for multipart'
let(:rewritten_fields) { rewritten_fields_hash('file' => uploaded_filepath) } let(:rewritten_fields) { rewritten_fields_hash('file' => uploaded_filepath) }
let(:params) { upload_parameters_for(filepath: uploaded_filepath, key: 'file', filename: filename, remote_id: remote_id) } let(:params) { upload_parameters_for(filepath: uploaded_filepath, key: 'file', mode: mode, filename: filename, remote_id: remote_id) }
it 'builds an UploadedFile' do it 'builds an UploadedFile' do
expect_uploaded_files(filepath: uploaded_filepath, original_filename: filename, remote_id: remote_id, size: uploaded_file.size, params_path: %w(file)) expect_uploaded_files(filepath: uploaded_filepath, original_filename: filename, remote_id: remote_id, size: uploaded_file.size, params_path: %w(file))
...@@ -19,8 +19,8 @@ RSpec.shared_examples 'handling all upload parameters conditions' do ...@@ -19,8 +19,8 @@ RSpec.shared_examples 'handling all upload parameters conditions' do
let(:rewritten_fields) { rewritten_fields_hash('file1' => uploaded_filepath, 'file2' => uploaded_filepath2) } let(:rewritten_fields) { rewritten_fields_hash('file1' => uploaded_filepath, 'file2' => uploaded_filepath2) }
let(:params) do let(:params) do
upload_parameters_for(filepath: uploaded_filepath, key: 'file1', filename: filename, remote_id: remote_id).merge( upload_parameters_for(filepath: uploaded_filepath, key: 'file1', mode: mode, filename: filename, remote_id: remote_id).merge(
upload_parameters_for(filepath: uploaded_filepath2, key: 'file2', filename: filename2, remote_id: remote_id2) upload_parameters_for(filepath: uploaded_filepath2, key: 'file2', mode: mode, filename: filename2, remote_id: remote_id2)
) )
end end
...@@ -38,7 +38,7 @@ RSpec.shared_examples 'handling all upload parameters conditions' do ...@@ -38,7 +38,7 @@ RSpec.shared_examples 'handling all upload parameters conditions' do
include_context 'with one temporary file for multipart' include_context 'with one temporary file for multipart'
let(:rewritten_fields) { rewritten_fields_hash('user[avatar]' => uploaded_filepath) } let(:rewritten_fields) { rewritten_fields_hash('user[avatar]' => uploaded_filepath) }
let(:params) { { 'user' => { 'avatar' => upload_parameters_for(filepath: uploaded_filepath, filename: filename, remote_id: remote_id) } } } let(:params) { { 'user' => { 'avatar' => upload_parameters_for(filepath: uploaded_filepath, mode: mode, filename: filename, remote_id: remote_id) } } }
it 'builds an UploadedFile' do it 'builds an UploadedFile' do
expect_uploaded_files(filepath: uploaded_filepath, original_filename: filename, remote_id: remote_id, size: uploaded_file.size, params_path: %w(user avatar)) expect_uploaded_files(filepath: uploaded_filepath, original_filename: filename, remote_id: remote_id, size: uploaded_file.size, params_path: %w(user avatar))
...@@ -54,8 +54,8 @@ RSpec.shared_examples 'handling all upload parameters conditions' do ...@@ -54,8 +54,8 @@ RSpec.shared_examples 'handling all upload parameters conditions' do
let(:params) do let(:params) do
{ {
'user' => { 'user' => {
'avatar' => upload_parameters_for(filepath: uploaded_filepath, filename: filename, remote_id: remote_id), 'avatar' => upload_parameters_for(filepath: uploaded_filepath, mode: mode, filename: filename, remote_id: remote_id),
'screenshot' => upload_parameters_for(filepath: uploaded_filepath2, filename: filename2, remote_id: remote_id2) 'screenshot' => upload_parameters_for(filepath: uploaded_filepath2, mode: mode, filename: filename2, remote_id: remote_id2)
} }
} }
end end
...@@ -74,7 +74,7 @@ RSpec.shared_examples 'handling all upload parameters conditions' do ...@@ -74,7 +74,7 @@ RSpec.shared_examples 'handling all upload parameters conditions' do
include_context 'with one temporary file for multipart' include_context 'with one temporary file for multipart'
let(:rewritten_fields) { rewritten_fields_hash('user[avatar][bananas]' => uploaded_filepath) } let(:rewritten_fields) { rewritten_fields_hash('user[avatar][bananas]' => uploaded_filepath) }
let(:params) { { 'user' => { 'avatar' => { 'bananas' => upload_parameters_for(filepath: uploaded_filepath, filename: filename, remote_id: remote_id) } } } } let(:params) { { 'user' => { 'avatar' => { 'bananas' => upload_parameters_for(filepath: uploaded_filepath, mode: mode, filename: filename, remote_id: remote_id) } } } }
it 'builds an UploadedFile' do it 'builds an UploadedFile' do
expect_uploaded_files(filepath: uploaded_file, original_filename: filename, remote_id: remote_id, size: uploaded_file.size, params_path: %w(user avatar bananas)) expect_uploaded_files(filepath: uploaded_file, original_filename: filename, remote_id: remote_id, size: uploaded_file.size, params_path: %w(user avatar bananas))
...@@ -91,10 +91,10 @@ RSpec.shared_examples 'handling all upload parameters conditions' do ...@@ -91,10 +91,10 @@ RSpec.shared_examples 'handling all upload parameters conditions' do
{ {
'user' => { 'user' => {
'avatar' => { 'avatar' => {
'bananas' => upload_parameters_for(filepath: uploaded_filepath, filename: filename, remote_id: remote_id) 'bananas' => upload_parameters_for(filepath: uploaded_filepath, mode: mode, filename: filename, remote_id: remote_id)
}, },
'friend' => { 'friend' => {
'ananas' => upload_parameters_for(filepath: uploaded_filepath2, filename: filename2, remote_id: remote_id2) 'ananas' => upload_parameters_for(filepath: uploaded_filepath2, mode: mode, filename: filename2, remote_id: remote_id2)
} }
} }
} }
...@@ -122,11 +122,11 @@ RSpec.shared_examples 'handling all upload parameters conditions' do ...@@ -122,11 +122,11 @@ RSpec.shared_examples 'handling all upload parameters conditions' do
end end
let(:params) do let(:params) do
upload_parameters_for(filepath: uploaded_filepath, filename: filename, key: 'file', remote_id: remote_id).merge( upload_parameters_for(filepath: uploaded_filepath, filename: filename, key: 'file', mode: mode, remote_id: remote_id).merge(
'user' => { 'user' => {
'avatar' => upload_parameters_for(filepath: uploaded_filepath2, filename: filename2, remote_id: remote_id2), 'avatar' => upload_parameters_for(filepath: uploaded_filepath2, mode: mode, filename: filename2, remote_id: remote_id2),
'friend' => { 'friend' => {
'avatar' => upload_parameters_for(filepath: uploaded_filepath3, filename: filename3, remote_id: remote_id3) 'avatar' => upload_parameters_for(filepath: uploaded_filepath3, mode: mode, filename: filename3, remote_id: remote_id3)
} }
} }
) )
......
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