shell: Manifest validation upon import #21449

mvollmer · 2024-12-18T12:53:27Z

unit tests

mvollmer · 2024-12-18T12:57:48Z

This is quite WIPy still...

This way, it is only validated once and not on every render.

This make sure that typed code never sees values that don't conform to the claimed types, no matter what people put into their manifests.

cockpituous · 2025-01-07T16:27:11Z

pkg/lib/import-json.ts

+function validation_error(msg: string): never {
+    console.error(`JSON validation error for ${validation_path.join("")}: ${msg}`);
+    throw new ValidationError();


These 3 added lines are not executed by any test.

cockpituous · 2025-01-07T16:27:12Z

pkg/lib/import-json.ts

+export function import_string(val: JsonValue): string {
+    if (typeof val == "string")
+        return val;
+    validation_error(`Not a string: ${JSON.stringify(val)}`);


This added line is not executed by any test.

cockpituous · 2025-01-07T16:27:12Z

pkg/lib/import-json.ts

+export function import_number(val: JsonValue): number {
+    if (typeof val == "number")
+        return val;
+    validation_error(`Not a number: ${JSON.stringify(val)}`);


This added line is not executed by any test.

cockpituous · 2025-01-07T16:27:12Z

pkg/lib/import-json.ts

+export function import_boolean(val: JsonValue): boolean {
+    if (typeof val == "boolean")
+        return val;
+    validation_error(`Not a boolean: ${JSON.stringify(val)}`);


These 4 added lines are not executed by any test.

cockpituous · 2025-01-07T16:27:12Z

pkg/lib/import-json.ts

+export function import_json_object(val: JsonValue): JsonObject {
+    if (!!val && typeof val == "object" && val.length === undefined)
+        return val as JsonObject;
+    validation_error(`Not an object: ${JSON.stringify(val)}`);


This added line is not executed by any test.

What if we want to add a "length" property to some future thing we store in manifests?

cockpituous · 2025-01-07T16:27:12Z

pkg/lib/import-json.ts

+    } catch (e) {
+        if (!(e instanceof ValidationError))
+            throw e;


These 3 added lines are not executed by any test.

cockpituous · 2025-01-07T16:27:12Z

pkg/lib/import-json.ts

+    if (obj[field as string] === undefined) {
+        validation_error(`Field "${String(field)}" is missing`);


These 2 added lines are not executed by any test.

cockpituous · 2025-01-07T16:27:12Z

pkg/lib/import-json.ts

+
+export function validate<T>(path: string, val: JsonValue | undefined, importer: (val: JsonValue) => T, fallback: T): T {
+    if (val === undefined)
+        return fallback;


This added line is not executed by any test.

cockpituous · 2025-01-07T16:27:12Z

pkg/lib/import-json.ts

+    } catch (e) {
+        if (!(e instanceof ValidationError))
+            throw e;
+        return fallback;


These 4 added lines are not executed by any test.

cockpituous · 2025-01-07T16:27:13Z

pkg/shell/shell-modals.tsx

@@ -144,7 +142,7 @@ export const LangModal = ({ dialogResult }) => {
                        <MenuList>
                            {
                                (() => {
-                                    const locales = manifest.locales || {};
+                                    const locales = state.config.manifest.locales || {};


This added line is not executed by any test.

martinpitt

Thanks @mvollmer ! I like the approach a lot -- explicit, clean, no new dependencies, and pleasant to use.

martinpitt · 2025-01-09T10:43:20Z

pkg/base1/test-import-json.ts

+QUnit.test("import_array", function(assert) {
+    assert.deepEqual(import_array(["a", "b", "c"], import_string), ["a", "b", "c"], "array of strings");
+    assert.deepEqual(import_array([1, 2, 3], import_number), [1, 2, 3], "array of numbers");
+    assert.deepEqual(import_array([1, 2, "c"], import_number), [1, 2], "array of numbers with a string");


IMHO this should also throw, instead of giving you a truncated array -- that just silently hides errors, and may make the rest of the entries invalid as well.

Yes, this is the big philosophical question. Personally, I like the non-strict approach, but I have no problem with making this strict.

My best argument for making this strict is to amplify that something is wrong. If you make a mistake with your search terms, your whole page doesn't get loaded instead of some search terms being omitted.

martinpitt · 2025-01-09T10:44:41Z

pkg/base1/test-import-json.ts

+
+QUnit.test("import_record", function(assert) {
+    assert.deepEqual(
+        import_record({ a: "a", b: "b", c: "c" }, import_string),


JSON parlance is "object", not "record". I suppose you want to give this a different name to point out that all values have to be of the same type. But a "record" (or "struct") is precisely the opposite -- fixed number and names of keys, and different values.

One term that comes to my mind that conveys this meaning is "dictionary" (they usually map one type to a single value type).

The "record" term is from the TypeScript utility class Record<Keys, Type>, which is indeed a Map or Dict, not a structure.

Ugh -- this is opposite to pretty much everywhere else https://en.wikipedia.org/wiki/Record_(computer_science) -- shame on you, MS.

martinpitt · 2025-01-09T10:59:45Z

pkg/base1/test-import-json.ts

+
+import { JsonValue } from 'cockpit';
+import {
+    import_json_object, import_optional, import_mandatory,


get_() would be more consistent with our Python API; "import" sounds a bit exaggerated, but no strong opinion.

martinpitt · 2025-01-09T11:00:22Z

pkg/base1/test-import-json.ts

+    assert.deepEqual(
+        import_record({ a: 1, b: 2, c: "c" }, import_number),
+        { a: 1, b: 2 },
+        "record of numbers with a string");


Similar to the inconsistent array above, IMHO this should throw and ignore the value in its entirety.

martinpitt · 2025-01-09T11:05:18Z

pkg/lib/import-json.ts

+   Validation is generally lenient: If a validation error occurs deep
+   inside a nested structure, only the affected part of the structure
+   is omitted.  More conretely, if an element of an array is invalid,
+   this element is omitted from the array. If a optional field of an
+   object is invalid, it will be omitted. This doesn't happen


This is what I mentioned above: IMHO this isn't safe in general. Think about "bridges": [] in the shell manifest. Silently ignoring a non-string argument from sudo or pkexec is potentially disastrous. It's of course less of an issue for docs or locales, but IMHO it would make more sense to be strict and completely ignore the value -- that will greatly increase the chance of catching bugs in our tests.

(Also, "conretely" typo)

Yeah, command lines are a very good example of something that better be rejected in its entirety.

Put me also in the "reject outright" camp.

martinpitt · 2025-01-09T11:11:11Z

pkg/base1/test-import-json.ts

+    const checks: [JsonValue, Teams][] = [
+        [
+            {
+                MAC: {


Sorry, I just have to ask: What does "MAC" mean in soccer context?

It's supposed to be the short form of "ManCity"...

martinpitt · 2025-01-09T11:12:08Z

pkg/base1/test-import-json.ts

+                        { name: "Haaland", age: 24 },
+                        { name: "De Bruyne", age: 33 }
+                    ],
+                },


Can you please add a description comment before each scenario? Like, this one is probably "No stadium (it's optional)" or so

martinpitt · 2025-01-09T11:14:02Z

pkg/lib/import-json.ts

+
+function validation_error(msg: string): never {
+    console.error(`JSON validation error for ${validation_path.join("")}: ${msg}`);
+    throw new ValidationError();


Couldn't hurt to pass on msg into the exception object?

pkg/lib/import-json.ts

pkg/shell/machines/machines.js

allisonkarlitskaya · 2025-01-10T15:53:52Z

pkg/lib/import-json.ts

+    try {
+        with_validation_path(`.${String(field)}`, () => { res[field] = importer(obj[field]) });
+    } catch (e) {
+        if (!(e instanceof ValidationError))


I disagree with silently dropping validation errors. It should either be "the key exists and is valid" or "the key does not exist". The key existing with an invalid type is definitely an error.

allisonkarlitskaya · 2025-01-10T15:54:40Z

pkg/lib/import-json.ts

+}
+
+export function import_optional<T, F extends keyof T>(res: T, obj: JsonObject, field: F, importer: (val: JsonValue) => T[F]): void {
+    if (obj[field as string] === undefined)


Why not in or whatever? The as is a bit unsightly here.

You could probably also further constraint the type of F to be keyof T & string.

allisonkarlitskaya · 2025-01-10T16:01:36Z

pkg/lib/import-json.ts

+    return res;
+}
+
+export function import_optional<T, F extends keyof T>(res: T, obj: JsonObject, field: F, importer: (val: JsonValue) => T[F]): void {


(thinking out loud — not necessarily a very strong request for change or anything)

The way this function works is a bit weird. The assumption that the name of the input key and output key are the same is probably true for most cases, but might not be what the user wants. It also involves using mutable state, which I generally try to avoid if possible.

Two possible alternatives:

just write null or undefined explicitly to the dictionary. That's what the equivalent code in the bridge does (although str | None is a lot more Pythonic — JS seems to be quite happy when just blindly deferencing things and getting undefined).

Do something like having this function return something like { F?: T } and use it like:

const myobj = { field: import_mandatory(input, "x", ...), ... import_optional(input, "y", ...), ... import_optional(input, "z", ...) };

ie: the function will return either an empty dictionary or the dictionary to merge with the optional value.

I sort of like the null thing, personally. It clashes with in and ?, though...

allisonkarlitskaya · 2025-01-10T16:03:12Z

pkg/lib/import-json.ts

+}
+
+export function import_mandatory<T>(obj: JsonObject, field: string, importer: (val: JsonValue) => T): T {
+    if (obj[field as string] === undefined) {


More as abuse...

allisonkarlitskaya · 2025-01-10T16:06:26Z

pkg/lib/import-json.ts

+
+export function import_json_array(val: JsonValue): JsonValue[] {
+    if (!!val && typeof val == "object" && val.length !== undefined)
+        return val as JsonValue[];


You should try to get this working without resorting to unsafe casts. The compiler has a fairly impressive set of narrowing mechanisms...

See also https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/isArray

allisonkarlitskaya · 2025-01-10T16:10:20Z

pkg/lib/import-json.ts

+export function import_json_object(val: JsonValue): JsonObject {
+    if (!!val && typeof val == "object" && val.length === undefined)
+        return val as JsonObject;
+    validation_error(`Not an object: ${JSON.stringify(val)}`);


What if we want to add a "length" property to some future thing we store in manifests?

allisonkarlitskaya · 2025-01-10T16:10:39Z

pkg/lib/import-json.ts

+
+export function import_json_object(val: JsonValue): JsonObject {
+    if (!!val && typeof val == "object" && val.length === undefined)
+        return val as JsonObject;


mvollmer added the blocked Don't land until something else happens first (see task list) label Dec 18, 2024

mvollmer changed the title ~~Shell validation~~ shell: Manifest validation upon import Dec 18, 2024

mvollmer added no-test For doc/workflow changes, or experiments which don't need a full CI run, and removed blocked Don't land until something else happens first (see task list) labels Dec 18, 2024

mvollmer force-pushed the shell-validation branch from c3fd607 to e89ae83 Compare December 18, 2024 16:24

mvollmer force-pushed the shell-validation branch from e89ae83 to 5791647 Compare January 2, 2025 10:22

shell: Import ShellManifest as part of maintaining ShellState

5dbcb6b

This way, it is only validated once and not on every render.

mvollmer force-pushed the shell-validation branch from 5791647 to a3dc6f2 Compare January 7, 2025 12:42

mvollmer removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Jan 7, 2025

shell: Validate manifests before exposing them to TypeScript

428e940

This make sure that typed code never sees values that don't conform to the claimed types, no matter what people put into their manifests.

mvollmer force-pushed the shell-validation branch from a3dc6f2 to 428e940 Compare January 7, 2025 15:09

cockpituous reviewed Jan 7, 2025

View reviewed changes

mvollmer requested a review from martinpitt January 8, 2025 10:01

mvollmer marked this pull request as ready for review January 8, 2025 10:01

martinpitt requested changes Jan 9, 2025

View reviewed changes

mvollmer requested a review from allisonkarlitskaya January 9, 2025 14:19

allisonkarlitskaya requested changes Jan 10, 2025

View reviewed changes

		if (obj[field as string] === undefined) {
		validation_error(`Field "${String(field)}" is missing`);

shell: Manifest validation upon import #21449

Are you sure you want to change the base?

shell: Manifest validation upon import #21449

Conversation

mvollmer commented Dec 18, 2024 • edited Loading

mvollmer commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvollmer commented Dec 18, 2024 •

edited

Loading