From a360142bb99b37e48051c2455246f0fd4e6cd542 Mon Sep 17 00:00:00 2001
From: Alex Kalderimis <akalderimis@gitlab.com>
Date: Thu, 5 Aug 2021 13:29:18 +0100
Subject: [PATCH] Update apollo_upload_server dependency
This enables us to make use of `strict_mode`, preventing denial of
service attacks.
Changelog: security
---
Gemfile | 2 +-
Gemfile.lock | 6 ++---
config/initializers/apollo_upload_server.rb | 5 ++++
lib/gitlab/middleware/multipart.rb | 2 +-
.../file_uploads/graphql_add_design_spec.rb | 12 +++++++++-
.../design_management/upload_spec.rb | 23 +++++++++++++++++--
6 files changed, 42 insertions(+), 8 deletions(-)
create mode 100644 config/initializers/apollo_upload_server.rb
diff --git a/Gemfile b/Gemfile
index df805a6bcf91..ecb1e526e4d3 100644
--- a/Gemfile
+++ b/Gemfile
@@ -101,7 +101,7 @@ gem 'graphql', '~> 1.11.8'
# TODO: remove app/views/graphiql/rails/editors/show.html.erb when https://github.com/rmosolgo/graphiql-rails/pull/71 is released:
# https://gitlab.com/gitlab-org/gitlab/issues/31747
gem 'graphiql-rails', '~> 1.4.10'
-gem 'apollo_upload_server', '~> 2.0.2'
+gem 'apollo_upload_server', '~> 2.1.0'
gem 'graphql-docs', '~> 1.6.0', group: [:development, :test]
gem 'graphlient', '~> 0.4.0' # Used by BulkImport feature (group::import)
diff --git a/Gemfile.lock b/Gemfile.lock
index 658a7c68fa3a..3ff3f6ca76f8 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -80,9 +80,9 @@ GEM
aes_key_wrap (1.1.0)
akismet (3.0.0)
android_key_attestation (0.3.0)
- apollo_upload_server (2.0.2)
+ apollo_upload_server (2.1.0)
+ actionpack (>= 4.2)
graphql (>= 1.8)
- rails (>= 4.2)
asana (0.10.3)
faraday (~> 1.0)
faraday_middleware (~> 1.0)
@@ -1391,7 +1391,7 @@ DEPENDENCIES
acts-as-taggable-on (~> 7.0)
addressable (~> 2.8)
akismet (~> 3.0)
- apollo_upload_server (~> 2.0.2)
+ apollo_upload_server (~> 2.1.0)
asana (~> 0.10.3)
asciidoctor (~> 2.0.10)
asciidoctor-include-ext (~> 0.3.1)
diff --git a/config/initializers/apollo_upload_server.rb b/config/initializers/apollo_upload_server.rb
new file mode 100644
index 000000000000..295cb5d8ce88
--- /dev/null
+++ b/config/initializers/apollo_upload_server.rb
@@ -0,0 +1,5 @@
+# frozen_string_literal: true
+
+require 'apollo_upload_server'
+
+ApolloUploadServer::Middleware.strict_mode = true
diff --git a/lib/gitlab/middleware/multipart.rb b/lib/gitlab/middleware/multipart.rb
index 30b3fe3d8931..49be3ffc8394 100644
--- a/lib/gitlab/middleware/multipart.rb
+++ b/lib/gitlab/middleware/multipart.rb
@@ -176,7 +176,7 @@ def call(env)
::Gitlab::Middleware::Multipart::Handler.new(env, message).with_open_files do
@app.call(env)
end
- rescue UploadedFile::InvalidPathError => e
+ rescue UploadedFile::InvalidPathError, ApolloUploadServer::GraphQLDataBuilder::OutOfBounds => e
[400, { 'Content-Type' => 'text/plain' }, [e.message]]
end
end
diff --git a/spec/features/file_uploads/graphql_add_design_spec.rb b/spec/features/file_uploads/graphql_add_design_spec.rb
index f805ea86b4c9..17fbf5f6838d 100644
--- a/spec/features/file_uploads/graphql_add_design_spec.rb
+++ b/spec/features/file_uploads/graphql_add_design_spec.rb
@@ -19,8 +19,18 @@
let_it_be(:user) { create(:user, :admin) }
let_it_be(:personal_access_token) { create(:personal_access_token, user: user) }
let_it_be(:design) { create(:design) }
- let_it_be(:operations) { { "operationName": "uploadDesign", "variables": { "files": [], "projectPath": design.project.full_path, "iid": design.issue.iid }, "query": query }.to_json }
let_it_be(:map) { { "1": ["variables.files.0"] }.to_json }
+ let_it_be(:operations) do
+ {
+ "operationName": "uploadDesign",
+ "variables": {
+ "files": [nil],
+ "projectPath": design.project.full_path,
+ "iid": design.issue.iid
+ },
+ "query": query
+ }.to_json
+ end
let(:url) { capybara_url("/api/graphql?private_token=#{personal_access_token.token}") }
let(:file) { fixture_file_upload('spec/fixtures/dk.png') }
diff --git a/spec/requests/api/graphql/mutations/design_management/upload_spec.rb b/spec/requests/api/graphql/mutations/design_management/upload_spec.rb
index 2189ae3c5199..d3e6c689a59e 100644
--- a/spec/requests/api/graphql/mutations/design_management/upload_spec.rb
+++ b/spec/requests/api/graphql/mutations/design_management/upload_spec.rb
@@ -11,6 +11,7 @@
let(:project) { issue.project }
let(:files) { [fixture_file_upload("spec/fixtures/dk.png")] }
let(:variables) { {} }
+ let(:mutation_response) { graphql_mutation_response(:design_management_upload) }
def mutation
input = {
@@ -21,14 +22,32 @@ def mutation
graphql_mutation(:design_management_upload, input)
end
- let(:mutation_response) { graphql_mutation_response(:design_management_upload) }
-
before do
enable_design_management
project.add_developer(current_user)
end
+ context 'when the input does not include a null value for each mapped file' do
+ let(:operations) { { query: mutation.query, variables: mutation.variables.merge(files: []) } }
+ let(:mapping) { { '1' => ['variables.files.0'] } }
+ let(:params) do
+ { '1' => files.first, operations: operations.to_json, map: mapping.to_json }
+ end
+
+ it 'returns an error' do
+ workhorse_post_with_file(api('/', current_user, version: 'graphql'),
+ params: params,
+ file_key: '1'
+ )
+
+ expect(response).to have_attributes(
+ code: eq('400'),
+ body: include('out-of-bounds')
+ )
+ end
+ end
+
it "returns an error if the user is not allowed to upload designs" do
post_graphql_mutation_with_uploads(mutation, current_user: create(:user))
--
GitLab