runtime check for string type doesn't check instanceof String

[pmuellr]

Kibana version: 7.14.1

We're seeing an odd case of a user getting the following messages logged:

Expected "alert" to be a saved object type

We think this is coming from the following code, and we're guessing that somehow the value in this case could be a String() object vs the usual string type. We're not sure how that could happen.

Update: it wasn't a String() object, it was an array of strings! see: #112746 (comment)

kibana/src/core/server/saved_objects/serialization/serializer.ts

Lines 182 to 190 in 8cf0efe

	private trimIdPrefix(
	sourceNamespace: string | undefined,
	type: string,
	id: string,
	namespaceTreatment: 'strict' | 'lax'
	) {
	assertNonEmptyString(id, 'document id');
	assertNonEmptyString(type, 'saved object type');

kibana/src/core/server/saved_objects/serialization/serializer.ts

Lines 237 to 241 in 8cf0efe

	function assertNonEmptyString(value: string, name: string) {
	if (!value || typeof value !== 'string') {
	throw new TypeError(`Expected "${value}" to be a ${name}`);
	}
	}

The error we're seeing is rooted at a health check task (a task manager task) we have that runs every hour, starting here:

kibana/x-pack/plugins/alerting/server/health/task.ts

Lines 65 to 92 in 8cf0efe

	export function healthCheckTaskRunner(
	logger: Logger,
	coreStartServices: Promise<[CoreStart, AlertingPluginsStart, unknown]>
	) {
	return ({ taskInstance }: RunContext) => {
	const { state } = taskInstance;
	return {
	async run() {
	try {
	return await getAlertingHealthStatus(
	(
	await coreStartServices
	)[0].savedObjects,
	state.runs
	);
	} catch (errMsg) {
	logger.warn(`Error executing alerting health check task: ${errMsg}`);
	return {
	state: {
	runs: (state.runs || 0) + 1,
	health_status: HealthStatus.Error,
	},
	};
	}
	},
	};
	};
	}

That code will eventually call the code in https://github.com/elastic/kibana/blob/master/x-pack/plugins/alerting/server/health/get_health.ts, which appears to reference the alert string as a literal string, so ... not sure what could have happened. Build error? Or maybe I'm not looking at the right thing.

So, this is super-weird, seems hard to believe what we're seeing actually :-), but figured I'd report it.

If this ends up being a real issue (strings can morph to Strings), then we'll need to arrange to do better "string validation", by checking for instanceof String or such. A quick VSCode search through Kibana of typeof.*'string' yielded 777 hits over 525 files.

[elasticmachine]

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

[mshustov]

notes for the Core team: there is no new String() in the Kibana codebase. The given type is read from deserialized field _source.type of SO

kibana/src/core/server/saved_objects/serialization/serializer.ts

Lines 87 to 88 in 9b410ce

	const { type, namespaces, originId, migrationVersion, references, coreMigrationVersion } =
	_source;

which can have JS primitives only.
TODO: check seralization / deseralization logic in @elastic/elasticsearch client.

[wayneseymour]

Well off the cuff, if this is reproducible locally, I'd be curious what typeof value yields on line
#238, within assertNonEmptyString(). If it is indeed 'object' then I'm inclined to agree with Mikhail.

I did not inspect the call site, but given that most of the parameters of trimIdPrefix() are strings. I'd be curious if they could have been called with the string params out of order.

Just an idea.

Years ago, as a Java dev, I've seen that happen.

[mshustov]

@pmuellr maybe we can start with improving the error message to include a type of value?

import typeDetect from 'type-detect';
function assertNonEmptyString(value: string, name: string) {
  if (!value) {
    throw new TypeError(`Expected ${name} to be a non-empty string but given ${value}`);
  }
  if (typeof value !== 'string') {
    throw new TypeError(`Expected ${name} to be a string but given ${typeDetect(value)}`);
  }
}

[pmuellr]

Ya, was thinking that something like that might be good. Will at least call out what type of object it actually was.

[pmuellr]

A little more info from the case that spawned this issue (SDH referenced ^^^).

At one point the user tried using the saved object HTTP APIs to do some kind of searches, and was getting a 500 response back from Kibana. I've requested a dump of the .kibana index - maybe we can spot an error from there. But it's looking more like it's something corrupt in the .kibana index to me - or some other systemic problem - and this instanceof String issue may just be a symptom of that problem.

[pgayvallet]

But it's looking more like it's something corrupt in the .kibana index to me - or some other systemic problem - and this instanceof String issue may just be a symptom of that problem.

Yea, that's my feeling too, especially given this specific issue seems like a (very) isolated case.

[pmuellr]

I think we found the source of the original problem. The user that reported a rule saved object that was saved in .kibana in "doc" format, instead of "source" format. So, instead of type: "alert" it was stored as type: ["alert"] (along with all the other fields).

Is there any known way that could happen?

And so this is fun, turns out if x is a string, ${x} and ${[x]} have the same value! That's why that weird message from assertNonEmptyString() says Expected "alert" to be a saved object type, even though the the SO type alert shows as a string there, but the value is actual an array of that string.

> s = 'sss'
'sss'
> `-${s}-`
'-sss-'
> `-${[s]}-`
'-sss-'

Perhaps the "proper" fix for this is to do a super-simple validation for the "top-level" SO properties and ensure they are the type they are expected to be.

[mshustov]

Is there any known way that could happen?

Type is stored in _source, so a user can write them as any value until we enforce a validation.

And so this is fun, turns out if x is a string, ${x} and ${[x]} have the same value!

#115175 will highlight the problem on "read" operation but agree we shouldn't accept an invalid object on a "write". cc @rudolf @pgayvallet

[pgayvallet]

#115175 was merged a while ago now, and we never re-encountered such problem on any other deployment. I'll go ahead and close this.