feat(lavamoat/lavadome): update integration to improve security #25653
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #25653 +/- ##
========================================
Coverage 70.02% 70.02%
========================================
Files 1443 1443
Lines 50164 50164
Branches 14039 14039
========================================
Hits 35126 35126
Misses 15038 15038 ☔ View full report in Codecov by Sentry. |
weizman
changed the title
weizman
changed the title
weizman
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
6bdd808 passed successfully |
This comment was marked as outdated.
This comment was marked as outdated.
Builds ready [6338a91]
Page Load Metrics (1744 ± 91 ms)
Bundle size diffs
|
legobeat
force-pushed
the
weizman-patch-lavadome-try
branch
from
6338a91 to
f7a170b
Compare
legobeat
force-pushed
the
weizman-patch-lavadome-try
branch
from
f7a170b to
965044f
Compare
|
Builds ready [965044f]
Page Load Metrics (1949 ± 214 ms)
Bundle size diffs
|
Builds ready [ad0e483]
Page Load Metrics (1878 ± 64 ms)
Bundle size diffs
|
| @@ -1,5 +1,9 @@ | |||
| // Disabled to allow setting up initial state hooks first | |||
|
|
|||
| // This import sets up safe intrinsics required for LavaDome to function securely. | |||
| // It must be run first so that no other code can undermine it. | |||
| import '@lavamoat/lavadome-react'; | |||
There was a problem hiding this comment.
are the intrinsic not sufficiently protected by lavamoat?
There was a problem hiding this comment.
no because LavaMoat protects TC39 JS intrinsics without taking platform oriented intrinsics into account (such as dom/web apis)
There was a problem hiding this comment.
Just FYI: there are 8 or 9 other files that already run before this "first" thing here (not including the lavamoat runtime wrapper):
- _initialize.ts (in webpack build only)
- sentry-install
- @lavamoat/snow/snow.prod
- use-snow
- lockdown-install
- lockdown-run
- lockdown-more
- init-globals
- runtime-cjs
And the next line after this import '@lavamoat/lavadome-react'; is:
// This import sets up global functions required for Sentry to function.
// It must be run first in case an error is thrown later during initialization.
import './lib/setup-initial-state-hooks';
So it is looking like we have a few files that already run before our "first" files, and we have two files that now "must be run first" (that won't be). I don't mind having a "first" comment in this file (despite it not actually being first in the context of the entire application), but we should certainly have only one of these comments in this file.
My current thinking is that ./lib/setup-initial-state-hooks should be run before lavadome-react unless you can guarantee that @lavamoat/lavadome-react will absolutely never be the cause of an error.
In either case (setup-initial-state-hooks or lavadoem-react running "first"), can you update the comments to reflect their actual "firstness"? :-)
Builds ready [a1b9251]
Page Load Metrics (1567 ± 47 ms)
Bundle size diffs
|
Builds ready [bfb52b7]
Page Load Metrics (1602 ± 45 ms)
Bundle size diffs
|
Address concerns under Safe Usage:
This should go with #27756