Enforce correct shape for SO attributes and id during create operations (#187876)

## Summary Fix https://github.com/elastic/kibana/issues/123575 Fix https://github.com/elastic/kibana/issues/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)
2025-04-23 17:28:26 -04:00 · 2024-07-11 17:10:48 +02:00 · 2024-07-11 17:10:48 +02:00 · 10edbf1054
commit 10edbf1054
parent 7520f28a61
7 changed files with 96 additions and 24 deletions
--- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/update.test.ts
+++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/update.test.ts
@ -149,7 +149,7 @@ 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();
@ -157,7 +157,7 @@ describe('#update', () => {
      });

      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,7 +172,7 @@ 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();
@ -180,7 +180,7 @@ describe('#update', () => {
      });

      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({
--- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.encryption_extension.test.ts
+++ b/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
      );
--- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.spaces_extension.test.ts
+++ b/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();
--- a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/schema.test.ts
+++ b/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]"`
+      );
+    });
+  });
 });
--- a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/schema.ts
+++ b/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;
+  }
 };
--- a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts
+++ b/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', () => {
--- a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts
+++ b/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);