[ResponseOps][Actions] Should trim values before validation in UI and API (#136840)

* Updating action create function to trim values before validation

* Adding tests

* Removes trimming and adds validation to the api layer

* Adding lint changes

* Updating functional tests

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

x-pack/plugins/actions/server/lib/index.ts

@@ -33,3 +33,4 @@ export {
   asHttpRequestExecutionSource,
   isHttpRequestExecutionSource,
 } from './action_execution_source';
+export { validateEmptyStrings } from './validate_empty_strings';

x-pack/plugins/actions/server/lib/validate_empty_strings.test.ts

@@ -0,0 +1,37 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0; you may not use this file except in compliance with the Elastic License
+ * 2.0.
+ */
+
+import { validateEmptyStrings } from './validate_empty_strings';
+
+describe('validateEmptyStrings', () => {
+  const action = {
+    name: 'my name',
+    actionTypeId: 'my-action-type',
+    config: {
+      param1: ' ',
+    },
+    secrets: {
+      param1: [' ', 'param1'],
+    },
+  };
+
+  it('should not throw an error if the trimmed string is not empty', () => {
+    expect(() => validateEmptyStrings(action.name)).not.toThrow();
+  });
+
+  it('should throw an error if the trimmed strings in an array are empty', () => {
+    expect(() => validateEmptyStrings(action.secrets)).toThrowErrorMatchingInlineSnapshot(
+      `"value '' is not valid"`
+    );
+  });
+
+  it('should throw an error if the trimmed strings in an object are empty', () => {
+    expect(() => validateEmptyStrings(action)).toThrowErrorMatchingInlineSnapshot(
+      `"value '' is not valid"`
+    );
+  });
+});

x-pack/plugins/actions/server/lib/validate_empty_strings.ts

@@ -0,0 +1,24 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0; you may not use this file except in compliance with the Elastic License
+ * 2.0.
+ */
+
+import { isObject, keys, get, isString } from 'lodash';
+
+export function validateEmptyStrings(value: unknown) {
+  if (isString(value)) {
+    if (value.trim() === '') {
+      throw new Error(`value '' is not valid`);
+    }
+  } else if (Array.isArray(value)) {
+    value.forEach((item) => {
+      validateEmptyStrings(item);
+    });
+  } else if (isObject(value)) {
+    keys(value).forEach((key) => {
+      validateEmptyStrings(get(value, key));
+    });
+  }
+}

x-pack/plugins/actions/server/routes/create.test.ts

@@ -5,7 +5,7 @@
  * 2.0.
  */
 
-import { createActionRoute } from './create';
+import { createActionRoute, bodySchema } from './create';
 import { httpServiceMock } from '@kbn/core/server/mocks';
 import { licenseStateMock } from '../lib/license_state.mock';
 import { mockHandlerArguments } from './legacy/_mock_handler_arguments';
@@ -168,4 +168,16 @@ describe('createActionRoute', () => {
 
     expect(handler(context, req, res)).rejects.toMatchInlineSnapshot(`[Error: OMG]`);
   });
+
+  test('validates body to prevent empty strings', async () => {
+    const body = {
+      name: 'My name',
+      connector_type_id: 'abc',
+      config: { foo: ' ' },
+      secrets: {},
+    };
+    expect(() => bodySchema.validate(body)).toThrowErrorMatchingInlineSnapshot(
+      `"[config.foo]: value '' is not valid"`
+    );
+  });
 });

x-pack/plugins/actions/server/routes/create.ts

@@ -8,16 +8,20 @@
 import { schema } from '@kbn/config-schema';
 import { IRouter } from '@kbn/core/server';
 import { ActionResult, ActionsRequestHandlerContext } from '../types';
