[Actions] Removed double parsing when passing action url for validati…

…on (elastic#87928) (elastic#88729) * Removed double parsing when passing action url for validation * Fixing functional test * PR fixes * PR fixes Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
thomasneirynck · Jan 19, 2021 · 97d0874 · 97d0874
1 parent 0e4f495
commit 97d0874
Show file tree

Hide file tree

Showing 7 changed files with 21 additions and 22 deletions.
diff --git 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<boolean>(() => 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
@@ -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
@@ -70,7 +70,7 @@ export function getActionType({
     }),
     validate: {
       secrets: schema.object(secretsSchemaProps, {
-        validate: curry(valdiateActionTypeConfig)(configurationUtilities),
+        validate: curry(validateActionTypeConfig)(configurationUtilities),
       }),
       params: ParamsSchema,
     },
@@ -88,21 +88,21 @@ 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',
     });
   }
 
   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
@@ -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
@@ -71,17 +71,17 @@ 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',
     });
   }
 
   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
@@ -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/.../alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/slack.ts b/.../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`,
           });
         });
     });