From 8b3e6f8b2e73ca45ab7239e6f0294159565fb5ce Mon Sep 17 00:00:00 2001 From: Chad Woolley <cwoolley@gitlab.com> Date: Mon, 21 Jun 2021 18:33:05 -0700 Subject: [PATCH] Migrate Axios CAPTCHA Interceptor to use headers - See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/64463 --- .../captcha_modal_axios_interceptor.js | 56 +++++++++++-------- .../javascripts/issue_show/services/index.js | 2 - .../javascripts/lib/utils/axios_utils.js | 3 + app/controllers/concerns/spammable_actions.rb | 18 +++--- app/controllers/projects/issues_controller.rb | 1 - .../boards/components/boards_selector_spec.js | 7 ++- .../projects/issues_controller_spec.rb | 12 ++-- .../captcha_modal_axios_interceptor_spec.js | 55 +++++++++++------- 8 files changed, 91 insertions(+), 63 deletions(-) diff --git a/app/assets/javascripts/captcha/captcha_modal_axios_interceptor.js b/app/assets/javascripts/captcha/captcha_modal_axios_interceptor.js index c9eac44eb28a2018..fdab188f6be3cef8 100644 --- a/app/assets/javascripts/captcha/captcha_modal_axios_interceptor.js +++ b/app/assets/javascripts/captcha/captcha_modal_axios_interceptor.js @@ -1,4 +1,33 @@ -const supportedMethods = ['patch', 'post', 'put']; +const SUPPORTED_METHODS = ['patch', 'post', 'put']; + +function needsCaptchaResponse(err) { + return ( + SUPPORTED_METHODS.includes(err?.config?.method) && err?.response?.data?.needs_captcha_response + ); +} + +const showCaptchaModalAndResubmit = async (axios, data, errConfig) => { + // NOTE: We asynchronously import and unbox the module. Since this is included globally, we don't + // do a regular import because that would increase the size of the webpack bundle. + const { waitForCaptchaToBeSolved } = await import('~/captcha/wait_for_captcha_to_be_solved'); + + // show the CAPTCHA modal and wait for it to be solved or closed + const captchaResponse = await waitForCaptchaToBeSolved(data.captcha_site_key); + + // resubmit the original request with the captcha_response and spam_log_id in the headers + const originalData = JSON.parse(errConfig.data); + const originalHeaders = errConfig.headers; + return axios({ + method: errConfig.method, + url: errConfig.url, + headers: { + ...originalHeaders, + 'X-GitLab-Captcha-Response': captchaResponse, + 'X-GitLab-Spam-Log-Id': data.spam_log_id, + }, + data: originalData, + }); +}; export function registerCaptchaModalInterceptor(axios) { return axios.interceptors.response.use( @@ -6,29 +35,8 @@ export function registerCaptchaModalInterceptor(axios) { return response; }, (err) => { - if ( - supportedMethods.includes(err?.config?.method) && - err?.response?.data?.needs_captcha_response - ) { - const { data } = err.response; - const captchaSiteKey = data.captcha_site_key; - const spamLogId = data.spam_log_id; - // eslint-disable-next-line promise/no-promise-in-callback - return import('~/captcha/wait_for_captcha_to_be_solved') - .then(({ waitForCaptchaToBeSolved }) => waitForCaptchaToBeSolved(captchaSiteKey)) - .then((captchaResponse) => { - const errConfig = err.config; - const originalData = JSON.parse(errConfig.data); - return axios({ - method: errConfig.method, - url: errConfig.url, - data: { - ...originalData, - captcha_response: captchaResponse, - spam_log_id: spamLogId, - }, - }); - }); + if (needsCaptchaResponse(err)) { + return showCaptchaModalAndResubmit(axios, err.response.data, err.config); } return Promise.reject(err); diff --git a/app/assets/javascripts/issue_show/services/index.js b/app/assets/javascripts/issue_show/services/index.js index 08b04ebfdafc601d..b1deeaae0fcda543 100644 --- a/app/assets/javascripts/issue_show/services/index.js +++ b/app/assets/javascripts/issue_show/services/index.js @@ -1,11 +1,9 @@ -import { registerCaptchaModalInterceptor } from '~/captcha/captcha_modal_axios_interceptor'; import axios from '../../lib/utils/axios_utils'; export default class Service { constructor(endpoint) { this.endpoint = `${endpoint}.json`; this.realtimeEndpoint = `${endpoint}/realtime_changes`; - registerCaptchaModalInterceptor(axios); } getData() { diff --git a/app/assets/javascripts/lib/utils/axios_utils.js b/app/assets/javascripts/lib/utils/axios_utils.js index 204c84b879e0075a..0a26f78e2538adec 100644 --- a/app/assets/javascripts/lib/utils/axios_utils.js +++ b/app/assets/javascripts/lib/utils/axios_utils.js @@ -1,4 +1,5 @@ import axios from 'axios'; +import { registerCaptchaModalInterceptor } from '~/captcha/captcha_modal_axios_interceptor'; import setupAxiosStartupCalls from './axios_startup_calls'; import csrf from './csrf'; import suppressAjaxErrorsDuringNavigation from './suppress_ajax_errors_during_navigation'; @@ -41,6 +42,8 @@ axios.interceptors.response.use( (err) => suppressAjaxErrorsDuringNavigation(err, isUserNavigating), ); +registerCaptchaModalInterceptor(axios); + export default axios; /** diff --git a/app/controllers/concerns/spammable_actions.rb b/app/controllers/concerns/spammable_actions.rb index 5f47d9fadef66391..eb1223f22a940f01 100644 --- a/app/controllers/concerns/spammable_actions.rb +++ b/app/controllers/concerns/spammable_actions.rb @@ -47,17 +47,13 @@ def recaptcha_check_with_fallback(should_redirect = true, &fallback) end end - # TODO: This method is currently only needed for issue create and update. It can be removed when: - # - # 1. Issue create is is converted to a client/JS based approach instead of the legacy HAML - # `_recaptcha_form.html.haml` which is rendered via the `projects/issues/verify` template. - # In this case, which is based on the legacy reCAPTCHA implementation using the HTML/HAML form, - # the 'g-recaptcha-response' field name comes from `Recaptcha::ClientHelper#recaptcha_tags` in the - # recaptcha gem, which is called from the HAML `_recaptcha_form.html.haml` form. - # 2. Issue update is converted to use the headers-based approach, which will require adding - # support to captcha_modal_axios_interceptor.js like we have already added to - # apollo_captcha_link.js. - # In this case, the `captcha_response` field name comes from our captcha_modal_axios_interceptor.js. + # TODO: This method is currently only needed for issue create, to convert spam/CAPTCHA values from + # params, and instead be passed as headers, as the spam services now all expect. It can be removed + # when issue create is is converted to a client/JS based approach instead of the legacy HAML + # `_recaptcha_form.html.haml` which is rendered via the `projects/issues/verify` template. + # In that case, which is based on the legacy reCAPTCHA implementation using the HTML/HAML form, + # the 'g-recaptcha-response' field name comes from `Recaptcha::ClientHelper#recaptcha_tags` in the + # recaptcha gem, which is called from the HAML `_recaptcha_form.html.haml` form. def extract_legacy_spam_params_to_headers request.headers['X-GitLab-Captcha-Response'] = params['g-recaptcha-response'] || params[:captcha_response] request.headers['X-GitLab-Spam-Log-Id'] = params[:spam_log_id] diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 0bb0228cfeac511c..5d38e431c8a266c5 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -336,7 +336,6 @@ def serializer end def update_service - extract_legacy_spam_params_to_headers spam_params = ::Spam::SpamParams.new_from_request(request: request) ::Issues::UpdateService.new(project: project, current_user: current_user, params: issue_params, spam_params: spam_params) end diff --git a/ee/spec/frontend/boards/components/boards_selector_spec.js b/ee/spec/frontend/boards/components/boards_selector_spec.js index 8ac27ab91ebd5e44..f7c84084271b3837 100644 --- a/ee/spec/frontend/boards/components/boards_selector_spec.js +++ b/ee/spec/frontend/boards/components/boards_selector_spec.js @@ -139,8 +139,11 @@ describe('BoardsSelector', () => { await wrapper.setData({ loadingBoards: false, }); - await Promise.all([allBoardsResponse, recentBoardsResponse]); - return nextTick(); + // NOTE: Due to timing issues, this `return` of `Promise.all` is required because + // `app/assets/javascripts/boards/components/boards_selector.vue` does a `$nextTick()` in + // loadRecentBoards. For some unknown reason it doesn't work with `await`, the `return` + // is required. + return Promise.all([allBoardsResponse, recentBoardsResponse]).then(() => nextTick()); }); it('hides loading spinner', async () => { diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 3385505bb6236a9f..922ecb6052aff715 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -1016,10 +1016,13 @@ def go(id:) let(:spammy_title) { 'Whatever' } let!(:spam_logs) { create_list(:spam_log, 2, user: user, title: spammy_title) } + before do + request.headers['X-GitLab-Captcha-Response'] = 'a-valid-captcha-response' + request.headers['X-GitLab-Spam-Log-Id'] = spam_logs.last.id + end + def update_verified_issue - update_issue( - issue_params: { title: spammy_title }, - additional_params: { spam_log_id: spam_logs.last.id, 'g-recaptcha-response': true }) + update_issue(issue_params: { title: spammy_title }) end it 'returns 200 status' do @@ -1036,8 +1039,9 @@ def update_verified_issue it 'does not mark spam log as recaptcha_verified when it does not belong to current_user' do spam_log = create(:spam_log) + request.headers['X-GitLab-Spam-Log-Id'] = spam_log.id - expect { update_issue(issue_params: { spam_log_id: spam_log.id, 'g-recaptcha-response': true }) } + expect { update_issue } .not_to change { SpamLog.last.recaptcha_verified } end end diff --git a/spec/frontend/captcha/captcha_modal_axios_interceptor_spec.js b/spec/frontend/captcha/captcha_modal_axios_interceptor_spec.js index df81b78d010e5fb8..553ca52f9ceafd71 100644 --- a/spec/frontend/captcha/captcha_modal_axios_interceptor_spec.js +++ b/spec/frontend/captcha/captcha_modal_axios_interceptor_spec.js @@ -1,6 +1,7 @@ import MockAdapter from 'axios-mock-adapter'; import { registerCaptchaModalInterceptor } from '~/captcha/captcha_modal_axios_interceptor'; +import UnsolvedCaptchaError from '~/captcha/unsolved_captcha_error'; import { waitForCaptchaToBeSolved } from '~/captcha/wait_for_captcha_to_be_solved'; import axios from '~/lib/utils/axios_utils'; import httpStatusCodes from '~/lib/utils/http_status'; @@ -25,22 +26,24 @@ describe('registerCaptchaModalInterceptor', () => { let mock; beforeEach(() => { + waitForCaptchaToBeSolved.mockRejectedValue(new UnsolvedCaptchaError()); + mock = new MockAdapter(axios); - mock.onAny('/no-captcha').reply(200, AXIOS_RESPONSE); - mock.onAny('/error').reply(404, AXIOS_RESPONSE); - mock.onAny('/captcha').reply((config) => { + mock.onAny('/endpoint-without-captcha').reply(200, AXIOS_RESPONSE); + mock.onAny('/endpoint-with-unrelated-error').reply(404, AXIOS_RESPONSE); + mock.onAny('/endpoint-with-captcha').reply((config) => { if (!supportedMethods.includes(config.method)) { return [httpStatusCodes.METHOD_NOT_ALLOWED, { method: config.method }]; } - try { - const { captcha_response, spam_log_id, ...rest } = JSON.parse(config.data); - // eslint-disable-next-line babel/camelcase - if (captcha_response === CAPTCHA_RESPONSE && spam_log_id === SPAM_LOG_ID) { - return [httpStatusCodes.OK, { ...rest, method: config.method, CAPTCHA_SUCCESS }]; - } - } catch (e) { - return [httpStatusCodes.BAD_REQUEST, { method: config.method }]; + const data = JSON.parse(config.data); + const { + 'X-GitLab-Captcha-Response': captchaResponse, + 'X-GitLab-Spam-Log-Id': spamLogId, + } = config.headers; + + if (captchaResponse === CAPTCHA_RESPONSE && spamLogId === SPAM_LOG_ID) { + return [httpStatusCodes.OK, { ...data, method: config.method, CAPTCHA_SUCCESS }]; } return [httpStatusCodes.CONFLICT, NEEDS_CAPTCHA_RESPONSE]; @@ -56,7 +59,7 @@ describe('registerCaptchaModalInterceptor', () => { describe.each([...supportedMethods, ...unsupportedMethods])('For HTTP method %s', (method) => { it('successful requests are passed through', async () => { - const { data, status } = await axios[method]('/no-captcha'); + const { data, status } = await axios[method]('/endpoint-without-captcha'); expect(status).toEqual(httpStatusCodes.OK); expect(data).toEqual(AXIOS_RESPONSE); @@ -64,7 +67,7 @@ describe('registerCaptchaModalInterceptor', () => { }); it('error requests without needs_captcha_response_errors are passed through', async () => { - await expect(() => axios[method]('/error')).rejects.toThrow( + await expect(() => axios[method]('/endpoint-with-unrelated-error')).rejects.toThrow( expect.objectContaining({ response: expect.objectContaining({ status: httpStatusCodes.NOT_FOUND, @@ -79,21 +82,35 @@ describe('registerCaptchaModalInterceptor', () => { describe.each(supportedMethods)('For HTTP method %s', (method) => { describe('error requests with needs_captcha_response_errors', () => { const submittedData = { ID: 12345 }; + const submittedHeaders = { 'Submitted-Header': 67890 }; it('re-submits request if captcha was solved correctly', async () => { - waitForCaptchaToBeSolved.mockResolvedValue(CAPTCHA_RESPONSE); - const { data: returnedData } = await axios[method]('/captcha', submittedData); + waitForCaptchaToBeSolved.mockResolvedValueOnce(CAPTCHA_RESPONSE); + const axiosResponse = await axios[method]('/endpoint-with-captcha', submittedData, { + headers: submittedHeaders, + }); + const { + data: returnedData, + config: { headers: returnedHeaders }, + } = axiosResponse; expect(waitForCaptchaToBeSolved).toHaveBeenCalledWith(CAPTCHA_SITE_KEY); expect(returnedData).toEqual({ ...submittedData, CAPTCHA_SUCCESS, method }); + expect(returnedHeaders).toEqual( + expect.objectContaining({ + ...submittedHeaders, + 'X-GitLab-Captcha-Response': CAPTCHA_RESPONSE, + 'X-GitLab-Spam-Log-Id': SPAM_LOG_ID, + }), + ); expect(mock.history[method]).toHaveLength(2); }); it('does not re-submit request if captcha was not solved', async () => { - const error = new Error('Captcha not solved'); - waitForCaptchaToBeSolved.mockRejectedValue(error); - await expect(() => axios[method]('/captcha', submittedData)).rejects.toThrow(error); + await expect(() => axios[method]('/endpoint-with-captcha', submittedData)).rejects.toThrow( + new UnsolvedCaptchaError(), + ); expect(waitForCaptchaToBeSolved).toHaveBeenCalledWith(CAPTCHA_SITE_KEY); expect(mock.history[method]).toHaveLength(1); @@ -103,7 +120,7 @@ describe('registerCaptchaModalInterceptor', () => { describe.each(unsupportedMethods)('For HTTP method %s', (method) => { it('ignores captcha response', async () => { - await expect(() => axios[method]('/captcha')).rejects.toThrow( + await expect(() => axios[method]('/endpoint-with-captcha')).rejects.toThrow( expect.objectContaining({ response: expect.objectContaining({ status: httpStatusCodes.METHOD_NOT_ALLOWED, -- GitLab