-import { ILicenseState } from '../lib';
+import { ILicenseState, validateEmptyStrings } from '../lib';
 import { BASE_ACTION_API_PATH, RewriteRequestCase, RewriteResponseCase } from '../../common';
 import { verifyAccessAndContext } from './verify_access_and_context';
 import { CreateOptions } from '../actions_client';
 
 export const bodySchema = schema.object({
-  name: schema.string(),
-  connector_type_id: schema.string(),
-  config: schema.recordOf(schema.string(), schema.any(), { defaultValue: {} }),
-  secrets: schema.recordOf(schema.string(), schema.any(), { defaultValue: {} }),
+  name: schema.string({ validate: validateEmptyStrings }),
+  connector_type_id: schema.string({ validate: validateEmptyStrings }),
+  config: schema.recordOf(schema.string(), schema.any({ validate: validateEmptyStrings }), {
+    defaultValue: {},
+  }),
+  secrets: schema.recordOf(schema.string(), schema.any({ validate: validateEmptyStrings }), {
+    defaultValue: {},
+  }),
 });
 
 const rewriteBodyReq: RewriteRequestCase<CreateOptions['action']> = ({

x-pack/plugins/actions/server/routes/update.test.ts

@@ -5,7 +5,7 @@
  * 2.0.
  */
 
-import { updateActionRoute } from './update';
+import { bodySchema, updateActionRoute } from './update';
 import { httpServiceMock } from '@kbn/core/server/mocks';
 import { licenseStateMock } from '../lib/license_state.mock';
 import { mockHandlerArguments } from './legacy/_mock_handler_arguments';
@@ -174,4 +174,15 @@ describe('updateActionRoute', () => {
 
     expect(verifyAccessAndContext).toHaveBeenCalledWith(licenseState, expect.any(Function));
   });
+
+  test('validates body to prevent empty strings', async () => {
+    const body = {
+      name: ' ',
+      config: { foo: true },
+      secrets: { key: 'i8oh34yf9783y39' },
+    };
+    expect(() => bodySchema.validate(body)).toThrowErrorMatchingInlineSnapshot(
+      `"[name]: value '' is not valid"`
+    );
+  });
 });

x-pack/plugins/actions/server/routes/update.ts

@@ -7,7 +7,7 @@
 
 import { schema } from '@kbn/config-schema';
 import { IRouter } from '@kbn/core/server';
-import { ILicenseState } from '../lib';
+import { ILicenseState, validateEmptyStrings } from '../lib';
 import { BASE_ACTION_API_PATH, RewriteResponseCase } from '../../common';
 import { ActionResult, ActionsRequestHandlerContext } from '../types';
 import { verifyAccessAndContext } from './verify_access_and_context';
@@ -16,10 +16,14 @@ const paramSchema = schema.object({
   id: schema.string(),
 });
 
-const bodySchema = schema.object({
-  name: schema.string(),
-  config: schema.recordOf(schema.string(), schema.any(), { defaultValue: {} }),
-  secrets: schema.recordOf(schema.string(), schema.any(), { defaultValue: {} }),
+export const bodySchema = schema.object({
+  name: schema.string({ validate: validateEmptyStrings }),
+  config: schema.recordOf(schema.string(), schema.any({ validate: validateEmptyStrings }), {
+    defaultValue: {},
+  }),
+  secrets: schema.recordOf(schema.string(), schema.any({ validate: validateEmptyStrings }), {
+    defaultValue: {},
+  }),
 });
 
 const rewriteBodyRes: RewriteResponseCase<ActionResult> = ({

x-pack/test/alerting_api_integration/security_and_spaces/group2/tests/actions/create.ts

@@ -224,6 +224,39 @@ export default function createActionTests({ getService }: FtrProviderContext) {
               throw new Error(`Scenario untested: ${JSON.stringify(scenario)}`);
           }
         });
+
+        it(`should handle create action request appropriately when empty strings are submitted`, async () => {
+          const response = await supertestWithoutAuth
+            .post(`${getUrlPrefix(space.id)}/api/actions/connector`)
+            .set('kbn-xsrf', 'foo')
+            .auth(user.username, user.password)
+            .send({
+              name: 'my name',
+              connector_type_id: 'test.index-record',
+              config: {
+                encrypted: ' ',
+              },
+            });
+
+          switch (scenario.id) {
+            case 'no_kibana_privileges at space1':
+            case 'global_read at space1':
+            case 'space_1_all_alerts_none_actions at space1':
+            case 'space_1_all at space2':
+            case 'superuser at space1':
+            case 'space_1_all at space1':
+            case 'space_1_all_with_restricted_fixture at space1':
+              expect(response.statusCode).to.eql(400);
+              expect(response.body).to.eql({
+                statusCode: 400,
+                error: 'Bad Request',
+                message: `[request body.config.encrypted]: value '' is not valid`,
+              });
+              break;
+            default:
+              throw new Error(`Scenario untested: ${JSON.stringify(scenario)}`);
+          }
+        });
       });
     }
   });

