Merge branch 'sh-bring-back-rack-2.1.4' into 'master'

Reintroduce Rack v2.1.4

See merge request gitlab-org/gitlab!45340

Gemfile

@@ -172,7 +172,7 @@ gem 'diffy', '~> 3.3'
 gem 'diff_match_patch', '~> 0.1.0'
 
 # Application server
-gem 'rack', '~> 2.0.9'
+gem 'rack', '~> 2.1.4'
 # https://github.com/sharpstone/rack-timeout/blob/master/README.md#rails-apps-manually
 gem 'rack-timeout', '~> 0.5.1', require: 'rack/timeout/base'
 

Gemfile.lock

@@ -855,7 +855,7 @@ GEM
     public_suffix (4.0.6)
     pyu-ruby-sasl (0.0.3.3)
     raabro (1.1.6)
-    rack (2.0.9)
+    rack (2.1.4)
     rack-accept (0.4.5)
       rack (>= 0.4)
     rack-attack (6.3.0)
@@ -1429,7 +1429,7 @@ DEPENDENCIES
   prometheus-client-mmap (~> 0.12.0)
   pry-byebug (~> 3.9.0)
   pry-rails (~> 0.3.9)
-  rack (~> 2.0.9)
+  rack (~> 2.1.4)
   rack-attack (~> 6.3.0)
   rack-cors (~> 1.0.6)
   rack-oauth2 (~> 1.9.3)

app/controllers/admin/users_controller.rb

@@ -183,7 +183,7 @@ class Admin::UsersController < Admin::ApplicationController
         # restore username to keep form action url.
         user.username = params[:id]
         format.html { render "edit" }
-        format.json { render json: [result[:message]], status: result[:status] }
+        format.json { render json: [result[:message]], status: :internal_server_error }
       end
     end
   end

app/controllers/projects/merge_requests/drafts_controller.rb

@@ -45,7 +45,7 @@ class Projects::MergeRequests::DraftsController < Projects::MergeRequests::Appli
     if result[:status] == :success
       head :ok
     else
-      render json: { message: result[:message] }, status: result[:status]
+      render json: { message: result[:message] }, status: :internal_server_error
     end
   end
 

changelogs/unreleased/sh-update-rack-2-1-4.yml

+---
+title: Update to Rack v2.1.4
+merge_request: 45340
+author:
+type: fixed

lib/api/api_guard.rb

@@ -19,6 +19,7 @@ module API
       end
 
       use AdminModeMiddleware
+      use ResponseCoercerMiddleware
 
       helpers HelperMethods
 
@@ -188,6 +189,44 @@ module API
       end
     end
 
+    # Prior to Rack v2.1.x, returning a body of [nil] or [201] worked
+    # because the body was coerced to a string. However, this no longer
+    # works in Rack v2.1.0+. The Rack spec
+    # (https://github.com/rack/rack/blob/master/SPEC.rdoc#the-body-)
+    # says:
+    #
+    # The Body must respond to `each` and must only yield String values
+    #
+    # Because it's easy to return the wrong body type, this middleware
+    # will:
+    #
+    # 1. Inspect each element of the body if it is an Array.
+    # 2. Coerce each value to a string if necessary.
+    # 3. Flag a test and development error.
+    class ResponseCoercerMiddleware < ::Grape::Middleware::Base
+      def call(env)
+        response = super(env)
+
+        status = response[0]
+        body = response[2]
+
+        return response if Rack::Utils::STATUS_WITH_NO_ENTITY_BODY[status]
+        return response unless body.is_a?(Array)
+
+        body.map! do |part|
+          if part.is_a?(String)
+            part
+          else
+            err = ArgumentError.new("The response body should be a String, but it is of type #{part.class}")
+            Gitlab::ErrorTracking.track_and_raise_for_dev_exception(err)
+            part.to_s
+          end
+        end
+
+        response
+      end
+    end
+
     class AdminModeMiddleware < ::Grape::Middleware::Base
       def after
         # Use a Grape middleware since the Grape `after` blocks might run

lib/api/ci/runner.rb

@@ -72,6 +72,7 @@ module API
         post '/verify' do
           authenticate_runner!
           status 200
+          body "200"
         end
       end
 
@@ -183,6 +184,7 @@ module API
           service.execute.then do |result|
             header 'X-GitLab-Trace-Update-Interval', result.backoff
             status result.status
+            body result.status.to_s
           end
         end
 
@@ -293,6 +295,7 @@ module API
 
           if result[:status] == :success
             status :created
+            body "201"
           else
             render_api_error!(result[:message], result[:http_status])
           end

lib/api/helpers.rb

@@ -522,7 +522,7 @@ module API
       else
         header(*Gitlab::Workhorse.send_url(file.url))
         status :ok
-        body
+        body ""
       end
     end
 

lib/api/internal/lfs.rb

