Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracking issue for getting 3rd party packages more SES friendly #576

Closed
erights opened this issue Feb 11, 2021 · 58 comments
Closed

Tracking issue for getting 3rd party packages more SES friendly #576

erights opened this issue Feb 11, 2021 · 58 comments
Assignees

Comments

@erights
Copy link
Contributor

erights commented Feb 11, 2021

Fear that tape overrode Function.prototype.apply cleared by @ljharb at #474

Good news about JSS and enzyme Agoric/agoric-sdk#2321 (comment)

Issue filed with vega vega/vega#3075

@erights
Copy link
Contributor Author

erights commented Mar 4, 2021

Agoric/agoric-sdk#2575 patches problems with vega and d3, including the vega problem mentioned above reported at vega/vega#3075

Still need to report problem to d3
Still need to report Agoric/agoric-sdk#2324 to node, that it gets confused if Object.prototype.constructor is an accessor property. Attn @bmeck

@erights
Copy link
Contributor Author

erights commented Mar 7, 2021

vega/vega#3109 would no longer fix vega/vega#3075

@erights
Copy link
Contributor Author

erights commented Mar 16, 2021

We need to file a bug against https://github.com/feross/buffer that their code is incompat with frozen primordials, causing Agoric/agoric-sdk#2663

@erights
Copy link
Contributor Author

erights commented Mar 16, 2021

vega/vega#3109 has been merged and vega/vega#3075 closed. Thanks @jheer !

@erights
Copy link
Contributor Author

erights commented Mar 17, 2021

#621 reports that regenerator-runtime overrides Object.prototype.constructor by assignment. We should file a bug against it. In the meantime we will probably patch it.

@erights
Copy link
Contributor Author

erights commented Mar 22, 2021

More on the regenerator problems
facebook/regenerator#410
facebook/regenerator#411

@kumavis
Copy link
Member

kumavis commented Mar 23, 2021

luxon assigns to toLocaleString on an obj 😿 https://npmfs.com/package/luxon/1.24.1/build/cjs-browser/luxon.js#L7081
however they are using class syntax in their src https://npmfs.com/package/luxon/1.26.0/src/datetime.js#L1503
so likely a result of using babel https://github.com/moment/luxon/blob/91c5f87e7eed66f09165419455f47809990bd4f8/tasks/build.js#L4
and could be fixed there

@kumavis
Copy link
Member

kumavis commented Mar 24, 2021

@formatjs/intl-utils src also has the same export hasOwnProperty that ends up as a conflicting exports.hasOwnProperty assignment. may be tsc performing the esm->cjs transform

@erights
Copy link
Contributor Author

erights commented Mar 26, 2021

We need to file a bug on React so that #642 is no longer needed to avoid https://github.com/Agoric/dapp-token-economy/issues/159 . If we modify React to fix that bug, we should push that PR upstream.

@ljharb
Copy link

ljharb commented Mar 26, 2021

That last one is private; is there somewhere i can read about the bug?

@erights
Copy link
Contributor Author

erights commented Mar 29, 2021

#612 is another conflict with react. Attn @rbuckton @ljharb @AidenRourke

@erights
Copy link
Contributor Author

erights commented Mar 29, 2021

Note that @AidenRourke at #642 says

If I just have an html file that imports ses and calls lockdown there is no error

We do that as well before any of the rest of react. The need for this is yet another problem with react we need to file.

@ljharb
Copy link

ljharb commented Mar 29, 2021

React uses reflect.metadata?

@erights
Copy link
Contributor Author

erights commented Mar 29, 2021

React uses reflect.metadata?

