Skip to content

Commit

Permalink
Enforce correct shape for SO attributes and id during create operatio…
Browse files Browse the repository at this point in the history 
…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)
  • Loading branch information
@pgayvallet
pgayvallet authored Jul 11, 2024
1 parent 7520f28 commit 10edbf1
Show file tree
Hide file tree
Showing 7 changed files with 96 additions and 24 deletions.
28 changes: 14 additions & 14 deletions ...ges/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/update.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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',
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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!
Expand All @@ -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,
});
Expand All @@ -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,
});
Expand All @@ -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,
});
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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({
Expand Down
2 changes: 1 addition & 1 deletion ...ts/core-saved-objects-api-server-internal/src/lib/repository.encryption_extension.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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
);
Expand Down
3 changes: 3 additions & 0 deletions ...bjects/core-saved-objects-api-server-internal/src/lib/repository.spaces_extension.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
36 changes: 36 additions & 0 deletions .../core/saved-objects/core-saved-objects-base-server-internal/src/validation/schema.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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' });
Expand Down Expand Up @@ -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]"`
);
});
});
});
16 changes: 10 additions & 6 deletions packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/schema.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand All @@ -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())),
});

/**
Expand All @@ -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;
}
};
28 changes: 28 additions & 0 deletions ...re/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
7 changes: 4 additions & 3 deletions ...es/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit 10edbf1

Please sign in to comment.