Merge branch 'caw-captcha-rest-api-support' into 'master'

Add CAPTCHA to REST API

See merge request gitlab-org/gitlab!80793

app/controllers/concerns/spammable_actions/captcha_check/rest_api_actions_support.rb

+# frozen_string_literal: true
+
+# This module should be included to support CAPTCHA check for REST API actions via Grape.
+#
+# If the request is directly handled by a controller action, then the corresponding module which
+# supports HTML or JSON formats should be used instead.
+module SpammableActions::CaptchaCheck::RestApiActionsSupport
+  extend ActiveSupport::Concern
+  include SpammableActions::CaptchaCheck::Common
+  include Spam::Concerns::HasSpamActionResponseFields
+
+  private
+
+  def with_captcha_check_rest_api(spammable:, &block)
+    # In the case of the REST API, the request is handled by Grape, so if there is a spam-related
+    # error, we don't render directly, instead we will pass the error message and other necessary
+    # fields to the Grape api error helper for it to handle.
+    captcha_render_lambda = -> do
+      fields = spam_action_response_fields(spammable)
+
+      fields.delete :spam
+      # NOTE: "409 - Conflict" seems to be the most appropriate HTTP status code for a response
+      # which requires a CAPTCHA to be solved in order for the request to be resubmitted.
+      # https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.10
+      status = 409
+
+      # NOTE: This nested 'error' key may not be consistent with all other API error responses,
+      # because they are not currently consistent across different API endpoints
+      # and models. Some (snippets) will nest errors in an errors key like this,
+      # while others (issues) will return the model's errors hash without an errors key,
+      # while still others just return a plain string error.
+      # See https://gitlab.com/groups/gitlab-org/-/epics/5527#revisit-inconsistent-shape-of-error-responses-in-rest-api
+      fields[:message] = { error: spammable.errors.full_messages.to_sentence }
+      render_structured_api_error!(fields, status)
+    end
+
+    with_captcha_check_common(spammable: spammable, captcha_render_lambda: captcha_render_lambda, &block)
+  end
+end

app/services/spam/spam_params.rb

@@ -25,6 +25,7 @@ module Spam
   #      then the spam check may fail, or the SpamLog or UserAgentDetail may have missing fields.
   class SpamParams
     def self.new_from_request(request:)
