Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SavedObjects] creating objects using undefined or primitives as attributes causes errors #123575

Closed
pgayvallet opened this issue Jan 24, 2022 · 3 comments · Fixed by #187876
Closed
Labels
bug Fixes for quality problems that affect the customer experience Feature:Saved Objects Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@pgayvallet
Copy link
Contributor

Creating saved objects using SavedObjectClient.create and/or SavedObjectClient.bulkCreate does not perform any kind of basic surface validation of the shape of the provided attributes.

Even if we're not enforcing that the provided attributes are an object, even within the SavedObject type,

export interface SavedObject<T = unknown> {
   attributes: T;
   // ...
}

there are numerous parts of the code were we make this assumption by trying to access obj.attributes.someStuff.

For example, by creating a dashboard with empty attributes, using

await soClient.create('dashboard', undefined, { id: 'test-undefined' });

I could reproduce two errors in a few minutes,

one coming from the associated collector:

[plugins.usageCollection.usage-collection.collector-set] TypeError: Cannot read properties of undefined (reading 'panelsJSON')
    at injectReferences (/kibana/src/plugins/dashboard/common/saved_dashboard_references.ts:154:25)
    at collectDashboardTelemetry (/kibana/src/plugins/dashboard/server/usage/dashboard_telemetry.ts:172:24)

and one when accessing the SOM listing page

[ERROR][http] TypeError: Cannot read properties of undefined (reading 'title')
    at getTitle (/kibana/src/plugins/dashboard/server/saved_objects/dashboard.ts:29:29)
    at SavedObjectsManagement.getTitle (/kibana/src/plugins/saved_objects_management/server/services/management.ts:31:23)
    at injectMetaAttributes (/kibana/src/plugins/saved_objects_management/server/lib/inject_meta_attributes.ts:24:46)

Per-type attributes validation was introduced in #118969, however, as it will take some time before teams starts defining their schemas, and as it's likely that it will not even be done for all our registered types, we may want to add some surface validation around the expected shape of the attributes of a saved object during its creation.

We may also want to change our SavedObject type definition to reflect that the type T of the attributes must be an object.

@pgayvallet pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Saved Objects labels Jan 24, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@mshustov
Copy link
Contributor

Shouldn't we consider it as a bug? IIRC #118969 always expected attributes to be an object. And as you said We may also want to change our SavedObject type definition to reflect that the type T of the attributes must be an object.

@pgayvallet
Copy link
Contributor Author

Shouldn't we consider it as a bug?

Given that we don't currently document anywhere that the attributes associated to a type should be an object, it's subject to interpretation. We could potentially say instead that every part of the codebase should not make the assumption that these attributes have an object shape.

But I agree that, in practice, all our SO types are using an object/record shape for their attributes, so it shouldn't be an API breaking change to add such validation and update our API documentation. In that case, we can consider this a bug.

I'll go ahead and flag the issue accordingly.

@pgayvallet pgayvallet added the bug Fixes for quality problems that affect the customer experience label Jan 24, 2022
pgayvallet added a commit that referenced this issue Jul 11, 2024
…ns (#187876)

## Summary

Fix #123575
Fix #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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Saved Objects Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants