Mixing and matching the use of Rack::Request and ActionDispatch::Request
in Rails 5 is bad, particularly if you have middleware that
manipulates or accesses environment variables.
`Gitlab::Middleware::Multipart` attempts to rewrite request parameters
to the proper values (e.g. replacing `data_file` with
`UploadedFile`). It does this by calling `Rack::Request#update_params`,
which essentially updates `env['rack.request.form_hash']`.
By changing to `ActionDispatch::Request`, the Go middleware was causing
the request parameters to be stored inside
`env['action_dispatch.request.request_parameters']`. Later calls to
`Rack::Request#update_params` would not have any effect because it would
attempt to update `env['rack.request.form_has']` instead of
`env['action_dispatch.request.request_parameters']`. As a result, the
controller still saw the old parameters.
Since the Go middleware appears to be using `ActionDispatch::Request`
for authorization methods, we can switch the multipart middleware to
use it too.
Closes https://gitlab.com/gitlab-org/gitlab-ee/issues/9035
When direct_upload is enabled and a for file is being uploaded,
then workhorse uses `public/uploads/tmp` path. If `uploads.storage_path`
i sset to a different directory, then upload fails because
`public/uploads/tmp` is not in allowed paths.
Currently we check if uploaded file is under
`Gitlab.config.uploads.storage_path`, the problem is that
uploads are placed in `uploads` subdirectory which is symlink.
In allow_path? method we check real (expanded) paths, which causes
that `Gitlab.config.uploads.storage_path` is expaned into symlink
path and there is a mismatch with upload file path.
By adding `Gitlab.config.uploads.storage_path/uploads` into allowed
paths, this path is expaned during path check.
`Gitlab.config.uploads.storage_path` is left there intentionally in case
some uploader wouldn't use `uploads` subdir.
In the attempt to unify file uploading at workhorse level gitlab-org/gitlab-workhorse!230
we moved to a prefix-based tempfile creation in order to avoid upload collisions.
Artifacts and LFS uploads already set original_filename to workhorse provided filename
This commit add the same feature to `Gitlab::Middleware::Multipart`
I mistakenly concluded Rack::Multipart injects File instances into the
params. These should be UploadedFile instances. This reuses a mock
UploadedFile class we already had in GitLab.
GitLab Bot
Alessio Caiazza
dineshpanda
Stan Hu
gfyoung
Andrew Newdigate
Jan Provaznik
Kamil Trzciński
🙈 jacopo beschi 🙉
Douwe Maan
Jacob Vosmaer