Reintroduce Rack v2.1.4

This is needed to address a few outstanding CVEs and fix cookie
timestamp formats.

Full list of changes:
https://github.com/rack/rack/compare/2.0.9..2.1.4

Relates to:
* https://gitlab.com/gitlab-org/gitlab/-/issues/36362
* https://gitlab.com/gitlab-org/gitlab/-/issues/228622

Rack v2.1.x no longer coerces the body to a string. 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

Previously in a few places the Grape API was returning the status code
as an integer, which Grape used as the response body. To preserve the
legacy behavior, we explicitly set the body to the stringified integer.

In https://gitlab.com/gitlab-org/gitlab/-/issues/267598, we saw Maven
packages report 500 errors because a `nil` body was being returned. This
has been fixed in
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/45271.

Because it's easy to mistakenly return the wrong type in the Grape body,
this commit also adds a new Grape middleware that will automatically
coerce values to strings but raise an exception in development and test.

This reverts https://gitlab.com/gitlab-org/gitlab/-/merge_requests/45246
and brings back
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/44518.

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