Skip to content
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

feat(ses): enablements needed by (old?) mobx #2030

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

erights
Copy link
Contributor

@erights erights commented Feb 2, 2024

closes: #XXXX
refs: #2029

Description

This does not close #2029 , but it does close the first hurdle, the

TypeError: Cannot assign to read only property 'concat' of object '[object Object]'

problem. I have yet to diagnose or fix the

TypeError#2: Cannot perform 'ownKeys' on a proxy that has been revoked

The problem fixed by this PR is interesting. The override mistake is in mobx generated code, where the corresponding mobx source code is a TypeScript class definition. The generated code is not a JS class definition, but rather, seems to target pre-class JS. The surprising mistake is that the generated code is initializing the methods on the "class" .prototype using assignment rather than defineProperty in blatant violation of the JS class semantics. Presumably, this is also a blatant violation of the current TS class semantics, but I don't know. Is some ancient TS compiler at play here? Or some non-standard TS or TS-like compiler? I don't know.

Security Considerations

The errant TS class compilation is itself worrisome, both from general correctness and from security. But that issue is only revealed by this bug and PR, not affected by it. Nevertheless, we should try to understand what's going on, and to send upstream a mobx fix that repairs their class semantics. Since mobx is only a very indirect dependency of ours to which we do not impute any particular security properties, this is likely low urgency.

In the absence of this PR, as explained at #2029, much more violent inhibitions of SES's security were attempted (unsafe-fast.js) and might have been used in practice, resulting is a great loss of hardening and integrity protection. This PR makes that unnecessary, at least as far as this first #2029 problem which raised that possibility.

Scaling Considerations

none

Documentation Considerations

none

Testing Considerations

none

Compatibility Considerations

none

Upgrade Considerations

none. Nothing breaking. Nothing that warrants a NEWS.md update.

  • Includes *BREAKING*: in the commit message with migration instructions for any breaking change.
  • Updates NEWS.md for user-facing changes.

@erights erights force-pushed the markm-2029-workaround-mobix-override-mistake branch from ce78523 to 3005c39 Compare February 2, 2024 08:25
Copy link

@samsiegart samsiegart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to fix the issue in https://github.com/agoric-labs/cosmos-kit-ses, thanks!

I checkout out this branch locally and did:

yarn
cd packages/ses
yarn build
yarn link

Then in https://github.com/agoric-labs/cosmos-kit-ses I did:

yarn link "ses"
yarn build
yarn preview

And was able to run the app without changing any of the source code in main branch. The lockdown options it uses are:

lockdown({
  errorTaming: 'unsafe',
  overrideTaming: 'severe',
  consoleTaming,
});

...which seem to be necessary still.

For using yarn dev, vite seems finicky when doing yarn link, and I had to delete node_modules first so that it would use the linked version of ses instead of the published version that I previously installed.

@erights erights marked this pull request as ready for review February 2, 2024 19:18
@erights erights merged commit 553cb52 into master Feb 2, 2024
14 checks passed
@erights erights deleted the markm-2029-workaround-mobix-override-mistake branch February 2, 2024 19:19
@erights
Copy link
Contributor Author

erights commented Feb 2, 2024

See #1950 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cosmos-kit incompatible with lockdown
2 participants