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.

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.

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

shell: Manifest validation upon import #21449

shell: Manifest validation upon import #21449

mvollmer commented Dec 18, 2024 •

edited

Loading

mvollmer commented Dec 18, 2024

cockpituous Jan 7, 2025

cockpituous Jan 7, 2025

cockpituous Jan 7, 2025

cockpituous Jan 7, 2025

cockpituous Jan 7, 2025

cockpituous Jan 7, 2025

cockpituous Jan 7, 2025

cockpituous Jan 7, 2025

cockpituous Jan 7, 2025

cockpituous Jan 7, 2025

martinpitt left a comment

martinpitt Jan 9, 2025

mvollmer Jan 9, 2025

martinpitt Jan 9, 2025

mvollmer Jan 9, 2025

martinpitt Jan 9, 2025

martinpitt Jan 9, 2025

martinpitt Jan 9, 2025

martinpitt Jan 9, 2025

mvollmer Jan 9, 2025

martinpitt Jan 9, 2025

mvollmer Jan 9, 2025

martinpitt Jan 9, 2025

mvollmer Jan 9, 2025

martinpitt Jan 9, 2025

		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

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

mvollmer commented Dec 18, 2024 •

edited

Loading