From 23a8341b28be2df92795fde51fbc3b19eab0fc68 Mon Sep 17 00:00:00 2001 From: Natalie Tay Date: Thu, 3 Feb 2022 11:26:53 +0800 Subject: [PATCH] FEATURE: Validate domain settings for blocked_onebox_domain only (#15754) We want to prevent the user from adding ? or * minimally when setting domains in sitesettings --- .../components/site-settings/host-list.js | 21 +++++++++++++ .../components/site-settings/host-list.hbs | 11 ++++++- .../acceptance/admin-site-settings-test.js | 31 ++++++++++++++++++- .../discourse/tests/fixtures/site-settings.js | 14 ++++++++- .../components/select-kit/host-list-test.js | 31 +++++++++++++++++++ config/locales/server.en.yml | 3 +- config/site_settings.yml | 2 +- lib/site_settings/type_supervisor.rb | 2 ++ lib/validators/host_list_setting_validator.rb | 15 +++++++++ .../host_list_setting_validator_spec.rb | 31 +++++++++++++++++++ 10 files changed, 156 insertions(+), 5 deletions(-) create mode 100644 app/assets/javascripts/admin/addon/components/site-settings/host-list.js create mode 100644 app/assets/javascripts/discourse/tests/integration/components/select-kit/host-list-test.js create mode 100644 lib/validators/host_list_setting_validator.rb create mode 100644 spec/components/validators/host_list_setting_validator_spec.rb diff --git a/app/assets/javascripts/admin/addon/components/site-settings/host-list.js b/app/assets/javascripts/admin/addon/components/site-settings/host-list.js new file mode 100644 index 00000000000..8cc320da8f8 --- /dev/null +++ b/app/assets/javascripts/admin/addon/components/site-settings/host-list.js @@ -0,0 +1,21 @@ +import Component from "@ember/component"; +import { action, computed } from "@ember/object"; + +export default Component.extend({ + tokenSeparator: "|", + choices: null, + + @computed("value") + get settingValue() { + return this.value.toString().split(this.tokenSeparator).filter(Boolean); + }, + + @action + onChange(value) { + if (value.some((v) => v.includes("?") || v.includes("*"))) { + return; + } + + this.set("value", value.join(this.tokenSeparator)); + }, +}); diff --git a/app/assets/javascripts/admin/addon/templates/components/site-settings/host-list.hbs b/app/assets/javascripts/admin/addon/templates/components/site-settings/host-list.hbs index d1486292ddf..3460bf78df3 100644 --- a/app/assets/javascripts/admin/addon/templates/components/site-settings/host-list.hbs +++ b/app/assets/javascripts/admin/addon/templates/components/site-settings/host-list.hbs @@ -1,3 +1,12 @@ -{{value-list values=value addKey="admin.site_settings.add_host"}} +{{list-setting + value=settingValue + settingName=setting.setting + choices=settingValue + onChange=(action "onChange") + options=(hash + allowAny=allowAny + ) +}} + {{setting-validation-message message=validationMessage}}
{{html-safe setting.description}}
diff --git a/app/assets/javascripts/discourse/tests/acceptance/admin-site-settings-test.js b/app/assets/javascripts/discourse/tests/acceptance/admin-site-settings-test.js index d5ace605562..8a1c4458cc5 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/admin-site-settings-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/admin-site-settings-test.js @@ -13,6 +13,9 @@ import { } from "@ember/test-helpers"; import siteSettingFixture from "discourse/tests/fixtures/site-settings"; import { test } from "qunit"; +import pretender from "discourse/tests/helpers/create-pretender"; + +const ENTER_KEYCODE = 13; acceptance("Admin - Site Settings", function (needs) { let updatedTitle; @@ -105,7 +108,7 @@ acceptance("Admin - Site Settings", function (needs) { ); await fillIn(".input-setting-string", "Test"); - await triggerKeyEvent(".input-setting-string", "keydown", 13); // enter + await triggerKeyEvent(".input-setting-string", "keydown", ENTER_KEYCODE); assert.ok( exists(".row.setting.overridden"), "saving via Enter key marks setting as overriden" @@ -163,4 +166,30 @@ acceptance("Admin - Site Settings", function (needs) { "/admin/site_settings/category/all_results?filter=contact" ); }); + + test("filters * and ? for domain lists", async (assert) => { + pretender.put("/admin/site_settings/blocked_onebox_domains", () => [200]); + + await visit("/admin/site_settings"); + await fillIn("#setting-filter", "domains"); + + await click(".select-kit-header.multi-select-header"); + + await fillIn(".select-kit-filter input", "cat.?.domain"); + await triggerKeyEvent(".select-kit-filter input", "keydown", ENTER_KEYCODE); + + await fillIn(".select-kit-filter input", "*.domain"); + await triggerKeyEvent(".select-kit-filter input", "keydown", ENTER_KEYCODE); + + await fillIn(".select-kit-filter input", "proper.com"); + await triggerKeyEvent(".select-kit-filter input", "keydown", ENTER_KEYCODE); + + await click("button.ok"); + + assert.strictEqual( + pretender.handledRequests[pretender.handledRequests.length - 1] + .requestBody, + "blocked_onebox_domains=proper.com" + ); + }); }); diff --git a/app/assets/javascripts/discourse/tests/fixtures/site-settings.js b/app/assets/javascripts/discourse/tests/fixtures/site-settings.js index f5fca9c6613..8b3bbbee644 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/site-settings.js +++ b/app/assets/javascripts/discourse/tests/fixtures/site-settings.js @@ -64,7 +64,19 @@ export default { secret: false, type: "upload", plugin: "discourse-logo" - } + }, + { + category: "onebox", + default: "", + description: + "A list of domains that will never be oneboxed e.g. wikipedia.org\n(Wildcard symbols * ? not supported)", + placeholder: null, + preview: null, + secret: false, + setting: "blocked_onebox_domains", + type: "host_list", + value: "", + }, ], diags: { last_message_processed: null diff --git a/app/assets/javascripts/discourse/tests/integration/components/select-kit/host-list-test.js b/app/assets/javascripts/discourse/tests/integration/components/select-kit/host-list-test.js new file mode 100644 index 00000000000..dbb318176c0 --- /dev/null +++ b/app/assets/javascripts/discourse/tests/integration/components/select-kit/host-list-test.js @@ -0,0 +1,31 @@ +import componentTest, { + setupRenderingTest, +} from "discourse/tests/helpers/component-test"; +import { discourseModule, query } from "discourse/tests/helpers/qunit-helpers"; +import hbs from "htmlbars-inline-precompile"; + +discourseModule( + "Integration | Component | site-setting | host-list", + function (hooks) { + setupRenderingTest(hooks); + + componentTest("displays setting value", { + template: hbs`{{site-setting setting=setting}}`, + + beforeEach() { + this.set("setting", { + setting: "blocked_onebox_domains", + value: "a.com|b.com", + type: "host_list", + }); + }, + + async test(assert) { + assert.strictEqual( + query(".formated-selection").innerText, + "a.com, b.com" + ); + }, + }); + } +); diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 6b2a4ef2175..7dde1ebb06d 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1530,7 +1530,7 @@ en: show_pinned_excerpt_mobile: "Show excerpt on pinned topics in mobile view." show_pinned_excerpt_desktop: "Show excerpt on pinned topics in desktop view." post_onebox_maxlength: "Maximum length of a oneboxed Discourse post in characters." - blocked_onebox_domains: "A list of domains that will never be oneboxed." + blocked_onebox_domains: "A list of domains that will never be oneboxed e.g. wikipedia.org\n(Wildcard symbols * ? not supported)" allowed_inline_onebox_domains: "A list of domains that will be oneboxed in miniature form if linked without a title" enable_inline_onebox_on_all_domains: "Ignore inline_onebox_domain_allowlist site setting and allow inline onebox on all domains." force_custom_user_agent_hosts: "Hosts for which to use the custom onebox user agent on all requests. (Especially useful for hosts that limit access by user agent)." @@ -2365,6 +2365,7 @@ en: invalid_json: "Invalid JSON." invalid_reply_by_email_address: "Value must contain '%{reply_key}' and be different from the notification email." invalid_alternative_reply_by_email_addresses: "All values must contain '%{reply_key}' and be different from the notification email." + invalid_domain_hostname: "Must not include * or ? characters." pop3_polling_host_is_empty: "You must set a 'pop3 polling host' before enabling POP3 polling." pop3_polling_username_is_empty: "You must set a 'pop3 polling username' before enabling POP3 polling." pop3_polling_password_is_empty: "You must set a 'pop3 polling password' before enabling POP3 polling." diff --git a/config/site_settings.yml b/config/site_settings.yml index 897a2c6d0f1..168d3755779 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1700,7 +1700,7 @@ onebox: zh_TW: 200 blocked_onebox_domains: default: "" - type: list + type: host_list list_type: compact max_oneboxes_per_post: default: 50 diff --git a/lib/site_settings/type_supervisor.rb b/lib/site_settings/type_supervisor.rb index 428aadfe4c3..5c37f26c89f 100644 --- a/lib/site_settings/type_supervisor.rb +++ b/lib/site_settings/type_supervisor.rb @@ -269,6 +269,8 @@ class SiteSettings::TypeSupervisor RegexSettingValidator when self.class.types[:string], self.class.types[:list], self.class.types[:enum] StringSettingValidator + when self.class.types[:host_list] + HostListSettingValidator else nil end end diff --git a/lib/validators/host_list_setting_validator.rb b/lib/validators/host_list_setting_validator.rb new file mode 100644 index 00000000000..93d51aa2b03 --- /dev/null +++ b/lib/validators/host_list_setting_validator.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class HostListSettingValidator + def initialize(opts = {}) + @opts = opts + end + + def valid_value?(val) + val.exclude?("*") && val.exclude?("?") + end + + def error_message + I18n.t('site_settings.errors.invalid_domain_hostname') + end +end diff --git a/spec/components/validators/host_list_setting_validator_spec.rb b/spec/components/validators/host_list_setting_validator_spec.rb new file mode 100644 index 00000000000..afeaa8c5c4f --- /dev/null +++ b/spec/components/validators/host_list_setting_validator_spec.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe HostListSettingValidator do + subject(:validator) { described_class.new() } + + describe '#valid_value?' do + describe "returns false for values containing *" do + it { expect(validator.valid_value?("*")).to eq false } + it { expect(validator.valid_value?("**")).to eq false } + it { expect(validator.valid_value?(".*")).to eq false } + it { expect(validator.valid_value?("a")).to eq true } + end + + describe "returns false for values containing ?" do + it { expect(validator.valid_value?("?")).to eq false } + it { expect(validator.valid_value?("??")).to eq false } + it { expect(validator.valid_value?(".?")).to eq false } + it { expect(validator.valid_value?("a")).to eq true } + end + end + + describe "#error_message" do + it "returns invalid domain hostname error" do + expect(validator.error_message).to eq(I18n.t( + 'site_settings.errors.invalid_domain_hostname' + )) + end + end +end