+      self.normalize_grape_request_headers(request: request)
       self.new(
         captcha_response: request.headers['X-GitLab-Captcha-Response'],
         spam_log_id: request.headers['X-GitLab-Spam-Log-Id'],
@@ -52,5 +53,14 @@ module Spam
         other.user_agent == user_agent &&
         other.referer == referer
     end
+
+    def self.normalize_grape_request_headers(request:)
+      # If needed, make a normalized copy of Grape headers with the case of 'GitLab' (with an
+      # uppercase 'L') instead of 'Gitlab' (with a lowercase 'l'), because Grape header helper keys
+      # are "coerced into a capitalized kebab case". See https://github.com/ruby-grape/grape#request
+      %w[X-Gitlab-Captcha-Response X-Gitlab-Spam-Log-Id].each do |header|
+        request.headers[header.gsub('Gitlab', 'GitLab')] = request.headers[header] if request.headers.key?(header)
+      end
+    end
   end
 end

doc/api/graphql/index.md

@@ -187,55 +187,74 @@ NOTE:
 The complexity limits may be revised in future, and additionally, the complexity
 of a query may be altered.
 
-## Spam
-
-GraphQL mutations can be detected as spam. If this happens, a
-[GraphQL top-level error](https://spec.graphql.org/June2018/#sec-Errors) is raised. For example:
-
-```json
-{
-  "errors": [
-    {
-      "message": "Request denied. Spam detected",
-      "locations": [ { "line": 6, "column": 7 } ],
-      "path": [ "updateSnippet" ],
-      "extensions": {
-        "spam": true
+## Resolve mutations detected as spam
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/327360) in GitLab 13.11.
+
+GraphQL mutations can be detected as spam. If a mutation is detected as spam and:
+
+- A CAPTCHA service is not configured, a
+  [GraphQL top-level error](https://spec.graphql.org/June2018/#sec-Errors) is raised. For example:
+
+  ```json
+  {
+    "errors": [
+      {
+        "message": "Request denied. Spam detected",
+        "locations": [ { "line": 6, "column": 7 } ],
+        "path": [ "updateSnippet" ],
+        "extensions": {
+          "spam": true
+        }
+      }
+    ],
+    "data": {
+      "updateSnippet": {
+        "snippet": null
       }
     }
-  ],
-  "data": {
-    "updateSnippet": {
-      "snippet": null
+  }
+  ```
+
+- A CAPTCHA service is configured, you receive a response with:
+  - `needsCaptchaResponse` set to `true`.
+  - The `spamLogId` and `captchaSiteKey` fields set.
+
+  For example:
+
+  ```json
+  {
+    "errors": [
+      {
+        "message": "Request denied. Solve CAPTCHA challenge and retry",
+        "locations": [ { "line": 6, "column": 7 } ],
+        "path": [ "updateSnippet" ],
+        "extensions": {
+          "needsCaptchaResponse": true,
+          "captchaSiteKey": "6LeIxAcTAAAAAJcZVRqyHh71UMIEGNQ_MXjiZKhI",
+          "spamLogId": 67
+        }
+      }
+    ],
+    "data": {
+      "updateSnippet": {
+        "snippet": null,
+      }
     }
   }
-}
-```
-
-If a mutation is detected as potential spam and a CAPTCHA service is configured:
+  ```
 
 - Use the `captchaSiteKey` to obtain a CAPTCHA response value using the appropriate CAPTCHA API.
   Only [Google reCAPTCHA v2](https://developers.google.com/recaptcha/docs/display) is supported.
 - Resubmit the request with the `X-GitLab-Captcha-Response` and `X-GitLab-Spam-Log-Id` headers set.
-
-```json
-{
-  "errors": [
-    {
-      "message": "Request denied. Solve CAPTCHA challenge and retry",
-      "locations": [ { "line": 6, "column": 7 } ],
-      "path": [ "updateSnippet" ],
-      "extensions": {
-        "needsCaptchaResponse": true,
-        "captchaSiteKey": "6LeIxAcTAAAAAJcZVRqyHh71UMIEGNQ_MXjiZKhI",
-        "spamLogId": 67
-      }
-    }
-  ],
-  "data": {
-    "updateSnippet": {
-      "snippet": null,
-    }
-  }
-}
+  
+NOTE:
+The GitLab GraphiQL implementation doesn't permit passing of headers, so we must write
+this as a cURL query. `--data-binary` is used to properly handle escaped double quotes
+in the JSON-embedded query.  
+
+```shell
+export CAPTCHA_RESPONSE="<CAPTCHA response obtained from CAPTCHA service>"
+export SPAM_LOG_ID="<spam_log_id obtained from initial REST response>"
+curl --header "Authorization: Bearer $PRIVATE_TOKEN" --header "Content-Type: application/json" --header "X-GitLab-Captcha-Response: $CAPTCHA_RESPONSE" --header "X-GitLab-Spam-Log-Id: $SPAM_LOG_ID" --request POST --data-binary '{"query": "mutation {createSnippet(input: {title: \"Title\" visibilityLevel: public blobActions: [ { action: create filePath: \"BlobPath\" content: \"BlobContent\" } ] }) { snippet { id title } errors }}"}' "https://gitlab.example.com/api/graphql"
 ```

doc/api/index.md

@@ -767,3 +767,35 @@ some API endpoints also support `text/plain`.
 
 In [GitLab 13.10 and later](https://gitlab.com/gitlab-org/gitlab/-/issues/250342),
 API endpoints do not support `text/plain` by default, unless it's explicitly documented.
+
+## Resolve requests detected as spam
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/352913) in GitLab 14.9.
+
+REST API requests can be detected as spam. If a request is detected as spam and:
+
+- A CAPTCHA service is not configured, an error response is returned. For example:
+
+  ```json
+  {"message":{"error":"Your snippet has been recognized as spam and has been discarded."}}
+  ```
+
+- A CAPTCHA service is configured, you receive a response with:
+  - `needs_captcha_response` set to `true`.
+  - The `spam_log_id` and `captcha_site_key` fields set.
+
+  For example:
+
+  ```json
+  {"needs_captcha_response":true,"spam_log_id":42,"captcha_site_key":"6LeIxAcTAAAAAJcZVRqyHh71UMIEGNQ_MXjiZKhI","message":{"error":"Your snippet has been recognized as spam. Please, change the content or solve the reCAPTCHA to proceed."}}
+  ```
+
+- Use the `captcha_site_key` to obtain a CAPTCHA response value using the appropriate CAPTCHA API.
+  Only [Google reCAPTCHA v2](https://developers.google.com/recaptcha/docs/display) is supported.
+- Resubmit the request with the `X-GitLab-Captcha-Response` and `X-GitLab-Spam-Log-Id` headers set.
+
+```shell
+export CAPTCHA_RESPONSE="<CAPTCHA response obtained from CAPTCHA service>"
+export SPAM_LOG_ID="<spam_log_id obtained from initial REST response>"
+curl --request POST --header "PRIVATE-TOKEN: $PRIVATE_TOKEN" --header "X-GitLab-Captcha-Response: $CAPTCHA_RESPONSE" --header "X-GitLab-Spam-Log-Id: $SPAM_LOG_ID" "https://gitlab.example.com/api/v4/snippets?title=Title&file_name=FileName&content=Content&visibility=public"
+```

lib/api/helpers.rb

@@ -474,17 +474,22 @@ module API
       model.errors.messages
     end
 
-    def render_spam_error!
-      render_api_error!({ error: 'Spam detected' }, 400)
+    def render_api_error!(message, status)
+      render_structured_api_error!({ 'message' => message }, status)
     end
 
-    def render_api_error!(message, status)
+    def render_structured_api_error!(hash, status)
+      # Use this method instead of `render_api_error!` when you have additional top-level
+      # hash entries in addition to 'message' which need to be passed to `#error!`
+      set_status_code_in_env(status)
+      error!(hash, status, header)
+    end
+
+    def set_status_code_in_env(status)
       # grape-logging doesn't pass the status code, so this is a
       # workaround for getting that information in the loggers:
       # https://github.com/aserafin/grape_logging/issues/71
       env[API_RESPONSE_STATUS_CODE] = Rack::Utils.status_code(status)
-
-      error!({ 'message' => message }, status, header)
     end
 
     def handle_api_exception(exception)

lib/api/issues.rb

@@ -4,6 +4,7 @@ module API
   class Issues < ::API::Base
     include PaginationParams
     helpers Helpers::IssuesHelpers
+    helpers SpammableActions::CaptchaCheck::RestApiActionsSupport
 
     before { authenticate_non_get! }
 
@@ -275,14 +276,12 @@ module API
                                               params: issue_params,
                                               spam_params: spam_params).execute
 
-          if issue.spam?
-            render_api_error!({ error: 'Spam detected' }, 400)
-          end
-
           if issue.valid?
             present issue, with: Entities::Issue, current_user: current_user, project: user_project
           else
-            render_validation_error!(issue)
+            with_captcha_check_rest_api(spammable: issue) do
+              render_validation_error!(issue)
+            end
           end
         rescue ::ActiveRecord::RecordNotUnique
           render_api_error!('Duplicated issue', 409)
@@ -320,12 +319,12 @@ module API
                                             params: update_params,
                                             spam_params: spam_params).execute(issue)
 
-        render_spam_error! if issue.spam?
-
         if issue.valid?
           present issue, with: Entities::Issue, current_user: current_user, project: user_project
         else
-          render_validation_error!(issue)
+          with_captcha_check_rest_api(spammable: issue) do
+            render_validation_error!(issue)
+          end
         end
       end
       # rubocop: enable CodeReuse/ActiveRecord

lib/api/project_snippets.rb

@@ -13,6 +13,7 @@ module API
     end
     resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
       helpers Helpers::SnippetsHelpers
+      helpers SpammableActions::CaptchaCheck::RestApiActionsSupport
       helpers do
         def check_snippets_enabled
           forbidden! unless user_project.feature_available?(:snippets, current_user)
@@ -82,9 +83,9 @@ module API
         if service_response.success?
           present snippet, with: Entities::ProjectSnippet, current_user: current_user
         else
-          render_spam_error! if snippet.spam?
-
-          render_api_error!({ error: service_response.message }, service_response.http_status)
+          with_captcha_check_rest_api(spammable: snippet) do
+            render_api_error!({ error: service_response.message }, service_response.http_status)
+          end
         end
       end
 
@@ -124,9 +125,9 @@ module API
         if service_response.success?
           present snippet, with: Entities::ProjectSnippet, current_user: current_user
         else
-          render_spam_error! if snippet.spam?
-
-          render_api_error!({ error: service_response.message }, service_response.http_status)
+          with_captcha_check_rest_api(spammable: snippet) do
+            render_api_error!({ error: service_response.message }, service_response.http_status)
+          end
         end
       end
       # rubocop: enable CodeReuse/ActiveRecord

lib/api/snippets.rb

@@ -9,6 +9,7 @@ module API
 
     resource :snippets do
       helpers Helpers::SnippetsHelpers
+      helpers SpammableActions::CaptchaCheck::RestApiActionsSupport
       helpers do
         def snippets_for_current_user
           SnippetsFinder.new(current_user, author: current_user).execute
@@ -91,9 +92,9 @@ module API
         if service_response.success?
           present snippet, with: Entities::PersonalSnippet, current_user: current_user
         else
-          render_spam_error! if snippet.spam?
-
-          render_api_error!({ error: service_response.message }, service_response.http_status)
+          with_captcha_check_rest_api(spammable: snippet) do
+            render_api_error!({ error: service_response.message }, service_response.http_status)
+          end
         end
       end
 
@@ -135,9 +136,9 @@ module API
         if service_response.success?
           present snippet, with: Entities::PersonalSnippet, current_user: current_user
         else
-          render_spam_error! if snippet.spam?
-
-          render_api_error!({ error: service_response.message }, service_response.http_status)
+          with_captcha_check_rest_api(spammable: snippet) do
+            render_api_error!({ error: service_response.message }, service_response.http_status)
+          end
         end
       end
 

spec/controllers/concerns/spammable_actions/captcha_check/rest_api_actions_support_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe SpammableActions::CaptchaCheck::RestApiActionsSupport do
+  include Rack::Test::Methods
+
+  subject do
+    Class.new(Grape::API) do
+      helpers API::Helpers
+      helpers SpammableActions::CaptchaCheck::RestApiActionsSupport
+
+      get ':id' do
+        # NOTE: This was the only way that seemed to work to inject the mock spammable into the
+        # Grape rack app instance. If there's a better way, improvements are welcome.
+        spammable = Object.fake_spammable_factory
+        with_captcha_check_rest_api(spammable: spammable) do
+          render_api_error!(spammable.errors, 400)
+        end
+      end
+    end
+  end
+
+  def app
+    subject
+  end
+
+  before do
+    allow(Gitlab::Recaptcha).to receive(:load_configurations!) { true }
+  end
+
+  describe '#with_captcha_check_json_format' do
+    let(:spammable) { instance_double(Snippet) }
+
+    before do
+      expect(spammable).to receive(:render_recaptcha?).at_least(:once) { render_recaptcha }
+      allow(Object).to receive(:fake_spammable_factory) { spammable }
+    end
+
+    context 'when spammable.render_recaptcha? is true' do
+      let(:render_recaptcha) { true }
+      let(:spam_log) { instance_double(SpamLog, id: 1) }
+      let(:spammable) { instance_double(Snippet, spam?: true, render_recaptcha?: render_recaptcha, spam_log: spam_log) }
+      let(:recaptcha_site_key) { 'abc123' }
+      let(:err_msg) { 'You gotta solve the CAPTCHA' }
+      let(:spam_action_response_fields) do
+        {
+          spam: true,
+          needs_captcha_response: render_recaptcha,
+          spam_log_id: 1,
+          captcha_site_key: recaptcha_site_key
+        }
+      end
+
+      it 'renders json containing spam_action_response_fields' do
+        allow(spammable).to receive_message_chain('errors.full_messages.to_sentence') { err_msg }
+        allow(Gitlab::CurrentSettings).to receive(:recaptcha_site_key) { recaptcha_site_key }
+        response = get '/test'
+        expected_response = {
+          'needs_captcha_response' => render_recaptcha,
+          'spam_log_id' => 1,
+          'captcha_site_key' => recaptcha_site_key,
+          'message' => { 'error' => err_msg }
+        }
+        expect(Gitlab::Json.parse(response.body)).to eq(expected_response)
+        expect(response.status).to eq(409)
+      end
+    end
+
+    context 'when spammable.render_recaptcha? is false' do
+      let(:render_recaptcha) { false }
+      let(:errors) { { 'base' => "It's definitely spam" } }
+
+      it 'yields to block' do
+        allow(spammable).to receive(:errors) { errors }
+
+        response = get 'test'
+        expected_response = {
+          'message' => errors
+        }
+        expect(Gitlab::Json.parse(response.body)).to eq(expected_response)
+        expect(response.status).to eq(400)
+      end
+    end
+  end
+end

spec/controllers/projects/issues_controller_spec.rb

@@ -1018,6 +1018,7 @@ RSpec.describe Projects::IssuesController do
             end
 
             it 'returns 200 status' do
+              update_verified_issue
               expect(response).to have_gitlab_http_status(:ok)
             end
 

spec/requests/api/issues/post_projects_issues_spec.rb

@@ -447,7 +447,7 @@ RSpec.describe API::Issues do
         post_issue
 
         expect(response).to have_gitlab_http_status(:bad_request)
-        expect(json_response['message']).to eq({ 'error' => 'Spam detected' })
+        expect(json_response['message']['base']).to match_array([/issue has been recognized as spam/])
       end
 
       it 'creates a new spam log entry' do

spec/requests/api/issues/put_projects_issues_spec.rb

@@ -217,7 +217,7 @@ RSpec.describe API::Issues do
         update_issue
 
         expect(response).to have_gitlab_http_status(:bad_request)
-        expect(json_response).to include('message' => { 'error' => 'Spam detected' })
+        expect(json_response['message']['base']).to match_array([/issue has been recognized as spam/])
       end
 
       it 'creates a new spam log entry' do

spec/requests/api/project_snippets_spec.rb

@@ -276,7 +276,7 @@ RSpec.describe API::ProjectSnippets do
         it 'rejects the snippet' do
           expect { subject }.not_to change { Snippet.count }
           expect(response).to have_gitlab_http_status(:bad_request)
-          expect(json_response['message']).to eq({ "error" => "Spam detected" })
+          expect(json_response['message']['error']).to match(/snippet has been recognized as spam/)
         end
 
         it 'creates a spam log' do
@@ -344,7 +344,7 @@ RSpec.describe API::ProjectSnippets do
             .not_to change { snippet.reload.title }
 
           expect(response).to have_gitlab_http_status(:bad_request)
-          expect(json_response['message']).to eq({ "error" => "Spam detected" })
+          expect(json_response['message']['error']).to match(/snippet has been recognized as spam/)
         end
 
         it 'creates a spam log' do

spec/requests/api/snippets_spec.rb

@@ -325,7 +325,7 @@ RSpec.describe API::Snippets, factory_default: :keep do
           expect { subject }.not_to change { Snippet.count }
 
           expect(response).to have_gitlab_http_status(:bad_request)
-          expect(json_response['message']).to eq({ "error" => "Spam detected" })
+          expect(json_response['message']['error']).to match(/snippet has been recognized as spam/)
         end
 
         it 'creates a spam log' do
@@ -392,7 +392,7 @@ RSpec.describe API::Snippets, factory_default: :keep do
             .not_to change { snippet.reload.title }
 
           expect(response).to have_gitlab_http_status(:bad_request)
-          expect(json_response['message']).to eq({ "error" => "Spam detected" })
+          expect(json_response['message']['error']).to match(/snippet has been recognized as spam/)
         end
 
         it 'creates a spam log' do

spec/services/spam/spam_params_spec.rb

@@ -3,18 +3,25 @@
 require 'spec_helper'
 
 RSpec.describe Spam::SpamParams do
+  shared_examples 'constructs from a request' do
+    it 'constructs from a request' do
+      expected = ::Spam::SpamParams.new(
+        captcha_response: captcha_response,
+        spam_log_id: spam_log_id,
+        ip_address: ip_address,
+        user_agent: user_agent,
+        referer: referer
+      )
+      expect(described_class.new_from_request(request: request)).to eq(expected)
+    end
+  end
+
   describe '.new_from_request' do
     let(:captcha_response) { 'abc123' }
     let(:spam_log_id) { 42 }
     let(:ip_address) { '0.0.0.0' }
     let(:user_agent) { 'Lynx' }
     let(:referer) { 'http://localhost' }
-    let(:headers) do
-      {
-        'X-GitLab-Captcha-Response' => captcha_response,
-        'X-GitLab-Spam-Log-Id' => spam_log_id
-      }
-    end
 
     let(:env) do
       {
@@ -24,17 +31,28 @@ RSpec.describe Spam::SpamParams do
       }
     end
 
-    let(:request) {double(:request, headers: headers, env: env)}
+    let(:request) { double(:request, headers: headers, env: env) }
 
-    it 'constructs from a request' do
-      expected = ::Spam::SpamParams.new(
-        captcha_response: captcha_response,
-        spam_log_id: spam_log_id,
-        ip_address: ip_address,
-        user_agent: user_agent,
-        referer: referer
-      )
-      expect(described_class.new_from_request(request: request)).to eq(expected)
+    context 'with a normal Rails request' do
+      let(:headers) do
+        {
+          'X-GitLab-Captcha-Response' => captcha_response,
+          'X-GitLab-Spam-Log-Id' => spam_log_id
+        }
+      end
+
+      it_behaves_like 'constructs from a request'
+    end
+
+    context 'with a grape request' do
+      let(:headers) do
+        {
+          'X-Gitlab-Captcha-Response' => captcha_response,
+          'X-Gitlab-Spam-Log-Id' => spam_log_id
+        }
+      end
+
+      it_behaves_like 'constructs from a request'
     end
   end
 end