From 97d0874b8d24bdd4f52bd1547165b2db048cccee Mon Sep 17 00:00:00 2001 From: ymao1 Date: Tue, 19 Jan 2021 16:24:47 -0500 Subject: [PATCH] [Actions] Removed double parsing when passing action url for validation (#87928) (#88729) * Removed double parsing when passing action url for validation * Fixing functional test * PR fixes * PR fixes Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> --- x-pack/plugins/actions/server/actions_config.ts | 14 +++++++------- .../server/builtin_action_types/slack.test.ts | 2 +- .../actions/server/builtin_action_types/slack.ts | 10 +++++----- .../server/builtin_action_types/teams.test.ts | 2 +- .../actions/server/builtin_action_types/teams.ts | 6 +++--- .../actions/server/builtin_action_types/webhook.ts | 6 +++--- .../tests/actions/builtin_action_types/slack.ts | 3 +-- 7 files changed, 21 insertions(+), 22 deletions(-) diff --git a/x-pack/plugins/actions/server/actions_config.ts b/x-pack/plugins/actions/server/actions_config.ts index 609e4969222f9..ebac80e70f4a8 100644 --- a/x-pack/plugins/actions/server/actions_config.ts +++ b/x-pack/plugins/actions/server/actions_config.ts @@ -6,7 +6,7 @@ import { i18n } from '@kbn/i18n'; import { tryCatch, map, mapNullable, getOrElse } from 'fp-ts/lib/Option'; -import { URL } from 'url'; +import url from 'url'; import { curry } from 'lodash'; import { pipe } from 'fp-ts/lib/pipeable'; @@ -22,7 +22,7 @@ export enum EnabledActionTypes { } enum AllowListingField { - url = 'url', + URL = 'url', hostname = 'hostname', } @@ -56,17 +56,17 @@ function disabledActionTypeErrorMessage(actionType: string) { }); } -function isAllowed({ allowedHosts }: ActionsConfigType, hostname: string): boolean { +function isAllowed({ allowedHosts }: ActionsConfigType, hostname: string | null): boolean { const allowed = new Set(allowedHosts); if (allowed.has(AllowedHosts.Any)) return true; - if (allowed.has(hostname)) return true; + if (hostname && allowed.has(hostname)) return true; return false; } function isHostnameAllowedInUri(config: ActionsConfigType, uri: string): boolean { return pipe( - tryCatch(() => new URL(uri)), - map((url) => url.hostname), + tryCatch(() => url.parse(uri)), + map((parsedUrl) => parsedUrl.hostname), mapNullable((hostname) => isAllowed(config, hostname)), getOrElse(() => false) ); @@ -94,7 +94,7 @@ export function getActionsConfigurationUtilities( isActionTypeEnabled, ensureUriAllowed(uri: string) { if (!isUriAllowed(uri)) { - throw new Error(allowListErrorMessage(AllowListingField.url, uri)); + throw new Error(allowListErrorMessage(AllowListingField.URL, uri)); } }, ensureHostnameAllowed(hostname: string) { diff --git a/x-pack/plugins/actions/server/builtin_action_types/slack.test.ts b/x-pack/plugins/actions/server/builtin_action_types/slack.test.ts index cc2c0eda76f52..e73f8d91b0847 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/slack.test.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/slack.test.ts @@ -117,7 +117,7 @@ describe('validateActionTypeSecrets()', () => { logger: mockedLogger, configurationUtilities: { ...actionsConfigMock.create(), - ensureHostnameAllowed: () => { + ensureUriAllowed: () => { throw new Error(`target hostname is not added to allowedHosts`); }, }, diff --git a/x-pack/plugins/actions/server/builtin_action_types/slack.ts b/x-pack/plugins/actions/server/builtin_action_types/slack.ts index 07ea7b62f3606..02026eb729727 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/slack.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/slack.ts @@ -70,7 +70,7 @@ export function getActionType({ }), validate: { secrets: schema.object(secretsSchemaProps, { - validate: curry(valdiateActionTypeConfig)(configurationUtilities), + validate: curry(validateActionTypeConfig)(configurationUtilities), }), params: ParamsSchema, }, @@ -88,13 +88,13 @@ function renderParameterTemplates( }; } -function valdiateActionTypeConfig( +function validateActionTypeConfig( configurationUtilities: ActionsConfigurationUtilities, secretsObject: ActionTypeSecretsType ) { - let url: URL; + const configuredUrl = secretsObject.webhookUrl; try { - url = new URL(secretsObject.webhookUrl); + new URL(configuredUrl); } catch (err) { return i18n.translate('xpack.actions.builtin.slack.slackConfigurationErrorNoHostname', { defaultMessage: 'error configuring slack action: unable to parse host name from webhookUrl', @@ -102,7 +102,7 @@ function valdiateActionTypeConfig( } try { - configurationUtilities.ensureHostnameAllowed(url.hostname); + configurationUtilities.ensureUriAllowed(configuredUrl); } catch (allowListError) { return i18n.translate('xpack.actions.builtin.slack.slackConfigurationError', { defaultMessage: 'error configuring slack action: {message}', diff --git a/x-pack/plugins/actions/server/builtin_action_types/teams.test.ts b/x-pack/plugins/actions/server/builtin_action_types/teams.test.ts index ffa7c778c0489..a9fce2f0a9ebf 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/teams.test.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/teams.test.ts @@ -117,7 +117,7 @@ describe('validateActionTypeSecrets()', () => { logger: mockedLogger, configurationUtilities: { ...actionsConfigMock.create(), - ensureHostnameAllowed: () => { + ensureUriAllowed: () => { throw new Error(`target hostname is not added to allowedHosts`); }, }, diff --git a/x-pack/plugins/actions/server/builtin_action_types/teams.ts b/x-pack/plugins/actions/server/builtin_action_types/teams.ts index 8575ae75d1e6c..088e30db4e3ce 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/teams.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/teams.ts @@ -71,9 +71,9 @@ function validateActionTypeConfig( configurationUtilities: ActionsConfigurationUtilities, secretsObject: ActionTypeSecretsType ) { - let url: URL; + const configuredUrl = secretsObject.webhookUrl; try { - url = new URL(secretsObject.webhookUrl); + new URL(configuredUrl); } catch (err) { return i18n.translate('xpack.actions.builtin.teams.teamsConfigurationErrorNoHostname', { defaultMessage: 'error configuring teams action: unable to parse host name from webhookUrl', @@ -81,7 +81,7 @@ function validateActionTypeConfig( } try { - configurationUtilities.ensureHostnameAllowed(url.hostname); + configurationUtilities.ensureUriAllowed(configuredUrl); } catch (allowListError) { return i18n.translate('xpack.actions.builtin.teams.teamsConfigurationError', { defaultMessage: 'error configuring teams action: {message}', diff --git a/x-pack/plugins/actions/server/builtin_action_types/webhook.ts b/x-pack/plugins/actions/server/builtin_action_types/webhook.ts index 4479f7c69bebb..fa6d2663c94ab 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/webhook.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/webhook.ts @@ -112,9 +112,9 @@ function validateActionTypeConfig( configurationUtilities: ActionsConfigurationUtilities, configObject: ActionTypeConfigType ) { - let url: URL; + const configuredUrl = configObject.url; try { - url = new URL(configObject.url); + new URL(configuredUrl); } catch (err) { return i18n.translate('xpack.actions.builtin.webhook.webhookConfigurationErrorNoHostname', { defaultMessage: 'error configuring webhook action: unable to parse url: {err}', @@ -125,7 +125,7 @@ function validateActionTypeConfig( } try { - configurationUtilities.ensureUriAllowed(url.toString()); + configurationUtilities.ensureUriAllowed(configuredUrl); } catch (allowListError) { return i18n.translate('xpack.actions.builtin.webhook.webhookConfigurationError', { defaultMessage: 'error configuring webhook action: {message}', diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/slack.ts b/x-pack/test/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/slack.ts index 83ad17757f3a6..24174acef98dc 100644 --- a/x-pack/test/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/slack.ts +++ b/x-pack/test/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/slack.ts @@ -114,8 +114,7 @@ export default function slackTest({ getService }: FtrProviderContext) { expect(resp.body).to.eql({ statusCode: 400, error: 'Bad Request', - message: - 'error validating action type secrets: error configuring slack action: target hostname "slack.mynonexistent.com" is not added to the Kibana config xpack.actions.allowedHosts', + message: `error validating action type secrets: error configuring slack action: target url \"http://slack.mynonexistent.com/other/stuff/in/the/path\" is not added to the Kibana config xpack.actions.allowedHosts`, }); }); });