[spec] During addModule(), switch to allow navigator but disallow navigator.locks

[xyaoinum]

Currently, navigator.locks is the only available attribute on navigator (i.e., a SharedStorageWorkletNavigator), and we want to prevent Web Locks functionality during addModule().

This change avoids unintentionally breaking existing code that might legitimately read the navigator object (e.g., expects no Exception thrown) during module loading for purposes unrelated to Web Locks.

---

Preview | Diff

[xyaoinum]

@wanderview PTAL. Thanks! (Note that the Web Lock owner approved the behavior change (code)).

[wanderview]

The changes look reasonable for what they do, but I have a questions/comment.

This "filter out things we don't want" approach tends to create a fragile system because new items can be added to navigator later that you have not considered yet. Therefore a new storage-like thing could be added and create a security vulnerability.

As an alternative, have you considered listing exactly what attributes on navigator you believe are safe instead?

[xyaoinum]

This "filter out things we don't want" approach tends to create a fragile system because new items can be added to navigator later that you have not considered yet. Therefore a new storage-like thing could be added and create a security vulnerability.

As an alternative, have you considered listing exactly what attributes on navigator you believe are safe instead?

Thanks for the review @wanderview

The SharedStorageWorkletNavigator is a newly introduced interface specifically for shared storage worklets. Currently, it's defined to expose only the locks attribute. Any future additions would require explicit approval and specification changes by the shared storage team. So I don't see a big risk of unintended exposure.

Regarding "listing exactly what attributes on navigator you believe are safe instead" -- I'm unsure of a normative way to do this within the specification. I imagine we still need to do some checks within the getter methods? And in that case, if we miss a check in any getter, we still introduce the same vulnerability.

[wanderview]

Hmm, I see now I misunderstood the goal. Sorry for my confusion.

Have you considered what this would do to feature detection code that might be running? What if its trying to feature detect the existence of navigator.lock during the initial evaluation of the script. Would this break existing scripts in the wild?

It feels like it would be better to make lock manager methods reject immediately if a lock is created or accessed in addModule instead.

WDYT?

[xyaoinum]

Have you considered what this would do to feature detection code that might be running? What if its trying to feature detect the existence of navigator.lock during the initial evaluation of the script. Would this break existing scripts in the wild?

It feels like it would be better to make lock manager methods reject immediately if a lock is created or accessed in addModule instead.

WDYT?

@wanderview You're right that feature detection could break if errors aren't properly handled or if incorrect assumptions are made about behavior. That's the main motivation behind this change -- moving the throwing logic down a level. However, throwing at the individual method level would make it harder to audit new usage and increase maintenance costs (e.g., the getter methods reside in the shard storage specification, while individual methods are spread across their own specifications).

[wanderview]

Ok, if you have considered the tradeoffs then I'm willing to approve.