From 441183cc607af0366954a553a483fd03bb051a17 Mon Sep 17 00:00:00 2001 From: Niklas Date: Wed, 24 Jan 2024 19:35:31 +0000 Subject: [PATCH 01/10] Add custom payload template to webhooks Changelog: added --- Gemfile | 2 + Gemfile.checksum | 1 + Gemfile.lock | 2 + .../concerns/web_hooks/hook_actions.rb | 3 +- app/services/web_hook_service.rb | 26 ++++++-- app/views/shared/web_hooks/_form.html.haml | 5 ++ .../beta/custom_webhook_payload.yml | 9 +++ ...add_custom_payload_template_to_web_hook.rb | 11 ++++ db/schema_migrations/20240124181406 | 1 + db/structure.sql | 4 +- .../project/integrations/webhook_events.md | 24 +++++++ ee/lib/api/group_hooks.rb | 1 + lib/api/entities/hook.rb | 2 + lib/api/project_hooks.rb | 1 + locale/gitlab.pot | 3 + .../projects/hooks_controller_spec.rb | 2 + spec/services/web_hook_service_spec.rb | 65 +++++++++++++++++++ 17 files changed, 155 insertions(+), 7 deletions(-) create mode 100644 config/feature_flags/beta/custom_webhook_payload.yml create mode 100644 db/migrate/20240124181406_add_custom_payload_template_to_web_hook.rb create mode 100644 db/schema_migrations/20240124181406 diff --git a/Gemfile b/Gemfile index 865897f5eaa79f..3dbcea0da62cf4 100644 --- a/Gemfile +++ b/Gemfile @@ -658,3 +658,5 @@ gem 'net-http', '= 0.1.1' # rubocop:todo Gemfile/MissingFeatureCategory gem 'duo_api', '~> 1.3' # rubocop:todo Gemfile/MissingFeatureCategory gem 'gitlab-sdk', '~> 0.3.0', feature_category: :application_instrumentation + +gem "liquid", "~> 5.4" diff --git a/Gemfile.checksum b/Gemfile.checksum index 64878c18f51321..64fc36ff5aae77 100644 --- a/Gemfile.checksum +++ b/Gemfile.checksum @@ -354,6 +354,7 @@ {"name":"libyajl2","version":"2.1.0","platform":"ruby","checksum":"aa5df6c725776fc050c8418450de0f7c129cb7200b811907c4c0b3b5c0aea0ef"}, {"name":"license_finder","version":"7.0.1","platform":"ruby","checksum":"0b22c9567e2a8b102c7245da49ebeddaec60f66d237d2bb91b9feddf5d242f6a"}, {"name":"licensee","version":"9.16.1","platform":"ruby","checksum":"04c0b57d20b91fa82ac9dcb3637da9cdc1c3823e217c0781e5d0514958e2e515"}, +{"name":"liquid","version":"5.4.0","platform":"ruby","checksum":"e362448c71cb88683d9c2df2819d7e47da3cdb37f757d0c99e3400d7e0030d40"}, {"name":"listen","version":"3.7.1","platform":"ruby","checksum":"3b80caa7aa77fae836916c2f9e3fbcafbd15f5d695dd487c1f5b5e7e465efe29"}, {"name":"llhttp-ffi","version":"0.4.0","platform":"ruby","checksum":"e5f7327db3cf8007e648342ef76347d6e0ae545a8402e519cca9c886eb37b001"}, {"name":"locale","version":"2.1.3","platform":"ruby","checksum":"b6ddee011e157817cb98e521b3ce7cb626424d5882f1e844aafdee3e8b212725"}, diff --git a/Gemfile.lock b/Gemfile.lock index 0775a3d8c70980..ff613be747ca81 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1047,6 +1047,7 @@ GEM reverse_markdown (>= 1, < 3) rugged (>= 0.24, < 2.0) thor (>= 0.19, < 2.0) + liquid (5.4.0) listen (3.7.1) rb-fsevent (~> 0.10, >= 0.10.3) rb-inotify (~> 0.9, >= 0.9.10) @@ -1995,6 +1996,7 @@ DEPENDENCIES letter_opener_web (~> 2.0.0) license_finder (~> 7.0) licensee (~> 9.16) + liquid (~> 5.4) listen (~> 3.7) lockbox (~> 1.3.0) lograge (~> 0.5) diff --git a/app/controllers/concerns/web_hooks/hook_actions.rb b/app/controllers/concerns/web_hooks/hook_actions.rb index 5aeb10dfb878be..a42fd590e39e5a 100644 --- a/app/controllers/concerns/web_hooks/hook_actions.rb +++ b/app/controllers/concerns/web_hooks/hook_actions.rb @@ -71,7 +71,8 @@ def hook_params end def hook_param_names - %i[enable_ssl_verification name description token url push_events_branch_filter branch_filter_strategy] + %i[enable_ssl_verification name description token url push_events_branch_filter branch_filter_strategy + custom_payload_template] end def destroy_hook(hook) diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index 035f1754cbbf9a..e20e2fab4af13d 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class WebHookService + include Gitlab::Utils::StrongMemoize + class InternalErrorResponse ERROR_MESSAGE = 'internal error' @@ -87,15 +89,18 @@ def execute ) ServiceResponse.success(message: response.body, payload: { http_status: response.code }) - rescue *Gitlab::HTTP::HTTP_ERRORS, + rescue *Gitlab::HTTP::HTTP_ERRORS, Liquid::SyntaxError, JSON::ParserError, Gitlab::Json::LimitedEncoder::LimitExceeded, URI::InvalidURIError => e execution_duration = ::Gitlab::Metrics::System.monotonic_time - start_time error_message = e.to_s + request_data = [Liquid::SyntaxError, JSON::ParserError].include?(e.class) ? {} : request_payload + log_execution( response: InternalErrorResponse.new, execution_duration: execution_duration, - error_message: error_message + error_message: error_message, + request_data: request_data ) Gitlab::AppLogger.error("WebHook Error after #{execution_duration.to_i.seconds}s => #{e}") @@ -129,7 +134,7 @@ def parsed_url def make_request(url, basic_auth = false) Gitlab::HTTP.post(url, - body: Gitlab::Json::LimitedEncoder.encode(data, limit: REQUEST_BODY_SIZE_LIMIT), + body: Gitlab::Json::LimitedEncoder.encode(request_payload, limit: REQUEST_BODY_SIZE_LIMIT), headers: build_headers, verify: hook.enable_ssl_verification, basic_auth: basic_auth, @@ -145,7 +150,7 @@ def make_request_with_auth make_request(post_url, basic_auth) end - def log_execution(response:, execution_duration:, error_message: nil) + def log_execution(response:, execution_duration:, error_message: nil, request_data: request_payload) category = response_category(response) log_data = { trigger: hook_name, @@ -153,7 +158,7 @@ def log_execution(response:, execution_duration:, error_message: nil) interpolated_url: hook.interpolated_url, execution_duration: execution_duration, request_headers: build_headers, - request_data: data, + request_data: request_data, response_headers: safe_response_headers(response), response_body: safe_response_body(response), response_status: response.code, @@ -267,4 +272,15 @@ def string_size_limit(str, limit) def enforce_utf8(str) Gitlab::EncodingHelper.encode_utf8(str) end + + def request_payload + return data unless Feature.enabled?(:custom_webhook_payload, hook.parent, type: :beta) + return data unless hook.custom_payload_template.present? + + rendered_template = Liquid::Template.parse(hook.custom_payload_template).render!( + { 'event' => data.deep_stringify_keys } + ) + Gitlab::Json.parse(rendered_template) + end + strong_memoize_attr :request_payload end diff --git a/app/views/shared/web_hooks/_form.html.haml b/app/views/shared/web_hooks/_form.html.haml index cdef45a041531f..750dae73f56f9c 100644 --- a/app/views/shared/web_hooks/_form.html.haml +++ b/app/views/shared/web_hooks/_form.html.haml @@ -77,6 +77,11 @@ integration_webhook_event_human_name(:emoji_events), help_text: s_('Webhooks|An emoji is awarded or revoked. %{help_link}?').html_safe % { help_link: emoji_help_link } +- if Feature.enabled?(:custom_webhook_payload, hook.parent, type: :beta) + .form-group + = form.label :custom_payload_template, s_('Webhooks|Custom Payload Template (optional)'), class: 'label-bold' + = form.text_area :custom_payload_template, value: hook.custom_payload_template, class: 'form-control gl-form-input gl-form-input-xl', rows: 8, maxlength: 4096 + .form-group = form.label :enable_ssl_verification, s_('Webhooks|SSL verification'), class: 'label-bold checkbox' %ul.list-unstyled diff --git a/config/feature_flags/beta/custom_webhook_payload.yml b/config/feature_flags/beta/custom_webhook_payload.yml new file mode 100644 index 00000000000000..1937cfb24e7734 --- /dev/null +++ b/config/feature_flags/beta/custom_webhook_payload.yml @@ -0,0 +1,9 @@ +--- +name: custom_webhook_payload +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/362504 +introduced_by_url: +rollout_issue_url: +milestone: '16.9' +group: group::import and integrate +type: beta +default_enabled: false diff --git a/db/migrate/20240124181406_add_custom_payload_template_to_web_hook.rb b/db/migrate/20240124181406_add_custom_payload_template_to_web_hook.rb new file mode 100644 index 00000000000000..737bf647f8fbe0 --- /dev/null +++ b/db/migrate/20240124181406_add_custom_payload_template_to_web_hook.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class AddCustomPayloadTemplateToWebHook < Gitlab::Database::Migration[2.2] + disable_ddl_transaction! + milestone '16.9' + + def change + add_column :web_hooks, :custom_payload_template, :text, null: true + add_text_limit :web_hooks, :custom_payload_template, 4096 + end +end diff --git a/db/schema_migrations/20240124181406 b/db/schema_migrations/20240124181406 new file mode 100644 index 00000000000000..8cfd02da327533 --- /dev/null +++ b/db/schema_migrations/20240124181406 @@ -0,0 +1 @@ +9b85ae64f559377cf86fa1d971f7d2d19984115904699f51ab0e9645387b445d \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index f46c5208840a65..7f46b38e53f1bf 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -25896,8 +25896,10 @@ CREATE TABLE web_hooks ( emoji_events boolean DEFAULT false NOT NULL, name text, description text, + custom_payload_template text, CONSTRAINT check_1e4d5cbdc5 CHECK ((char_length(name) <= 255)), - CONSTRAINT check_23a96ad211 CHECK ((char_length(description) <= 2048)) + CONSTRAINT check_23a96ad211 CHECK ((char_length(description) <= 2048)), + CONSTRAINT check_78df43030f CHECK ((char_length(custom_payload_template) <= 4096)) ); CREATE SEQUENCE web_hooks_id_seq diff --git a/doc/user/project/integrations/webhook_events.md b/doc/user/project/integrations/webhook_events.md index 5253fd9d64b944..3112bed396435a 100644 --- a/doc/user/project/integrations/webhook_events.md +++ b/doc/user/project/integrations/webhook_events.md @@ -2047,3 +2047,27 @@ Payload example: } } ``` + +## Custom Payload Template + +> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/142738) in GitLab 16.9 [with a flag](../../../administration/feature_flags.md) named `custom_webhook_payload`. Disabled by default. + +FLAG: +On self-managed GitLab, by default this feature is not available. To make it available, an administrator can +[enable the feature flag](../../../administration/feature_flags.md) named `custom_webhook_payload`. +On GitLab.com, this feature is not available. + +You can set a custom payload template in the webhook configuration. The request body is rendered from the template +with the data for the current event. The data available to the template can be seen by the payload examples on this page. + +The template uses [liquid markup](https://shopify.github.io/liquid/basics/introduction/). It must render to valid JSON. + +Example: + +Custom Payload Template: `{"event": "{{event.object_kind}}"}` + +Request Payload: + +```json +{"event": "push"} +``` diff --git a/ee/lib/api/group_hooks.rb b/ee/lib/api/group_hooks.rb index b3b19c3c8fac91..ebffc64d124f9a 100644 --- a/ee/lib/api/group_hooks.rb +++ b/ee/lib/api/group_hooks.rb @@ -36,6 +36,7 @@ def hook_scope optional :emoji_events, type: Boolean, desc: "Trigger hook on emoji events" optional :enable_ssl_verification, type: Boolean, desc: "Do SSL verification when triggering the hook" optional :token, type: String, desc: "Secret token to validate received payloads; this will not be returned in the response" + optional :custom_payload_template, type: String, desc: "Custom template for the request payload" use :url_variables end end diff --git a/lib/api/entities/hook.rb b/lib/api/entities/hook.rb index d92331f7dea4f4..68561c8975f125 100644 --- a/lib/api/entities/hook.rb +++ b/lib/api/entities/hook.rb @@ -18,6 +18,8 @@ class Hook < Grape::Entity if: ->(_, options) { options[:with_url_variables] != false }, documentation: { type: 'Hash', example: { "token" => "secr3t" }, is_array: true } + expose :custom_payload_template, documentation: { type: 'string', example: '{"event":"{{event.object_kind}}"}' } + def url_variables object.url_variables.keys.map { { key: _1 } } end diff --git a/lib/api/project_hooks.rb b/lib/api/project_hooks.rb index 011d5e69f006a9..8e4c4da7bfdfce 100644 --- a/lib/api/project_hooks.rb +++ b/lib/api/project_hooks.rb @@ -35,6 +35,7 @@ def hook_scope optional :enable_ssl_verification, type: Boolean, desc: "Do SSL verification when triggering the hook" optional :token, type: String, desc: "Secret token to validate received payloads; this will not be returned in the response" optional :push_events_branch_filter, type: String, desc: "Trigger hook on specified branch only" + optional :custom_payload_template, type: String, desc: "Custom template for the request payload" use :url_variables end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index f05ee97e45d985..0d41107663be17 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -55405,6 +55405,9 @@ msgstr "" msgid "Webhooks|Confidential issues events" msgstr "" +msgid "Webhooks|Custom Payload Template (optional)" +msgstr "" + msgid "Webhooks|Delete webhook" msgstr "" diff --git a/spec/controllers/projects/hooks_controller_spec.rb b/spec/controllers/projects/hooks_controller_spec.rb index 8ba2e2a55fadc5..0a210f9eca2d19 100644 --- a/spec/controllers/projects/hooks_controller_spec.rb +++ b/spec/controllers/projects/hooks_controller_spec.rb @@ -138,6 +138,8 @@ def it_renders_correctly wiki_page_events: true, deployment_events: true, + custom_payload_template: '{"test":"test"}', + url_variables: [{ key: 'token', value: 'some secret value' }] } diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index c33273348f6122..ea06b9737ef370 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -369,6 +369,71 @@ end end + context 'when custom_payload_template is set' do + context 'when template is valid' do + before do + project_hook.custom_payload_template = '{"before":"{{event.before}}"}' + end + + it 'renders custom_payload_template for body' do + stub_full_request(project_hook.url, method: :post) + + service_instance.execute + + expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)) + .with(headers: headers, body: '{"before":"oldrev"}') + .once + end + + context 'when feature flag is disabled' do + before do + stub_feature_flags(custom_webhook_payload: false) + stub_full_request(project_hook.url, method: :post) + end + + it 'does not render custom template', :aggregate_failures do + allow(Liquid::Template).to receive(:new) + + service_instance.execute + + expect(Liquid::Template).not_to have_received(:new) + expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)) + .with(headers: headers, body: '{"before":"oldrev","after":"newrev","ref":"ref"}') + .once + end + end + end + + context 'when template is invalid' do + before do + project_hook.custom_payload_template = '{"test":"{{event}"}' + end + + it 'handles the error', :aggregate_failures do + expect(service_instance.execute).to have_attributes( + status: :error, + message: 'Liquid syntax error: Variable \'{{event}\' was not properly terminated with regexp: /\\}\\}/' + ) + expect { service_instance.execute }.not_to raise_error + end + end + + context 'when template renders invalid json' do + before do + project_hook.custom_payload_template = '{"test":"{{event}}}' + end + + it 'handles the error', :aggregate_failures do + expect(service_instance.execute).to have_attributes( + status: :error, + message: 'unexpected character (after test) at line 1, column 12 [parse.c:804] in ' \ + '\'{"test":"{"before"=>"oldrev", "after"=>"newrev", "ref"=>"ref"}}' + ) + expect { service_instance.execute }.not_to raise_error + end + end + end + it 'handles 200 status code' do stub_full_request(project_hook.url, method: :post).to_return(status: 200, body: 'Success') -- GitLab From 92894c8159d17ba467a6524d7ea42b2893b2ad00 Mon Sep 17 00:00:00 2001 From: Niklas Date: Fri, 26 Jan 2024 19:53:59 +0000 Subject: [PATCH 02/10] Fix migration and broken specs --- Gemfile | 5 +- .../beta/custom_webhook_payload.yml | 2 +- ...add_custom_payload_template_to_web_hook.rb | 6 +- .../api/schemas/public_api/v4/group_hook.json | 9 ++- .../schemas/public_api/v4/project_hook.json | 9 ++- .../schemas/public_api/v4/system_hook.json | 65 +++++++++++++++---- spec/lib/api/entities/hook_spec.rb | 3 +- 7 files changed, 79 insertions(+), 20 deletions(-) diff --git a/Gemfile b/Gemfile index 3dbcea0da62cf4..093b1ba9d8c3e9 100644 --- a/Gemfile +++ b/Gemfile @@ -311,6 +311,9 @@ gem 'ruby-fogbugz', '~> 0.3.0', feature_category: :importers # Kubernetes integration gem 'kubeclient', '~> 4.11.0' # rubocop:todo Gemfile/MissingFeatureCategory +# Custom webhook payload template rendering +gem "liquid", "~> 5.4", feature_category: :integrations + # AI gem 'ruby-openai', '~> 3.7' # rubocop:todo Gemfile/MissingFeatureCategory gem 'circuitbox', '2.0.0' # rubocop:todo Gemfile/MissingFeatureCategory @@ -658,5 +661,3 @@ gem 'net-http', '= 0.1.1' # rubocop:todo Gemfile/MissingFeatureCategory gem 'duo_api', '~> 1.3' # rubocop:todo Gemfile/MissingFeatureCategory gem 'gitlab-sdk', '~> 0.3.0', feature_category: :application_instrumentation - -gem "liquid", "~> 5.4" diff --git a/config/feature_flags/beta/custom_webhook_payload.yml b/config/feature_flags/beta/custom_webhook_payload.yml index 1937cfb24e7734..0b86fb5fe34f29 100644 --- a/config/feature_flags/beta/custom_webhook_payload.yml +++ b/config/feature_flags/beta/custom_webhook_payload.yml @@ -1,7 +1,7 @@ --- name: custom_webhook_payload feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/362504 -introduced_by_url: +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/142738 rollout_issue_url: milestone: '16.9' group: group::import and integrate diff --git a/db/migrate/20240124181406_add_custom_payload_template_to_web_hook.rb b/db/migrate/20240124181406_add_custom_payload_template_to_web_hook.rb index 737bf647f8fbe0..bfa6c231955350 100644 --- a/db/migrate/20240124181406_add_custom_payload_template_to_web_hook.rb +++ b/db/migrate/20240124181406_add_custom_payload_template_to_web_hook.rb @@ -4,8 +4,12 @@ class AddCustomPayloadTemplateToWebHook < Gitlab::Database::Migration[2.2] disable_ddl_transaction! milestone '16.9' - def change + def up add_column :web_hooks, :custom_payload_template, :text, null: true add_text_limit :web_hooks, :custom_payload_template, 4096 end + + def down + remove_column :web_hooks, :custom_payload_template + end end diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/group_hook.json b/ee/spec/fixtures/api/schemas/public_api/v4/group_hook.json index 66cb247d98aa64..699f9e59afc176 100644 --- a/ee/spec/fixtures/api/schemas/public_api/v4/group_hook.json +++ b/ee/spec/fixtures/api/schemas/public_api/v4/group_hook.json @@ -24,7 +24,8 @@ "emoji_events", "alert_status", "disabled_until", - "url_variables" + "url_variables", + "custom_payload_template" ], "properties": { "id": { @@ -128,6 +129,12 @@ } } } + }, + "custom_payload_template": { + "type": [ + "string", + "null" + ] } }, "additionalProperties": false diff --git a/spec/fixtures/api/schemas/public_api/v4/project_hook.json b/spec/fixtures/api/schemas/public_api/v4/project_hook.json index c42a4cad71255b..1eaf5f3482cdf4 100644 --- a/spec/fixtures/api/schemas/public_api/v4/project_hook.json +++ b/spec/fixtures/api/schemas/public_api/v4/project_hook.json @@ -22,7 +22,8 @@ "releases_events", "alert_status", "disabled_until", - "emoji_events" + "emoji_events", + "custom_payload_template" ], "optional": [ "url_variables" @@ -126,6 +127,12 @@ } } } + }, + "custom_payload_template": { + "type": [ + "string", + "null" + ] } }, "additionalProperties": false diff --git a/spec/fixtures/api/schemas/public_api/v4/system_hook.json b/spec/fixtures/api/schemas/public_api/v4/system_hook.json index b6f56b948a0ad9..a4d0ae1d0bfdfe 100644 --- a/spec/fixtures/api/schemas/public_api/v4/system_hook.json +++ b/spec/fixtures/api/schemas/public_api/v4/system_hook.json @@ -11,29 +11,68 @@ "enable_ssl_verification", "alert_status", "disabled_until", - "url_variables" + "url_variables", + "custom_payload_template" ], "properties": { - "id": { "type": "integer" }, - "url": { "type": "string" }, - "created_at": { "type": "string" }, - "push_events": { "type": "boolean" }, - "tag_push_events": { "type": "boolean" }, - "merge_requests_events": { "type": "boolean" }, - "repository_update_events": { "type": "boolean" }, - "enable_ssl_verification": { "type": "boolean" }, - "alert_status": { "type": "string", "enum": ["executable", "disabled", "temporarily_disabled"] }, - "disabled_until": { "type": ["string", "null"] }, + "id": { + "type": "integer" + }, + "url": { + "type": "string" + }, + "created_at": { + "type": "string" + }, + "push_events": { + "type": "boolean" + }, + "tag_push_events": { + "type": "boolean" + }, + "merge_requests_events": { + "type": "boolean" + }, + "repository_update_events": { + "type": "boolean" + }, + "enable_ssl_verification": { + "type": "boolean" + }, + "alert_status": { + "type": "string", + "enum": [ + "executable", + "disabled", + "temporarily_disabled" + ] + }, + "disabled_until": { + "type": [ + "string", + "null" + ] + }, "url_variables": { "type": "array", "items": { "type": "object", "additionalProperties": false, - "required": ["key"], + "required": [ + "key" + ], "properties": { - "key": { "type": "string" } + "key": { + "type": "string" + } } } + }, + "custom_payload_template": { + "type": [ + "string", + "null" + ] } }, "additionalProperties": false diff --git a/spec/lib/api/entities/hook_spec.rb b/spec/lib/api/entities/hook_spec.rb index 45648d6fb645e1..68b18ba46c1632 100644 --- a/spec/lib/api/entities/hook_spec.rb +++ b/spec/lib/api/entities/hook_spec.rb @@ -11,7 +11,8 @@ it 'exposes correct attributes' do expect(json.keys).to contain_exactly(:alert_status, :created_at, :disabled_until, :enable_ssl_verification, :id, - :merge_requests_events, :push_events, :repository_update_events, :tag_push_events, :url, :url_variables + :merge_requests_events, :push_events, :repository_update_events, :tag_push_events, :url, :url_variables, + :custom_payload_template ) end -- GitLab From 62d38015a56237b6c48ec097ef807de64275f75a Mon Sep 17 00:00:00 2001 From: Niklas Date: Mon, 29 Jan 2024 17:30:58 +0000 Subject: [PATCH 03/10] Apply review feedback --- .../beta/custom_webhook_payload.yml | 2 +- ...add_custom_payload_template_to_web_hook.rb | 11 +++----- ...it_to_web_hooks_custom_payload_template.rb | 14 +++++++++++ db/schema_migrations/20240124181407 | 1 + .../project/integrations/webhook_events.md | 25 ++++++++++++------- 5 files changed, 36 insertions(+), 17 deletions(-) create mode 100644 db/migrate/20240124181407_add_text_limit_to_web_hooks_custom_payload_template.rb create mode 100644 db/schema_migrations/20240124181407 diff --git a/config/feature_flags/beta/custom_webhook_payload.yml b/config/feature_flags/beta/custom_webhook_payload.yml index 0b86fb5fe34f29..2cde183d8359e5 100644 --- a/config/feature_flags/beta/custom_webhook_payload.yml +++ b/config/feature_flags/beta/custom_webhook_payload.yml @@ -2,7 +2,7 @@ name: custom_webhook_payload feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/362504 introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/142738 -rollout_issue_url: +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/439610 milestone: '16.9' group: group::import and integrate type: beta diff --git a/db/migrate/20240124181406_add_custom_payload_template_to_web_hook.rb b/db/migrate/20240124181406_add_custom_payload_template_to_web_hook.rb index bfa6c231955350..9afdce7876405b 100644 --- a/db/migrate/20240124181406_add_custom_payload_template_to_web_hook.rb +++ b/db/migrate/20240124181406_add_custom_payload_template_to_web_hook.rb @@ -1,15 +1,12 @@ # frozen_string_literal: true class AddCustomPayloadTemplateToWebHook < Gitlab::Database::Migration[2.2] - disable_ddl_transaction! + enable_lock_retries! milestone '16.9' - def up + def change + # rubocop:disable Migration/AddLimitToTextColumns -- limit is added in 20240124181407 add_column :web_hooks, :custom_payload_template, :text, null: true - add_text_limit :web_hooks, :custom_payload_template, 4096 - end - - def down - remove_column :web_hooks, :custom_payload_template + # rubocop:enable Migration/AddLimitToTextColumns end end diff --git a/db/migrate/20240124181407_add_text_limit_to_web_hooks_custom_payload_template.rb b/db/migrate/20240124181407_add_text_limit_to_web_hooks_custom_payload_template.rb new file mode 100644 index 00000000000000..3b33b66225c38a --- /dev/null +++ b/db/migrate/20240124181407_add_text_limit_to_web_hooks_custom_payload_template.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class AddTextLimitToWebHooksCustomPayloadTemplate < Gitlab::Database::Migration[2.2] + disable_ddl_transaction! + milestone '16.9' + + def up + add_text_limit :web_hooks, :custom_payload_template, 4096 + end + + def down + remove_text_limit :web_hooks, :custom_payload_template + end +end diff --git a/db/schema_migrations/20240124181407 b/db/schema_migrations/20240124181407 new file mode 100644 index 00000000000000..58ce4b2e22ee42 --- /dev/null +++ b/db/schema_migrations/20240124181407 @@ -0,0 +1 @@ +7e1f9fe893986d49e201a6adb82a4c74e11f36ded1db2eb1210b0eadcf16bd7a \ No newline at end of file diff --git a/doc/user/project/integrations/webhook_events.md b/doc/user/project/integrations/webhook_events.md index 3112bed396435a..1004d1af7c0446 100644 --- a/doc/user/project/integrations/webhook_events.md +++ b/doc/user/project/integrations/webhook_events.md @@ -2048,7 +2048,7 @@ Payload example: } ``` -## Custom Payload Template +## Custom payload template > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/142738) in GitLab 16.9 [with a flag](../../../administration/feature_flags.md) named `custom_webhook_payload`. Disabled by default. @@ -2058,16 +2058,23 @@ On self-managed GitLab, by default this feature is not available. To make it ava On GitLab.com, this feature is not available. You can set a custom payload template in the webhook configuration. The request body is rendered from the template -with the data for the current event. The data available to the template can be seen by the payload examples on this page. +with the data for the current event. The template uses [liquid markup](https://shopify.github.io/liquid/basics/introduction/) +and must render as valid JSON. -The template uses [liquid markup](https://shopify.github.io/liquid/basics/introduction/). It must render to valid JSON. +For example: -Example: +- Custom payload template: -Custom Payload Template: `{"event": "{{event.object_kind}}"}` + ```json + { + "event": "{{event.object_kind}}" + } + ``` -Request Payload: +- Request payload that combines the template with a `push` event: -```json -{"event": "push"} -``` + ```json + { + "event": "push" + } + ``` -- GitLab From 4d48984441be6251da311dfc398d05d8e27c21fb Mon Sep 17 00:00:00 2001 From: Niklas Date: Thu, 1 Feb 2024 20:05:55 +0000 Subject: [PATCH 04/10] More review feedback --- app/models/hooks/web_hook.rb | 7 +++++ app/services/web_hook_service.rb | 22 ++++++++++++-- .../project/integrations/webhook_events.md | 3 +- spec/models/hooks/web_hook_spec.rb | 22 ++++++++++++++ spec/services/web_hook_service_spec.rb | 30 ++++++++++++++++--- 5 files changed, 76 insertions(+), 8 deletions(-) diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb index c0bfe31fb389da..273f548ceb7583 100644 --- a/app/models/hooks/web_hook.rb +++ b/app/models/hooks/web_hook.rb @@ -44,6 +44,7 @@ class WebHook < ApplicationRecord validates :url_variables, json_schema: { filename: 'web_hooks_url_variables' } validate :no_missing_url_variables validates :interpolated_url, public_url: true, if: ->(hook) { hook.url_variables? && hook.errors.empty? } + validate :valid_custom_payload_template, if: ->(hook) { hook.custom_payload_template.present? } enum branch_filter_strategy: { wildcard: 0, @@ -168,6 +169,12 @@ def no_missing_url_variables errors.add(:url, "Invalid URL template. Missing keys: #{missing}") end + def valid_custom_payload_template + Liquid::Template.parse(custom_payload_template) + rescue Liquid::SyntaxError => e + errors.add(:custom_payload_template, e.message) + end + def set_branch_filter_nil self.push_events_branch_filter = nil end diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index e20e2fab4af13d..b1d31f2d9b7d44 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -35,6 +35,8 @@ def initialize RESPONSE_HEADERS_COUNT_LIMIT = 50 RESPONSE_HEADERS_SIZE_LIMIT = 1.kilobytes + EXCEPTIONS_WITHOUT_REQUEST_BODY = [Liquid::SyntaxError, Liquid::MemoryError, JSON::ParserError].freeze + attr_accessor :hook, :data, :hook_name, :request_options attr_reader :uniqueness_token @@ -89,12 +91,12 @@ def execute ) ServiceResponse.success(message: response.body, payload: { http_status: response.code }) - rescue *Gitlab::HTTP::HTTP_ERRORS, Liquid::SyntaxError, JSON::ParserError, + rescue *Gitlab::HTTP::HTTP_ERRORS, Liquid::SyntaxError, Liquid::MemoryError, JSON::ParserError, Gitlab::Json::LimitedEncoder::LimitExceeded, URI::InvalidURIError => e execution_duration = ::Gitlab::Metrics::System.monotonic_time - start_time error_message = e.to_s - request_data = [Liquid::SyntaxError, JSON::ParserError].include?(e.class) ? {} : request_payload + request_data = EXCEPTIONS_WITHOUT_REQUEST_BODY.include?(e.class) ? {} : request_payload log_execution( response: InternalErrorResponse.new, @@ -277,10 +279,24 @@ def request_payload return data unless Feature.enabled?(:custom_webhook_payload, hook.parent, type: :beta) return data unless hook.custom_payload_template.present? - rendered_template = Liquid::Template.parse(hook.custom_payload_template).render!( + template = Liquid::Template.parse(hook.custom_payload_template) + template.resource_limits.render_score_limit = 500 + template.resource_limits.assign_score_limit = 500 + rendered_template = template.render!( { 'event' => data.deep_stringify_keys } ) + Gitlab::AppLogger.info( + message: "Rendered custom webhook payload template", + hook_id: hook.id, + render_score: template.resource_limits.render_score, + assign_score: template.resource_limits.assign_score + ) Gitlab::Json.parse(rendered_template) + rescue Liquid::MemoryError => e + raise Liquid::MemoryError, + "Error while rendering custom payload template: #{e.message.delete_prefix('Liquid error: ')}" + rescue JSON::ParserError => e + raise JSON::ParserError, "Error while parsing rendered custom payload template: #{e.message}" end strong_memoize_attr :request_payload end diff --git a/doc/user/project/integrations/webhook_events.md b/doc/user/project/integrations/webhook_events.md index 1004d1af7c0446..2aee47de8b598a 100644 --- a/doc/user/project/integrations/webhook_events.md +++ b/doc/user/project/integrations/webhook_events.md @@ -2061,7 +2061,8 @@ You can set a custom payload template in the webhook configuration. The request with the data for the current event. The template uses [liquid markup](https://shopify.github.io/liquid/basics/introduction/) and must render as valid JSON. -For example: +You can use any field from the payload of any event, such as `{{event.build_name}}` for a job event and `{{event.deployable_url}}` +for a deployment event. For example: - Custom payload template: diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb index 308e16328d7ef5..98683f7d7225d6 100644 --- a/spec/models/hooks/web_hook_spec.rb +++ b/spec/models/hooks/web_hook_spec.rb @@ -290,6 +290,28 @@ expected_valid_types = %w[all_branches regex wildcard] expect(described_class.branch_filter_strategies.keys).to contain_exactly(*expected_valid_types) end + + describe 'custom_payload_template' do + subject { build_stubbed(:project_hook, project: project, url: 'http://example.com', custom_payload_template: custom_payload_template) } + + context 'when template is not set' do + let(:custom_payload_template) { nil } + + it { is_expected.to be_valid } + end + + context 'when template is valid' do + let(:custom_payload_template) { '{"event":"{{event.object_kind}}"}' } + + it { is_expected.to be_valid } + end + + context 'when template is invalid' do + let(:custom_payload_template) { '{"event":"{{event.object_kind}"}' } + + it { is_expected.not_to be_valid } + end + end end describe 'encrypted attributes' do diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index ea06b9737ef370..45b7d962614649 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -370,14 +370,16 @@ end context 'when custom_payload_template is set' do + before do + stub_full_request(project_hook.url, method: :post) + end + context 'when template is valid' do before do project_hook.custom_payload_template = '{"before":"{{event.before}}"}' end it 'renders custom_payload_template for body' do - stub_full_request(project_hook.url, method: :post) - service_instance.execute expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)) @@ -388,7 +390,6 @@ context 'when feature flag is disabled' do before do stub_feature_flags(custom_webhook_payload: false) - stub_full_request(project_hook.url, method: :post) end it 'does not render custom template', :aggregate_failures do @@ -418,6 +419,26 @@ end end + context 'when template is too complex' do + before do + project_hook.custom_payload_template = '{ + {% for i in (1..500) -%} + "event": "static", + {% endfor %} + "key":"value" + } + ' + end + + it 'stops rendering the template', :aggregate_failures do + expect(service_instance.execute).to have_attributes( + status: :error, + message: 'Liquid error: Error while rendering custom payload template: Memory limits exceeded' + ) + expect { service_instance.execute }.not_to raise_error + end + end + context 'when template renders invalid json' do before do project_hook.custom_payload_template = '{"test":"{{event}}}' @@ -426,7 +447,8 @@ it 'handles the error', :aggregate_failures do expect(service_instance.execute).to have_attributes( status: :error, - message: 'unexpected character (after test) at line 1, column 12 [parse.c:804] in ' \ + message: 'Error while parsing rendered custom payload template: unexpected character (after test) ' \ + 'at line 1, column 12 [parse.c:804] in ' \ '\'{"test":"{"before"=>"oldrev", "after"=>"newrev", "ref"=>"ref"}}' ) expect { service_instance.execute }.not_to raise_error -- GitLab From 6723e1c4083f297c6f7f9a7d6a0f343cc4adffd6 Mon Sep 17 00:00:00 2001 From: Niklas Date: Wed, 7 Feb 2024 19:42:11 +0000 Subject: [PATCH 05/10] Apply review feedback --- Gemfile | 3 -- Gemfile.checksum | 1 - Gemfile.lock | 2 - .../concerns/web_hooks/hook_actions.rb | 2 +- app/models/hooks/web_hook.rb | 7 --- app/services/web_hook_service.rb | 39 ++++++++-------- app/views/shared/web_hooks/_form.html.haml | 8 ++-- ...ayload.yml => custom_webhook_template.yml} | 2 +- ...dd_custom_webhook_template_to_web_hook.rb} | 4 +- ...it_to_web_hooks_custom_payload_template.rb | 14 ------ ...it_to_web_hooks_custom_webhook_template.rb | 14 ++++++ db/structure.sql | 4 +- .../project/integrations/webhook_events.md | 32 ------------- doc/user/project/integrations/webhooks.md | 31 +++++++++++++ ee/lib/api/group_hooks.rb | 2 +- lib/api/entities/hook.rb | 2 +- lib/api/project_hooks.rb | 2 +- locale/gitlab.pot | 5 +- .../projects/hooks_controller_spec.rb | 2 +- spec/lib/api/entities/hook_spec.rb | 2 +- spec/models/hooks/web_hook_spec.rb | 22 --------- spec/services/web_hook_service_spec.rb | 46 +++++-------------- 22 files changed, 96 insertions(+), 150 deletions(-) rename config/feature_flags/beta/{custom_webhook_payload.yml => custom_webhook_template.yml} (91%) rename db/migrate/{20240124181406_add_custom_payload_template_to_web_hook.rb => 20240124181406_add_custom_webhook_template_to_web_hook.rb} (66%) delete mode 100644 db/migrate/20240124181407_add_text_limit_to_web_hooks_custom_payload_template.rb create mode 100644 db/migrate/20240124181407_add_text_limit_to_web_hooks_custom_webhook_template.rb diff --git a/Gemfile b/Gemfile index 093b1ba9d8c3e9..865897f5eaa79f 100644 --- a/Gemfile +++ b/Gemfile @@ -311,9 +311,6 @@ gem 'ruby-fogbugz', '~> 0.3.0', feature_category: :importers # Kubernetes integration gem 'kubeclient', '~> 4.11.0' # rubocop:todo Gemfile/MissingFeatureCategory -# Custom webhook payload template rendering -gem "liquid", "~> 5.4", feature_category: :integrations - # AI gem 'ruby-openai', '~> 3.7' # rubocop:todo Gemfile/MissingFeatureCategory gem 'circuitbox', '2.0.0' # rubocop:todo Gemfile/MissingFeatureCategory diff --git a/Gemfile.checksum b/Gemfile.checksum index 64fc36ff5aae77..64878c18f51321 100644 --- a/Gemfile.checksum +++ b/Gemfile.checksum @@ -354,7 +354,6 @@ {"name":"libyajl2","version":"2.1.0","platform":"ruby","checksum":"aa5df6c725776fc050c8418450de0f7c129cb7200b811907c4c0b3b5c0aea0ef"}, {"name":"license_finder","version":"7.0.1","platform":"ruby","checksum":"0b22c9567e2a8b102c7245da49ebeddaec60f66d237d2bb91b9feddf5d242f6a"}, {"name":"licensee","version":"9.16.1","platform":"ruby","checksum":"04c0b57d20b91fa82ac9dcb3637da9cdc1c3823e217c0781e5d0514958e2e515"}, -{"name":"liquid","version":"5.4.0","platform":"ruby","checksum":"e362448c71cb88683d9c2df2819d7e47da3cdb37f757d0c99e3400d7e0030d40"}, {"name":"listen","version":"3.7.1","platform":"ruby","checksum":"3b80caa7aa77fae836916c2f9e3fbcafbd15f5d695dd487c1f5b5e7e465efe29"}, {"name":"llhttp-ffi","version":"0.4.0","platform":"ruby","checksum":"e5f7327db3cf8007e648342ef76347d6e0ae545a8402e519cca9c886eb37b001"}, {"name":"locale","version":"2.1.3","platform":"ruby","checksum":"b6ddee011e157817cb98e521b3ce7cb626424d5882f1e844aafdee3e8b212725"}, diff --git a/Gemfile.lock b/Gemfile.lock index ff613be747ca81..0775a3d8c70980 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1047,7 +1047,6 @@ GEM reverse_markdown (>= 1, < 3) rugged (>= 0.24, < 2.0) thor (>= 0.19, < 2.0) - liquid (5.4.0) listen (3.7.1) rb-fsevent (~> 0.10, >= 0.10.3) rb-inotify (~> 0.9, >= 0.9.10) @@ -1996,7 +1995,6 @@ DEPENDENCIES letter_opener_web (~> 2.0.0) license_finder (~> 7.0) licensee (~> 9.16) - liquid (~> 5.4) listen (~> 3.7) lockbox (~> 1.3.0) lograge (~> 0.5) diff --git a/app/controllers/concerns/web_hooks/hook_actions.rb b/app/controllers/concerns/web_hooks/hook_actions.rb index a42fd590e39e5a..aae38e67e23008 100644 --- a/app/controllers/concerns/web_hooks/hook_actions.rb +++ b/app/controllers/concerns/web_hooks/hook_actions.rb @@ -72,7 +72,7 @@ def hook_params def hook_param_names %i[enable_ssl_verification name description token url push_events_branch_filter branch_filter_strategy - custom_payload_template] + custom_webhook_template] end def destroy_hook(hook) diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb index 273f548ceb7583..c0bfe31fb389da 100644 --- a/app/models/hooks/web_hook.rb +++ b/app/models/hooks/web_hook.rb @@ -44,7 +44,6 @@ class WebHook < ApplicationRecord validates :url_variables, json_schema: { filename: 'web_hooks_url_variables' } validate :no_missing_url_variables validates :interpolated_url, public_url: true, if: ->(hook) { hook.url_variables? && hook.errors.empty? } - validate :valid_custom_payload_template, if: ->(hook) { hook.custom_payload_template.present? } enum branch_filter_strategy: { wildcard: 0, @@ -169,12 +168,6 @@ def no_missing_url_variables errors.add(:url, "Invalid URL template. Missing keys: #{missing}") end - def valid_custom_payload_template - Liquid::Template.parse(custom_payload_template) - rescue Liquid::SyntaxError => e - errors.add(:custom_payload_template, e.message) - end - def set_branch_filter_nil self.push_events_branch_filter = nil end diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index b1d31f2d9b7d44..5c5925cbf79749 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -35,8 +35,6 @@ def initialize RESPONSE_HEADERS_COUNT_LIMIT = 50 RESPONSE_HEADERS_SIZE_LIMIT = 1.kilobytes - EXCEPTIONS_WITHOUT_REQUEST_BODY = [Liquid::SyntaxError, Liquid::MemoryError, JSON::ParserError].freeze - attr_accessor :hook, :data, :hook_name, :request_options attr_reader :uniqueness_token @@ -91,12 +89,13 @@ def execute ) ServiceResponse.success(message: response.body, payload: { http_status: response.code }) - rescue *Gitlab::HTTP::HTTP_ERRORS, Liquid::SyntaxError, Liquid::MemoryError, JSON::ParserError, + rescue *Gitlab::HTTP::HTTP_ERRORS, JSON::ParserError, Gitlab::Json::LimitedEncoder::LimitExceeded, URI::InvalidURIError => e execution_duration = ::Gitlab::Metrics::System.monotonic_time - start_time error_message = e.to_s - request_data = EXCEPTIONS_WITHOUT_REQUEST_BODY.include?(e.class) ? {} : request_payload + # An exception raised while rendering the custom template prevents us from calling `#request_payload` + request_data = e.instance_of?(JSON::ParserError) ? {} : request_payload log_execution( response: InternalErrorResponse.new, @@ -276,27 +275,27 @@ def enforce_utf8(str) end def request_payload - return data unless Feature.enabled?(:custom_webhook_payload, hook.parent, type: :beta) - return data unless hook.custom_payload_template.present? - - template = Liquid::Template.parse(hook.custom_payload_template) - template.resource_limits.render_score_limit = 500 - template.resource_limits.assign_score_limit = 500 - rendered_template = template.render!( - { 'event' => data.deep_stringify_keys } - ) + return data unless hook.custom_webhook_template.present? + return data unless Feature.enabled?(:custom_webhook_template, hook.parent, type: :beta) + + start_time = Gitlab::Metrics::System.monotonic_time + rendered_template = render_custom_template(hook.custom_webhook_template, { event: data }.deep_stringify_keys) + duration = Gitlab::Metrics::System.monotonic_time - start_time + Gitlab::AppLogger.info( - message: "Rendered custom webhook payload template", + message: "Rendered custom webhook template", hook_id: hook.id, - render_score: template.resource_limits.render_score, - assign_score: template.resource_limits.assign_score + duration: duration ) Gitlab::Json.parse(rendered_template) - rescue Liquid::MemoryError => e - raise Liquid::MemoryError, - "Error while rendering custom payload template: #{e.message.delete_prefix('Liquid error: ')}" rescue JSON::ParserError => e - raise JSON::ParserError, "Error while parsing rendered custom payload template: #{e.message}" + raise JSON::ParserError, "Error while parsing rendered custom webhook template: #{e.message}" end strong_memoize_attr :request_payload + + def render_custom_template(template, params) + matches = template.scan(/{{(.+?)}}/).flatten.uniq + replacements = matches.to_h { |match| ["{{#{match}}}", params.dig(*match.split('.'))] } + template.gsub(/{{.+?}}/, replacements) + end end diff --git a/app/views/shared/web_hooks/_form.html.haml b/app/views/shared/web_hooks/_form.html.haml index 750dae73f56f9c..5f920d9c982e96 100644 --- a/app/views/shared/web_hooks/_form.html.haml +++ b/app/views/shared/web_hooks/_form.html.haml @@ -77,10 +77,12 @@ integration_webhook_event_human_name(:emoji_events), help_text: s_('Webhooks|An emoji is awarded or revoked. %{help_link}?').html_safe % { help_link: emoji_help_link } -- if Feature.enabled?(:custom_webhook_payload, hook.parent, type: :beta) +- if Feature.enabled?(:custom_webhook_template, hook.parent, type: :beta) .form-group - = form.label :custom_payload_template, s_('Webhooks|Custom Payload Template (optional)'), class: 'label-bold' - = form.text_area :custom_payload_template, value: hook.custom_payload_template, class: 'form-control gl-form-input gl-form-input-xl', rows: 8, maxlength: 4096 + = form.label :custom_webhook_template, s_('Webhooks|Custom webhook template (optional)'), class: 'label-bold' + = form.text_area :custom_webhook_template, value: hook.custom_webhook_template, class: 'form-control gl-form-input gl-form-input-xl', rows: 8, maxlength: 4096 + %p.form-text.text-muted + = link_to s_('Webhooks|How to create a custom webhook template?'), help_page_path('user/project/integrations/webhooks', anchor: 'custom-webhook-template') .form-group = form.label :enable_ssl_verification, s_('Webhooks|SSL verification'), class: 'label-bold checkbox' diff --git a/config/feature_flags/beta/custom_webhook_payload.yml b/config/feature_flags/beta/custom_webhook_template.yml similarity index 91% rename from config/feature_flags/beta/custom_webhook_payload.yml rename to config/feature_flags/beta/custom_webhook_template.yml index 2cde183d8359e5..ee3a572b3fc443 100644 --- a/config/feature_flags/beta/custom_webhook_payload.yml +++ b/config/feature_flags/beta/custom_webhook_template.yml @@ -1,5 +1,5 @@ --- -name: custom_webhook_payload +name: custom_webhook_template feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/362504 introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/142738 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/439610 diff --git a/db/migrate/20240124181406_add_custom_payload_template_to_web_hook.rb b/db/migrate/20240124181406_add_custom_webhook_template_to_web_hook.rb similarity index 66% rename from db/migrate/20240124181406_add_custom_payload_template_to_web_hook.rb rename to db/migrate/20240124181406_add_custom_webhook_template_to_web_hook.rb index 9afdce7876405b..1304628e19eb8d 100644 --- a/db/migrate/20240124181406_add_custom_payload_template_to_web_hook.rb +++ b/db/migrate/20240124181406_add_custom_webhook_template_to_web_hook.rb @@ -1,12 +1,12 @@ # frozen_string_literal: true -class AddCustomPayloadTemplateToWebHook < Gitlab::Database::Migration[2.2] +class AddCustomWebhookTemplateToWebHook < Gitlab::Database::Migration[2.2] enable_lock_retries! milestone '16.9' def change # rubocop:disable Migration/AddLimitToTextColumns -- limit is added in 20240124181407 - add_column :web_hooks, :custom_payload_template, :text, null: true + add_column :web_hooks, :custom_webhook_template, :text, null: true # rubocop:enable Migration/AddLimitToTextColumns end end diff --git a/db/migrate/20240124181407_add_text_limit_to_web_hooks_custom_payload_template.rb b/db/migrate/20240124181407_add_text_limit_to_web_hooks_custom_payload_template.rb deleted file mode 100644 index 3b33b66225c38a..00000000000000 --- a/db/migrate/20240124181407_add_text_limit_to_web_hooks_custom_payload_template.rb +++ /dev/null @@ -1,14 +0,0 @@ -# frozen_string_literal: true - -class AddTextLimitToWebHooksCustomPayloadTemplate < Gitlab::Database::Migration[2.2] - disable_ddl_transaction! - milestone '16.9' - - def up - add_text_limit :web_hooks, :custom_payload_template, 4096 - end - - def down - remove_text_limit :web_hooks, :custom_payload_template - end -end diff --git a/db/migrate/20240124181407_add_text_limit_to_web_hooks_custom_webhook_template.rb b/db/migrate/20240124181407_add_text_limit_to_web_hooks_custom_webhook_template.rb new file mode 100644 index 00000000000000..6d8087dff2a794 --- /dev/null +++ b/db/migrate/20240124181407_add_text_limit_to_web_hooks_custom_webhook_template.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class AddTextLimitToWebHooksCustomWebhookTemplate < Gitlab::Database::Migration[2.2] + disable_ddl_transaction! + milestone '16.9' + + def up + add_text_limit :web_hooks, :custom_webhook_template, 4096 + end + + def down + remove_text_limit :web_hooks, :custom_webhook_template + end +end diff --git a/db/structure.sql b/db/structure.sql index 7f46b38e53f1bf..50a31c8406d54c 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -25896,10 +25896,10 @@ CREATE TABLE web_hooks ( emoji_events boolean DEFAULT false NOT NULL, name text, description text, - custom_payload_template text, + custom_webhook_template text, CONSTRAINT check_1e4d5cbdc5 CHECK ((char_length(name) <= 255)), CONSTRAINT check_23a96ad211 CHECK ((char_length(description) <= 2048)), - CONSTRAINT check_78df43030f CHECK ((char_length(custom_payload_template) <= 4096)) + CONSTRAINT check_69ef76ee0c CHECK ((char_length(custom_webhook_template) <= 4096)) ); CREATE SEQUENCE web_hooks_id_seq diff --git a/doc/user/project/integrations/webhook_events.md b/doc/user/project/integrations/webhook_events.md index 2aee47de8b598a..5253fd9d64b944 100644 --- a/doc/user/project/integrations/webhook_events.md +++ b/doc/user/project/integrations/webhook_events.md @@ -2047,35 +2047,3 @@ Payload example: } } ``` - -## Custom payload template - -> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/142738) in GitLab 16.9 [with a flag](../../../administration/feature_flags.md) named `custom_webhook_payload`. Disabled by default. - -FLAG: -On self-managed GitLab, by default this feature is not available. To make it available, an administrator can -[enable the feature flag](../../../administration/feature_flags.md) named `custom_webhook_payload`. -On GitLab.com, this feature is not available. - -You can set a custom payload template in the webhook configuration. The request body is rendered from the template -with the data for the current event. The template uses [liquid markup](https://shopify.github.io/liquid/basics/introduction/) -and must render as valid JSON. - -You can use any field from the payload of any event, such as `{{event.build_name}}` for a job event and `{{event.deployable_url}}` -for a deployment event. For example: - -- Custom payload template: - - ```json - { - "event": "{{event.object_kind}}" - } - ``` - -- Request payload that combines the template with a `push` event: - - ```json - { - "event": "push" - } - ``` diff --git a/doc/user/project/integrations/webhooks.md b/doc/user/project/integrations/webhooks.md index 7ff9f05d0a9266..74917979c35bcc 100644 --- a/doc/user/project/integrations/webhooks.md +++ b/doc/user/project/integrations/webhooks.md @@ -106,6 +106,37 @@ You must define the following variables: Variable names can contain only lowercase letters (`a-z`), numbers (`0-9`), or underscores (`_`). You can define URL variables directly using the REST API. +## Custom webhook template + +> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/142738) in GitLab 16.9 [with a flag](../../../administration/feature_flags.md) named `custom_webhook_template`. Disabled by default. + +FLAG: +On self-managed GitLab, by default this feature is not available. To make it available, an administrator can +[enable the feature flag](../../../administration/feature_flags.md) named `custom_webhook_template`. +On GitLab.com, this feature is not available. + +You can set a custom payload template in the webhook configuration. The request body is rendered from the template +with the data for the current event. The template uses mustache like syntax and must render as valid JSON. + +You can use any field from the [payload of any event](webhook_events.md), such as `{{event.build_name}}` for a job event and `{{event.deployable_url}}` +for a deployment event. For example: + +- Custom payload template: + + ```json + { + "event": "{{event.object_kind}}" + } + ``` + +- Request payload that combines the template with a `push` event: + + ```json + { + "event": "push" + } + ``` + ## Configure your webhook receiver endpoint Webhook receiver endpoints should be fast and stable. diff --git a/ee/lib/api/group_hooks.rb b/ee/lib/api/group_hooks.rb index ebffc64d124f9a..3b3a8cec6dbf2d 100644 --- a/ee/lib/api/group_hooks.rb +++ b/ee/lib/api/group_hooks.rb @@ -36,7 +36,7 @@ def hook_scope optional :emoji_events, type: Boolean, desc: "Trigger hook on emoji events" optional :enable_ssl_verification, type: Boolean, desc: "Do SSL verification when triggering the hook" optional :token, type: String, desc: "Secret token to validate received payloads; this will not be returned in the response" - optional :custom_payload_template, type: String, desc: "Custom template for the request payload" + optional :custom_webhook_template, type: String, desc: "Custom template for the request payload" use :url_variables end end diff --git a/lib/api/entities/hook.rb b/lib/api/entities/hook.rb index 68561c8975f125..473d21d409ba09 100644 --- a/lib/api/entities/hook.rb +++ b/lib/api/entities/hook.rb @@ -18,7 +18,7 @@ class Hook < Grape::Entity if: ->(_, options) { options[:with_url_variables] != false }, documentation: { type: 'Hash', example: { "token" => "secr3t" }, is_array: true } - expose :custom_payload_template, documentation: { type: 'string', example: '{"event":"{{event.object_kind}}"}' } + expose :custom_webhook_template, documentation: { type: 'string', example: '{"event":"{{event.object_kind}}"}' } def url_variables object.url_variables.keys.map { { key: _1 } } diff --git a/lib/api/project_hooks.rb b/lib/api/project_hooks.rb index 8e4c4da7bfdfce..508026f6fda0ae 100644 --- a/lib/api/project_hooks.rb +++ b/lib/api/project_hooks.rb @@ -35,7 +35,7 @@ def hook_scope optional :enable_ssl_verification, type: Boolean, desc: "Do SSL verification when triggering the hook" optional :token, type: String, desc: "Secret token to validate received payloads; this will not be returned in the response" optional :push_events_branch_filter, type: String, desc: "Trigger hook on specified branch only" - optional :custom_payload_template, type: String, desc: "Custom template for the request payload" + optional :custom_webhook_template, type: String, desc: "Custom template for the request payload" use :url_variables end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 0d41107663be17..2c3cc3de029c1a 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -55405,7 +55405,7 @@ msgstr "" msgid "Webhooks|Confidential issues events" msgstr "" -msgid "Webhooks|Custom Payload Template (optional)" +msgid "Webhooks|Custom webhook template (optional)" msgstr "" msgid "Webhooks|Delete webhook" @@ -55438,6 +55438,9 @@ msgstr "" msgid "Webhooks|How it looks in the UI" msgstr "" +msgid "Webhooks|How to create a custom webhook template?" +msgstr "" + msgid "Webhooks|Issues events" msgstr "" diff --git a/spec/controllers/projects/hooks_controller_spec.rb b/spec/controllers/projects/hooks_controller_spec.rb index 0a210f9eca2d19..0d1a452350719f 100644 --- a/spec/controllers/projects/hooks_controller_spec.rb +++ b/spec/controllers/projects/hooks_controller_spec.rb @@ -138,7 +138,7 @@ def it_renders_correctly wiki_page_events: true, deployment_events: true, - custom_payload_template: '{"test":"test"}', + custom_webhook_template: '{"test":"test"}', url_variables: [{ key: 'token', value: 'some secret value' }] } diff --git a/spec/lib/api/entities/hook_spec.rb b/spec/lib/api/entities/hook_spec.rb index 68b18ba46c1632..9b1db1ad258d12 100644 --- a/spec/lib/api/entities/hook_spec.rb +++ b/spec/lib/api/entities/hook_spec.rb @@ -12,7 +12,7 @@ it 'exposes correct attributes' do expect(json.keys).to contain_exactly(:alert_status, :created_at, :disabled_until, :enable_ssl_verification, :id, :merge_requests_events, :push_events, :repository_update_events, :tag_push_events, :url, :url_variables, - :custom_payload_template + :custom_webhook_template ) end diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb index 98683f7d7225d6..308e16328d7ef5 100644 --- a/spec/models/hooks/web_hook_spec.rb +++ b/spec/models/hooks/web_hook_spec.rb @@ -290,28 +290,6 @@ expected_valid_types = %w[all_branches regex wildcard] expect(described_class.branch_filter_strategies.keys).to contain_exactly(*expected_valid_types) end - - describe 'custom_payload_template' do - subject { build_stubbed(:project_hook, project: project, url: 'http://example.com', custom_payload_template: custom_payload_template) } - - context 'when template is not set' do - let(:custom_payload_template) { nil } - - it { is_expected.to be_valid } - end - - context 'when template is valid' do - let(:custom_payload_template) { '{"event":"{{event.object_kind}}"}' } - - it { is_expected.to be_valid } - end - - context 'when template is invalid' do - let(:custom_payload_template) { '{"event":"{{event.object_kind}"}' } - - it { is_expected.not_to be_valid } - end - end end describe 'encrypted attributes' do diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 45b7d962614649..e9ec99e4adcc7f 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -369,17 +369,17 @@ end end - context 'when custom_payload_template is set' do + context 'when custom_webhook_template is set' do before do stub_full_request(project_hook.url, method: :post) end context 'when template is valid' do before do - project_hook.custom_payload_template = '{"before":"{{event.before}}"}' + project_hook.custom_webhook_template = '{"before":"{{event.before}}"}' end - it 'renders custom_payload_template for body' do + it 'renders custom_webhook_template for body' do service_instance.execute expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)) @@ -389,15 +389,12 @@ context 'when feature flag is disabled' do before do - stub_feature_flags(custom_webhook_payload: false) + stub_feature_flags(custom_webhook_template: false) end it 'does not render custom template', :aggregate_failures do - allow(Liquid::Template).to receive(:new) - service_instance.execute - expect(Liquid::Template).not_to have_received(:new) expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)) .with(headers: headers, body: '{"before":"oldrev","after":"newrev","ref":"ref"}') .once @@ -407,47 +404,28 @@ context 'when template is invalid' do before do - project_hook.custom_payload_template = '{"test":"{{event}"}' + project_hook.custom_webhook_template = '{"test":"{{event}"}' end - it 'handles the error', :aggregate_failures do - expect(service_instance.execute).to have_attributes( - status: :error, - message: 'Liquid syntax error: Variable \'{{event}\' was not properly terminated with regexp: /\\}\\}/' - ) - expect { service_instance.execute }.not_to raise_error - end - end - - context 'when template is too complex' do - before do - project_hook.custom_payload_template = '{ - {% for i in (1..500) -%} - "event": "static", - {% endfor %} - "key":"value" - } - ' - end + it 'renders without problems', :aggregate_failures do + service_instance.execute - it 'stops rendering the template', :aggregate_failures do - expect(service_instance.execute).to have_attributes( - status: :error, - message: 'Liquid error: Error while rendering custom payload template: Memory limits exceeded' - ) + expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)) + .with(headers: headers, body: '{"test":"{{event}"}') + .once expect { service_instance.execute }.not_to raise_error end end context 'when template renders invalid json' do before do - project_hook.custom_payload_template = '{"test":"{{event}}}' + project_hook.custom_webhook_template = '{"test":"{{event}}}' end it 'handles the error', :aggregate_failures do expect(service_instance.execute).to have_attributes( status: :error, - message: 'Error while parsing rendered custom payload template: unexpected character (after test) ' \ + message: 'Error while parsing rendered custom webhook template: unexpected character (after test) ' \ 'at line 1, column 12 [parse.c:804] in ' \ '\'{"test":"{"before"=>"oldrev", "after"=>"newrev", "ref"=>"ref"}}' ) -- GitLab From 5888e4e8b397d539766968a6a0cd38cfa14b8ec4 Mon Sep 17 00:00:00 2001 From: Niklas Date: Wed, 7 Feb 2024 20:19:44 +0000 Subject: [PATCH 06/10] Update schema fixtures --- ee/spec/fixtures/api/schemas/public_api/v4/group_hook.json | 4 ++-- spec/fixtures/api/schemas/public_api/v4/project_hook.json | 4 ++-- spec/fixtures/api/schemas/public_api/v4/system_hook.json | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/group_hook.json b/ee/spec/fixtures/api/schemas/public_api/v4/group_hook.json index 699f9e59afc176..496462a2357329 100644 --- a/ee/spec/fixtures/api/schemas/public_api/v4/group_hook.json +++ b/ee/spec/fixtures/api/schemas/public_api/v4/group_hook.json @@ -25,7 +25,7 @@ "alert_status", "disabled_until", "url_variables", - "custom_payload_template" + "custom_webhook_template" ], "properties": { "id": { @@ -130,7 +130,7 @@ } } }, - "custom_payload_template": { + "custom_webhook_template": { "type": [ "string", "null" diff --git a/spec/fixtures/api/schemas/public_api/v4/project_hook.json b/spec/fixtures/api/schemas/public_api/v4/project_hook.json index 1eaf5f3482cdf4..dcf04ee6a3a2eb 100644 --- a/spec/fixtures/api/schemas/public_api/v4/project_hook.json +++ b/spec/fixtures/api/schemas/public_api/v4/project_hook.json @@ -23,7 +23,7 @@ "alert_status", "disabled_until", "emoji_events", - "custom_payload_template" + "custom_webhook_template" ], "optional": [ "url_variables" @@ -128,7 +128,7 @@ } } }, - "custom_payload_template": { + "custom_webhook_template": { "type": [ "string", "null" diff --git a/spec/fixtures/api/schemas/public_api/v4/system_hook.json b/spec/fixtures/api/schemas/public_api/v4/system_hook.json index a4d0ae1d0bfdfe..62154d92acffcc 100644 --- a/spec/fixtures/api/schemas/public_api/v4/system_hook.json +++ b/spec/fixtures/api/schemas/public_api/v4/system_hook.json @@ -12,7 +12,7 @@ "alert_status", "disabled_until", "url_variables", - "custom_payload_template" + "custom_webhook_template" ], "properties": { "id": { @@ -68,7 +68,7 @@ } } }, - "custom_payload_template": { + "custom_webhook_template": { "type": [ "string", "null" -- GitLab From 4a50decb54cc82219d9a98a973348297ddd22930 Mon Sep 17 00:00:00 2001 From: Niklas Date: Tue, 13 Feb 2024 15:39:16 +0000 Subject: [PATCH 07/10] Apply review feedback --- app/models/hooks/web_hook.rb | 1 + app/services/web_hook_service.rb | 8 +++++--- config/feature_flags/beta/custom_webhook_template.yml | 2 +- ...0213181406_add_custom_webhook_template_to_web_hook.rb} | 4 ++-- ...dd_text_limit_to_web_hooks_custom_webhook_template.rb} | 2 +- db/schema_migrations/20240124181406 | 1 - db/schema_migrations/20240124181407 | 1 - db/schema_migrations/20240213181406 | 1 + db/schema_migrations/20240213181407 | 1 + doc/user/project/integrations/webhooks.md | 2 +- spec/models/hooks/web_hook_spec.rb | 1 + 11 files changed, 14 insertions(+), 10 deletions(-) rename db/migrate/{20240124181406_add_custom_webhook_template_to_web_hook.rb => 20240213181406_add_custom_webhook_template_to_web_hook.rb} (88%) rename db/migrate/{20240124181407_add_text_limit_to_web_hooks_custom_webhook_template.rb => 20240213181407_add_text_limit_to_web_hooks_custom_webhook_template.rb} (93%) delete mode 100644 db/schema_migrations/20240124181406 delete mode 100644 db/schema_migrations/20240124181407 create mode 100644 db/schema_migrations/20240213181406 create mode 100644 db/schema_migrations/20240213181407 diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb index c0bfe31fb389da..24f19b932e0e74 100644 --- a/app/models/hooks/web_hook.rb +++ b/app/models/hooks/web_hook.rb @@ -44,6 +44,7 @@ class WebHook < ApplicationRecord validates :url_variables, json_schema: { filename: 'web_hooks_url_variables' } validate :no_missing_url_variables validates :interpolated_url, public_url: true, if: ->(hook) { hook.url_variables? && hook.errors.empty? } + validates :custom_webhook_template, length: { maximum: 4096 } enum branch_filter_strategy: { wildcard: 0, diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index 5c5925cbf79749..70c9233c2ea26b 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -35,6 +35,8 @@ def initialize RESPONSE_HEADERS_COUNT_LIMIT = 50 RESPONSE_HEADERS_SIZE_LIMIT = 1.kilobytes + CUSTOM_TEMPLATE_INTERPOLATION_REGEX = /{{(.+?)}}/ + attr_accessor :hook, :data, :hook_name, :request_options attr_reader :uniqueness_token @@ -285,7 +287,7 @@ def request_payload Gitlab::AppLogger.info( message: "Rendered custom webhook template", hook_id: hook.id, - duration: duration + duration_s: duration ) Gitlab::Json.parse(rendered_template) rescue JSON::ParserError => e @@ -294,8 +296,8 @@ def request_payload strong_memoize_attr :request_payload def render_custom_template(template, params) - matches = template.scan(/{{(.+?)}}/).flatten.uniq + matches = template.scan(CUSTOM_TEMPLATE_INTERPOLATION_REGEX).flatten.uniq replacements = matches.to_h { |match| ["{{#{match}}}", params.dig(*match.split('.'))] } - template.gsub(/{{.+?}}/, replacements) + template.gsub(CUSTOM_TEMPLATE_INTERPOLATION_REGEX, replacements) end end diff --git a/config/feature_flags/beta/custom_webhook_template.yml b/config/feature_flags/beta/custom_webhook_template.yml index ee3a572b3fc443..8fa1097d71c84a 100644 --- a/config/feature_flags/beta/custom_webhook_template.yml +++ b/config/feature_flags/beta/custom_webhook_template.yml @@ -3,7 +3,7 @@ name: custom_webhook_template feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/362504 introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/142738 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/439610 -milestone: '16.9' +milestone: '16.10' group: group::import and integrate type: beta default_enabled: false diff --git a/db/migrate/20240124181406_add_custom_webhook_template_to_web_hook.rb b/db/migrate/20240213181406_add_custom_webhook_template_to_web_hook.rb similarity index 88% rename from db/migrate/20240124181406_add_custom_webhook_template_to_web_hook.rb rename to db/migrate/20240213181406_add_custom_webhook_template_to_web_hook.rb index 1304628e19eb8d..87e982b36c580a 100644 --- a/db/migrate/20240124181406_add_custom_webhook_template_to_web_hook.rb +++ b/db/migrate/20240213181406_add_custom_webhook_template_to_web_hook.rb @@ -2,10 +2,10 @@ class AddCustomWebhookTemplateToWebHook < Gitlab::Database::Migration[2.2] enable_lock_retries! - milestone '16.9' + milestone '16.10' def change - # rubocop:disable Migration/AddLimitToTextColumns -- limit is added in 20240124181407 + # rubocop:disable Migration/AddLimitToTextColumns -- limit is added in 20240213181407 add_column :web_hooks, :custom_webhook_template, :text, null: true # rubocop:enable Migration/AddLimitToTextColumns end diff --git a/db/migrate/20240124181407_add_text_limit_to_web_hooks_custom_webhook_template.rb b/db/migrate/20240213181407_add_text_limit_to_web_hooks_custom_webhook_template.rb similarity index 93% rename from db/migrate/20240124181407_add_text_limit_to_web_hooks_custom_webhook_template.rb rename to db/migrate/20240213181407_add_text_limit_to_web_hooks_custom_webhook_template.rb index 6d8087dff2a794..0f6084b12cc7c1 100644 --- a/db/migrate/20240124181407_add_text_limit_to_web_hooks_custom_webhook_template.rb +++ b/db/migrate/20240213181407_add_text_limit_to_web_hooks_custom_webhook_template.rb @@ -2,7 +2,7 @@ class AddTextLimitToWebHooksCustomWebhookTemplate < Gitlab::Database::Migration[2.2] disable_ddl_transaction! - milestone '16.9' + milestone '16.10' def up add_text_limit :web_hooks, :custom_webhook_template, 4096 diff --git a/db/schema_migrations/20240124181406 b/db/schema_migrations/20240124181406 deleted file mode 100644 index 8cfd02da327533..00000000000000 --- a/db/schema_migrations/20240124181406 +++ /dev/null @@ -1 +0,0 @@ -9b85ae64f559377cf86fa1d971f7d2d19984115904699f51ab0e9645387b445d \ No newline at end of file diff --git a/db/schema_migrations/20240124181407 b/db/schema_migrations/20240124181407 deleted file mode 100644 index 58ce4b2e22ee42..00000000000000 --- a/db/schema_migrations/20240124181407 +++ /dev/null @@ -1 +0,0 @@ -7e1f9fe893986d49e201a6adb82a4c74e11f36ded1db2eb1210b0eadcf16bd7a \ No newline at end of file diff --git a/db/schema_migrations/20240213181406 b/db/schema_migrations/20240213181406 new file mode 100644 index 00000000000000..2d3ecaba285ee4 --- /dev/null +++ b/db/schema_migrations/20240213181406 @@ -0,0 +1 @@ +c08cf74a779336874a525308791a9974543591692e0fbcb2c66c13455617c52d \ No newline at end of file diff --git a/db/schema_migrations/20240213181407 b/db/schema_migrations/20240213181407 new file mode 100644 index 00000000000000..b67e12deb557b8 --- /dev/null +++ b/db/schema_migrations/20240213181407 @@ -0,0 +1 @@ +33e45972396db4163da1089bccf7aeb6da5e2a88e2e7ea4d4e3fe00b6347a54a \ No newline at end of file diff --git a/doc/user/project/integrations/webhooks.md b/doc/user/project/integrations/webhooks.md index 74917979c35bcc..3c9eec01eef28a 100644 --- a/doc/user/project/integrations/webhooks.md +++ b/doc/user/project/integrations/webhooks.md @@ -108,7 +108,7 @@ You can define URL variables directly using the REST API. ## Custom webhook template -> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/142738) in GitLab 16.9 [with a flag](../../../administration/feature_flags.md) named `custom_webhook_template`. Disabled by default. +> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/142738) in GitLab 16.10 [with a flag](../../../administration/feature_flags.md) named `custom_webhook_template`. Disabled by default. FLAG: On self-managed GitLab, by default this feature is not available. To make it available, an administrator can diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb index 308e16328d7ef5..4b5ab3277db554 100644 --- a/spec/models/hooks/web_hook_spec.rb +++ b/spec/models/hooks/web_hook_spec.rb @@ -23,6 +23,7 @@ describe 'validations' do it { is_expected.to validate_presence_of(:url) } + it { is_expected.to validate_length_of(:custom_webhook_template).is_at_most(4096) } describe 'url_variables' do it { is_expected.to allow_value({}).for(:url_variables) } -- GitLab From a4dceb4b461f015fade96b0c0b78b9c2dd3468e0 Mon Sep 17 00:00:00 2001 From: Niklas Date: Tue, 13 Feb 2024 17:40:03 +0000 Subject: [PATCH 08/10] Remove mention of mustache-like syntax --- doc/user/project/integrations/webhooks.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/user/project/integrations/webhooks.md b/doc/user/project/integrations/webhooks.md index 3c9eec01eef28a..f81a1e45e3489c 100644 --- a/doc/user/project/integrations/webhooks.md +++ b/doc/user/project/integrations/webhooks.md @@ -116,7 +116,7 @@ On self-managed GitLab, by default this feature is not available. To make it ava On GitLab.com, this feature is not available. You can set a custom payload template in the webhook configuration. The request body is rendered from the template -with the data for the current event. The template uses mustache like syntax and must render as valid JSON. +with the data for the current event. The template must render as valid JSON. You can use any field from the [payload of any event](webhook_events.md), such as `{{event.build_name}}` for a job event and `{{event.deployable_url}}` for a deployment event. For example: -- GitLab From 8781942108e87ce98d641b6ae2eaaf70dc27942d Mon Sep 17 00:00:00 2001 From: Niklas Date: Wed, 14 Feb 2024 13:39:24 +0000 Subject: [PATCH 09/10] Remove event prefix --- app/services/web_hook_service.rb | 2 +- doc/user/project/integrations/webhooks.md | 4 ++-- spec/services/web_hook_service_spec.rb | 9 ++++----- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index 70c9233c2ea26b..9ab26bf4e1cdd6 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -281,7 +281,7 @@ def request_payload return data unless Feature.enabled?(:custom_webhook_template, hook.parent, type: :beta) start_time = Gitlab::Metrics::System.monotonic_time - rendered_template = render_custom_template(hook.custom_webhook_template, { event: data }.deep_stringify_keys) + rendered_template = render_custom_template(hook.custom_webhook_template, data.deep_stringify_keys) duration = Gitlab::Metrics::System.monotonic_time - start_time Gitlab::AppLogger.info( diff --git a/doc/user/project/integrations/webhooks.md b/doc/user/project/integrations/webhooks.md index f81a1e45e3489c..0f120a2284192b 100644 --- a/doc/user/project/integrations/webhooks.md +++ b/doc/user/project/integrations/webhooks.md @@ -118,14 +118,14 @@ On GitLab.com, this feature is not available. You can set a custom payload template in the webhook configuration. The request body is rendered from the template with the data for the current event. The template must render as valid JSON. -You can use any field from the [payload of any event](webhook_events.md), such as `{{event.build_name}}` for a job event and `{{event.deployable_url}}` +You can use any field from the [payload of any event](webhook_events.md), such as `{{build_name}}` for a job event and `{{deployable_url}}` for a deployment event. For example: - Custom payload template: ```json { - "event": "{{event.object_kind}}" + "event": "{{object_kind}}" } ``` diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index e9ec99e4adcc7f..ea76d8cb79d60d 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -376,7 +376,7 @@ context 'when template is valid' do before do - project_hook.custom_webhook_template = '{"before":"{{event.before}}"}' + project_hook.custom_webhook_template = '{"before":"{{before}}"}' end it 'renders custom_webhook_template for body' do @@ -419,15 +419,14 @@ context 'when template renders invalid json' do before do - project_hook.custom_webhook_template = '{"test":"{{event}}}' + project_hook.custom_webhook_template = '{"test":"{{before}}}' end it 'handles the error', :aggregate_failures do expect(service_instance.execute).to have_attributes( status: :error, - message: 'Error while parsing rendered custom webhook template: unexpected character (after test) ' \ - 'at line 1, column 12 [parse.c:804] in ' \ - '\'{"test":"{"before"=>"oldrev", "after"=>"newrev", "ref"=>"ref"}}' + message: 'Error while parsing rendered custom webhook template: quoted string not terminated ' \ + '(after test) at line 1, column 16 [parse.c:379] in \'{"test":"oldrev}' ) expect { service_instance.execute }.not_to raise_error end -- GitLab From f6047f26b28101593d070714c2b0f50e09ff2265 Mon Sep 17 00:00:00 2001 From: Niklas Date: Thu, 15 Feb 2024 19:48:28 +0000 Subject: [PATCH 10/10] Apply reviewer feedback --- app/services/web_hook_service.rb | 4 +--- doc/user/project/integrations/webhooks.md | 24 +++++++++++------------ lib/api/entities/hook.rb | 2 +- spec/services/web_hook_service_spec.rb | 18 +++++++++++++++++ 4 files changed, 32 insertions(+), 16 deletions(-) diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index 9ab26bf4e1cdd6..2373e1de3dd807 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -296,8 +296,6 @@ def request_payload strong_memoize_attr :request_payload def render_custom_template(template, params) - matches = template.scan(CUSTOM_TEMPLATE_INTERPOLATION_REGEX).flatten.uniq - replacements = matches.to_h { |match| ["{{#{match}}}", params.dig(*match.split('.'))] } - template.gsub(CUSTOM_TEMPLATE_INTERPOLATION_REGEX, replacements) + template.gsub(CUSTOM_TEMPLATE_INTERPOLATION_REGEX) { params.dig(*Regexp.last_match(1).split('.')) } end end diff --git a/doc/user/project/integrations/webhooks.md b/doc/user/project/integrations/webhooks.md index 0f120a2284192b..ad1fd104c87592 100644 --- a/doc/user/project/integrations/webhooks.md +++ b/doc/user/project/integrations/webhooks.md @@ -121,21 +121,21 @@ with the data for the current event. The template must render as valid JSON. You can use any field from the [payload of any event](webhook_events.md), such as `{{build_name}}` for a job event and `{{deployable_url}}` for a deployment event. For example: -- Custom payload template: +Given this custom payload template: - ```json - { - "event": "{{object_kind}}" - } - ``` +```json +{ + "event": "{{object_kind}}" +} +``` -- Request payload that combines the template with a `push` event: +You'll have this request payload that combines the template with a `push` event: - ```json - { - "event": "push" - } - ``` +```json +{ + "event": "push" +} +``` ## Configure your webhook receiver endpoint diff --git a/lib/api/entities/hook.rb b/lib/api/entities/hook.rb index 473d21d409ba09..61f3d60bd48a25 100644 --- a/lib/api/entities/hook.rb +++ b/lib/api/entities/hook.rb @@ -18,7 +18,7 @@ class Hook < Grape::Entity if: ->(_, options) { options[:with_url_variables] != false }, documentation: { type: 'Hash', example: { "token" => "secr3t" }, is_array: true } - expose :custom_webhook_template, documentation: { type: 'string', example: '{"event":"{{event.object_kind}}"}' } + expose :custom_webhook_template, documentation: { type: 'string', example: '{"event":"{{object_kind}}"}' } def url_variables object.url_variables.keys.map { { key: _1 } } diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index ea76d8cb79d60d..c3230dcd64e62b 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -387,6 +387,24 @@ .once end + context 'when using nested values' do + let(:data) do + { before: 'before', nested: { key: 'value' } } + end + + before do + project_hook.custom_webhook_template = '{"before":"{{before}}","nested_key":"{{nested.key}}"}' + end + + it 'renders custom_webhook_template for body' do + service_instance.execute + + expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)) + .with(headers: headers, body: '{"before":"before","nested_key":"value"}') + .once + end + end + context 'when feature flag is disabled' do before do stub_feature_flags(custom_webhook_template: false) -- GitLab