refactor(shims): bundle endoify dependencies #21
Conversation
Adds an initial test suite (copied from `@endo/ses`) ensuring that this bundled shims work as intended after import. The primary behavior under test is that `import '@ocap/shims/endoify'` produces the same environment as `import 'ses'`. Future tests may also ensure that the bundled `eventual-send` also works as intended.
grypez
force-pushed
the
refactor-shims-inline-endoify
branch
from
5fc51f1 to
b989843
Compare
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is a deprecated package?The maintainer of the package marked it as deprecated. This could indicate that a single version should not be used, or that the package is no longer maintained and any new vulnerabilities will not be fixed. Research the state of the package and determine if there are non-deprecated versions that can be used, or if it should be replaced with a new, supported solution. What is network access?This module accesses the network. Packages should remove all network access that is functionally unnecessary. Consumers should audit network access to ensure legitimate use. What is new author?A new npm collaborator published a version of the package for the first time. New collaborators are usually benign additions to a project, but do indicate a change to the security surface area of a package. Scrutinize new collaborator additions to packages because they now have the ability to publish code into your dependency tree. Packages should avoid frequent or unnecessary additions or changes to publishing rights. What is shell access?This module accesses the system shell. Accessing the system shell increases the risk of executing arbitrary code. Packages should avoid accessing the shell which can reduce portability, and make it easier for malicious shell access to be introduced. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
rekmarks
left a comment
rekmarks
left a comment
There was a problem hiding this comment.
The implementation of bundle.js looks great. Regarding that I only have one nit, namely that it would be great to avoid the extra dependency (which is unmaintained, not that that's really a concern). I'm wondering whether we can serially pipe the read streams to the same or multiple write streams, skipping the intermediary buffer? Potentially worth a try.
My biggest feedback regards the tests. We should definitely test the shims. However, entraining ava and copying over ses tests is too high of a price. The point of all this is to avoid transforming the ses shim, and thereby preclude any issues with its fidelity. All we need to do for now is convince ourselves that the different components of our shims have been applied successfully. Even something like this would do:
assert(Object.isFrozen(Array.prototype)); // Due to `lockdown()`, and therefore `ses`
assert(typeof HandledPromise !== 'undefined'); // Due to eventual sendvitest should work as the runner for this, and if not, we can consider our options.
packages/shims/scripts/bundle.js
Outdated
| await copyFile(path.resolve(src, fileName), path.resolve(dist, fileName)); | ||
| } | ||
| await mkdirp(pkgDist); | ||
| await rimraf(`${pkgDist}/*`); |
There was a problem hiding this comment.
This is my sin originally, but this should be:
| await rimraf(`${pkgDist}/*`); | |
| await rimraf(`${pkgDist}/*`, { glob: true }); |
Without the glob setting, this call will do nothing.
There was a problem hiding this comment.
With regards to the streamcat dependency, I can get rid of it and have an implementation that works without it.
With regards to testing, see #9 (comment)
Resolution to the testing issue is a broader problem; in the interest of timely resolution, and considering that the logic of the implementation is quite small and easily human-evaluated, I am considering simply not testing the implementation. My two reservations are that on principle I prefer tested code to untested code, and in particular since everything we do in this repo will depend on the correct behavior of the shim I think contributors would prefer to know as quickly as possible during development that the shim is broken. I can alleviate these reservations slightly by believing that 'almost all of the tests are suddenly failing' would point us to 'perhaps the shim is broken'.
However, given that ava is capable of testing the shim without these issues, my proposed compromise would be that we keep the ava dev dependency and I adapt the minimal test you proposed:
assert(Object.isFrozen(Array.prototype)); // Due to `lockdown()`, and therefore `ses`
assert(typeof HandledPromise !== 'undefined'); // Due to eventual sendAs @SMotaal is already doing a deep dive into vitest's AST parser (and whatever conflicts lie beyond), it seems likely we will be able to remove the ava dependency in the future.
Closes #9
Changes the way the
@ocap/shimspackage buildssrc/endoifyto copy the contents of its imports directly into the file.Adds tests in
@ocap/shims/testthat suggest the intended behavior of our bundled shims.Adds two new dev dependencies in @ocap/shims