That was within the symptoms we were seeing as well. Specifically, we saw an attempt to add a Reflect."@decorate" method. (Not sure I'm getting the name right)

@rbuckton
Copy link

As far as I can tell, react doesn't use reflect-metadata or Reflect.decorate. I'd need more to go on to be any help.

@AidenRourke
Copy link
Contributor

Hey all, the error I get when trying to use window.lockdown() with react is

index.js:1 failed to delete intrinsics.Array.@wry/context:Slot of type function:  (TypeError#1)

@kumavis
Copy link
Member

kumavis commented Mar 29, 2021

@AidenRourke is this only react or some additional react packages
heres what directly uses @wry/context

was not familiar with wry 👀
https://github.com/benjamn/wryware

A collection of packages that are probably a little too clever. Use at your own wrisk.

perhaps too clever, yes

@rbuckton
Copy link

https://github.com/benjamn/wryware/blob/569d7a6294b77aa9e71d7c7709344aca7cbb75a1/packages/context/src/slot.ts#L126-L135

@kumavis
Copy link
Member

kumavis commented Mar 29, 2021

@AidenRourke this global pollution should gracefully fail if you run lockdown before your dependencies that include @wry/context

@AidenRourke
Copy link
Contributor

@kumavis btw I think what you're doing with lavamoat is super cool.

To answer your question, I experienced the error even with a stock create react app.

Last time I tried to put the ses import and subsequent call to lockdown in the head of the index.html file, I still got the error.

@ljharb
Copy link

ljharb commented Mar 29, 2021

Any chance you have NODE_OPTIONS set?

@erights
Copy link
Contributor Author

erights commented Mar 29, 2021

Any chance you have NODE_OPTIONS set?

We don't. I grepped and the only mention of NODE_OPTIONS anywhere in our code is in an esm-*.patch file: https://github.com/Agoric/agoric-sdk/blob/master/patches/esm%2B3.2.25.patch

@ljharb
Copy link

ljharb commented Mar 29, 2021

@erights meaning, you can reproduce this issue with a stock CRA, with no NODE_OPTIONS set?

@michaelfig
Copy link
Member

@erights meaning, you can reproduce this issue with a stock CRA, with no NODE_OPTIONS set?

That's correct.

@kumavis
Copy link
Member

kumavis commented Mar 30, 2021

stock create react app

lots of stuff in a stock CRA
thanks for the report

@erights
Copy link
Contributor Author

erights commented Mar 31, 2021

@erights
Copy link
Contributor Author

erights commented Jun 9, 2021

@ljharb
Copy link

ljharb commented Jul 7, 2022

Why not stash it on the global instead, behind a symbol when available, with spaces in the name so it doesn’t show up in repl autocomplete? (https://npmjs.com/global-cache does this, fwiw)

@erights
Copy link
Contributor Author

erights commented Jul 7, 2022

spaces in the name so it doesn’t show up in repl autocomplete

Cool! I did not know about that hack.

Putting it on the initial global (with or without the space) is perfectly compatible with Hardened JS. It will not propagate by default to the global of constructed compartments, which is certainly ok by us. We already assume that the start compartment's global contains powerful things we do not know about. We do not remove or harden them by default.

@benjamn
Copy link

benjamn commented Jul 7, 2022

Alright, I think I might be able to use globalThis when available, instead of abusing Array as a namespace.

I've had many headaches over the years obtaining the global object in a reliable cross-browser/environment CSP-respecting way (people will yell at you if you resort to Function("return this")() without trying everything else first), but presumably the environments SES cares about all have globalThis at this point?

@erights
Copy link
Contributor Author

erights commented Jul 8, 2022

but presumably the environments SES cares about all have globalThis at this point?

Yes. (Attn @kriskowal )

@ljharb
Copy link

ljharb commented Jul 8, 2022

@benjamn fwiw global-cache uses global, which non-broken node module bundlers automatically should convert to window or globalThis or self etc

aduh95 pushed a commit to nodejs/node that referenced this issue Jul 24, 2022
PR-URL: #43907
Fixes: #43496
Refs: endojs/endo#576
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
@erights
Copy link
Contributor Author

erights commented Jul 25, 2022

Alright, I think I might be able to use globalThis when available, instead of abusing Array as a namespace.

Hi @benjamn , any progress on this? Anything we can do to help?

Filed benjamn/wryware#347 so we can track this from your side too.

Done!

danielleadams pushed a commit to nodejs/node that referenced this issue Jul 26, 2022
PR-URL: #43907
Fixes: #43496
Refs: endojs/endo#576
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
targos pushed a commit to nodejs/node that referenced this issue Jul 31, 2022
PR-URL: #43907
Fixes: #43496
Refs: endojs/endo#576
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
targos pushed a commit to nodejs/node that referenced this issue Jul 31, 2022
PR-URL: #43907
Fixes: #43496
Refs: endojs/endo#576
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
targos pushed a commit to nodejs/node that referenced this issue Aug 1, 2022
PR-URL: #43907
Fixes: #43496
Refs: endojs/endo#576
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Fyko pushed a commit to Fyko/node that referenced this issue Sep 15, 2022
PR-URL: nodejs#43907
Fixes: nodejs#43496
Refs: endojs/endo#576
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
guangwong pushed a commit to noslate-project/node that referenced this issue Oct 10, 2022
PR-URL: nodejs/node#43907
Fixes: nodejs/node#43496
Refs: endojs/endo#576
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
@erights
Copy link
Contributor Author

erights commented Oct 13, 2022

The stack-utils packages trips the override mistake. Patches at tapjs/stack-utils#70 and Agoric/agoric-sdk#6451 . PR at tapjs/stack-utils#71

Done!

@erights
Copy link
Contributor Author

erights commented Jan 8, 2023

Immer

See immerjs/immer#914 and #1433

See messages starting at #576 (comment) above

@dckc
Copy link
Contributor

dckc commented Aug 27, 2023

When trying to import google-sheets, I hit Cannot assign to read only property 'constructor' somewhere in axios:

stack trace
TypeError#1: Cannot assign to read only property 'constructor' of object '[object Object]'
  at file:///home/connolly/projects/finquick/packages/fincaps/node_modules/axios/lib/utils.js:545:32
  at forEach (file:///home/connolly/projects/finquick/packages/fincaps/node_modules/axios/lib/utils.js:265:10)
  at Object.reduceDescriptors (file:///home/connolly/projects/finquick/packages/fincaps/node_modules/axios/lib/utils.js:542:3)
  at file:///home/connolly/projects/finquick/packages/fincaps/node_modules/axios/lib/core/AxiosHeaders.js:286:7

The usual Object.defineProperty alternative seems to make the symptoms go away:

axios+1.5.0.patch
diff --git a/node_modules/axios/lib/utils.js b/node_modules/axios/lib/utils.js
index a386b77..ea8bbd6 100644
--- a/node_modules/axios/lib/utils.js
+++ b/node_modules/axios/lib/utils.js
@@ -542,7 +542,7 @@ const reduceDescriptors = (obj, reducer) => {
   forEach(descriptors, (descriptor, name) => {
     let ret;
     if ((ret = reducer(descriptor, name, obj)) !== false) {
-      reducedDescriptors[name] = ret || descriptor;
+      Object.defineProperty(reducedDescriptors, name, { value: ret || descriptor });
     }
   });

@erights
Copy link
Contributor Author

erights commented Aug 27, 2023

Tracking rbuckton/reflect-metadata#130 .

I should have added it to this list ages ago, but somehow forgot.

@mhofman
Copy link
Contributor

mhofman commented Sep 22, 2023

I just hit some issues trying to get agoric-cli working without severe taming.

Relevant stack traces:
(TypeError#1)
TypeError#1: Override property constructor
  at Error.setter (file:///tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/ses/src/enable-property-overrides.js:116:27)
  at createErrorType (/tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/follow-redirects/index.js:600:37)
  at Object.<anonymous> (/tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/follow-redirects/index.js:18:23)

(TypeError#6)
TypeError#6: Override property constructor
  at Object.setter (file:///tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/ses/src/enable-property-overrides.js:116:27)
  at newError (/tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/cosmjs-types/node_modules/protobufjs/src/util/minimal.js:283:74)
  at Object.<anonymous> (/tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/cosmjs-types/node_modules/protobufjs/src/util/minimal.js:313:22)

(TypeError#8)
TypeError#8: Override property constructor
  at Object.setter (file:///tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/ses/src/enable-property-overrides.js:116:27)
  at Object.createErrorClass (/tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/rxjs/dist/cjs/internal/util/createErrorClass.js:11:36)

(TypeError#15)
TypeError#15: Override property constructor
  at __.setter (file:///tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/ses/src/enable-property-overrides.js:116:27)
  at new __ (/tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/external-editor/main/errors/CreateFileError.js:17:42)
  at __extends (/tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/external-editor/main/errors/CreateFileError.js:18:84)
  at /tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/external-editor/main/errors/CreateFileError.js:23:5
  at Object.<anonymous> (/tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/external-editor/main/errors/CreateFileError.js:38:2)

(TypeError#16)
TypeError#16: Override property constructor
  at __.setter (file:///tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/ses/src/enable-property-overrides.js:116:27)
  at new __ (/tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/external-editor/main/errors/LaunchEditorError.js:17:42)
  at __extends (/tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/external-editor/main/errors/LaunchEditorError.js:18:84)
  at /tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/external-editor/main/errors/LaunchEditorError.js:23:5
  at Object.<anonymous> (/tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/external-editor/main/errors/LaunchEditorError.js:38:2)

(TypeError#17)
TypeError#17: Override property constructor
  at __.setter (file:///tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/ses/src/enable-property-overrides.js:116:27)
  at new __ (/tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/external-editor/main/errors/ReadFileError.js:17:42)
  at __extends (/tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/external-editor/main/errors/ReadFileError.js:18:84)
  at /tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/external-editor/main/errors/ReadFileError.js:23:5
  at Object.<anonymous> (/tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/external-editor/main/errors/ReadFileError.js:38:2)

(TypeError#18)
TypeError#18: Override property constructor
  at __.setter (file:///tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/ses/src/enable-property-overrides.js:116:27)
  at new __ (/tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/external-editor/main/errors/RemoveFileError.js:17:42)
  at __extends (/tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/external-editor/main/errors/RemoveFileError.js:18:84)
  at /tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/external-editor/main/errors/RemoveFileError.js:23:5
  at Object.<anonymous> (/tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/external-editor/main/errors/RemoveFileError.js:38:2)

(TypeError#19)
TypeError#19: Override property constructor
  at Object.setter (file:///tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/ses/src/enable-property-overrides.js:116:27)
  at _interopRequireWildcard (/tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/@babel/highlight/lib/index.js:12:710)
  at Object.<anonymous> (/tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/@babel/highlight/lib/index.js:10:14)

(TypeError#20)
TypeError#20: Override property constructor
  at Object.setter (file:///tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/ses/src/enable-property-overrides.js:116:27)
  at _interopRequireWildcard (/tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/@babel/code-frame/lib/index.js:11:710)
  at Object.<anonymous> (/tmp/registry-home.UVpYi/.npm/_npx/417013ce1422aeac/node_modules/@babel/code-frame/lib/index.js:9:14)

Issues already mitigated by moderate taming:

New issue requiring taming of %ObjectPrototype%.constructor:

@erights
Copy link
Contributor Author

erights commented Nov 3, 2023

See #1846

@kriskowal
Copy link
Member

kriskowal commented Nov 4, 2023

chalk@4 all versions chalk/chalk#619

@erights
Copy link
Contributor Author

erights commented Dec 8, 2023

@endojs endojs locked and limited conversation to collaborators Jan 10, 2024
@kriskowal kriskowal converted this issue into discussion #1950 Jan 10, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests