feat(compartment-mapper): Custom parser support #2304

boneskull · 2024-05-28T22:39:03Z

Closes: #2303

Description

This PR adds custom parser support as discussed in #2303. To support the feature:

A new prop options was added to the PackagePolicy type.
Parsers now optionally accept compartmentDescriptor objects, so that parsers can enforce policy

Security Considerations

See #2303

Scaling Considerations

See #2303

Documentation Considerations

Add documentation for a new option parsers for e.g., importLocation
Add documentation for a new prop options in PackagePolicy

Testing Considerations

A round-trip test
A round-trip test w/ policy enforcement
Assert options is ignored by policy validator
Assert custom parsers cannot override builtin parsers

Compatibility Considerations

None

Upgrade Considerations

Update NEWS.md

kriskowal

Design direction seems fine. I like to do import, archive, and import-archive together for any feature that applies to all three, to make sure the design is sound. A scaffold test should verify that an application has the same behavior through import or archive + import-archive using custom parsers. Any commentary on why archive and import-archive should not support custom parsers would be welcome. Please request a review again when this PR is out of draft.

Thank you for sharing LiteralUnion. There are many places where I imagine this pattern should be applied. I welcome a comment from @turadg whether this should become integral to our style for enum literals.

packages/compartment-mapper/src/link.js

packages/compartment-mapper/src/node-modules.js

packages/compartment-mapper/test/custom-parser.test.js

packages/compartment-mapper/src/types.js

packages/compartment-mapper/test/custom-parser.test.js

packages/compartment-mapper/src/import.js

packages/compartment-mapper/src/types.js

packages/compartment-mapper/src/node-modules.js

boneskull · 2024-05-29T20:22:13Z

@kriskowal

I like to do import, archive, and import-archive together for any feature that applies to all three, to make sure the design is sound.

Is "import-archive" the "unarchive" to the "archive" case? If so, I'm assuming that the same custom parser would need to be provided to both invocations. What happens (or should happen) if this is not true? Would we need to do something like add the parser itself to the archive?

kriskowal · 2024-05-29T20:26:30Z

Is "import-archive" the "unarchive" to the "archive" case?

Indeed.

If so, I'm assuming that the same custom parser would need to be provided to both invocations. What happens (or should happen) if this is not true? Would we need to do something like add the parser itself to the archive?

It’s good enough to require that the same custom parsers be present for makeArchive and parseArchive (and their analogues). I don’t care to fantasize about capturing the custom parser in the archive at this time, and that can be a different route if we do go that direction.

kriskowal · 2024-05-29T20:28:07Z

Analogously, makeArchive (and writeArchive etc) has to receive compatible exits with parseArchive (and importArchive etc). Coördinating out of band is expected.

packages/compartment-mapper/src/link.js

kriskowal · 2024-05-29T23:51:26Z

packages/compartment-mapper/src/link.js

+  /** @type {Record<string, Language>} */
+  const customLanguageForExtension = Object.create(null);
+  for (const { parser, extensions, language } of parsers) {
+    if (
+      language in parserForLanguage &&
+      parserForLanguage[language] !== parser
+    ) {
+      throw new Error(`Parser for language ${q(language)} already defined`);
+    }
+    parserForLanguage[language] = parser;
+    for (const extension of extensions) {
+      if (
+        extension in customLanguageForExtension &&
+        customLanguageForExtension[extension] !== language
+      ) {
+        throw new Error(
+          `Extension ${q(extension)} already assigned language ${q(customLanguageForExtension[extension])}`,
+        );
+      }
+      customLanguageForExtension[extension] = language;
+    }
+  }


I’m beginning to suspect it’s better to thread separate “defaults for all compartments” options for parserForLanguage and languageForExtension than to merge them into parsers and then unmerge them here. In #2294, I would have to make @endo/compartment-mapper/import-parsers.js express an opinion about the language for each extension, and there is no clear opinion at that juncture.

