Merge branch 'ajk-snippet-raise-on-spam' into 'master'

Create new spam protection concern for mutations

See merge request gitlab-org/gitlab!56190

app/assets/javascripts/lib/graphql.js

@@ -4,6 +4,7 @@ import { ApolloLink } from 'apollo-link';
 import { BatchHttpLink } from 'apollo-link-batch-http';
 import { createHttpLink } from 'apollo-link-http';
 import { createUploadLink } from 'apollo-upload-client';
+import { apolloCaptchaLink } from '~/captcha/apollo_captcha_link';
 import { StartupJSLink } from '~/lib/utils/apollo_startup_js_link';
 import csrf from '~/lib/utils/csrf';
 import PerformanceBarService from '~/performance_bar/services/performance_bar_service';
@@ -78,6 +79,7 @@ export default (resolvers = {}, config = {}) => {
       requestCounterLink,
       performanceBarLink,
       new StartupJSLink(),
+      apolloCaptchaLink,
       uploadsLink,
     ]),
     cache: new InMemoryCache({

app/assets/javascripts/snippets/components/edit.vue

@@ -33,7 +33,6 @@ export default {
     SnippetBlobActionsEdit,
     TitleField,
     FormFooterActions,
-    CaptchaModal: () => import('~/captcha/captcha_modal.vue'),
     GlButton,
     GlLoadingIcon,
   },
@@ -68,10 +67,6 @@ export default {
         description: '',
         visibilityLevel: this.selectedLevel,
       },
-      captchaResponse: '',
-      needsCaptchaResponse: false,
-      captchaSiteKey: '',
-      spamLogId: '',
     };
   },
   computed: {
@@ -103,8 +98,6 @@ export default {
         description: this.snippet.description,
         visibilityLevel: this.snippet.visibilityLevel,
         blobActions: this.actions,
-        ...(this.spamLogId && { spamLogId: this.spamLogId }),
-        ...(this.captchaResponse && { captchaResponse: this.captchaResponse }),
       };
     },
     saveButtonLabel() {
@@ -171,20 +164,14 @@ export default {
     },
     handleFormSubmit() {
       this.isUpdating = true;
+
       this.$apollo
         .mutate(this.newSnippet ? this.createMutation() : this.updateMutation())
         .then(({ data }) => {
           const baseObj = this.newSnippet ? data?.createSnippet : data?.updateSnippet;
 
-          if (baseObj.needsCaptchaResponse) {
-            // If we need a captcha response, start process for receiving captcha response.
-            // We will resubmit after the response is obtained.
-            this.requestCaptchaResponse(baseObj.captchaSiteKey, baseObj.spamLogId);
-            return;
-          }
-
           const errors = baseObj?.errors;
-          if (errors.length) {
+          if (errors?.length) {
             this.flashAPIFailure(errors[0]);
           } else {
             redirectTo(baseObj.snippet.webUrl);
@@ -200,38 +187,6 @@ export default {
     updateActions(actions) {
       this.actions = actions;
     },
-    /**
-     * Start process for getting captcha response from user
-     *
-     * @param captchaSiteKey Stored in data and used to display the captcha.
-     * @param spamLogId Stored in data and included when the form is re-submitted.
-     */
-    requestCaptchaResponse(captchaSiteKey, spamLogId) {
-      this.captchaSiteKey = captchaSiteKey;
-      this.spamLogId = spamLogId;
-      this.needsCaptchaResponse = true;
-    },
-    /**
-     * Handle the captcha response from the user
-     *
-     * @param captchaResponse The captchaResponse value emitted from the modal.
-     */
-    receivedCaptchaResponse(captchaResponse) {
-      this.needsCaptchaResponse = false;
-      this.captchaResponse = captchaResponse;
-
-      if (this.captchaResponse) {
-        // If the user solved the captcha, resubmit the form.
-        // NOTE: we do not need to clear out the captchaResponse and spamLogId
-        // data values after submit, because this component always does a full page reload.
-        // Otherwise, we would need to.
-        this.handleFormSubmit();
-      } else {
-        // If the user didn't solve the captcha (e.g. they just closed the modal),
-        // finish the update and allow them to continue editing or manually resubmit the form.
-        this.isUpdating = false;
-      }
-    },
   },
 };
 </script>
@@ -249,11 +204,6 @@ export default {
       class="loading-animation prepend-top-20 gl-mb-6"
     />
     <template v-else>
-      <captcha-modal
-        :captcha-site-key="captchaSiteKey"
-        :needs-captcha-response="needsCaptchaResponse"
-        @receivedCaptchaResponse="receivedCaptchaResponse"
-      />
       <title-field
         id="snippet-title"
         v-model="snippet.title"

app/assets/javascripts/snippets/mutations/createSnippet.mutation.graphql

@@ -4,7 +4,5 @@ mutation CreateSnippet($input: CreateSnippetInput!) {
     snippet {
       webUrl
     }
-    needsCaptchaResponse
-    captchaSiteKey
   }
 }

app/assets/javascripts/snippets/mutations/updateSnippet.mutation.graphql

@@ -4,8 +4,5 @@ mutation UpdateSnippet($input: UpdateSnippetInput!) {
     snippet {
       webUrl
     }
-    needsCaptchaResponse
-    captchaSiteKey
-    spamLogId
   }
 }

app/graphql/mutations/concerns/mutations/can_mutate_spammable.rb

 # frozen_string_literal: true
 
 module Mutations
-  # This concern can be mixed into a mutation to provide support for spam checking,
-  # and optionally support the workflow to allow clients to display and solve CAPTCHAs.
+  # This concern is deprecated and will be deleted in 14.6
+  #
+  # Use the SpamProtection concern instead.
   module CanMutateSpammable
     extend ActiveSupport::Concern
-    include Spam::Concerns::HasSpamActionResponseFields
 
-    # NOTE: The arguments and fields are intentionally named with 'captcha' instead of 'recaptcha',
-    # so that they can be applied to future alternative CAPTCHA implementations other than
-    # reCAPTCHA (e.g. FriendlyCaptcha) without having to change the names and descriptions in the API.
+    DEPRECATION_NOTICE = {
+      reason: 'Use spam protection with HTTP headers instead',
+      milestone: '13.11'
+    }.freeze
+
     included do
       argument :captcha_response, GraphQL::STRING_TYPE,
                required: false,
+               deprecated: DEPRECATION_NOTICE,
                description: 'A valid CAPTCHA response value obtained by using the provided captchaSiteKey with a CAPTCHA API to present a challenge to be solved on the client. Required to resubmit if the previous operation returned "NeedsCaptchaResponse: true".'
 
       argument :spam_log_id, GraphQL::INT_TYPE,
                required: false,
+               deprecated: DEPRECATION_NOTICE,
                description: 'The spam log ID which must be passed along with a valid CAPTCHA response for the operation to be completed. Required to resubmit if the previous operation returned "NeedsCaptchaResponse: true".'
 
       field :spam,
             GraphQL::BOOLEAN_TYPE,
             null: true,
+            deprecated: DEPRECATION_NOTICE,
             description: 'Indicates whether the operation was detected as definite spam. There is no option to resubmit the request with a CAPTCHA response.'
 
       field :needs_captcha_response,
             GraphQL::BOOLEAN_TYPE,
             null: true,
+            deprecated: DEPRECATION_NOTICE,
             description: 'Indicates whether the operation was detected as possible spam and not completed. If CAPTCHA is enabled, the request must be resubmitted with a valid CAPTCHA response and spam_log_id included for the operation to be completed. Included only when an operation was not completed because "NeedsCaptchaResponse" is true.'
 
       field :spam_log_id,
             GraphQL::INT_TYPE,
             null: true,
+            deprecated: DEPRECATION_NOTICE,
             description: 'The spam log ID which must be passed along with a valid CAPTCHA response for an operation to be completed. Included only when an operation was not completed because "NeedsCaptchaResponse" is true.'
 
       field :captcha_site_key,
             GraphQL::STRING_TYPE,
             null: true,
+            deprecated: DEPRECATION_NOTICE,
             description: 'The CAPTCHA site key which must be used to render a challenge for the user to solve to obtain a valid captchaResponse value. Included only when an operation was not completed because "NeedsCaptchaResponse" is true.'
     end
-
-    private
-
-    # additional_spam_params    -> hash
-    #
-    # Used from a spammable mutation's #resolve method to generate
-    # the required additional spam/recaptcha params which must be merged into the params
-    # passed to the constructor of a service, where they can then be used in the service
-    # to perform spam checking via SpamActionService.
-    #
-    # Also accesses the #context of the mutation's Resolver superclass to obtain the request.
-    #
-    # Example:
-    #
-    # existing_args.merge!(additional_spam_params)
-    def additional_spam_params
-      {
-        api: true,
-        request: context[:request]
-      }
-    end
   end
 end

app/graphql/mutations/concerns/mutations/spam_protection.rb

+# frozen_string_literal: true
+
+module Mutations
+  # This concern can be mixed into a mutation to provide support for spam checking,
+  # and optionally support the workflow to allow clients to display and solve CAPTCHAs.
+  module SpamProtection
+    extend ActiveSupport::Concern
+    include Spam::Concerns::HasSpamActionResponseFields
+
+    SpamActionError = Class.new(GraphQL::ExecutionError)
+    NeedsCaptchaResponseError = Class.new(SpamActionError)
+    SpamDisallowedError = Class.new(SpamActionError)
+
+    NEEDS_CAPTCHA_RESPONSE_MESSAGE = "Request denied. Solve CAPTCHA challenge and retry"
+    SPAM_DISALLOWED_MESSAGE = "Request denied. Spam detected"
+
+    private
+
+    # additional_spam_params    -> hash
+    #
+    # Used from a spammable mutation's #resolve method to generate
+    # the required additional spam/CAPTCHA params which must be merged into the params
+    # passed to the constructor of a service, where they can then be used in the service
+    # to perform spam checking via SpamActionService.
+    #
+    # Also accesses the #context of the mutation's Resolver superclass to obtain the request.
+    #
+    # Example:
+    #
+    # existing_args.merge!(additional_spam_params)
+    def additional_spam_params
+      {
+        api: true,
+        request: context[:request]
+      }
+    end
+
+    def spam_action_response(object)
+      fields = spam_action_response_fields(object)
+
+      # If the SpamActionService detected something as spam,
+      # this is non-recoverable and the needs_captcha_response
+      # should not be considered
+      kind = if fields[:spam]
+               :spam
+             elsif fields[:needs_captcha_response]
+               :needs_captcha_response
+             end
+
+      [kind, fields]
+    end
+
+    def check_spam_action_response!(object)
+      kind, fields = spam_action_response(object)
+
+      case kind
+      when :needs_captcha_response
+        fields.delete :spam
+        raise NeedsCaptchaResponseError.new(NEEDS_CAPTCHA_RESPONSE_MESSAGE, extensions: fields)
+      when :spam
+        raise SpamDisallowedError.new(SPAM_DISALLOWED_MESSAGE, extensions: { spam: true })
+      else
+        nil
+      end
+    end
+  end
+end

app/graphql/mutations/snippets/create.rb

@@ -5,6 +5,7 @@ module Mutations
     class Create < BaseMutation
       include ServiceCompatibility
       include CanMutateSpammable
+      include Mutations::SpamProtection
 
       authorize :create_snippet
 
@@ -56,12 +57,12 @@ module Mutations
         end
 
         snippet = service_response.payload[:snippet]
-        with_spam_action_response_fields(snippet) do
-          {
-            snippet: service_response.success? ? snippet : nil,
-            errors: errors_on_object(snippet)
-          }
-        end
+        check_spam_action_response!(snippet)
+
+        {
+          snippet: service_response.success? ? snippet : nil,
+          errors: errors_on_object(snippet)
+        }
       end
 
       private

app/graphql/mutations/snippets/update.rb

@@ -5,6 +5,7 @@ module Mutations
     class Update < Base
       include ServiceCompatibility
       include CanMutateSpammable
+      include Mutations::SpamProtection
 
       graphql_name 'UpdateSnippet'
 
@@ -45,12 +46,12 @@ module Mutations
         end
 
         snippet = service_response.payload[:snippet]
-        with_spam_action_response_fields(snippet) do
-          {
-            snippet: service_response.success? ? snippet : snippet.reset,
-            errors: errors_on_object(snippet)
-          }
-        end
+        check_spam_action_response!(snippet)
+
+        {
+          snippet: service_response.success? ? snippet : snippet.reset,
+          errors: errors_on_object(snippet)
+        }
       end
 
       private

app/services/issues/create_service.rb

@@ -6,7 +6,7 @@ module Issues
 
     def execute(skip_system_notes: false)
       @request = params.delete(:request)
-      @spam_params = Spam::SpamActionService.filter_spam_params!(params)
+      @spam_params = Spam::SpamActionService.filter_spam_params!(params, @request)
 
       @issue = BuildService.new(project, current_user, params).execute
 

app/services/issues/update_service.rb

@@ -8,7 +8,7 @@ module Issues
       handle_move_between_ids(issue)
 
       @request = params.delete(:request)
-      @spam_params = Spam::SpamActionService.filter_spam_params!(params)
+      @spam_params = Spam::SpamActionService.filter_spam_params!(params, @request)
 
       change_issue_duplicate(issue)
       move_issue_to_new_project(issue) || clone_issue(issue) || update_task_event(issue) || update(issue)

app/services/snippets/create_service.rb

@@ -6,7 +6,7 @@ module Snippets
       # NOTE: disable_spam_action_service can be removed when the ':snippet_spam' feature flag is removed.
       disable_spam_action_service = params.delete(:disable_spam_action_service) == true
       @request = params.delete(:request)
-      @spam_params = Spam::SpamActionService.filter_spam_params!(params)
+      @spam_params = Spam::SpamActionService.filter_spam_params!(params, @request)
 
       @snippet = build_from_params
 

app/services/snippets/update_service.rb

@@ -10,7 +10,7 @@ module Snippets
       # NOTE: disable_spam_action_service can be removed when the ':snippet_spam' feature flag is removed.
       disable_spam_action_service = params.delete(:disable_spam_action_service) == true
       @request = params.delete(:request)
-      @spam_params = Spam::SpamActionService.filter_spam_params!(params)
+      @spam_params = Spam::SpamActionService.filter_spam_params!(params, @request)
 
       return invalid_params_error(snippet) unless valid_params?
 

app/services/spam/spam_action_service.rb

@@ -11,22 +11,30 @@ module Spam
     # Takes a hash of parameters from an incoming request to modify a model (via a controller,
     # service, or GraphQL mutation). The parameters will either be camelCase (if they are
     # received directly via controller params) or underscore_case (if they have come from
-    # a GraphQL mutation which has converted them to underscore)
+    # a GraphQL mutation which has converted them to underscore), or in the
+    # headers when using the header based flow.
     #
     # Deletes the parameters which are related to spam and captcha processing, and returns
     # them in a SpamParams parameters object. See:
     # https://refactoring.com/catalog/introduceParameterObject.html
-    def self.filter_spam_params!(params)
+    def self.filter_spam_params!(params, request)
       # NOTE: The 'captcha_response' field can be expanded to multiple fields when we move to future
       # alternative captcha implementations such as FriendlyCaptcha. See
       # https://gitlab.com/gitlab-org/gitlab/-/issues/273480
-      captcha_response = params.delete(:captcha_response) || params.delete(:captchaResponse)
+      headers = request&.headers || {}
+      api = params.delete(:api)
+      captcha_response = read_parameter(:captcha_response, params, headers)
+      spam_log_id      = read_parameter(:spam_log_id, params, headers)&.to_i
 
-      SpamParams.new(
-        api: params.delete(:api),
-        captcha_response: captcha_response,
-        spam_log_id: params.delete(:spam_log_id) || params.delete(:spamLogId)
-      )
+      SpamParams.new(api: api, captcha_response: captcha_response, spam_log_id: spam_log_id)
+    end
+
+    def self.read_parameter(name, params, headers)
+      [
+        params.delete(name),
+        params.delete(name.to_s.camelize(:lower).to_sym),
+        headers["X-GitLab-#{name.to_s.titlecase(keep_id_suffix: true).tr(' ', '-')}"]
+      ].compact.first
     end
 
     attr_accessor :target, :request, :options
@@ -40,6 +48,7 @@ module Spam
       @options = {}
     end
 
+    # rubocop:disable Metrics/AbcSize
     def execute(spam_params:)
       if request
         options[:ip_address] = request.env['action_dispatch.remote_ip'].to_s
@@ -58,19 +67,20 @@ module Spam
       )
 
       if recaptcha_verified
-        # If it's a request which is already verified through captcha,
+        # If it's a request which is already verified through CAPTCHA,
         # update the spam log accordingly.
         SpamLog.verify_recaptcha!(user_id: user.id, id: spam_params.spam_log_id)
-        ServiceResponse.success(message: "Captcha was successfully verified")
+        ServiceResponse.success(message: "CAPTCHA successfully verified")
       else
         return ServiceResponse.success(message: 'Skipped spam check because user was allowlisted') if allowlisted?(user)
         return ServiceResponse.success(message: 'Skipped spam check because request was not present') unless request
         return ServiceResponse.success(message: 'Skipped spam check because it was not required') unless check_for_spam?
 
         perform_spam_service_check(spam_params.api)
-        ServiceResponse.success(message: "Spam check performed, check #{target.class.name} spammable model for any errors or captcha requirement")
+        ServiceResponse.success(message: "Spam check performed. Check #{target.class.name} spammable model for any errors or CAPTCHA requirement")
       end
     end
+    # rubocop:enable Metrics/AbcSize
 
     delegate :check_for_spam?, to: :target
 

app/services/spam/spam_params.rb

@@ -23,10 +23,10 @@ module Spam
     end
 
     def ==(other)
-      other.class == self.class &&
-        other.api == self.api &&
-        other.captcha_response == self.captcha_response &&
-        other.spam_log_id == self.spam_log_id
+      other.class <= self.class &&
+        other.api == api &&
+        other.captcha_response == captcha_response &&
+        other.spam_log_id == spam_log_id
     end
   end
 end

doc/api/graphql/index.md

@@ -70,7 +70,7 @@ possible.
 The GitLab GraphQL API is [versionless](https://graphql.org/learn/best-practices/#versioning) and
 changes are made to the API in a way that maintains backwards-compatibility.
 
-Occassionally GitLab needs to change the GraphQL API in a way that is not backwards-compatible.
+Occasionally GitLab needs to change the GraphQL API in a way that is not backwards-compatible.
 These changes include the removal or renaming of fields, arguments or other parts of the schema.
 
 In these situations, GitLab follows a [Deprecation and removal process](#deprecation-and-removal-process)
@@ -177,6 +177,59 @@ of a query may be altered.
 
 Requests time out at 30 seconds.
 
+### 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
+      }
+    }
+  ],
+  "data": {
+    "updateSnippet": {
+      "snippet": null
+    }
+  }
+}
+```
+
+If mutation is detected as potential spam and a CAPTCHA service is configured:
+
+- The `captchaSiteKey` should be used to obtain a CAPTCHA response value using the appropriate CAPTCHA API.
+  Only [Google reCAPTCHA v2](https://developers.google.com/recaptcha/docs/display) is supported.
+- The request can be resubmitted 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,
+    }
+  }
+}
+```
+
 ## Reference
 
 The GitLab GraphQL reference [is available](reference/index.md).

doc/api/graphql/reference/index.md

@@ -1824,13 +1824,13 @@ Autogenerated return type of CreateSnippet.
 
 | Field | Type | Description |
 | ----- | ---- | ----------- |
-| `captchaSiteKey` | [`String`](#string) | The CAPTCHA site key which must be used to render a challenge for the user to solve to obtain a valid captchaResponse value. Included only when an operation was not completed because "NeedsCaptchaResponse" is true. |
+| `captchaSiteKey` **{warning-solid}** | [`String`](#string) | **Deprecated** in 13.11. Use spam protection with HTTP headers instead. |
 | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. |
 | `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. |
-| `needsCaptchaResponse` | [`Boolean`](#boolean) | Indicates whether the operation was detected as possible spam and not completed. If CAPTCHA is enabled, the request must be resubmitted with a valid CAPTCHA response and spam_log_id included for the operation to be completed. Included only when an operation was not completed because "NeedsCaptchaResponse" is true. |
+| `needsCaptchaResponse` **{warning-solid}** | [`Boolean`](#boolean) | **Deprecated** in 13.11. Use spam protection with HTTP headers instead. |
 | `snippet` | [`Snippet`](#snippet) | The snippet after mutation. |
-| `spam` | [`Boolean`](#boolean) | Indicates whether the operation was detected as definite spam. There is no option to resubmit the request with a CAPTCHA response. |
-| `spamLogId` | [`Int`](#int) | The spam log ID which must be passed along with a valid CAPTCHA response for an operation to be completed. Included only when an operation was not completed because "NeedsCaptchaResponse" is true. |
+| `spam` **{warning-solid}** | [`Boolean`](#boolean) | **Deprecated** in 13.11. Use spam protection with HTTP headers instead. |
+| `spamLogId` **{warning-solid}** | [`Int`](#int) | **Deprecated** in 13.11. Use spam protection with HTTP headers instead. |
 
 ### `CreateTestCasePayload`
 
@@ -6622,13 +6622,13 @@ Autogenerated return type of UpdateSnippet.
 
 | Field | Type | Description |
 | ----- | ---- | ----------- |
-| `captchaSiteKey` | [`String`](#string) | The CAPTCHA site key which must be used to render a challenge for the user to solve to obtain a valid captchaResponse value. Included only when an operation was not completed because "NeedsCaptchaResponse" is true. |
+| `captchaSiteKey` **{warning-solid}** | [`String`](#string) | **Deprecated** in 13.11. Use spam protection with HTTP headers instead. |
 | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. |
 | `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. |
-| `needsCaptchaResponse` | [`Boolean`](#boolean) | Indicates whether the operation was detected as possible spam and not completed. If CAPTCHA is enabled, the request must be resubmitted with a valid CAPTCHA response and spam_log_id included for the operation to be completed. Included only when an operation was not completed because "NeedsCaptchaResponse" is true. |
+| `needsCaptchaResponse` **{warning-solid}** | [`Boolean`](#boolean) | **Deprecated** in 13.11. Use spam protection with HTTP headers instead. |
 | `snippet` | [`Snippet`](#snippet) | The snippet after mutation. |
-| `spam` | [`Boolean`](#boolean) | Indicates whether the operation was detected as definite spam. There is no option to resubmit the request with a CAPTCHA response. |
-| `spamLogId` | [`Int`](#int) | The spam log ID which must be passed along with a valid CAPTCHA response for an operation to be completed. Included only when an operation was not completed because "NeedsCaptchaResponse" is true. |
+| `spam` **{warning-solid}** | [`Boolean`](#boolean) | **Deprecated** in 13.11. Use spam protection with HTTP headers instead. |
+| `spamLogId` **{warning-solid}** | [`Int`](#int) | **Deprecated** in 13.11. Use spam protection with HTTP headers instead. |
 
 ### `UsageTrendsMeasurement`
 

lib/spam/concerns/has_spam_action_response_fields.rb

@@ -23,15 +23,6 @@ module Spam
           captcha_site_key: Gitlab::CurrentSettings.recaptcha_site_key
         }
       end
-
-      # with_spam_action_response_fields(spammable) { {other_fields: true} }    -> hash
-      #
-      # Takes a Spammable and a block as arguments.
-      #
-      # The block passed should be a hash, which the spam_action_fields will be merged into.
-      def with_spam_action_response_fields(spammable)
-        yield.merge(spam_action_response_fields(spammable))
-      end
     end
   end
 end

spec/frontend/captcha/apollo_captcha_link_spec.js

@@ -44,7 +44,7 @@ describe('apolloCaptchaLink', () => {
     },
     errors: [
       {
-        message: 'Your Query was detected to be SPAM.',
+        message: 'Your Query was detected to be spam.',
         path: ['user'],
         locations: [{ line: 2, column: 3 }],
         extensions: {
@@ -116,7 +116,7 @@ describe('apolloCaptchaLink', () => {
     });
   });
 
-  it('unresolvable SPAM errors are passed through', (done) => {
+  it('unresolvable spam errors are passed through', (done) => {
     setupLink(SPAM_ERROR_RESPONSE);
     link.request(mockOperation()).subscribe((result) => {
       expect(result).toEqual(SPAM_ERROR_RESPONSE);
@@ -127,8 +127,8 @@ describe('apolloCaptchaLink', () => {
     });
   });
 
-  describe('resolvable SPAM errors', () => {
-    it('re-submits request with SPAM headers if the captcha modal was solved correctly', (done) => {
+  describe('resolvable spam errors', () => {
+    it('re-submits request with spam headers if the captcha modal was solved correctly', (done) => {
       waitForCaptchaToBeSolved.mockResolvedValue(CAPTCHA_RESPONSE);
       setupLink(CAPTCHA_ERROR_RESPONSE, SUCCESS_RESPONSE);
       link.request(mockOperation()).subscribe((result) => {

spec/frontend/snippets/components/edit_spec.js

@@ -5,10 +5,9 @@ import { nextTick } from 'vue';
 import VueApollo, { ApolloMutation } from 'vue-apollo';
 import { useFakeDate } from 'helpers/fake_date';
 import createMockApollo from 'helpers/mock_apollo_helper';
-import { stubComponent } from 'helpers/stub_component';
 import waitForPromises from 'helpers/wait_for_promises';
 import GetSnippetQuery from 'shared_queries/snippet/snippet.query.graphql';
-import CaptchaModal from '~/captcha/captcha_modal.vue';
+import UnsolvedCaptchaError from '~/captcha/unsolved_captcha_error';
 import { deprecatedCreateFlash as Flash } from '~/flash';
 import * as urlUtils from '~/lib/utils/url_utility';
 import SnippetEditApp from '~/snippets/components/edit.vue';
@@ -30,9 +29,8 @@ jest.mock('~/flash');
 
 const TEST_UPLOADED_FILES = ['foo/bar.txt', 'alpha/beta.js'];
 const TEST_API_ERROR = new Error('TEST_API_ERROR');
+const TEST_CAPTCHA_ERROR = new UnsolvedCaptchaError();
 const TEST_MUTATION_ERROR = 'Test mutation error';
-const TEST_CAPTCHA_RESPONSE = 'i-got-a-captcha';
-const TEST_CAPTCHA_SITE_KEY = 'abc123';
 const TEST_ACTIONS = {
   NO_CONTENT: merge({}, testEntries.created.diff, { content: '' }),
   NO_PATH: merge({}, testEntries.created.diff, { filePath: '' }),
@@ -59,9 +57,6 @@ const createMutationResponse = (key, obj = {}) => ({
           __typename: 'Snippet',
           webUrl: TEST_WEB_URL,
         },
-        spamLogId: null,
-        needsCaptchaResponse: false,
-        captchaSiteKey: null,
       },
       obj,
     ),
@@ -71,13 +66,6 @@ const createMutationResponse = (key, obj = {}) => ({
 const createMutationResponseWithErrors = (key) =>
   createMutationResponse(key, { errors: [TEST_MUTATION_ERROR] });
 
-const createMutationResponseWithRecaptcha = (key) =>
-  createMutationResponse(key, {
-    errors: ['ignored captcha error message'],
-    needsCaptchaResponse: true,
-    captchaSiteKey: TEST_CAPTCHA_SITE_KEY,
-  });
-
 const getApiData = ({
   id,
   title = '',
@@ -126,7 +114,6 @@ describe('Snippet Edit app', () => {
   });
 
   const findBlobActions = () => wrapper.find(SnippetBlobActionsEdit);
-  const findCaptchaModal = () => wrapper.find(CaptchaModal);
   const findSubmitButton = () => wrapper.find('[data-testid="snippet-submit-btn"]');
   const findCancelButton = () => wrapper.find('[data-testid="snippet-cancel-btn"]');
   const hasDisabledSubmit = () => Boolean(findSubmitButton().attributes('disabled'));
@@ -159,7 +146,6 @@ describe('Snippet Edit app', () => {
       stubs: {
         ApolloMutation,
         FormFooterActions,
-        CaptchaModal: stubComponent(CaptchaModal),
       },
       provide: {
         selectedLevel,
@@ -209,7 +195,6 @@ describe('Snippet Edit app', () => {
     });
 
     it('should render components', () => {
-      expect(wrapper.find(CaptchaModal).exists()).toBe(true);
       expect(wrapper.find(TitleField).exists()).toBe(true);
       expect(wrapper.find(SnippetDescriptionEdit).exists()).toBe(true);
       expect(wrapper.find(SnippetVisibilityEdit).exists()).toBe(true);
@@ -338,10 +323,10 @@ describe('Snippet Edit app', () => {
         },
       );
 
-      describe('with apollo network error', () => {
+      describe.each([TEST_API_ERROR, TEST_CAPTCHA_ERROR])('with apollo network error', (error) => {
         beforeEach(async () => {
           jest.spyOn(console, 'error').mockImplementation();
-          mutateSpy.mockRejectedValue(TEST_API_ERROR);
+          mutateSpy.mockRejectedValue(error);
 
           await createComponentAndSubmit();
         });
@@ -353,7 +338,7 @@ describe('Snippet Edit app', () => {
         it('should flash', () => {
           // Apollo automatically wraps the resolver's error in a NetworkError
           expect(Flash).toHaveBeenCalledWith(
-            `Can't update snippet: Network error: ${TEST_API_ERROR.message}`,
+            `Can't update snippet: Network error: ${error.message}`,
           );
         });
 
@@ -363,54 +348,10 @@ describe('Snippet Edit app', () => {
           // eslint-disable-next-line no-console
           expect(console.error).toHaveBeenCalledWith(
             '[gitlab] unexpected error while updating snippet',
-            expect.objectContaining({ message: `Network error: ${TEST_API_ERROR.message}` }),
+            expect.objectContaining({ message: `Network error: ${error.message}` }),
           );
         });
       });
-
-      describe('when needsCaptchaResponse is true', () => {
-        let modal;
-
-        beforeEach(async () => {
-          mutateSpy
-            .mockResolvedValueOnce(createMutationResponseWithRecaptcha('updateSnippet'))
-            .mockResolvedValueOnce(createMutationResponseWithErrors('updateSnippet'));
-
-          await createComponentAndSubmit();
-
-          modal = findCaptchaModal();
-
-          mutateSpy.mockClear();
-        });
-
-        it('should display captcha modal', () => {
-          expect(urlUtils.redirectTo).not.toHaveBeenCalled();
-          expect(modal.props()).toEqual({
-            needsCaptchaResponse: true,
-            captchaSiteKey: TEST_CAPTCHA_SITE_KEY,
-          });
-        });
-
-        describe.each`
-          response                 | expectedCalls
-          ${null}                  | ${[]}
-          ${TEST_CAPTCHA_RESPONSE} | ${[['updateSnippet', { input: { ...getApiData(createSnippet()), captchaResponse: TEST_CAPTCHA_RESPONSE } }]]}
-        `('when captcha response is $response', ({ response, expectedCalls }) => {
-          beforeEach(async () => {
-            modal.vm.$emit('receivedCaptchaResponse', response);
-
-            await nextTick();
-          });
-
-          it('sets needsCaptchaResponse to false', () => {
-            expect(modal.props('needsCaptchaResponse')).toEqual(false);
-          });
-
-          it(`expected to call times = ${expectedCalls.length}`, () => {
-            expect(mutateSpy.mock.calls).toEqual(expectedCalls);
-          });
-        });
-      });
     });
   });
 

spec/graphql/mutations/concerns/mutations/can_mutate_spammable_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Mutations::CanMutateSpammable do
-  let(:mutation_class) do
-    Class.new(Mutations::BaseMutation) do
-      include Mutations::CanMutateSpammable
-    end
-  end
-
-  let(:request) { double(:request) }
-  let(:query) { double(:query, schema: GitlabSchema) }
-  let(:context) { GraphQL::Query::Context.new(query: query, object: nil, values: { request: request }) }
-
-  subject(:mutation) { mutation_class.new(object: nil, context: context, field: nil) }
-
-  describe '#additional_spam_params' do
-    it 'returns additional spam-related params' do
-      expect(subject.send(:additional_spam_params)).to eq({ api: true, request: request })
-    end
-  end
-
-  describe '#with_spam_action_fields' do
-    let(:spam_log) { double(:spam_log, id: 1) }
-    let(:spammable) { double(:spammable, spam?: true, render_recaptcha?: true, spam_log: spam_log) }
-
-    before do
-      allow(Gitlab::CurrentSettings).to receive(:recaptcha_site_key) { 'abc123' }
-    end
-
-    it 'merges in spam action fields from spammable' do
-      result = subject.send(:with_spam_action_response_fields, spammable) do
-        { other_field: true }
-      end
-      expect(result)
-        .to eq({
-                 spam: true,
-                 needs_captcha_response: true,
-                 spam_log_id: 1,
-                 captcha_site_key: 'abc123',
-                 other_field: true
-               })
-    end
-  end
-end

spec/requests/api/graphql/mutations/snippets/create_spec.rb

@@ -211,5 +211,9 @@ RSpec.describe 'Creating a Snippet' do
         end
       end
     end
+
+    it_behaves_like 'has spam protection' do
+      let(:mutation_class) { ::Mutations::Snippets::Create }
+    end
   end
 end

spec/requests/api/graphql/mutations/snippets/update_spec.rb

@@ -157,6 +157,9 @@ RSpec.describe 'Updating a Snippet' do
     it_behaves_like 'graphql update actions'
     it_behaves_like 'when the snippet is not found'
     it_behaves_like 'snippet edit usage data counters'
+    it_behaves_like 'has spam protection' do
+      let(:mutation_class) { ::Mutations::Snippets::Update }
+    end
   end
 
   describe 'ProjectSnippet' do
@@ -201,6 +204,10 @@ RSpec.describe 'Updating a Snippet' do
       end
 
       it_behaves_like 'snippet edit usage data counters'
+
+      it_behaves_like 'has spam protection' do
+        let(:mutation_class) { ::Mutations::Snippets::Update }
+      end
     end
 
     it_behaves_like 'when the snippet is not found'

spec/services/issues/create_service_spec.rb

@@ -461,7 +461,7 @@ RSpec.describe Issues::CreateService do
     end
 
     context 'checking spam' do
-      let(:request) { double(:request) }
+      let(:request) { double(:request, headers: nil) }
       let(:api) { true }
       let(:captcha_response) { 'abc123' }
       let(:spam_log_id) { 1 }

spec/services/spam/spam_action_service_spec.rb

@@ -5,6 +5,8 @@ require 'spec_helper'
 RSpec.describe Spam::SpamActionService do
   include_context 'includes Spam constants'
 
+  let(:request) { double(:request, env: env, headers: {}) }
+  let(:issue) { create(:issue, project: project, author: user) }
   let(:fake_ip) { '1.2.3.4' }
   let(:fake_user_agent) { 'fake-user-agent' }
   let(:fake_referrer) { 'fake-http-referrer' }
@@ -14,11 +16,8 @@ RSpec.describe Spam::SpamActionService do
       'HTTP_REFERRER' => fake_referrer }
   end
 
-  let(:request) { double(:request, env: env) }
-
   let_it_be(:project) { create(:project, :public) }
   let_it_be(:user) { create(:user) }
-  let(:issue) { create(:issue, project: project, author: user) }
 
   before do
     issue.spam = false
@@ -48,7 +47,7 @@ RSpec.describe Spam::SpamActionService do
 
   shared_examples 'creates a spam log' do
     it do
-      expect { subject }.to change { SpamLog.count }.by(1)
+      expect { subject }.to change(SpamLog, :count).by(1)
 
       new_spam_log = SpamLog.last
       expect(new_spam_log.user_id).to eq(user.id)
@@ -62,7 +61,7 @@ RSpec.describe Spam::SpamActionService do
   end
 
   describe '#execute' do
-    let(:request) { double(:request, env: env) }
+    let(:request) { double(:request, env: env, headers: nil) }
     let(:fake_captcha_verification_service) { double(:captcha_verification_service) }
     let(:fake_verdict_service) { double(:spam_verdict_service) }
     let(:allowlisted) { false }
@@ -70,7 +69,7 @@ RSpec.describe Spam::SpamActionService do
     let(:captcha_response) { 'abc123' }
     let(:spam_log_id) { existing_spam_log.id }
     let(:spam_params) do
-      Spam::SpamActionService.filter_spam_params!(
+      ::Spam::SpamParams.new(
         api: api,
         captcha_response: captcha_response,
         spam_log_id: spam_log_id
@@ -111,10 +110,30 @@ RSpec.describe Spam::SpamActionService do
       allow(Spam::SpamVerdictService).to receive(:new).with(verdict_service_args).and_return(fake_verdict_service)
     end
 
+    context 'when the captcha params are passed in the headers' do
+      let(:request) { double(:request, env: env, headers: headers) }
+      let(:spam_params) { Spam::SpamActionService.filter_spam_params!({ api: api }, request) }
+      let(:headers) do
+        {
+          'X-GitLab-Captcha-Response' => captcha_response,
+          'X-GitLab-Spam-Log-Id' => spam_log_id
+        }
+      end
+
+      it 'extracts the headers correctly' do
+        expect(fake_captcha_verification_service)
+          .to receive(:execute).with(captcha_response: captcha_response, request: request).and_return(true)
+        expect(SpamLog)
+          .to receive(:verify_recaptcha!).with(user_id: user.id, id: spam_log_id)
+
+        subject
+      end
+    end
+
     context 'when captcha response verification returns true' do
       before do
-        expect(fake_captcha_verification_service)
-          .to receive(:execute).with(captcha_response: captcha_response, request: request) { true }
+        allow(fake_captcha_verification_service)
+          .to receive(:execute).with(captcha_response: captcha_response, request: request).and_return(true)
       end
 
       it "doesn't check with the SpamVerdictService" do
@@ -136,8 +155,8 @@ RSpec.describe Spam::SpamActionService do
 
     context 'when captcha response verification returns false' do
       before do
-        expect(fake_captcha_verification_service)
-          .to receive(:execute).with(captcha_response: captcha_response, request: request) { false }
+        allow(fake_captcha_verification_service)
+          .to receive(:execute).with(captcha_response: captcha_response, request: request).and_return(false)
       end
 
       context 'when spammable attributes have not changed' do
@@ -146,21 +165,20 @@ RSpec.describe Spam::SpamActionService do
         end
 
         it 'does not create a spam log' do
-          expect { subject }
-            .not_to change { SpamLog.count }
+          expect { subject }.not_to change(SpamLog, :count)
         end
       end
 
       context 'when spammable attributes have changed' do
         let(:expected_service_check_response_message) do
-          /check Issue spammable model for any errors or captcha requirement/
+          /Check Issue spammable model for any errors or CAPTCHA requirement/
         end
 
         before do
-          issue.description = 'SPAM!'
+          issue.description = 'Lovely Spam! Wonderful Spam!'
         end
 
-        context 'if allowlisted' do
+        context 'when allowlisted' do
           let(:allowlisted) { true }
 
           it 'does not perform spam check' do
@@ -229,7 +247,7 @@ RSpec.describe Spam::SpamActionService do
               response = subject
 
               expect(response.message).to match(expected_service_check_response_message)
-              expect(issue.needs_recaptcha?).to be_truthy
+              expect(issue).to be_needs_recaptcha
             end
           end
 
@@ -253,8 +271,7 @@ RSpec.describe Spam::SpamActionService do
           end
 
           it 'does not create a spam log' do
-            expect { subject }
-              .not_to change { SpamLog.count }
+            expect { subject }.not_to change(SpamLog, :count)
           end
 
           it 'clears spam flags' do
@@ -264,9 +281,9 @@ RSpec.describe Spam::SpamActionService do
           end
         end
 
-        context 'spam verdict service options' do
+        context 'with spam verdict service options' do
           before do
-            allow(fake_verdict_service).to receive(:execute) { ALLOW }
+            allow(fake_verdict_service).to receive(:execute).and_return(ALLOW)
           end
 
           context 'when the request is nil' do

spec/spam/concerns/has_spam_action_response_fields_spec.rb

@@ -19,16 +19,12 @@ RSpec.describe Spam::Concerns::HasSpamActionResponseFields do
     end
 
     it 'merges in spam action fields from spammable' do
-      result = subject.send(:with_spam_action_response_fields, spammable) do
-        { other_field: true }
-      end
-      expect(result)
+      expect(subject.spam_action_response_fields(spammable))
         .to eq({
                  spam: true,
                  needs_captcha_response: true,
                  spam_log_id: 1,
-                 captcha_site_key: recaptcha_site_key,
-                 other_field: true
+                 captcha_site_key: recaptcha_site_key
                })
     end
   end

spec/support/shared_examples/graphql/mutations/can_mutate_spammable_examples.rb

@@ -21,13 +21,13 @@ RSpec.shared_examples 'a mutation which can mutate a spammable' do
     end
   end
 
-  describe "#with_spam_action_response_fields" do
+  describe "#spam_action_response_fields" do
     it 'resolves with spam action fields' do
       subject
 
       # NOTE: We do not need to assert on the specific values of spam action fields here, we only need
-      # to verify that #with_spam_action_response_fields was invoked and that the fields are present in the
-      # response. The specific behavior of #with_spam_action_response_fields is covered in the
+      # to verify that #spam_action_response_fields was invoked and that the fields are present in the
+      # response. The specific behavior of #spam_action_response_fields is covered in the
       # HasSpamActionResponseFields unit tests.
       expect(mutation_response.keys)
         .to include('spam', 'spamLogId', 'needsCaptchaResponse', 'captchaSiteKey')

spec/support/shared_examples/graphql/spam_protection_shared_examples.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.shared_examples 'has spam protection' do
+  include AfterNextHelpers
+
+  describe '#check_spam_action_response!' do
+    let(:variables) { nil }
+    let(:headers) { {} }
+    let(:spam_log_id) { 123 }
+    let(:captcha_site_key) { 'abc123' }
+
+    def send_request
+      post_graphql_mutation(mutation, current_user: current_user)
+    end
+
+    before do
+      allow_next(mutation_class).to receive(:spam_action_response_fields).and_return(
+        spam: spam,
+        needs_captcha_response: render_captcha,
+        spam_log_id: spam_log_id,
+        captcha_site_key: captcha_site_key
+      )
+    end
+
+    context 'when the object is spam (DISALLOW)' do
+      shared_examples 'disallow response' do
+        it 'informs the client that the request was denied as spam' do
+          send_request
+
+          expect(graphql_errors)
+            .to contain_exactly a_hash_including('message' => ::Mutations::SpamProtection::SPAM_DISALLOWED_MESSAGE)
+          expect(graphql_errors)
+            .to contain_exactly a_hash_including('extensions' => { "spam" => true })
+        end
+      end
+
+      let(:spam) { true }
+
+      context 'and no CAPTCHA is available' do
+        let(:render_captcha) { false }
+
+        it_behaves_like 'disallow response'
+      end
+
+      context 'and a CAPTCHA is required' do
+        let(:render_captcha) { true }
+
+        it_behaves_like 'disallow response'
+      end
+    end
+
+    context 'when the object is not spam (CONDITIONAL ALLOW)' do
+      let(:spam) { false }
+
+      context 'and no CAPTCHA is required' do
+        let(:render_captcha) { false }
+
+        it 'does not return a to-level error' do
+          send_request
+
+          expect(graphql_errors).to be_blank
+        end
+      end
+
+      context 'and a CAPTCHA is required' do
+        let(:render_captcha) { true }
+
+        it 'informs the client that the request may be retried after solving the CAPTCHA' do
+          send_request
+
+          expect(graphql_errors)
+            .to contain_exactly a_hash_including('message' => ::Mutations::SpamProtection::NEEDS_CAPTCHA_RESPONSE_MESSAGE)
+          expect(graphql_errors)
+            .to contain_exactly a_hash_including('extensions' => {
+              "captcha_site_key" => captcha_site_key,
+              "needs_captcha_response" => true,
+              "spam_log_id" => spam_log_id
+            })
+        end
+      end
+    end
+  end
+end

spec/support/shared_examples/services/snippets_shared_examples.rb

 # frozen_string_literal: true
 
 RSpec.shared_examples 'checking spam' do
-  let(:request) { double(:request) }
+  let(:request) { double(:request, headers: headers) }
+  let(:headers) { nil }
   let(:api) { true }
   let(:captcha_response) { 'abc123' }
   let(:spam_log_id) { 1 }
@@ -44,6 +45,44 @@ RSpec.shared_examples 'checking spam' do
     subject
   end
 
+  context 'when CAPTCHA arguments are passed in the headers' do
+    let(:headers) do
+      {
+        'X-GitLab-Spam-Log-Id' => spam_log_id,
+        'X-GitLab-Captcha-Response' => captcha_response
+      }
+    end
+
+    let(:extra_opts) do
+      {
+        request: request,
+        api: api,
+        disable_spam_action_service: disable_spam_action_service
+      }
+    end
+
+    it 'executes the SpamActionService correctly' do
+      spam_params = Spam::SpamParams.new(
+        api: api,
+        captcha_response: captcha_response,
+        spam_log_id: spam_log_id
+      )
+      expect_next_instance_of(
+        Spam::SpamActionService,
+        {
+          spammable: kind_of(Snippet),
+          request: request,
+          user: an_instance_of(User),
+          action: action
+        }
+      ) do |instance|
+        expect(instance).to receive(:execute).with(spam_params: spam_params)
+      end
+
+      subject
+    end
+  end
+
   context 'when spam action service is disabled' do
     let(:disable_spam_action_service) { true }