Skip to content

Conversation

@david0xd
Copy link
Contributor

@david0xd david0xd commented Mar 2, 2023

This PR will fix an issue after changing the prototype of the MessageEvent (source property).

This PR will unblock: MetaMask/snaps#1221

Related ticket: MetaMask/snaps#1132

Context

  • As part of the effort around securing snaps arch, we decided to scuttle all properties of Event based prototypes that might leak DOM/window objects.
  • However, when performing such scuttling, this dep is being affected because it counts on having a direct access to those properties that we scuttle.
  • We fix that by simply preserving the needed descriptors before we scuttle them so we can use them later instead of the direct props access.

@david0xd david0xd self-assigned this Mar 2, 2023
@weizman
Copy link
Contributor

weizman commented Mar 2, 2023

Explanation (context)

  • As part of the effort around securing snaps arch, we decided to scuttle all properties of Event based prototypes that might leak DOM/window objects.
  • However, when performing such scuttling, this dep is being affected because it counts on having a direct access to those properties that we scuttle.
  • We fix that by simply preserving the needed descriptors before we scuttle them so we can use them later instead of the direct props access.

Copy link
Contributor

@weizman weizman left a comment

Choose a reason for hiding this comment

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

Are all those ts-expect-error really needed? no way around those?

@david0xd david0xd marked this pull request as ready for review March 2, 2023 15:43
@david0xd david0xd force-pushed the dd/fix-for-event-security branch from 8f16338 to ef1b439 Compare March 6, 2023 12:35
@david0xd david0xd merged commit 22fe341 into main Mar 7, 2023
@david0xd david0xd deleted the dd/fix-for-event-security branch March 7, 2023 08:42
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.

5 participants