From 68b4fe4cf876ba4838dfc797655a038d34d2b27f Mon Sep 17 00:00:00 2001 From: David Taylor Date: Tue, 1 Nov 2022 16:33:17 +0000 Subject: [PATCH] SECURITY: Expand and improve SSRF Protections (#18815) See https://github.com/discourse/discourse/security/advisories/GHSA-rcc5-28r3-23rr Co-authored-by: OsamaSayegh Co-authored-by: Daniel Waterworth --- Gemfile.lock | 2 +- .../addon/controllers/admin-web-hooks-show.js | 40 +-- .../controllers/modals/admin-install-theme.js | 52 ++-- .../templates/modal/admin-install-theme.hbs | 24 +- .../admin-install-theme-modal-test.js | 16 -- .../discourse/tests/acceptance/themes-test.js | 1 - app/controllers/admin/themes_controller.rb | 6 +- app/controllers/admin/web_hooks_controller.rb | 16 +- app/jobs/regular/emit_web_hook_event.rb | 37 +-- app/models/remote_theme.rb | 9 +- app/models/web_hook.rb | 18 ++ app/services/web_hook_emitter.rb | 57 +++++ config/initializers/000-zeitwerk.rb | 4 +- config/locales/client.en.yml | 2 - config/locales/server.en.yml | 5 + lib/final_destination.rb | 96 ++----- lib/final_destination/faraday_adapter.rb | 22 ++ lib/final_destination/http.rb | 40 +++ lib/final_destination/resolver.rb | 54 ++++ lib/final_destination/ssrf_detector.rb | 88 +++++++ lib/git_url.rb | 20 ++ lib/onebox/engine/google_maps_onebox.rb | 2 +- lib/onebox/helpers.rb | 4 +- lib/onebox/status_check.rb | 8 +- lib/theme_store/git_importer.rb | 120 ++++++--- spec/jobs/emit_web_hook_event_spec.rb | 26 +- spec/lib/cooked_post_processor_spec.rb | 1 - spec/lib/final_destination/http_spec.rb | 110 ++++++++ spec/lib/final_destination/resolver_spec.rb | 44 ++++ .../final_destination/ssrf_detector_spec.rb | 104 ++++++++ spec/lib/final_destination_spec.rb | 241 +++++++----------- spec/lib/git_url_spec.rb | 15 ++ spec/lib/onebox/helpers_spec.rb | 22 +- spec/lib/onebox/status_check_spec.rb | 6 + spec/lib/theme_store/git_importer_spec.rb | 51 +++- spec/models/remote_theme_spec.rb | 19 +- spec/models/web_hook_spec.rb | 29 +++ spec/requests/admin/themes_controller_spec.rb | 18 +- .../admin/web_hooks_controller_spec.rb | 58 +++++ spec/services/themes_spec.rb | 35 ++- spec/support/final_destination_helper.rb | 29 +++ spec/support/mock_git_importer.rb | 56 ++++ 42 files changed, 1164 insertions(+), 443 deletions(-) create mode 100644 app/services/web_hook_emitter.rb create mode 100644 lib/final_destination/faraday_adapter.rb create mode 100644 lib/final_destination/http.rb create mode 100644 lib/final_destination/resolver.rb create mode 100644 lib/final_destination/ssrf_detector.rb create mode 100644 lib/git_url.rb create mode 100644 spec/lib/final_destination/http_spec.rb create mode 100644 spec/lib/final_destination/resolver_spec.rb create mode 100644 spec/lib/final_destination/ssrf_detector_spec.rb create mode 100644 spec/lib/git_url_spec.rb create mode 100644 spec/support/final_destination_helper.rb create mode 100644 spec/support/mock_git_importer.rb diff --git a/Gemfile.lock b/Gemfile.lock index 1121400e09e..84fa798d487 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -244,7 +244,7 @@ GEM multi_json (1.15.0) multi_xml (0.6.0) mustache (1.1.1) - net-http (0.3.0) + net-http (0.2.2) uri net-imap (0.3.1) net-protocol diff --git a/app/assets/javascripts/admin/addon/controllers/admin-web-hooks-show.js b/app/assets/javascripts/admin/addon/controllers/admin-web-hooks-show.js index ba75e291991..f1e8caadcf8 100644 --- a/app/assets/javascripts/admin/addon/controllers/admin-web-hooks-show.js +++ b/app/assets/javascripts/admin/addon/controllers/admin-web-hooks-show.js @@ -3,8 +3,6 @@ import EmberObject from "@ember/object"; import I18n from "I18n"; import { alias } from "@ember/object/computed"; import discourseComputed from "discourse-common/utils/decorators"; -import { extractDomainFromUrl } from "discourse/lib/utilities"; -import { isAbsoluteURL } from "discourse-common/lib/get-url"; import { isEmpty } from "@ember/utils"; import { popupAjaxError } from "discourse/lib/ajax-error"; import { inject as service } from "@ember/service"; @@ -89,38 +87,20 @@ export default Controller.extend({ actions: { save() { this.set("saved", false); - const url = this.get("model.payload_url"); - const domain = extractDomainFromUrl(url); const model = this.model; const isNew = model.get("isNew"); - const saveWebHook = () => { - return model - .save() - .then(() => { - this.set("saved", true); - this.adminWebHooks.get("model").addObject(model); + return model + .save() + .then(() => { + this.set("saved", true); + this.adminWebHooks.get("model").addObject(model); - if (isNew) { - this.transitionToRoute("adminWebHooks.show", model.get("id")); - } - }) - .catch(popupAjaxError); - }; - - if ( - domain === "localhost" || - domain.match(/192\.168\.\d+\.\d+/) || - domain.match(/127\.\d+\.\d+\.\d+/) || - isAbsoluteURL(url) - ) { - return this.dialog.yesNoConfirm({ - message: I18n.t("admin.web_hooks.warn_local_payload_url"), - didConfirm: () => saveWebHook(), - }); - } - - return saveWebHook(); + if (isNew) { + this.transitionToRoute("adminWebHooks.show", model.get("id")); + } + }) + .catch(popupAjaxError); }, destroy() { diff --git a/app/assets/javascripts/admin/addon/controllers/modals/admin-install-theme.js b/app/assets/javascripts/admin/addon/controllers/modals/admin-install-theme.js index d42446ddbb1..a3553d8f7cb 100644 --- a/app/assets/javascripts/admin/addon/controllers/modals/admin-install-theme.js +++ b/app/assets/javascripts/admin/addon/controllers/modals/admin-install-theme.js @@ -31,6 +31,7 @@ export default Controller.extend(ModalFunctionality, { advancedVisible: false, selectedType: alias("themesController.currentTab"), component: equal("selectedType", COMPONENTS), + urlPlaceholder: "https://github.com/discourse/sample_theme", init() { this._super(...arguments); @@ -79,29 +80,6 @@ export default Controller.extend(ModalFunctionality, { ); }, - @discourseComputed("privateChecked") - urlPlaceholder(privateChecked) { - return privateChecked - ? "git@github.com:discourse/sample_theme.git" - : "https://github.com/discourse/sample_theme"; - }, - - @observes("privateChecked") - privateWasChecked() { - const checked = this.privateChecked; - if (checked && !this._keyLoading) { - this._keyLoading = true; - ajax(this.keyGenUrl, { type: "POST" }) - .then((pair) => { - this.set("publicKey", pair.public_key); - }) - .catch(popupAjaxError) - .finally(() => { - this._keyLoading = false; - }); - } - }, - @discourseComputed("name") nameTooShort(name) { return !name || name.length < MIN_NAME_LENGTH; @@ -116,6 +94,22 @@ export default Controller.extend(ModalFunctionality, { } }, + @observes("checkPrivate") + privateWasChecked() { + const checked = this.checkPrivate; + if (checked && !this._keyLoading && !this.publicKey) { + this._keyLoading = true; + ajax(this.keyGenUrl, { type: "POST" }) + .then((pair) => { + this.set("publicKey", pair.public_key); + }) + .catch(popupAjaxError) + .finally(() => { + this._keyLoading = false; + }); + } + }, + @discourseComputed("selection", "themeCannotBeInstalled") submitLabel(selection, themeCannotBeInstalled) { if (themeCannotBeInstalled) { @@ -127,15 +121,14 @@ export default Controller.extend(ModalFunctionality, { }`; }, - @discourseComputed("privateChecked", "checkPrivate", "publicKey") - showPublicKey(privateChecked, checkPrivate, publicKey) { - return privateChecked && checkPrivate && publicKey; + @discourseComputed("checkPrivate", "publicKey") + showPublicKey(checkPrivate, publicKey) { + return checkPrivate && publicKey; }, onClose() { this.setProperties({ duplicateRemoteThemeWarning: null, - privateChecked: false, localFile: null, uploadUrl: null, publicKey: null, @@ -209,11 +202,8 @@ export default Controller.extend(ModalFunctionality, { options.data = { remote: this.uploadUrl, branch: this.branch, + public_key: this.publicKey, }; - - if (this.privateChecked) { - options.data.public_key = this.publicKey; - } } // User knows that theme cannot be installed, but they want to continue diff --git a/app/assets/javascripts/admin/addon/templates/modal/admin-install-theme.hbs b/app/assets/javascripts/admin/addon/templates/modal/admin-install-theme.hbs index cebb9a6121f..5f0f0c5413c 100644 --- a/app/assets/javascripts/admin/addon/templates/modal/admin-install-theme.hbs +++ b/app/assets/javascripts/admin/addon/templates/modal/admin-install-theme.hbs @@ -61,25 +61,15 @@
{{i18n "admin.customize.theme.remote_branch"}}
+ {{/if}} -
- -
- {{#if this.showPublicKey}} -
-
{{i18n "admin.customize.theme.public_key"}}
-
-