Create new spam protection concern for mutations
What are we doing right now
Create new spam protection concern for mutations
Currently we are raising spam errors as part of the mutation response, so for example:
{
"data": {
"updateSnippet": {
"errors": [
"Your snippet has been recognized as spam. Please, change the content or solve the CAPTCHA to proceed."
],
"snippet": {
"webUrl": "http://gitlab.leipert:3000/-/snippets/23",
"__typename": "Snippet"
},
"needsCaptchaResponse": true,
"captchaSiteKey": "6LeIxAcTAAAAAJcZVRqyHh71UMIEGNQ_MXjiZKhI",
"spamLogId": 67,
"__typename": "UpdateSnippetPayload"
}
}
}
This has a few downsides:
- Adding CAPTCHA fields and arguments to our GraphQL Schema. This means that we need to change mutations to support CAPTCHAs.
- The CAPTCHA fields are also part of a specific path and generally handling those SPAM Errors become harder.
- Sending the CAPTCHA Result means we need to mutate the variables of the query.
sequenceDiagram
participant U as User
participant V as Vue Application
participant A as Apollo
participant G as GitLab API
U->>V: Save snippet
V->>A: Request
A->>G: Request
G-->>A: Response with Error in mutation response
A-->>V: Response with Error in mutation response
V->>U: Please solve CAPTCHA
U->>V: CAPTCHA Solution
V->>A: Request with solved CAPTCHA Data
A->>G: Request with solved CAPTCHA Data
G-->>A: Response with Success
A-->>V: Response with Success
This MR proposes to change the implementation to raise a top-level error instead; references: graphql-ruby, graphql spec).
When sending the request again after solving the CAPTCHA,
we send the CAPTCHA solution via the HTTP header
X-GitLab-Captcha-Response
.
Semantically the most apt comparison might be authorization, so raising a top-level error makes sense. If the backend registers something as SPAM
- We fail un-recoverably (
SpamDisallowedError
), which would be like a403 Forbidden
- We fail with CAPTCHA Data (
NeedsCaptchaResponseError
), which would be like a401 Unauthorized
, requiring authorization
The same response from above might be looking something like this now:
{
"errors": [
{
"message": "Request has been 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,
"__typename": "UpdateSnippetPayload"
}
}
}
Handling errors this way allows us to:
- Parse the errors independently from contents of the query response
- Set the HTTP headers independently from the contents of the query
sequenceDiagram
participant U as User
participant V as Vue Application
participant A as Apollo
participant G as GitLab API
U->>V: Save snippet
V->>A: Request
A->>G: Request
G--xA: Response with top-level Error
A->>U: Please solve Captcha
U->>A: Captcha Solution
A->>G: Request with solved Captcha Data in headers
G-->>A: Response with Success
A-->>V: Response with Success
Screenshots (strongly suggested)
What | Before | After |
---|---|---|
General workflow | snippets-before | snippets-after |
SPAM detected without CAPTCHA configured |
Does this MR meet the acceptance criteria?
Conformity
-
📋 Does this MR need a changelog?-
I have included a changelog entry. -
I have not included a changelog entry because _____.
-
-
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
NOTE: See the CAPTCHA Improvements epic Testing Notes section for details on how to set up your local dev environment to test CAPTCHAs
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
-
Label as security and @ mention @gitlab-com/gl-security/appsec
-
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team
Related
- Related issue: https://gitlab.com/gitlab-org/gitlab/-/issues/320799
- Related epic: https://gitlab.com/groups/gitlab-org/-/epics/5527
- Related epic: https://gitlab.com/groups/gitlab-org/-/epics/5365