Enforce correct shape for SO attributes and id during create operatio…

…ns (elastic#187876)

## Summary

Fix elastic#123575
Fix elastic#105039

This PR does two things:
- adapt SO ID validation to block empty strings (`""`), we we were
already doing with `undefined`
- add validation of the `attributes` to reject primitives and
`undefined` (only accept objects)

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/update.test.ts

@@ -149,15 +149,15 @@ describe('#update', () => {
       it(`should use the ES get action then index action when type is not multi-namespace for existing objects`, async () => {
         const type = 'index-pattern';
         const id = 'logstash-*';
-        migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc, migrated: true }));
+        migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc }));
         await updateSuccess(client, repository, registry, type, id, attributes, { namespace });
         expect(client.get).toHaveBeenCalledTimes(1);
         expect(mockPreflightCheckForCreate).not.toHaveBeenCalled();
         expect(client.index).toHaveBeenCalledTimes(1);
       });
 
       it(`should use the ES get action then index action when type is multi-namespace for existing objects`, async () => {
-        migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc, migrated: true }));
+        migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc }));
         await updateSuccess(
           client,
           repository,
@@ -172,15 +172,15 @@ describe('#update', () => {
       });
 
       it(`should use the ES get action then index action when type is namespace agnostic for existing objects`, async () => {
-        migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc, migrated: true }));
+        migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc }));
         await updateSuccess(client, repository, registry, NAMESPACE_AGNOSTIC_TYPE, id, attributes);
         expect(client.get).toHaveBeenCalledTimes(1);
         expect(mockPreflightCheckForCreate).not.toHaveBeenCalled();
         expect(client.index).toHaveBeenCalledTimes(1);
       });
 
       it(`should use the ES index action with the merged attributes when mergeAttributes is not false`, async () => {
-        migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc, migrated: true }));
+        migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc }));
 
         await updateSuccess(client, repository, registry, NAMESPACE_AGNOSTIC_TYPE, id, {
           foo: 'bar',
@@ -201,7 +201,7 @@ describe('#update', () => {
       });
 
       it(`should use the ES index action only with the provided attributes when mergeAttributes is false`, async () => {
-        migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc, migrated: true }));
+        migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc }));
 
         await updateSuccess(
           client,
@@ -229,7 +229,7 @@ describe('#update', () => {
       });
 
       it(`should check for alias conflicts if a new multi-namespace object before create action would be created then create action to create the object`, async () => {
-        migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc, migrated: true }));
+        migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc }));
         await updateSuccess(
           client,
           repository,
@@ -246,7 +246,7 @@ describe('#update', () => {
       });
 
       it(`defaults to empty array with no input references`, async () => {
-        migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc, migrated: true }));
+        migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc }));
         await updateSuccess(client, repository, registry, type, id, attributes);
         expect(
           (client.index.mock.calls[0][0] as estypes.CreateRequest<SavedObjectsRawDocSource>).body!
@@ -256,7 +256,7 @@ describe('#update', () => {
 
       it(`accepts custom references array 1`, async () => {
         const test = async (references: SavedObjectReference[]) => {
-          migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc, migrated: true }));
+          migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc }));
           await updateSuccess(client, repository, registry, type, id, attributes, {
             references,
           });
@@ -271,7 +271,7 @@ describe('#update', () => {
 
       it(`accepts custom references array 2`, async () => {
         const test = async (references: SavedObjectReference[]) => {
-          migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc, migrated: true }));
+          migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc }));
           await updateSuccess(client, repository, registry, type, id, attributes, {
             references,
           });
@@ -286,7 +286,7 @@ describe('#update', () => {
 
       it(`accepts custom references array 3`, async () => {
         const test = async (references: SavedObjectReference[]) => {
-          migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc, migrated: true }));
+          migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc }));
           await updateSuccess(client, repository, registry, type, id, attributes, {
             references,
           });
