Skip to content
This repository has been archived by the owner on Dec 7, 2022. It is now read-only.

Add regression test for lockdown in UI #100

Open
samsiegart opened this issue Apr 8, 2022 · 11 comments
Open

Add regression test for lockdown in UI #100

samsiegart opened this issue Apr 8, 2022 · 11 comments
Assignees
Labels
bug Something isn't working Inter-protocol UI

Comments

@samsiegart
Copy link
Contributor

samsiegart commented Apr 8, 2022

We need some sort of test in CI that will break if lockdown breaks in the UI, based on the original issue below:

=============== ORIGINAL ISSUE ===============
Commit: 3ccf09b

Repro steps:

agoric install
node ui/use-on-chain-config.js
agoric start --reset
cd ui && yarn start

Open UI and get a blank page and the following in the console:

Uncaught TypeError: lockdown.prototype property not whitelisted
  at completePrototypes (intrinsics.js:100:1)
  at repairIntrinsics (lockdown-shim.js:285:1)
  at lockdown (lockdown-shim.js:407:1)
  at Module.<anonymous> (install-ses-lockdown.js:4:1)
  at Module../src/install-ses-lockdown.js (install-ses-lockdown.js:13:1)
  at __webpack_require__ (bootstrap:851:1)
  at fn (bootstrap:150:1)
  at Module.<anonymous> (defaults.js:2:1)
  at Module../src/index.jsx (index.jsx:11:1)
  at __webpack_require__ (bootstrap:851:1)
  at fn (bootstrap:150:1)
  at Object.1 (config.js:27:1)
  at __webpack_require__ (bootstrap:851:1)
  at checkDeferredModules (bootstrap:45:1)
  at Array.webpackJsonpCallback [as push] (bootstrap:32:1)
  at main.chunk.js:1:101
@samsiegart samsiegart self-assigned this Apr 8, 2022
@samsiegart samsiegart added the bug Something isn't working label Apr 8, 2022
@kriskowal
Copy link
Member

I’ve not seen this failure mode before.

This looks a bit like WebPack may be translating the lockdown arrow function to a function keyword function, which would give it a prototype that it wouldn’t have otherwise.

I’m not sure what changed that would cause this, but we did change package.json not too long ago so that Node.js and Endo would use index.js and not the precompiled ./dist/* files. This solved a problem of double-translation, which resulted in SES censorship. It’s also possible that WebPack got more sophisticated and started using a different entry through exports in package.json.

@turadg
Copy link
Member

turadg commented Apr 8, 2022

^ PR to revert.

I propose we leave this ticket open until there's also a test that would have caught this in CI.

@turadg
Copy link
Member

turadg commented May 18, 2022

@samsiegart is there a test now that would have failed upon 3ccf09b ?

@samsiegart
Copy link
Contributor Author

@samsiegart is there a test now that would have failed upon 3ccf09b ?

Ah no, sorry I closed this too hastily combing through my backlog.

@samsiegart samsiegart reopened this May 18, 2022
@samsiegart samsiegart changed the title UI doesn't load: lockdown.prototype property not whitelisted Add regression test for lockdown in UI May 18, 2022
@erights
Copy link
Member

erights commented May 20, 2022

Why are we still using WebPack? How can we stop?

@samsiegart
Copy link
Contributor Author

Why are we still using WebPack? How can we stop?

https://github.com/facebook/create-react-app includes webpack. We could look into using react without webpack, but that would be unorthodox, and maybe not an example that other devs would want to follow.

Does it matter that much that we use it as long as we don't transform ses?

@erights
Copy link
Member

erights commented May 21, 2022

This error demonstrates that we are transforming ses. But even if we were not, it does matter. How much it matters depends on how much we care about what threats. For example, for supply chain attacks against our UI, it will eventually matter a lot. But only once we have other tooling in place (multiple compartments, lavamoat, ...) that can mitigate those supply chain attacks.

For our immediate goals, once it no longer transforms ses, we can live with the rest for now.

But we should formulate a plan for how to write more defensive ui code over time. For that goal, we cannot have unvetted bundlers transforming any code written to be defensive.

Attn @kriskowal @kumavis @danfinlay @mhofman @dckc

@samsiegart
Copy link
Contributor Author

samsiegart commented May 21, 2022

Understood, and yes I did in fact manage to fix this error by circumventing the transform for ses (see linked PR)

@Tartuffo
Copy link

@samsiegart is this still relevant / needed?

@samsiegart
Copy link
Contributor Author

It's low priority imo. We probably need to set up webdriver tests to verify this behavior.

@Tartuffo
Copy link

okay, moving to MN-1.1

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working Inter-protocol UI
Projects
None yet
Development

No branches or pull requests

6 participants