diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-07-01 00:49:57 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-07-01 00:49:57 +0000 |
commit | c31c9f964a81f104f4c265b6082b469361fb1653 (patch) | |
tree | ff99939150d948ed8e8a1fb1139eb8fac778e69b | |
parent | c6c26f3b730d4bbc567aee33b4c6fd621517055e (diff) | |
download | gitlab-ce-c31c9f964a81f104f4c265b6082b469361fb1653.tar.gz |
Add latest changes from gitlab-org/security/gitlab@14-0-stable-ee
-rw-r--r-- | app/controllers/graphql_controller.rb | 29 | ||||
-rw-r--r-- | app/graphql/mutations/echo.rb | 33 | ||||
-rw-r--r-- | app/graphql/types/mutation_type.rb | 1 | ||||
-rw-r--r-- | doc/api/graphql/reference/index.md | 25 | ||||
-rw-r--r-- | spec/controllers/graphql_controller_spec.rb | 6 | ||||
-rw-r--r-- | spec/policies/global_policy_spec.rb | 8 | ||||
-rw-r--r-- | spec/requests/api/graphql_spec.rb | 150 | ||||
-rw-r--r-- | spec/support/helpers/graphql_helpers.rb | 21 |
8 files changed, 262 insertions, 11 deletions
diff --git a/app/controllers/graphql_controller.rb b/app/controllers/graphql_controller.rb index 725d8b62c77..515fbd7b482 100644 --- a/app/controllers/graphql_controller.rb +++ b/app/controllers/graphql_controller.rb @@ -20,12 +20,16 @@ class GraphqlController < ApplicationController # around in GraphiQL. protect_from_forgery with: :null_session, only: :execute - before_action :authorize_access_api! + # must come first: current_user is set up here before_action(only: [:execute]) { authenticate_sessionless_user!(:api) } + + before_action :authorize_access_api! before_action :set_user_last_activity before_action :track_vs_code_usage before_action :disable_query_limiting + before_action :disallow_mutations_for_get + # Since we deactivate authentication from the main ApplicationController and # defer it to :authorize_access_api!, we need to override the bypass session # callback execution order here @@ -62,6 +66,25 @@ class GraphqlController < ApplicationController private + def disallow_mutations_for_get + return unless request.get? || request.head? + return unless any_mutating_query? + + raise ::Gitlab::Graphql::Errors::ArgumentError, "Mutations are forbidden in #{request.request_method} requests" + end + + def any_mutating_query? + if multiplex? + multiplex_queries.any? { |q| mutation?(q[:query], q[:operation_name]) } + else + mutation?(query) + end + end + + def mutation?(query_string, operation_name = params[:operationName]) + ::GraphQL::Query.new(GitlabSchema, query_string, operation_name: operation_name).mutation? + end + # Tests may mark some GraphQL queries as exempt from SQL query limits def disable_query_limiting return unless Gitlab::QueryLimiting.enabled_for_env? @@ -130,7 +153,9 @@ class GraphqlController < ApplicationController end def authorize_access_api! - access_denied!("API not accessible for user.") unless can?(current_user, :access_api) + return if can?(current_user, :access_api) + + render_error('API not accessible for user', status: :forbidden) end # Overridden from the ApplicationController to make the response look like diff --git a/app/graphql/mutations/echo.rb b/app/graphql/mutations/echo.rb new file mode 100644 index 00000000000..61d39009ba4 --- /dev/null +++ b/app/graphql/mutations/echo.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module Mutations + class Echo < BaseMutation + graphql_name 'EchoCreate' + description <<~DOC + A mutation that does not perform any changes. + + This is expected to be used for testing of endpoints, to verify + that a user has mutation access. + DOC + + argument :errors, + type: [::GraphQL::STRING_TYPE], + required: false, + description: 'Errors to return to the user.' + + argument :messages, + type: [::GraphQL::STRING_TYPE], + as: :echoes, + required: false, + description: 'Messages to return to the user.' + + field :echoes, + type: [::GraphQL::STRING_TYPE], + null: true, + description: 'Messages returned to the user.' + + def resolve(**args) + args + end + end +end diff --git a/app/graphql/types/mutation_type.rb b/app/graphql/types/mutation_type.rb index 6b1146f8f09..6d3327f9735 100644 --- a/app/graphql/types/mutation_type.rb +++ b/app/graphql/types/mutation_type.rb @@ -104,6 +104,7 @@ module Types mount_mutation Mutations::Ci::RunnersRegistrationToken::Reset, feature_flag: :runner_graphql_query mount_mutation Mutations::Namespace::PackageSettings::Update mount_mutation Mutations::UserCallouts::Create + mount_mutation Mutations::Echo end end diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index e84769fa568..ffdf0ea5fd3 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -1895,6 +1895,31 @@ Input type: `DiscussionToggleResolveInput` | <a id="mutationdiscussiontoggleresolvediscussion"></a>`discussion` | [`Discussion`](#discussion) | The discussion after mutation. | | <a id="mutationdiscussiontoggleresolveerrors"></a>`errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | +### `Mutation.echoCreate` + +A mutation that does not perform any changes. + +This is expected to be used for testing of endpoints, to verify +that a user has mutation access. + +Input type: `EchoCreateInput` + +#### Arguments + +| Name | Type | Description | +| ---- | ---- | ----------- | +| <a id="mutationechocreateclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| <a id="mutationechocreateerrors"></a>`errors` | [`[String!]`](#string) | Errors to return to the user. | +| <a id="mutationechocreatemessages"></a>`messages` | [`[String!]`](#string) | Messages to return to the user. | + +#### Fields + +| Name | Type | Description | +| ---- | ---- | ----------- | +| <a id="mutationechocreateclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| <a id="mutationechocreateechoes"></a>`echoes` | [`[String!]`](#string) | Messages returned to the user. | +| <a id="mutationechocreateerrors"></a>`errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | + ### `Mutation.enableDevopsAdoptionNamespace` **BETA** This endpoint is subject to change without notice. diff --git a/spec/controllers/graphql_controller_spec.rb b/spec/controllers/graphql_controller_spec.rb index f2d86b1b166..aed97a01a72 100644 --- a/spec/controllers/graphql_controller_spec.rb +++ b/spec/controllers/graphql_controller_spec.rb @@ -44,7 +44,7 @@ RSpec.describe GraphqlController do expect(response).to have_gitlab_http_status(:ok) end - it 'returns access denied template when user cannot access API' do + it 'returns forbidden when user cannot access API' do # User cannot access API in a couple of cases # * When user is internal(like ghost users) # * When user is blocked @@ -54,7 +54,9 @@ RSpec.describe GraphqlController do post :execute expect(response).to have_gitlab_http_status(:forbidden) - expect(response).to render_template('errors/access_denied') + expect(json_response).to include( + 'errors' => include(a_hash_including('message' => /API not accessible/)) + ) end it 'updates the users last_activity_on field' do diff --git a/spec/policies/global_policy_spec.rb b/spec/policies/global_policy_spec.rb index e88619b9527..85026ced466 100644 --- a/spec/policies/global_policy_spec.rb +++ b/spec/policies/global_policy_spec.rb @@ -239,6 +239,14 @@ RSpec.describe GlobalPolicy do it { is_expected.not_to be_allowed(:access_api) } end + context 'with a deactivated user' do + before do + current_user.deactivate! + end + + it { is_expected.not_to be_allowed(:access_api) } + end + context 'user with expired password' do before do current_user.update!(password_expires_at: 2.minutes.ago) diff --git a/spec/requests/api/graphql_spec.rb b/spec/requests/api/graphql_spec.rb index a336d74b135..463fca43cb5 100644 --- a/spec/requests/api/graphql_spec.rb +++ b/spec/requests/api/graphql_spec.rb @@ -6,6 +6,9 @@ RSpec.describe 'GraphQL' do include AfterNextHelpers let(:query) { graphql_query_for('echo', text: 'Hello world') } + let(:mutation) { 'mutation { echoCreate(input: { messages: ["hello", "world"] }) { echoes } }' } + + let_it_be(:user) { create(:user) } describe 'logging' do shared_examples 'logging a graphql query' do @@ -70,6 +73,139 @@ RSpec.describe 'GraphQL' do end end + context 'when executing mutations' do + let(:mutation_with_variables) do + <<~GQL + mutation($a: String!, $b: String!) { + echoCreate(input: { messages: [$a, $b] }) { echoes } + } + GQL + end + + context 'with POST' do + it 'succeeds' do + post_graphql(mutation, current_user: user) + + expect(graphql_data_at(:echo_create, :echoes)).to eq %w[hello world] + end + + context 'with variables' do + it 'succeeds' do + post_graphql(mutation_with_variables, current_user: user, variables: { a: 'Yo', b: 'there' }) + + expect(graphql_data_at(:echo_create, :echoes)).to eq %w[Yo there] + end + end + end + + context 'with GET' do + it 'fails' do + get_graphql(mutation, current_user: user) + + expect(graphql_errors).to include(a_hash_including('message' => /Mutations are forbidden/)) + end + + context 'with variables' do + it 'fails' do + get_graphql(mutation_with_variables, current_user: user, variables: { a: 'Yo', b: 'there' }) + + expect(graphql_errors).to include(a_hash_including('message' => /Mutations are forbidden/)) + end + end + end + end + + context 'when executing queries' do + context 'with POST' do + it 'succeeds' do + post_graphql(query, current_user: user) + + expect(graphql_data_at(:echo)).to include 'Hello world' + end + end + + context 'with GET' do + it 'succeeds' do + get_graphql(query, current_user: user) + + expect(graphql_data_at(:echo)).to include 'Hello world' + end + end + end + + context 'when selecting a query by operation name' do + let(:query) { "query A #{graphql_query_for('echo', text: 'Hello world')}" } + let(:mutation) { 'mutation B { echoCreate(input: { messages: ["hello", "world"] }) { echoes } }' } + + let(:combined) { [query, mutation].join("\n\n") } + + context 'with POST' do + it 'succeeds when selecting the query' do + post_graphql(combined, current_user: user, params: { operationName: 'A' }) + + resp = json_response + + expect(resp.dig('data', 'echo')).to include 'Hello world' + end + + it 'succeeds when selecting the mutation' do + post_graphql(combined, current_user: user, params: { operationName: 'B' }) + + resp = json_response + + expect(resp.dig('data', 'echoCreate', 'echoes')).to eq %w[hello world] + end + end + + context 'with GET' do + it 'succeeds when selecting the query' do + get_graphql(combined, current_user: user, params: { operationName: 'A' }) + + resp = json_response + + expect(resp.dig('data', 'echo')).to include 'Hello world' + end + + it 'fails when selecting the mutation' do + get_graphql(combined, current_user: user, params: { operationName: 'B' }) + + resp = json_response + + expect(resp.dig('errors', 0, 'message')).to include "Mutations are forbidden" + end + end + end + + context 'when batching mutations and queries' do + let(:batched) do + [ + { query: "query A #{graphql_query_for('echo', text: 'Hello world')}" }, + { query: 'mutation B { echoCreate(input: { messages: ["hello", "world"] }) { echoes } }' } + ] + end + + context 'with POST' do + it 'succeeds' do + post_multiplex(batched, current_user: user) + + resp = json_response + + expect(resp.dig(0, 'data', 'echo')).to include 'Hello world' + expect(resp.dig(1, 'data', 'echoCreate', 'echoes')).to eq %w[hello world] + end + end + + context 'with GET' do + it 'fails with a helpful error message' do + get_multiplex(batched, current_user: user) + + resp = json_response + + expect(resp.dig('errors', 0, 'message')).to include "Mutations are forbidden" + end + end + end + context 'with invalid variables' do it 'returns an error' do post_graphql(query, variables: "This is not JSON") @@ -80,8 +216,6 @@ RSpec.describe 'GraphQL' do end describe 'authentication', :allow_forgery_protection do - let(:user) { create(:user) } - it 'allows access to public data without authentication' do post_graphql(query) @@ -109,11 +243,9 @@ RSpec.describe 'GraphQL' do context 'with token authentication' do let(:token) { create(:personal_access_token) } - before do + it 'authenticates users with a PAT' do stub_authentication_activity_metrics(debug: false) - end - it 'authenticates users with a PAT' do expect(authentication_metrics) .to increment(:user_authenticated_counter) .and increment(:user_session_override_counter) @@ -124,6 +256,14 @@ RSpec.describe 'GraphQL' do expect(graphql_data['echo']).to eq("\"#{token.user.username}\" says: Hello world") end + it 'prevents access by deactived users' do + token.user.deactivate! + + post_graphql(query, headers: { 'PRIVATE-TOKEN' => token.token }) + + expect(graphql_errors).to include({ 'message' => /API not accessible/ }) + end + context 'when the personal access token has no api scope' do it 'does not log the user in' do token.update!(scopes: [:read_user]) diff --git a/spec/support/helpers/graphql_helpers.rb b/spec/support/helpers/graphql_helpers.rb index 4857fa63114..38cf828ca5e 100644 --- a/spec/support/helpers/graphql_helpers.rb +++ b/spec/support/helpers/graphql_helpers.rb @@ -400,8 +400,13 @@ module GraphqlHelpers post api('/', current_user, version: 'graphql'), params: { _json: queries }, headers: headers end - def post_graphql(query, current_user: nil, variables: nil, headers: {}, token: {}) - params = { query: query, variables: serialize_variables(variables) } + def get_multiplex(queries, current_user: nil, headers: {}) + path = "/?#{queries.to_query('_json')}" + get api(path, current_user, version: 'graphql'), headers: headers + end + + def post_graphql(query, current_user: nil, variables: nil, headers: {}, token: {}, params: {}) + params = params.merge(query: query, variables: serialize_variables(variables)) post api('/', current_user, version: 'graphql', **token), params: params, headers: headers return unless graphql_errors @@ -410,6 +415,18 @@ module GraphqlHelpers expect(graphql_errors).not_to include(a_hash_including('message' => 'Internal server error')) end + def get_graphql(query, current_user: nil, variables: nil, headers: {}, token: {}, params: {}) + vars = "variables=#{CGI.escape(serialize_variables(variables))}" if variables + params = params.to_a.map { |k, v| v.to_query(k) } + path = ["/?query=#{CGI.escape(query)}", vars, *params].join('&') + get api(path, current_user, version: 'graphql', **token), headers: headers + + return unless graphql_errors + + # Errors are acceptable, but not this one: + expect(graphql_errors).not_to include(a_hash_including('message' => 'Internal server error')) + end + def post_graphql_mutation(mutation, current_user: nil, token: {}) post_graphql(mutation.query, current_user: current_user, |