@@ -300,7 +300,7 @@ describe('#update', () => {
       });
 
       it(`uses the 'upsertAttributes' option when specified for a single-namespace type that does not exist`, async () => {
-        migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc, migrated: true }));
+        migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc }));
         await updateSuccess(
           client,
           repository,
@@ -329,7 +329,7 @@ describe('#update', () => {
 
       it(`uses the 'upsertAttributes' option when specified for a multi-namespace type that does not exist`, async () => {
         const options = { upsert: { title: 'foo', description: 'bar' } };
-        migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc, migrated: true }));
+        migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc }));
         await updateSuccess(
           client,
           repository,
@@ -363,7 +363,7 @@ describe('#update', () => {
       it(`ignores the 'upsertAttributes' option when specified for a multi-namespace type that already exists`, async () => {
         // attributes don't change
         const options = { upsert: { title: 'foo', description: 'bar' } };
-        migrator.migrateDocument.mockImplementation((doc) => ({ ...doc, migrated: true }));
+        migrator.migrateDocument.mockImplementation((doc) => ({ ...doc }));
         await updateSuccess(
           client,
           repository,
@@ -700,7 +700,7 @@ describe('#update', () => {
       it('migrates the fetched document from get', async () => {
         const type = 'index-pattern';
         const id = 'logstash-*';
-        migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc, migrated: true }));
+        migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc }));
         await updateSuccess(client, repository, registry, type, id, attributes);
         expect(migrator.migrateDocument).toHaveBeenCalledTimes(2);
         expectMigrationArgs({

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.encryption_extension.test.ts

@@ -116,6 +116,7 @@ describe('SavedObjectsRepository Encryption Extension', () => {
 
     // create a mock saved objects encryption extension
     mockEncryptionExt = savedObjectsExtensionsMock.createEncryptionExtension();
+    mockEncryptionExt.encryptAttributes.mockImplementation((desc, attrs) => Promise.resolve(attrs));
 
     mockGetCurrentTime.mockReturnValue(mockTimestamp);
     mockGetSearchDsl.mockClear();
@@ -247,7 +248,6 @@ describe('SavedObjectsRepository Encryption Extension', () => {
         expect.objectContaining({
           ...encryptedSO,
           id: expect.objectContaining(/index-pattern:[0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12}/),
-          attributes: undefined,
         }),
         encryptedSO.attributes // original attributes
       );

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.spaces_extension.test.ts

@@ -1259,6 +1259,9 @@ describe('SavedObjectsRepository Spaces Extension', () => {
       serializer = createSpySerializer(registry);
       mockSpacesExt = savedObjectsExtensionsMock.createSpacesExtension();
       mockEncryptionExt = savedObjectsExtensionsMock.createEncryptionExtension();
+      mockEncryptionExt.encryptAttributes.mockImplementation((desc, attributes) =>
+        Promise.resolve(attributes)
+      );
       mockGetCurrentTime.mockReturnValue(mockTimestamp);
       mockGetSearchDsl.mockClear();
       repository = instantiateRepository();

packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/schema.test.ts

@@ -42,6 +42,15 @@ describe('Saved Objects type validation schema', () => {
     );
   });
 
+  it('should fail if invalid id is provided', () => {
+    const objectSchema = createSavedObjectSanitizedDocSchema(validationMap['1.0.0']);
+    const data = createMockObject({ foo: 'bar' });
+    data.id = '';
+    expect(() => objectSchema.validate(data)).toThrowErrorMatchingInlineSnapshot(
+      `"[id]: value has length [0] but it must have a minimum length of [1]."`
+    );
+  });
+
   it('should validate top-level properties', () => {
     const objectSchema = createSavedObjectSanitizedDocSchema(validationMap['1.0.0']);
     const data = createMockObject({ foo: 'heya' });
@@ -78,4 +87,31 @@ describe('Saved Objects type validation schema', () => {
       `"[id]: expected value of type [string] but got [boolean]"`
     );
   });
+
+  describe('default schema', () => {
+    it('validates a record of attributes', () => {
+      const objectSchema = createSavedObjectSanitizedDocSchema(undefined);
+      const data = createMockObject({ foo: 'heya' });
+
+      expect(() => objectSchema.validate(data)).not.toThrowError();
+    });
+
+    it('fails validation on undefined attributes', () => {
+      const objectSchema = createSavedObjectSanitizedDocSchema(undefined);
+      const data = createMockObject(undefined);
+
+      expect(() => objectSchema.validate(data)).toThrowErrorMatchingInlineSnapshot(
+        `"[attributes]: expected value of type [object] but got [undefined]"`
+      );
+    });
+
+    it('fails validation on primitive attributes', () => {
+      const objectSchema = createSavedObjectSanitizedDocSchema(undefined);
+      const data = createMockObject(42);
+
+      expect(() => objectSchema.validate(data)).toThrowErrorMatchingInlineSnapshot(
+        `"[attributes]: expected value of type [object] but got [number]"`
+      );
+    });
+  });
 });

packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/schema.ts

@@ -20,7 +20,7 @@ type SavedObjectSanitizedDocSchema = {
 };
 
 const baseSchema = schema.object<SavedObjectSanitizedDocSchema>({
-  id: schema.string(),
+  id: schema.string({ minLength: 1 }),
   type: schema.string(),
   references: schema.arrayOf(
     schema.object({
@@ -42,7 +42,7 @@ const baseSchema = schema.object<SavedObjectSanitizedDocSchema>({
   version: schema.maybe(schema.string()),
   originId: schema.maybe(schema.string()),
   managed: schema.maybe(schema.boolean()),
-  attributes: schema.maybe(schema.any()),
+  attributes: schema.recordOf(schema.string(), schema.maybe(schema.any())),
 });
 
 /**
@@ -52,9 +52,13 @@ const baseSchema = schema.object<SavedObjectSanitizedDocSchema>({
  * @internal
  */
 export const createSavedObjectSanitizedDocSchema = (
-  attributesSchema: SavedObjectsValidationSpec
+  attributesSchema: SavedObjectsValidationSpec | undefined
 ) => {
-  return baseSchema.extends({
-    attributes: attributesSchema,
-  });
+  if (attributesSchema) {
+    return baseSchema.extends({
+      attributes: attributesSchema,
+    });
+  } else {
+    return baseSchema;
+  }
 };

packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts

@@ -88,6 +88,34 @@ describe('Saved Objects type validator', () => {
       const data = createMockObject({ attributes: { foo: 'hi' } });
       expect(() => validator.validate(data)).not.toThrowError();
     });
+
+    it('validates attributes for types without defined schemas', () => {
+      validator = new SavedObjectsTypeValidator({
+        logger,
+        type,
+        validationMap: {},
+        defaultVersion,
+      });
+      const data = createMockObject({ attributes: undefined });
+      expect(() => validator.validate(data)).toThrowErrorMatchingInlineSnapshot(
+        `"[attributes]: expected value of type [object] but got [undefined]"`
+      );
+    });
+
+    it('validates top level properties for types without defined schemas', () => {
+      validator = new SavedObjectsTypeValidator({
+        logger,
+        type,
+        validationMap: {},
+        defaultVersion,
+      });
+      const data = createMockObject({ attributes: { foo: 'bar' } });
+      // @ts-expect-error Intentionally malformed object
+      data.updated_at = false;
+      expect(() => validator.validate(data)).toThrowErrorMatchingInlineSnapshot(
+        `"[updated_at]: expected value of type [string] but got [boolean]"`
+      );
+    });
   });
 
   describe('schema selection', () => {

packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts

@@ -9,6 +9,7 @@
 import Semver from 'semver';
 import type { Logger } from '@kbn/logging';
 import type {
+  SavedObjectsValidationSpec,
   SavedObjectsValidationMap,
   SavedObjectSanitizedDoc,
 } from '@kbn/core-saved-objects-server';
@@ -56,10 +57,10 @@ export class SavedObjectsTypeValidator {
     }
     const schemaVersion = previousVersionWithSchema(this.orderedVersions, usedVersion);
 
-    if (!schemaVersion || !this.validationMap[schemaVersion]) {
-      return;
+    let validationRule: SavedObjectsValidationSpec | undefined;
+    if (schemaVersion && this.validationMap[schemaVersion]) {
+      validationRule = this.validationMap[schemaVersion];
     }
-    const validationRule = this.validationMap[schemaVersion];
 
     try {
       const validationSchema = createSavedObjectSanitizedDocSchema(validationRule);