This branch captures my investigation and I am convinced that threading parserForLanguage and languageForExtension as separate options (instead of consolidated custom parsers) is robust with less ceremony and will compose better with changes I have coming for #400

boneskull/native-parser...kriskowal-native-parser

boneskull · 2024-05-30T23:13:27Z

@kriskowal OK, this is ready for Proper Review. Please take a looksee

boneskull · 2024-05-30T23:14:50Z

will update NEWS.md as a last step

boneskull · 2024-05-30T23:15:32Z

packages/compartment-mapper/src/archive.js

+const parserForLanguage = freeze(
+  /** @type {const} */ ({
+    mjs: parserArchiveMjs,
+    'pre-mjs-json': parserArchiveMjs,
+    cjs: parserArchiveCjs,
+    'pre-cjs-json': parserArchiveCjs,
+    json: parserJson,
+    text: parserText,
+    bytes: parserBytes,
+  }),
+);


This (and the other one) shouldn't be mutated. Calling it const isn't enough 😄

UPDATE: There are at least three. I hope I didn't miss one.

You caught them all.

boneskull · 2024-05-30T23:16:07Z

packages/compartment-mapper/src/import-archive.js

+/** @import {ParserForLanguage} from './types.js' */
+/** @import {ReadFn} from './types.js' */
+/** @import {ReadPowers} from './types.js' */
+/** @import {SomeObject} from './types.js' */


thank you, copilot

packages/compartment-mapper/src/node-modules.js

boneskull · 2024-05-30T23:21:37Z

packages/compartment-mapper/test/policy-format.test.js

@@ -62,6 +62,12 @@ const q = JSON.stringify;
  {
    globals: ['a', {}],
  },
