Pass upload params to rails multipart in a safe way
Problem to solve
Uploads processed by workhorse will set the upload params using body params. Those are then read by UploadedFile
from the multipart.rb
file.
The issue is that query or body params are not a safe way to transmit data between workhorse and rails, we should thus use a better way. In addition, rails, will merge both sources (body and query) and present params
. This can be an issue where a parameter is overriden if present in both sources.
Looking at how multipart uploads are handled, we can see in multipart.rb
that a JWT token is sent with a custom field rewritten_fields
, this one indicates what fields are uploads.
The form of this field is { file: 'TMP_FILE_PATH' }
where TMP_FILE_PATH
is set only if direct upload is not used (either object storage is disabled or it is enabled but direct upload is disabled).
Getting inspiration from this, we should:
- present the JWT header for body uploads. Currently they only set parameters in the body params.
- implement a safer way to transmit the remaining parameters (path, size, hashes and such).
Proposal
Add the multipart JWT header for body uploads
- Use the existing code from
SavedFileTracker
to add the multipart JWT header for body uploads
Safer way to transmit the parameters
(See #261 (comment 327126073))
- For each file upload
X
, set a new parameterX.gitlab-workhorse-upload
, this would be a JWT signed parameter and would have this structure:
{
"upload": {
"md5": "...",
"name": "...",
"path": "...",
"remote_id": "",
"remote_url": "",
"sha1": "...",
"sha256": "...",
"sha512": "...",
"size": "..."
}
}
- Note that for multipart uploads, we can have multiple files, each one with its own name. This should end up in
X1.gitlab-workhorse-upload
,X2.gitlab-workhorse-upload
. - For body uploads
X
is fixed tofile
, so on body uploadsfile.gitlab-workhorse-upload
will be set. - Rails will be able to decode theses additional parameters and use these to build a
UploadedFile
Other considerations
This issue is part of the effort to simplify how we deal with body uploads. See gitlab#213288 (comment 319357573).
Risks
As we change how body uploads are "transferred" to rails, this needs a migration path:
- These new parameters are sent in addition to the current behavior. In other words, the "old" query params are still sent. (this issue)
- All the adaptations (new middleware + update endpoints) are done on the rails part. (future issue)
- The old query params can be dropped (future issue)
Pay a specific attention to not break the following uploads:
- CI artifact job
- User avatar
- Design file in opened issue
Involved components
In Workhorse: