Skip to content

Return explicit error when preauth is denied

What does this MR do and why?

This MR aims to handle and improve how requests to Rails from Workhorse are handled when a HTTP 401 error is returned when authorizing uploads. Currently when a pre-auth request is denied by Rails (HTTP 401), Workhorse incorrectly returns a HTTP 500 instead of the original HTTP 401.

How to set up and validate locally

Replicating the bug in master

  1. Change into your <GDK_ROOT>/gitlab directory.
  2. Check out the current GitLab master branch.
  3. Apply the following patch to make it easier to create the unauthorised scenario:
    diff --git a/lib/api/internal/workhorse.rb b/lib/api/internal/workhorse.rb
    index 910cf52bc3bc..d1fa6b18a66d 100644
    --- a/lib/api/internal/workhorse.rb
    +++ b/lib/api/internal/workhorse.rb
    @@ -25,7 +25,7 @@ def request_authenticated?
          namespace 'internal' do
            namespace 'workhorse' do
              post 'authorize_upload' do
    -            unauthorized! unless request_authenticated?
    +            unauthorized! # unless request_authenticated?
    
                status 200
                { TempPath: File.join(::Gitlab.config.uploads.storage_path, 'uploads/tmp') }
  4. Change into your <GDK_ROOT> directory.
  5. Start up your GDK with gdk start.
  6. Build a new workhorse by running make gitlab-workhorse-update.
  7. Restart Workhorse gdk restart gitlab-workhorse.
  8. Run gdk tail workhorse | grep -A1 error to watch Workhorse output for errors.
  9. Once your GDK is ready, open up any project that has a repository setup.
  10. Click the + button and then 'Upload file': image
  11. Select any file to upload and click 'Upload file'.
  12. Observe the following similar lines in the output of the gdk tail workhorse | grep -A1 error command, noting a HTTP 500 status code/error is returned:
    2023-12-14_05:09:31.12270 gitlab-workhorse      : {"correlation_id":"01HHKBNPR5N5PDX283J35FHRA2","error":"handleFileUploads: extract files from multipart: no api response: status 401","level":"error","method":"POST","msg":"","time":"2023-12-14T16:09:31+11:00","uri":"/root/codeowners-test/-/create/main"}
    2023-12-14_05:09:31.12285 gitlab-workhorse      : {"content_type":"text/plain; charset=utf-8","correlation_id":"01HHKBNPR5N5PDX283J35FHRA2","duration_ms":108,"host":"gdk.test:3000","level":"info","method":"POST","msg":"access","proto":"HTTP/1.1","referrer":"http://gdk.test:3000/root/codeowners-test","remote_addr":"172.16.123.1:57696","remote_ip":"172.16.123.1","route":"","status":500,"system":"http","time":"2023-12-14T16:09:31+11:00","ttfb_ms":105,"uri":"/root/codeowners-test/-/create/main","user_agent":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:120.0) Gecko/20100101 Firefox/120.0","written_bytes":22}

Testing the bug is fixed in 412605-return-401-status-when-fixed-pre-authorizer-gets-401-from-gitlab-rails

  1. Repeat all steps above, changing the GitLab branch in step 2. from master to 412605-return-401-status-when-fixed-pre-authorizer-gets-401-from-gitlab-rails.
  2. Observe the following similar lines in the output of the gdk tail workhorse | grep -A1 error command, noting a HTTP 401 status code/error is returned instead:
    2023-12-14_05:11:26.48925 gitlab-workhorse      : {"correlation_id":"01HHKBS7CCD0WCXT88AED48MB0","error":"no api response: status 401","level":"error","method":"POST","msg":"","time":"2023-12-14T16:11:26+11:00","uri":"/root/codeowners-test/-/create/main"}
    2023-12-14_05:11:26.48939 gitlab-workhorse      : {"content_type":"text/plain; charset=utf-8","correlation_id":"01HHKBS7CCD0WCXT88AED48MB0","duration_ms":139,"host":"gdk.test:3000","level":"info","method":"POST","msg":"access","proto":"HTTP/1.1","referrer":"http://gdk.test:3000/root/codeowners-test","remote_addr":"172.16.123.1:57898","remote_ip":"172.16.123.1","route":"","status":401,"system":"http","time":"2023-12-14T16:11:26+11:00","ttfb_ms":135,"uri":"/root/codeowners-test/-/create/main","user_agent":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:120.0) Gecko/20100101 Firefox/120.0","written_bytes":17}

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #412605 (closed)

Edited by Ash McKenzie

Merge request reports

Loading