Add scuttler prop instead of fixed Snow integration by weizman · Pull Request #475 · LavaMoat/LavaMoat

weizman · 2023-03-26T13:12:48Z

Instead of recursive pass scuttler which will be a string for a reference on the global object for a scuttler function.
Also, a demo of how to use this option to scuttle all realms using snow.

TODO

make sure its clear this is experimental
document this experimental option
make sure node env can't pass this option (irrelevant in node)

legobeat · 2023-03-27T02:20:58Z

      }
-      snow(realm => performScuttleGlobalThis(realm, scuttleGlobalThisExceptions), globalRef)
+      scuttler(globalRef, {
+        scuttle: realm => performScuttleGlobalThis(realm, scuttleGlobalThisExceptions),


previously we had (cb, realm); here we have (realm, {scuttle:cb}).

I would prefer a more typical (realm, cb) and do away with wrapping the function in an object. If we ever need to introduce more extensibility, I would see options as a separate argument, like (realm, cb, {})

(cb, realm) was proposed originally because the only "option" was to use snow, which accepts (cb, realm) because cb is required while realm is optional.
Now that we go with a more generic option that just snow, it makes sense to go by standard which is first realm and cb after.

I do agree with (realm, cb, {}) however, that's a fair suggestion.

(You can see what I mean btw by visiting packages/browserify/examples/01-simple-js/index.html:5 on this PR)

legobeat · 2023-03-27T02:40:40Z

      enabled: scuttleGlobalThisEnabled,
      exceptions: scuttleGlobalThisExceptions,
-      recursive: scuttleGlobalThisRecursive,
+      scuttler: scuttleGlobalThisScuttler,


TBH I find this block (:79-83) a bit terse and tricky to parse and my eyes keep jumping back and forth especially now that scuttleGlobalThisScuttler is a field that is not "the final word" in this scope anymore.

I'd personally find it more readable to have a local options hash instead of local variables for each option.

Something like:

const scuttleOpts = scuttleGlobalThis === true ? scuttleGlobalThisDefaults : { ...scuttleGlobalThis, _scuttler: scuttleGlobalThis?.scuttler && globalRef[scuttleGlobalThis.scuttler] || scuttleGlobalThisDefaults._scuttler }

And then in the following local scope we would refer to e.g. scuttleOpts.enabled instead of scuttleGlobalThisEnabled. We would also be calling scuttleOpts._scuttler, here prefixed with an underscore to distinguish the function-referencing key to the name-string referencing one in the options.

Might just be that I'm not fully accustomed to the new syntax here, just noting.

I think it's really a matter of taste. I'm mostly trying to remain as align as possible with the rest of the codebase previous choices.
You can lookup } = in the codebase - this style is all over pretty much.

Sure, I guess my main point is that with the changes in this PR, to answer the question "what is the effective scuttler", the reader will need walk up through lines 93,85,83,77,63 and make the connection that the falsiness of the empty string in defaults changes the outcome in the default scenario. (or walk down and keep this in mind at :87).

In the proposed alternative above, the logic for the specific field is kept at the same location.

I accepted parts of your suggestion (left the part where an exception is thrown if a scuttler was specified but was not found) 6fe045a

leotm · 2023-03-27T12:19:46Z

i wasn't able to test our example at first ^ so updated our examples separately as follow-up in

Update lavamoat browserify docs #476

(both console errors from before unrelated to scuttler so looks good)

weizman · 2023-03-27T14:17:55Z

added support restriction for scuttler in node env e01281d

weizman · 2023-03-28T16:02:32Z

added documentation PR #477

leotm

lgtm, but would wait on @legobeat

legobeat · 2023-03-30T03:57:12Z

-    if (scuttleGlobalThisRecursive) {
-      if (!globalRef.SNOW) {
+
+    const scuttleOpts = scuttleGlobalThis === true ? scuttleGlobalThisDefaults : scuttleGlobalThis


This will still potentially modify element in the exceptions property of the supplied scuttleGlobalThis down at https://github.com/LavaMoat/LavaMoat/pull/475/files#diff-cbec1016c903fe52b8d76860ebf3400e172b83a3374b8796bab149b3721d8064R113.

Hence preferable to consolidate the instantion and transformations of options with a pattern like

const scuttleOpts = { // [...] scuttlerFunc: scuttleGlobalThis?.scuttler ? ///...., exceptions: scuttleGlobalThis?.exceptions?.map(e => scuttleExceptionToRegexp(e)) || scuttleGlobalThisDefaults.exceptions, }

(the same input validation can still be done, just moved to happen before (or if there is reason to do so, after) the creation of scuttleOpts)

legobeat · 2023-03-30T04:01:18Z

-    if (scuttleGlobalThisRecursive) {
-      if (!globalRef.SNOW) {
+
+    const scuttleOpts = scuttleGlobalThis === true ? scuttleGlobalThisDefaults : scuttleGlobalThis


Same as https://github.com/LavaMoat/LavaMoat/pull/475/files#r1152703084

kumavis · 2023-03-30T08:09:47Z

      // turn scuttleGlobalThis.exceptions regexes strings to actual regexes
-      for (let i = 0; i < scuttleGlobalThisExceptions.length; i++) {
-        const prop = scuttleGlobalThisExceptions[i]
+      for (let i = 0; i < scuttleOpts.exceptions.length; i++) {


for (const prop of ...)

kumavis · 2023-03-30T08:11:46Z

      enabled: true,
      exceptions: [],
-      recursive: false,
+      scuttler: '',


weird default

this prop is expected to be a string, this is the default value of type:string (thought it would make sense to stick to one type if possible)

kumavis · 2023-03-30T08:12:30Z


 function createKernel ({ projectRoot, lavamoatPolicy, canonicalNameMap, debugMode, statsMode, scuttleGlobalThis, scuttleGlobalThisExceptions }) {
+  if (scuttleGlobalThis?.scuttler) {
+    throw new Error('Lavamoat - "scuttleGlobalThis.scuttler" option is not supported for node environment.')


we decided to allow using "scuttlerName" config option in node env

since it's experimental, we decided to allow using it similar to how it's used for browser envs (by defining a prop on globalRef)

later on we'd like to allow passing an actual reference instead of a string prop

fix 6bd2c52

kumavis · 2023-03-30T08:19:14Z

      }
-      snow = globalRef.SNOW
+      scuttleOpts.scuttlerFunc = globalRef[scuttleOpts.scuttler]
    }


maybe should create a configuration obj that holds the scuttlerFunc instead of mutating the options obj

kumavis · 2023-03-30T08:20:23Z

      enabled: true,
      exceptions: [],
-      recursive: false,
+      scuttler: '',


should rename to scuttlerName or something

Co-authored-by: LeoTM <1881059+leotm@users.noreply.github.com>

legobeat · 2023-04-20T02:44:43Z

    }

-    const scuttleOpts = scuttleGlobalThis === true ? scuttleGlobalThisDefaults : scuttleGlobalThis
+    const scuttleOpts = Object.assign({}, scuttleGlobalThis === true ? scuttleGlobalThisDefaults : scuttleGlobalThis)


Why not use the pattern as here instead?

This isn't a deep copy, so for example the input scuttleOpts.exceptions still get mutated here.

you're right, my bad, thank you

#482

legobeat · 2023-04-20T02:45:41Z

    }

-    const scuttleOpts = scuttleGlobalThis === true ? scuttleGlobalThisDefaults : scuttleGlobalThis
+    const scuttleOpts = Object.assign({}, scuttleGlobalThis === true ? scuttleGlobalThisDefaults : scuttleGlobalThis)


Same as above, by https://github.com/LavaMoat/LavaMoat/pull/475/files/28ff29b5ae781bf3209b524934df2fe3e32810ce..bb15afce16a2e997e04e62bf7256c257959329e6#diff-69e3f93883cb0ad47e2b217ba9e92eb890d8855e05d1f6ac66c2051bb636cbdaR11186

you're right, my bad, thank you

#482

propose scuttler

d07f04d

weizman force-pushed the snow-scuttling-integration-propose-scuttler branch from a8d8176 to d07f04d Compare March 26, 2023 13:13

legobeat reviewed Mar 27, 2023

View reviewed changes

improve scuttler api

aea3bbc

weizman marked this pull request as ready for review March 27, 2023 12:07

leotm mentioned this pull request Mar 27, 2023

Update lavamoat browserify docs #476

Merged

This comment was marked as duplicate.

Sign in to view

improve code handling scuttle opts

6fe045a

weizman requested a review from legobeat March 27, 2023 13:53

do not allow scuttler option under node env

e01281d

weizman changed the title ~~[WIP] [Draft] Propose scuttler~~ Add scuttler prop instead of fixed Snow integration Mar 28, 2023

use custom scuttler instead of snow in example

0f7a43d

leotm approved these changes Mar 28, 2023

View reviewed changes

leotm reviewed Mar 29, 2023

View reviewed changes

Comment thread packages/browserify/examples/01-simple-js/index.html Outdated

leotm reviewed Mar 29, 2023

View reviewed changes

Comment thread packages/browserify/examples/01-simple-js/build.js Outdated

legobeat reviewed Mar 30, 2023

View reviewed changes

kumavis reviewed Mar 30, 2023

View reviewed changes

Comment thread packages/browserify/examples/01-simple-js/index.html Outdated

kumavis reviewed Mar 30, 2023

View reviewed changes

weizman and others added 2 commits April 19, 2023 14:06

Update packages/browserify/examples/01-simple-js/index.html

a74064d

Co-authored-by: LeoTM <1881059+leotm@users.noreply.github.com>

make scuttler a separate example

419e2a6

weizman force-pushed the snow-scuttling-integration-propose-scuttler branch from 3cec4e1 to 419e2a6 Compare April 19, 2023 12:53

weizman added 2 commits April 19, 2023 18:09

scuttler to scuttlerName

28ff29b

make sure original opts obj isnt touched

bb15afc

legobeat reviewed Apr 20, 2023

View reviewed changes

enable scuttlerName opt in node

6bd2c52

weizman mentioned this pull request Apr 24, 2023

refactor scuttler opts generation #482

Merged

legobeat mentioned this pull request May 24, 2023

Support configurable hooking into LavaMoat scuttling #462

Merged

refactor scuttler opts generation (#482)

f841974

weizman merged commit 66e156f into snow-scuttling-integration May 24, 2023

weizman deleted the snow-scuttling-integration-propose-scuttler branch May 24, 2023 09:03

Conversation

weizman commented Mar 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

legobeat Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leotm commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as duplicate.

weizman commented Mar 27, 2023

Uh oh!

weizman commented Mar 28, 2023

Uh oh!

leotm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

legobeat Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kumavis Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

weizman commented Mar 26, 2023 •

edited

Loading

legobeat Mar 27, 2023 •

edited

Loading

leotm commented Mar 27, 2023 •

edited

Loading

legobeat Mar 30, 2023 •

edited

Loading

kumavis Mar 30, 2023 •

edited

Loading