Intro

Thinking out loud on how configuration should look like.

There are a few things to be considered when deciding on the right configuration for the motivation this PR pushes forward:

• It needs to be easy for the user to understand.
• The idea is to be able to configure whether we want Snow to automatically ship scuttling security feature to all potential child realms in the browser LavaMoat build or not.
• In the future, Snow might be used for more stuff than just scuttling.
• In the future, Snow will still only serve the browser-based LavaMoat builds.

Option 1

We could turn scuttleGlobalThis into a multi-value option to represent the 3 possible states:

• 'none'[:default] - do not activate scuttling.
• 'top-realm' - activate scuttling for the top main realm only.
• 'all-realms' - activate scuttling through Snow so it will apply scuttling to all potential child realms automatically.

Few thoughts:

• Could use some help with the naming of the fields.
• This doesn't answer our future potential need where we'll likely be using Snow for other stuff than just scuttling.
• A bit messy?
• 'all-realms' is irrelevant in non-browser-based envs.

Option 2

We could just implement the activation of Snow as a separate config field, such as useSnow:

• false[:default] - don't use Snow for anything.
• true - whenever Snow can be helpful for shipping security to child realms (such as - but not limited to - scuttling), use it.
  • Unless such certain security feature is also configurable and is turned off.

Few thoughts:

• Could use some help with the naming of the field.
• This introduces yet another config field, although boolean is less messy.

Option 3

Any other suggestions? Would love to hear them.

In the future, Snow might be used for more stuff than just scuttling.
It will still be used as a mechanism to deliver changes to other realms transparently and the user should not need exposure to the word "snow" in the config. So no option2

In the future, Snow will still only serve the browser-based LavaMoat builds.
This means we want to make it less likely a Node.js user would attempt to use a feature that's irrelevant for them

For option1:

scuttleGlobalThis: [ 'no', 'yes', 'all-browser-realms' ]

But to address the browser being separate, we could do something like this:

scuttleGlobalThis: boolean,
protectBrowserRealms: [ 'scuttle', 'ban' ]

with 'scuttle' being the only option for now, but open for future expansion

protectBrowserRealms: [ 'scuttle', 'ban' ]

@naugtur why ban?

@weizman In general I agree with the point raised by @naugtur to not go for 2.

If I understand it right, I'm thinking of this as if/how recursion should be done.
I also think it can make sense to make the first-level value into a dict (for extensibility without polluting the options dict).
Putting that together along the line of Option 1:

// 3.1
{
   scuttleGlobalThis: {  // you could optionally recognize a bool here, with `true` being equal to whatever defaults make sense
    enabled: bool,
    exceptions: string[], // both this and existing field would initially be recognized, dropping the old one in a future major release
    recursion: 'full' | 'none', // don't know if it would be useful to have a `number` for "max recursion depth"?
  }
}

if we want to be even more general and not fully close the door on globalThis behavior:

// 3.2
{
   scuttleGlobals: {
    enabled: string[], // default ['globalThis']
    exceptions: string[], // both this and existing field would initially be recognized, dropping the old one in a future major release
    recursion: 'full' | 'none', // don't know if it would be useful to have a `number` for "max recursion depth"?
  }
}

or even

// 3.3
{
   scuttling: {
    globalThis: { // default; keys correspond to globals
      exceptions: string[], // fall back to globalThisExceptions
      recursion: 'full' | 'none', // don't know if it would be useful to have a `number` for "max recursion depth"?
    },
    // , window: {...} etc
  }
}

Going further: Inverting it (configuring options - in this case only scuttling - per global). domain-driven rather than feature-driven configuration:

// 3.4
{
  globals: {
    globalThis: {
      scuttling: {
        enabled: bool,
        exceptions: string[], // fall back to globalThisExceptions
        recursion: 'full' | 'none', // don't know if it would be useful to have a `number` for "max recursion depth"?
    },
    // , window: {...} etc
  }
}

Some variation of any of these seems sensible to me. It's possible that 3.3-3.4 are "overkill" and solving for unneeded granularity (opening for treating different globals differently)

Object.defineProperty(window, 'SCUTTLER', {
        value: (globalRef, scuttle) => {
          console.log('scuttler (start)');
          scuttle(globalRef);
          console.log('scuttler (end)');
        }
})

I don't understand too much, is this the solution?

Where should the following passage be executed?

Object.defineProperty(window, 'SCUTTLER', {
        value: (globalRef, scuttle) => {
          console.log('scuttler (start)');
          scuttle(globalRef);
          console.log('scuttler (end)');
        }
})

@weizman What do you say about rebasing this one on main (there are conflicts to be resolved) and then merging #475 into here as-is? As you're the author of both and #475 is in the desired direction in any case.

@legobeat i would love to, but let's first merge the easier ones #482 and #477 into #475 to handle less PRs

you got it right @weilun0510 - it should be executed at any point before the execution of LavaMoat, so when LavaMoat loads it'll pick up this scuttler option from the global object.

We took a bit too long on this one. Can we ship this in the next release cycle and finish #597 for the release after that?
We've got people waiting for this.

Are there any parts of #597 that are stable and finished that could be moved into this one? We still have time until the current release cycle is done. It'd be best to release node 18 support fully before this.

#597 is not ready to be merged, and picking specifics from it will delay us further, I am against.
These types of things should be reflected after this PR is merged.
I'd appreciate an approve on this PR @naugtur.