Possible (security?) hole in settings validation: we don't check for extra properties in objects

[telamonian]

I'm adding settings for overrides to theme CSS in a PR (#6926). In the course of this, I noticed that, although settings schemas are strictly checked/validated in various ways, we do not check settings objects for extra properties. For example, I added an override object to the schema:

jupyterlab/packages/apputils-extension/schema/themes.json

Lines 7 to 16 in b1b0169

	"definitions": {
	"themeOverrides": {
	"type": "object",
	"properties": {
	"code-font-size": { "type": "string" },
	"content-font-size1": { "type": "string" },
	"ui-font-size1": { "type": "string" }
	}
	}
	},

jupyterlab/packages/apputils-extension/schema/themes.json

Lines 30 to 33 in b1b0169

	"overrides": {
	"title": "Theme CSS Overrides",
	"description": "Override theme CSS variables by setting key-value pairs here",
	"$ref": "#/definitions/themeOverrides",

and then consume it in my code like so:

jupyterlab/packages/apputils/src/thememanager.ts

Lines 123 to 139 in b1b0169

	loadCssOverrides(): void {
	const newOverrides =
	(this._settings.user['overrides'] as Dict<string>) || {};
	Object.keys(this._overrides).forEach(key => {
	if (!(key in newOverrides)) {
	// unset the override
	this.loadCssOverride(key);
	}
	});
	Object.keys(newOverrides).forEach(key => {
	// set the override
	this.loadCssOverride(key);
	});
	this._overrides = newOverrides;
	}

Although I only defined keys for the font size CSS in the schema, I've found that my code will happily pick up any CSS override that I define in my theme user settings. For example, this is valid:

"overrides": {
        "layout-color0": "purple"
    },

Is this something we should be more strict about when validating settings? I think it probably should be.

[jasongrout]

I think by default, json schemas allow extra properties. You have to explicitly say in the schema that additional properties are not allowed. See https://json-schema.org/understanding-json-schema/reference/object.html#properties, for example.

[jasongrout]

So are you basically saying that our default schemas should have additionalProperties set to false by default?

[telamonian]

...our default schemas should have additionalProperties set to false by default?

@jasongrout Thanks for the reference. I think that would make sense. We could instead just be strict about never looping over the keys of a settings object (like I do above), but that doesn't cover any settings schemas added by non-core extensions.

Where do we actually do the schema validation in our code?

[telamonian]

As another alternative, I could just not worry about it. Maybe it's a good thing to have a way for a user to override any arbitrary CSS variable that starts with --jp-? But I'm definitely leaning towards your suggestion of setting additionalProperties to false by default

[jasongrout]

I think that makes sense to standardize on additionalProperties being false. It does mean that if a plugin introduces new settings, they will actually be invalid when moving back to an older version, as opposed to just being ignored now.

It's also probably a good idea to not just loop over the settings whatever they are, but just loop over what you know is good, i.e., keep in mind there can be arbitrary other properties.

[afshin]

We explicitly set additionalProperties: true here: https://github.com/jupyterlab/jupyterlab/blob/master/packages/coreutils/src/plugin-schema.json#L7

That is the schema against which we validate plugin schemas. I'm not sure I see the circumstance where this should be false. Can you come up with a scenario where it's problematic that we've set it to true?

[telamonian]

@afshin The scenario is the one from the PR (#6926) that I described in the first post on this thread: you have a (potentially very long) list of settings that can all be treated similarly on the Typescript side. For that sort of use case, I think it makes sense to iterate over the settings as a group and deal with them that way.

Obviously, since settings rely on user input we should be doing some kind of validation during that loop. It then becomes a question of where the validation should occur, since ideally it should happen only once based on a single source of truth. Since I already have a source of truth in the form of a carefully described schema, it makes sense to just validate against said schema, rather than rolling my own validation code.

The current setup (with additionalProperties defaulting to true) is problematic since it gives devs (myself included) a pretty easy way to shoot themselves in the foot when considering this sort of setting validation. Based on how otherwise strict the JSON parser is (eg it disallows stylistic choices like lagging commas), a dev may overreach and assume (as I did) that of course an object's properties would also be strictly validated. Thus they'd skip adding code that would duplicate the validation they (incorrectly) assumed was already happening, and there wouldn't be any obvious compile/runtime error that would point out their mistake.

By defaulting additionalProperties to false, we remove the risk of someone accidently introducing this pretty classic sort of security hole (acting on unsanitized user input) to core or an extension. And just because I can't think of any interesting hacks that start with injecting arbitrary CSS, that doesn't mean there aren't any.

[telamonian]

It's also probably a good idea to not just loop over the settings whatever they are, but just loop over what you know is good, i.e., keep in mind there can be arbitrary other properties.

@jasongrout Another way to think of the actual problem here is as one of developer expectations. I was able to fix my specific validation issue once you pointed out what additionalProperties does, and that it defaults to true.

I have to assume that most people who have contributed to Jlab are not familiar with the finer points of JSON schemas, and could repeat the same mistake I made. Possibly we could fix this with better docs, but I think it's better to sidestep this specific issue entirely by defaulting additionalProperties to false.

Separately, expanding the schema docs is a good idea too. Maybe add a number of example schemas to the Extension Dev Guide, and explain in detail what the automatic validation does/does not do.

[afshin]

Hm, let's talk about this at tomorrow's meeting. I'd like to try this out and consider the side-effects.

[telamonian]

Sounds good. I should be able to attend tomorrow

[jasongrout]

As a follow up, we decided in the meeting to:

1. Add documentation somewhere letting users know about JSON schema's permissive defaults
2. Make sure our core schemas use additionalProperties: false where appropriate

Adding doc label to this for the documentation part.