+  {
+    options: {


Do we care that it's possible to put unserializable junk in here? Should that be restricted via types or at runtime or both or neither?

I defer to @naugtur

boneskull · 2024-05-30T23:23:21Z

@kriskowal Note: there's no explicit test for the archive use-case. I am not sure if it's needed?

boneskull · 2024-05-30T23:25:04Z

packages/compartment-mapper/src/link.js

+    /** @type {ParserForLanguage} */
+    const finalParserForLanguage = Object.create(null);
+
+    for (const [language, parser] of [


I noticed that putting stuff in parserForLanguage seems to leak out of tests, so a) I stopped doing that, and b) I froze them.

boneskull · 2024-05-30T23:26:58Z

packages/compartment-mapper/src/link.js

+      finalLanguageForExtension[extension] = language;
+    }
+    for (const [extension, language] of entries(languageForExtension)) {
+      finalLanguageForExtension[extension] = language;


I'm not sure if I should write anything back to the CompartmentDescriptor.

No, this is fine.

boneskull · 2024-05-30T23:27:34Z

packages/compartment-mapper/src/link.js

+    /** @type {Record<string, Language>} */
+    const finalLanguageForExtension = Object.create(null);
+    for (const [extension, language] of entries(customLanguageForExtension)) {
+      if (extension in languageForExtension) {


Should I be using has() instead of this? It's habit (because TypeScript seems to prefer it).

Since they’re equivalent for Object.create(null), I’m inclined to overlook the question. Unless I’m wrong about them being equivalent. @erights for second opinion.

In boneskull/native-parser...kriskowal-native-parser, I did some additional work to ensure that in behaves as I would expect. In the case of options where a devious user might provide an object with a prototype chain that includes enumerable properties, the difference between has and in would be observable, but that can be avoided by capturing options with freeze(create(null), option).

kriskowal

This is close to ready.

I investigated changing the parsers option to separate parserForLanguage and languageForExtension options and I like the results. I offer these commits for your consideration. I would like to subsume it into this PR in some fashion or another and I am not concerned about preserving my authorship metadata. boneskull/native-parser...kriskowal-native-parser
We typically use a rebase and merge commit and preserve the narrative of the commit history. Let me know if it was your intention to squash+merge the PR or if you want me to review the narrative of the commit history.

kriskowal · 2024-05-31T01:41:04Z

packages/compartment-mapper/src/link.js

+      finalLanguageForExtension[extension] = language;
+    }
+    for (const [extension, language] of entries(languageForExtension)) {
+      finalLanguageForExtension[extension] = language;


No, this is fine.

packages/compartment-mapper/src/node-modules.js

kriskowal · 2024-05-31T01:45:00Z

packages/compartment-mapper/src/policy-format.js

@@ -200,7 +205,7 @@ export const assertPackagePolicy = (allegedPackagePolicy, path, url) => {
 * It also moonlights as a type guard.
 *
 * @param {unknown} allegedPolicy - Alleged `Policy` to test
- * @returns {asserts allegedPolicy is import('./types.js').Policy|undefined}
+ * @returns {asserts allegedPolicy is SomePolicy|undefined}


Is there a precedent for Some* types already? Can we consider Maybe* instead?

It's not a "maybe" type. This playground illustrates why this is a thing.

If I were to explain this in a nutshell: when we use Policy without a type argument, it artificially narrows the type to Policy<void, void, void, unknown>. We may be able to safely assume this currently, but that is not guaranteed. Using SomePolicy prevents future headaches, essentially.

kriskowal · 2024-05-31T01:46:49Z

packages/compartment-mapper/test/policy-format.test.js

@@ -62,6 +62,12 @@ const q = JSON.stringify;
  {
    globals: ['a', {}],
  },
+  {
+    options: {


I defer to @naugtur

kriskowal · 2024-05-31T01:47:11Z

packages/compartment-mapper/src/archive.js

+const parserForLanguage = freeze(
+  /** @type {const} */ ({
+    mjs: parserArchiveMjs,
+    'pre-mjs-json': parserArchiveMjs,
+    cjs: parserArchiveCjs,
+    'pre-cjs-json': parserArchiveCjs,
+    json: parserJson,
+    text: parserText,
+    bytes: parserBytes,
+  }),
+);


You caught them all.

…native module import

The reasoning behind this is that a custom parser may want to use the `CompartmentDescriptor` to do things like policy enforcement.

This swaps out the "native module" fixture for a "markdown" fixture. The test parser implementation is now a "markdown parser". While the native module fixture was useful, it presents some practical portability issues.

Due to the type parameter defaults, these assertions _actually_ said, e.g., ```ts asserts value is Policy<void, void, void, void> ``` ...which is not necessarily true. This fixes the problem by adding `SomePolicy` and `SomePackagePolicy`.

This adds an optional property, `options`, to the `PackagePolicy` type. Here, consumers can add any fields or metadata they need to the policy. Any values will pass through the policy validator _alive, and unspoiled_. Endo should not read the contents of this property directly. The default type of `unknown` means that access to the value must have an explicit type assertion; it is the responsibility of the consumer to provide a proper type for this field.

Changed `policy` prop to `SomePackagePolicy`.

Co-authored-by: Kris Kowal <kris@agoric.com>

- Adds tests for more situations - Fixes prevention of overriding builtin parsers - types for various options consolidated

…guage and languageForExtension

…xtension

boneskull · 2024-06-03T19:44:49Z

@kriskowal OK, I've cherry-picked your changes, then fixed a problem in bundle.js. This is ready for final review. Once approved, I'll squash and merge.

Co-authored-by: Kris Kowal <kris@agoric.com>

kriskowal

Great, thank you! And thanks for offering to squash+merge. No notes on commit structure or messages.

## Description This exposes `captureFromMap()` in `capture-lite.js`. This function is similar to e.g., `makeArchiveFromMap()` in `archive-lite.js`; but rather than creating a `.zip` archive, it simply returns the fully-completed `CompartmentMapDescriptor`, `Sources`, and a mapping of filename to compartment map name. This information is needed for next-gen-lavamoat-node ("endomoat")'s automatic policy generation. Another commit disables the hardcoded check for parsers in the compartment map validation functions (which are no longer necessary after #2304). ### Questions - Should this be split into two PRs? - Should any of this be renamed? - Internal functions were copy/pasted from `archive-lite.js` into `capture-lite.js`. Should these be extracted into a shared module? ### Security Considerations None that I'm aware of. ### Scaling Considerations If anything, it may shave a few nanoseconds off of compartment map validation. ### Documentation Considerations Probably should be added to `NEWS.md`. ### Testing Considerations - [ ] The compartment map validation is currently not tested in isolation and probably should be (removing the parser-name assertion did not cause a test to fail) - [x] `captureFromMap()` needs some sort of basic round-trip test. I think a snapshot of the return value may suffice? ### Compatibility Considerations None ### Upgrade Considerations None --------- Co-authored-by: Kris Kowal <kris@agoric.com>

boneskull requested a review from naugtur May 28, 2024 22:39

boneskull self-assigned this May 28, 2024

boneskull requested a review from kriskowal May 28, 2024 22:39

boneskull added the enhancement New feature or request label May 28, 2024

kriskowal requested changes May 29, 2024

View reviewed changes

boneskull commented May 29, 2024

View reviewed changes

packages/compartment-mapper/src/node-modules.js Outdated Show resolved Hide resolved

boneskull commented May 29, 2024

View reviewed changes

packages/compartment-mapper/src/node-modules.js Outdated Show resolved Hide resolved

boneskull commented May 29, 2024

View reviewed changes

packages/compartment-mapper/src/node-modules.js Outdated Show resolved Hide resolved

kriskowal reviewed May 29, 2024

View reviewed changes

boneskull force-pushed the boneskull/native-parser branch from e5f88ee to 491a795 Compare May 30, 2024 23:12

boneskull requested a review from kriskowal May 30, 2024 23:12

boneskull marked this pull request as ready for review May 30, 2024 23:13

boneskull commented May 30, 2024

View reviewed changes

packages/compartment-mapper/src/node-modules.js Outdated Show resolved Hide resolved

boneskull force-pushed the boneskull/native-parser branch from 44ed66f to c756eba Compare May 30, 2024 23:20

boneskull commented May 30, 2024

View reviewed changes

kriskowal requested changes May 31, 2024

View reviewed changes

boneskull and others added 3 commits June 3, 2024 12:40

feat(compartment-mapper): allow custom parsers [DRAFT]

9da9817

test(compartment-mapper): test whether custom parsers allow for node …

b15077a

…native module import

chore: renaming typo

7781750

boneskull and others added 19 commits June 3, 2024 12:40

feat(compartment-mapper): ParseFn now receives CompartmentDescriptor

a7deeed

The reasoning behind this is that a custom parser may want to use the `CompartmentDescriptor` to do things like policy enforcement.

chore(compartment-mapper): simplify custom parser test

6dbf4d9

This swaps out the "native module" fixture for a "markdown" fixture. The test parser implementation is now a "markdown parser". While the native module fixture was useful, it presents some practical portability issues.

fix(compartment-mapper): fix types for custom languages

6683bac

fix(compartment-mapper): fix type in CompartmentDescriptor

1f62ac0

Changed `policy` prop to `SomePackagePolicy`.

chore(compartment-mapper): make language prop of CustomParser singular

455ea4e

chore(compartment-mapper): better variable names

7aba42a

Co-authored-by: Kris Kowal <kris@agoric.com>

chore(compartment-mapper): use textdecoder in custom parser test

e8e412c

Co-authored-by: Kris Kowal <kris@agoric.com>

chore(compartment-mapper): remove errant newline

17fb28c

chore(compartment-mapper): lint

cb4960f

chore(compartment-mapper): rename variables for style

09b9e64

chore(compartment-mapper): swipe full docstrings from type-fest

fa164f7

chore(compartment-mapper): review updates, more tests

40dc47b

- Adds tests for more situations - Fixes prevention of overriding builtin parsers - types for various options consolidated

chore(compartment-mapper): freeze another parserForLanguage

ede29de

chore(compartment-mapper): update type imports

5ebfddf

refactor(compartment-mapper): Distribute parsers between parserForLan…

971bfe8

…guage and languageForExtension

docs(compartment-mapper): News for parserForLanguage and languageForE…

a728b7f

…xtension

fix(compartment-mapper): fix var in bundle.js

5aeb5e6

boneskull force-pushed the boneskull/native-parser branch from 7ff4779 to 5aeb5e6 Compare June 3, 2024 19:43

boneskull requested a review from kriskowal June 3, 2024 19:44

chore(compartment-mapper): update function signature types

3ec2ac3

Co-authored-by: Kris Kowal <kris@agoric.com>

kriskowal changed the title ~~feat(compartment-mapper): custom parser support~~ feat(compartment-mapper): Custom parser support Jun 3, 2024

kriskowal approved these changes Jun 3, 2024

View reviewed changes

docs(compartment-mapper): fix NEWS.md

e49a293

boneskull merged commit 43d867e into master Jun 3, 2024
17 checks passed

boneskull deleted the boneskull/native-parser branch June 3, 2024 23:07

boneskull mentioned this pull request Jun 5, 2024

expose captureFromMap() #2308

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(compartment-mapper): Custom parser support #2304

feat(compartment-mapper): Custom parser support #2304

boneskull commented May 28, 2024 •

edited by kriskowal

Loading

kriskowal left a comment

boneskull commented May 29, 2024

kriskowal commented May 29, 2024

kriskowal commented May 29, 2024

kriskowal May 29, 2024 •

edited

Loading

kriskowal May 31, 2024

boneskull commented May 30, 2024

boneskull commented May 30, 2024

boneskull May 30, 2024 •

edited

Loading

kriskowal May 31, 2024

boneskull May 30, 2024

boneskull May 30, 2024

kriskowal May 31, 2024

boneskull commented May 30, 2024 •

edited

Loading

boneskull May 30, 2024

boneskull May 30, 2024

kriskowal May 31, 2024

boneskull May 30, 2024 •

edited

Loading

kriskowal May 30, 2024

kriskowal May 31, 2024 •

edited

Loading

kriskowal left a comment

kriskowal May 31, 2024

kriskowal May 31, 2024

boneskull Jun 3, 2024

kriskowal May 31, 2024

kriskowal May 31, 2024

boneskull commented Jun 3, 2024

kriskowal left a comment

feat(compartment-mapper): Custom parser support #2304

feat(compartment-mapper): Custom parser support #2304

Conversation

boneskull commented May 28, 2024 • edited by kriskowal Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Compatibility Considerations

Upgrade Considerations

kriskowal left a comment

Choose a reason for hiding this comment

boneskull commented May 29, 2024

kriskowal commented May 29, 2024

kriskowal commented May 29, 2024

kriskowal May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boneskull commented May 30, 2024

boneskull commented May 30, 2024

boneskull May 30, 2024 • edited Loading

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

boneskull commented May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boneskull May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kriskowal May 31, 2024 • edited Loading

Choose a reason for hiding this comment

kriskowal 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

boneskull commented Jun 3, 2024

kriskowal left a comment

Choose a reason for hiding this comment

boneskull commented May 28, 2024 •

edited by kriskowal

Loading

kriskowal May 29, 2024 •

edited

Loading

boneskull May 30, 2024 •

edited

Loading

boneskull commented May 30, 2024 •

edited

Loading

boneskull May 30, 2024 •

edited

Loading

kriskowal May 31, 2024 •

edited

Loading