x-pack/test/alerting_api_integration/security_and_spaces/group2/tests/actions/update.ts

@@ -351,6 +351,41 @@ export default function updateActionTests({ getService }: FtrProviderContext) {
               throw new Error(`Scenario untested: ${JSON.stringify(scenario)}`);
           }
         });
+
+        it(`should handle update action request appropriately when empty strings are submitted`, async () => {
+          const response = await supertestWithoutAuth
+            .put(`${getUrlPrefix(space.id)}/api/actions/connector/1`)
+            .set('kbn-xsrf', 'foo')
+            .auth(user.username, user.password)
+            .send({
+              name: 'My action updated',
+              config: {
+                unencrypted: ' ',
+              },
+              secrets: {
+                encrypted: 'This value should be encrypted',
+              },
+            });
+
+          switch (scenario.id) {
+            case 'no_kibana_privileges at space1':
+            case 'space_1_all_alerts_none_actions at space1':
+            case 'space_1_all at space2':
+            case 'global_read at space1':
+            case 'superuser at space1':
+            case 'space_1_all at space1':
+            case 'space_1_all_with_restricted_fixture at space1':
+              expect(response.statusCode).to.eql(400);
+              expect(response.body).to.eql({
+                statusCode: 400,
+                error: 'Bad Request',
+                message: `[request body.config.unencrypted]: value '' is not valid`,
+              });
+              break;
+            default:
+              throw new Error(`Scenario untested: ${JSON.stringify(scenario)}`);
+          }
+        });
       });
     }
   });

x-pack/test/alerting_api_integration/spaces_only/tests/actions/create.ts

@@ -58,6 +58,27 @@ export default function createActionTests({ getService }: FtrProviderContext) {
       });
     });
 
+    it('should handle create action request appropriately when empty strings are submitted', async () => {
+      await supertest
+        .post(`${getUrlPrefix(Spaces.space1.id)}/api/actions/connector`)
+        .set('kbn-xsrf', 'foo')
+        .send({
+          name: 'My action',
+          connector_type_id: 'test.index-record',
+          config: {
+            unencrypted: ' ',
+          },
+          secrets: {
+            encrypted: 'This value should be encrypted',
+          },
+        })
+        .expect(400, {
+          statusCode: 400,
+          error: 'Bad Request',
+          message: `[request body.config.unencrypted]: value '' is not valid`,
+        });
+    });
+
     describe('legacy', () => {
       it('should handle create action request appropriately', async () => {
         const response = await supertest

x-pack/test/alerting_api_integration/spaces_only/tests/actions/update.ts

@@ -161,6 +161,26 @@ export default function updateActionTests({ getService }: FtrProviderContext) {
       expect(new Date(noopFeature.last_used).getTime()).to.be.greaterThan(updateStart.getTime());
     });
 
+    it('should handle update action request appropriately when empty strings are submitted', async () => {
+      await supertest
+        .put(`${getUrlPrefix(Spaces.space1.id)}/api/actions/connector/custom-system-abc-connector`)
+        .set('kbn-xsrf', 'foo')
+        .send({
+          name: ' ',
+          config: {
+            unencrypted: `This value shouldn't get encrypted`,
+          },
+          secrets: {
+            encrypted: 'This value should be encrypted',
+          },
+        })
+        .expect(400, {
+          statusCode: 400,
+          error: 'Bad Request',
+          message: `[request body.name]: value '' is not valid`,
+        });
+    });
+
     describe('legacy', () => {
       it('should handle update action request appropriately', async () => {
         const { body: createdAction } = await supertest