Skip to content

Fix for PR #382 [scuttling]#391

Merged
weizman merged 16 commits into
mainfrom
scuttle-start-comp-global-this-fix-pr-382-bug
Nov 14, 2022
Merged

Fix for PR #382 [scuttling]#391
weizman merged 16 commits into
mainfrom
scuttle-start-comp-global-this-fix-pr-382-bug

Conversation

@weizman

@weizman weizman commented Oct 30, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

1) Fix the bug

PR #382 code modifications introduce a bug where the array of props to scuttle is no longer an array of props but an array of arrays of props:

props = [a,b,c, d,e,f] // beofer bug introduced
props = [[a,b,c], [d,e,f]] // after bug introduced

This canceled scuttling completely, because instead of attempting to redefine a certain property my_prop:

Object.defineProperty(window, 'my_prop', {})

We ended up with:

Object.defineProperty(window, ['my_prop'], {})

commit introducing the bug reference

2) Improve and fix tests to work with scuttling feature for packages browserify,core,node

Tests infra was not compatible to work in scuttling mode, and the browserify infra required further adjustments to serve us in the future more simply:

  1. rewrite the scuttling tests to actually do their job
  2. change runScenarios calls so that each scenario will determine for itself whether to apply scuttling mode or not (instead of current situation where scuttling can be turned on/off only for all scenarios)
  3. fix browserify testing infra so that it'll rely on the local versions of core/node/lavapack/browserify deps rather than the ones from npm (made it very hard to test modifications to both browserify package and another package in a single pr)
  4. extend testing infra to not only allow expected failure, but also test the failure exception to include a certain rgx
  5. fix testing infra to allow passing array arguments via cli (so we can pass scuttleGlobalThisExceptions arg)
  6. extend testing infra vm globalThis to allow access to real global object in tests exclusively (here's why)
  7. split back __lavamoatDebugOptions__ and __lavamoatSecurityOptions__ and only replace the __lavamoatSecurityOptions__ placeholder if was explicitly required (this is because both lavapack and core can set __lavamoatSecurityOptions__, but since one uses the other potentially, then one must sometimes skip the replacement in order to allow the other to successfully set it)
  8. remove LavaPack from the avoidForLavaMoatCompatibility, and freeze it instead to @kumavis's request

@kumavis

kumavis commented Oct 31, 2022

Copy link
Copy Markdown
Member

@weizman nice catch - sorry to introduce this bug

@kumavis

kumavis commented Oct 31, 2022

Copy link
Copy Markdown
Member

this change reintroduces the exceptions, we shouldn't need exceptions for normal usage. in the case that we need them for the kernel, we should capture before scuttling and pull it from there

@weizman weizman requested review from kumavis and naugtur November 6, 2022 14:38
@weizman weizman mentioned this pull request Nov 7, 2022
Merged
@kumavis

kumavis commented Nov 7, 2022

Copy link
Copy Markdown
Member

since directories are modified after install, tests may break if deps change

@weizman weizman mentioned this pull request Nov 8, 2022
Open
@weizman weizman merged commit e90070f into main Nov 14, 2022
@weizman weizman deleted the scuttle-start-comp-global-this-fix-pr-382-bug branch November 14, 2022 15:16
legobeat added a commit that referenced this pull request May 30, 2023
@legobeat
node/version - 7.0.0
cbf12c3 
c4857f3 browserify,node,tofu,viz: bump @babel packages to latest
d557312 devDeps: eslint@^7.32.0->^8.40.0; plugins; lint:fix (#568)
93e1d2a devDeps: minor bump eslint packages (#568)
85a6ae2 core/version - 14.1.0
ccdd3d6 node/deps: yargs@16->17
73e54f6 deps: node-gyp-build@4.2.3->4.6.0
849f288 node/deps: json-stable-stringify@^1.0.1->^1.0.2
e0bf366 node/deps: remove object.fromentries (#555)
82beffa restrict engines.node to < 19.0.0
ced99e5 Bump minimatch from 3.0.4 to 3.1.2 in /packages/node/examples/express
d5de5d5 drop uneeded additionalOpts arg and migrate back into using scenario object (#471)
dc027eb core/version - 14.0.0
9a4759e node: yarn test:prep: force NODE_ENV=development for test projects setup
1de0a17 core/version - 13.0.0
23a9b06 Bump decode-uri-component in /packages/node/examples/express
ec1c9b7 Bump decode-uri-component in /packages/node/test/projects/2
d8ea54f Bump express from 4.17.1 to 4.17.3 in /packages/node/examples/express
73dec5d Bump versions for core/lavapack/browserify (#431)
28e98d4 Bump minimatch from 3.0.4 to 3.1.2 in /packages/node/test/projects/2
e12aedd lint:fix
190391a fix: remove redundant engine entries from package.json (#411)
536e663 Less scary warning in README.md? (#403)
084fbb6 Improve eslint (#409)
cee9caf Bump minimum nodejs version to 14.0.0
e90070f Fix for PR #382 [scuttling] (#391)
legobeat added a commit that referenced this pull request May 30, 2023
@legobeat
node/version - 7.0.0
d0f635f 
c4857f3 browserify,node,tofu,viz: bump @babel packages to latest
d557312 devDeps: eslint@^7.32.0->^8.40.0; plugins; lint:fix (#568)
93e1d2a devDeps: minor bump eslint packages (#568)
85a6ae2 core/version - 14.1.0
ccdd3d6 node/deps: yargs@16->17
73e54f6 deps: node-gyp-build@4.2.3->4.6.0
849f288 node/deps: json-stable-stringify@^1.0.1->^1.0.2
e0bf366 node/deps: remove object.fromentries (#555)
82beffa restrict engines.node to < 19.0.0
ced99e5 Bump minimatch from 3.0.4 to 3.1.2 in /packages/node/examples/express
d5de5d5 drop uneeded additionalOpts arg and migrate back into using scenario object (#471)
dc027eb core/version - 14.0.0
9a4759e node: yarn test:prep: force NODE_ENV=development for test projects setup
1de0a17 core/version - 13.0.0
23a9b06 Bump decode-uri-component in /packages/node/examples/express
ec1c9b7 Bump decode-uri-component in /packages/node/test/projects/2
d8ea54f Bump express from 4.17.1 to 4.17.3 in /packages/node/examples/express
73dec5d Bump versions for core/lavapack/browserify (#431)
28e98d4 Bump minimatch from 3.0.4 to 3.1.2 in /packages/node/test/projects/2
e12aedd lint:fix
190391a fix: remove redundant engine entries from package.json (#411)
536e663 Less scary warning in README.md? (#403)
084fbb6 Improve eslint (#409)
cee9caf Bump minimum nodejs version to 14.0.0
e90070f Fix for PR #382 [scuttling] (#391)
legobeat added a commit that referenced this pull request Jun 2, 2023
@legobeat
node/version - 7.0.0
5631d34 
c4857f3 browserify,node,tofu,viz: bump @babel packages to latest
d557312 devDeps: eslint@^7.32.0->^8.40.0; plugins; lint:fix (#568)
93e1d2a devDeps: minor bump eslint packages (#568)
85a6ae2 core/version - 14.1.0
ccdd3d6 node/deps: yargs@16->17
73e54f6 deps: node-gyp-build@4.2.3->4.6.0
849f288 node/deps: json-stable-stringify@^1.0.1->^1.0.2
e0bf366 node/deps: remove object.fromentries (#555)
82beffa restrict engines.node to < 19.0.0
ced99e5 Bump minimatch from 3.0.4 to 3.1.2 in /packages/node/examples/express
d5de5d5 drop uneeded additionalOpts arg and migrate back into using scenario object (#471)
dc027eb core/version - 14.0.0
9a4759e node: yarn test:prep: force NODE_ENV=development for test projects setup
1de0a17 core/version - 13.0.0
23a9b06 Bump decode-uri-component in /packages/node/examples/express
ec1c9b7 Bump decode-uri-component in /packages/node/test/projects/2
d8ea54f Bump express from 4.17.1 to 4.17.3 in /packages/node/examples/express
73dec5d Bump versions for core/lavapack/browserify (#431)
28e98d4 Bump minimatch from 3.0.4 to 3.1.2 in /packages/node/test/projects/2
e12aedd lint:fix
190391a fix: remove redundant engine entries from package.json (#411)
536e663 Less scary warning in README.md? (#403)
084fbb6 Improve eslint (#409)
cee9caf Bump minimum nodejs version to 14.0.0
e90070f Fix for PR #382 [scuttling] (#391)
legobeat added a commit that referenced this pull request Jun 2, 2023
@legobeat
node/version - 7.0.0
422f842 
c4857f3 browserify,node,tofu,viz: bump @babel packages to latest
d557312 devDeps: eslint@^7.32.0->^8.40.0; plugins; lint:fix (#568)
93e1d2a devDeps: minor bump eslint packages (#568)
85a6ae2 core/version - 14.1.0
ccdd3d6 node/deps: yargs@16->17
73e54f6 deps: node-gyp-build@4.2.3->4.6.0
849f288 node/deps: json-stable-stringify@^1.0.1->^1.0.2
e0bf366 node/deps: remove object.fromentries (#555)
82beffa restrict engines.node to < 19.0.0
ced99e5 Bump minimatch from 3.0.4 to 3.1.2 in /packages/node/examples/express
d5de5d5 drop uneeded additionalOpts arg and migrate back into using scenario object (#471)
dc027eb core/version - 14.0.0
9a4759e node: yarn test:prep: force NODE_ENV=development for test projects setup
1de0a17 core/version - 13.0.0
23a9b06 Bump decode-uri-component in /packages/node/examples/express
ec1c9b7 Bump decode-uri-component in /packages/node/test/projects/2
d8ea54f Bump express from 4.17.1 to 4.17.3 in /packages/node/examples/express
73dec5d Bump versions for core/lavapack/browserify (#431)
28e98d4 Bump minimatch from 3.0.4 to 3.1.2 in /packages/node/test/projects/2
e12aedd lint:fix
190391a fix: remove redundant engine entries from package.json (#411)
536e663 Less scary warning in README.md? (#403)
084fbb6 Improve eslint (#409)
cee9caf Bump minimum nodejs version to 14.0.0
e90070f Fix for PR #382 [scuttling] (#391)
legobeat added a commit that referenced this pull request Jun 2, 2023
@legobeat
node/version - 7.0.0
5915a0d 
- fix: Node.js 18 compatibility (#551)
- deps: lavamoat-core@^12.3.0->^14.1.1
- deps: bump @babel packages to latest (#587)
- deps: bump json-stable-stringify, node-gyp-build, resolve, yargs (#556)
- deps: remove object.fromentries (#555)
- change: BREAKING: Minimum Node.js version 14 (#398)
- fix: Properly add scuttling functionality (#382, #391)
- change: update examples
@legobeat legobeat mentioned this pull request Jun 2, 2023
Merged
boneskull pushed a commit to boneskull/LavaMoat that referenced this pull request Feb 6, 2024
@legobeat
node/version - 7.0.0
3e297cd 
- fix: Node.js 18 compatibility (LavaMoat#551)
- deps: lavamoat-core@^12.3.0->^14.1.1
- deps: bump @babel packages to latest (LavaMoat#587)
- deps: bump json-stable-stringify, node-gyp-build, resolve, yargs (LavaMoat#556)
- deps: remove object.fromentries (LavaMoat#555)
- change: BREAKING: Minimum Node.js version 14 (LavaMoat#398)
- fix: Properly add scuttling functionality (LavaMoat#382, LavaMoat#391)
- change: update examples
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kumavis kumavis kumavis approved these changes

@naugtur naugtur Awaiting requested review from naugtur naugtur is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@weizman @kumavis @naugtur