Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

expose captureFromMap() #2308

Merged
merged 5 commits into from
Jun 6, 2024
Merged

expose captureFromMap() #2308

merged 5 commits into from
Jun 6, 2024

Conversation

boneskull
Copy link
Collaborator

@boneskull boneskull commented Jun 5, 2024

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)
  • 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

@boneskull boneskull requested a review from kriskowal June 5, 2024 18:45
@boneskull boneskull self-assigned this Jun 5, 2024
@boneskull boneskull added the enhancement New feature or request label Jun 5, 2024
Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good enough. Some optional copy edits recommended.

I did not hold previous refactors to this bar, so please feel free to punt: would be nice to have a test that shows the freshly exposed functionality. For the parsers refactor, I’m relying on bundleSource to exercise the new interfaces.

packages/compartment-mapper/src/capture-lite.js Outdated Show resolved Hide resolved
packages/compartment-mapper/src/capture-lite.js Outdated Show resolved Hide resolved
@@ -0,0 +1,322 @@
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
/*

makeImportHookMaker,
} from './import-hook.js';
import { link } from './link.js';
import { resolve } from './node-module-specifier.js';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside: I wonder whether someday we come to rue this degree of coupling to Node.js module specifier math. I think when that day came, we’d want to have a resolverHook or something. Making a -lite-lite.js will be untenable.

But this module is not big. It might not be worth worrying about.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree. but this is one of those blessed things we can decide to not decide, since we do not yet have to.

@boneskull
Copy link
Collaborator Author

boneskull commented Jun 5, 2024

@kriskowal I've added some tests: https://github.com/endojs/endo/pull/2308/files#diff-83a914c4839dc5f99eee550a04270efaa250a614f292da572b717e3cde1db18f

Noting that a snapshot would be inappropriate for captureFromMap()'s output, since it contains many absolute paths.

@boneskull boneskull marked this pull request as ready for review June 5, 2024 22:28
@boneskull boneskull merged commit f845665 into master Jun 6, 2024
17 checks passed
@boneskull boneskull deleted the boneskull/capture branch June 6, 2024 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants