-
Notifications
You must be signed in to change notification settings - Fork 74
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,pass-style): use non-trapping integrity trait for safety #2675
base: markm-no-trapping-shim
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,8 @@ | |
/* global globalThis */ | ||
/* eslint-disable no-restricted-globals */ | ||
|
||
import '@endo/non-trapping-shim/shim.js'; | ||
|
||
Comment on lines
+17
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I kinda wish we could make SES and other endo packages compatible with the shim having been loaded or not, and then, we could move this from |
||
// We cannot use globalThis as the local name since it would capture the | ||
// lexical name. | ||
const universalThis = globalThis; | ||
|
@@ -75,6 +77,11 @@ export const { | |
setPrototypeOf, | ||
values, | ||
fromEntries, | ||
// https://github.com/endojs/endo/pull/2673 | ||
// @ts-expect-error TS does not yet have this on ObjectConstructor. | ||
isNonTrapping, | ||
// @ts-expect-error TS does not yet have this on ObjectConstructor. | ||
suppressTrapping, | ||
} = Object; | ||
|
||
export const { | ||
|
@@ -125,6 +132,9 @@ export const { | |
ownKeys, | ||
preventExtensions: reflectPreventExtensions, | ||
set: reflectSet, | ||
// https://github.com/endojs/endo/pull/2673 | ||
isNonTrapping: reflectIsNonTrapping, | ||
suppressTrapping: reflectSuppressTrapping, | ||
} = Reflect; | ||
|
||
export const { isArray, prototype: arrayPrototype } = Array; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,7 +30,6 @@ import { | |
apply, | ||
arrayForEach, | ||
defineProperty, | ||
freeze, | ||
getOwnPropertyDescriptor, | ||
getOwnPropertyDescriptors, | ||
getPrototypeOf, | ||
|
@@ -49,6 +48,8 @@ import { | |
FERAL_STACK_GETTER, | ||
FERAL_STACK_SETTER, | ||
isError, | ||
isFrozen, | ||
suppressTrapping, | ||
} from './commons.js'; | ||
import { assert } from './error/assert.js'; | ||
|
||
|
@@ -128,6 +129,10 @@ const freezeTypedArray = array => { | |
* @returns {Harden} | ||
*/ | ||
export const makeHardener = () => { | ||
// TODO Get the native hardener to suppressTrapping at each step, | ||
// rather than freeze. Until then, we cannot use it, which is *expensive*! | ||
// TODO Comment out the following to skip the native hardener. | ||
Comment on lines
+132
to
+134
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah IMO this is another reason to optionally support non-trapping, as it's currently incompatible with a native hardener. |
||
// | ||
// Use a native hardener if possible. | ||
if (typeof globalThis.harden === 'function') { | ||
const safeHarden = globalThis.harden; | ||
|
@@ -182,8 +187,17 @@ export const makeHardener = () => { | |
// Also throws if the object is an ArrayBuffer or any TypedArray. | ||
if (isTypedArray(obj)) { | ||
freezeTypedArray(obj); | ||
if (isFrozen(obj)) { | ||
Comment on lines
189
to
+190
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we modify |
||
// After `freezeTypedArray`, the typed array might actually be | ||
// frozen if | ||
// - it has no indexed properties | ||
// - it is backed by an Immutable ArrayBuffer as proposed. | ||
// In either case, this makes it a candidate to be made | ||
// non-trapping. | ||
suppressTrapping(obj); | ||
} | ||
} else { | ||
freeze(obj); | ||
suppressTrapping(obj); | ||
} | ||
|
||
// we rely upon certain commitments of Object.freeze and proxies here | ||
|
@@ -238,8 +252,6 @@ export const makeHardener = () => { | |
// NOTE: Calls getter during harden, which seems dangerous. | ||
// But we're only calling the problematic getter whose | ||
// hazards we think we understand. | ||
// @ts-expect-error TS should know FERAL_STACK_GETTER | ||
// cannot be `undefined` here. | ||
Comment on lines
-241
to
-242
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very confused why this is no longer necessary. |
||
// See https://github.com/endojs/endo/pull/2232#discussion_r1575179471 | ||
value: apply(FERAL_STACK_GETTER, obj, []), | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might also explain the issues you're trying to work around in #2684