@@ -44,7 +44,7 @@ module API
               workhorse_headers = Gitlab::Workhorse.send_url(file.url)
               header workhorse_headers[0], workhorse_headers[1]
               env['api.format'] = :binary
-              body nil
+              body ""
             end
           end
         end

spec/controllers/admin/users_controller_spec.rb

@@ -390,7 +390,7 @@ RSpec.describe Admin::UsersController do
 
   describe 'POST update' do
     context 'when the password has changed' do
-      def update_password(user, password = User.random_password, password_confirmation = password)
+      def update_password(user, password = User.random_password, password_confirmation = password, format = :html)
         params = {
           id: user.to_param,
           user: {
@@ -399,7 +399,7 @@ RSpec.describe Admin::UsersController do
           }
         }
 
-        post :update, params: params
+        post :update, params: params, format: format
       end
 
       context 'when admin changes their own password' do
@@ -498,6 +498,23 @@ RSpec.describe Admin::UsersController do
             .not_to change { user.reload.encrypted_password }
         end
       end
+
+      context 'when the update fails' do
+        let(:password) { User.random_password }
+
+        before do
+          expect_next_instance_of(Users::UpdateService) do |service|
+            allow(service).to receive(:execute).and_return({ message: 'failed', status: :error })
+          end
+        end
+
+        it 'returns a 500 error' do
+          expect { update_password(admin, password, password, :json) }
+            .not_to change { admin.reload.password_expired? }
+
+          expect(response).to have_gitlab_http_status(:error)
+        end
+      end
     end
 
     context 'admin notes' do

spec/lib/gitlab/middleware/go_spec.rb

@@ -142,8 +142,8 @@ RSpec.describe Gitlab::Middleware::Go do
                         response = go
 
                         expect(response[0]).to eq(403)
-                        expect(response[1]['Content-Length']).to eq('0')
-                        expect(response[2].body).to eq([''])
+                        expect(response[1]['Content-Length']).to be_nil
+                        expect(response[2]).to eq([''])
                       end
                     end
                   end
@@ -187,10 +187,11 @@ RSpec.describe Gitlab::Middleware::Go do
 
           it 'returns 404' do
             response = go
+
             expect(response[0]).to eq(404)
             expect(response[1]['Content-Type']).to eq('text/html')
             expected_body = %{<html><body>go get #{Gitlab.config.gitlab.url}/#{project.full_path}</body></html>}
-            expect(response[2].body).to eq([expected_body])
+            expect(response[2]).to eq([expected_body])
           end
         end
 
@@ -262,7 +263,7 @@ RSpec.describe Gitlab::Middleware::Go do
       expect(response[0]).to eq(200)
       expect(response[1]['Content-Type']).to eq('text/html')
       expected_body = %{<html><head><meta name="go-import" content="#{Gitlab.config.gitlab.host}/#{path} git #{repository_url}" /><meta name="go-source" content="#{Gitlab.config.gitlab.host}/#{path} #{project_url} #{project_url}/-/tree/#{branch}{/dir} #{project_url}/-/blob/#{branch}{/dir}/{file}#L{line}" /></head><body>go get #{Gitlab.config.gitlab.url}/#{path}</body></html>}
-      expect(response[2].body).to eq([expected_body])
+      expect(response[2]).to eq([expected_body])
     end
   end
 end

spec/lib/gitlab/middleware/same_site_cookies_spec.rb

@@ -60,12 +60,12 @@ RSpec.describe Gitlab::Middleware::SameSiteCookies do
       end
 
       context 'with no cookies' do
-        let(:cookies) { nil }
+        let(:cookies) { "" }
 
         it 'does not add headers' do
           response = do_request
 
-          expect(response['Set-Cookie']).to be_nil
+          expect(response['Set-Cookie']).to eq("")
         end
       end
 

spec/requests/api/api_guard/response_coercer_middleware_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe API::APIGuard::ResponseCoercerMiddleware do
+  using RSpec::Parameterized::TableSyntax
+
+  it 'is loaded' do
+    expect(API::API.middleware).to include([:use, described_class])
+  end
+
+  describe '#call' do
+    let(:app) do
+      Class.new(API::API)
+    end
+
+    [
+      nil, 201, 10.5, "test"
+    ].each do |val|
+      it 'returns a String body' do
+        app.get 'bodytest' do
+          status 200
+          env['api.format'] = :binary
+          body val
+        end
+
+        unless val.is_a?(String)
+          expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception).with(instance_of(ArgumentError))
+        end
+
+        get api('/bodytest')
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response.body).to eq(val.to_s)
+      end
+    end
+
+    [100, 204, 304].each do |status|
+      it 'allows nil body' do
+        app.get 'statustest' do
+          status status
+          env['api.format'] = :binary
+          body nil
+        end
+
+        expect(Gitlab::ErrorTracking).not_to receive(:track_and_raise_for_dev_exception)
+
+        get api('/statustest')
+
+        expect(response.status).to eq(status)
+        expect(response.body).to eq('')
+      end
+    end
+  end
+end