refactor: prepare for non-trapping integrity trait #2679
Conversation
erights
force-pushed
the
markm-prepare-for-stabilize-2
branch
2 times, most recently
from
ce2d645 to
526e404
Compare
erights
changed the title
erights
force-pushed
the
markm-prepare-for-stabilize-2
branch
from
526e404 to
b12eb43
Compare
erights
force-pushed
the
markm-prepare-for-stabilize-2
branch
2 times, most recently
from
9925c0c to
6ff2c89
Compare
erights
force-pushed
the
markm-prepare-for-stabilize-2
branch
from
6ff2c89 to
c3b3d92
Compare
There was a problem hiding this comment.
Reading this and thinking through the implications, I have concerns we may be painting ourselves in a corner. Our recommendation is that one should harden values before passing them around. That implies we will never be able to use a Proxy as a remotable value. Is that something we're ok with? I know you have reservations with using O style objects as presences, but I'm a little worried about forever closing the door to this possibility.
| /** | ||
| * `freeze` but not `harden` the proxy target so it remains trapping. | ||
| * @see https://github.com/endojs/endo/blob/master/packages/ses/docs/preparing-for-stabilize.md | ||
| */ | ||
| const funcTarget = freeze(() => {}); | ||
| /** | ||
| * `freeze` but not `harden` the proxy target so it remains trapping. | ||
| * @see https://github.com/endojs/endo/blob/master/packages/ses/docs/preparing-for-stabilize.md | ||
| */ | ||
| const objTarget = freeze(create(null)); |
There was a problem hiding this comment.
Maybe mention it's safe to share these targets between multiple E proxy instances because they're never actually used?
|
|
||
| ## How passable objects should prepare | ||
|
|
||
| Although we think of `passStyleOf` as requiring its input to be hardened, `passStyleOf` instead checked that each relevant object is frozen. Manually freezing all objects reachable from a root object had been equivalent to hardening that root object. With these changes, even such manual transitive freezing will not make an object passable. To prepare for these changes, use `harden` explicitly instead. |
There was a problem hiding this comment.
Oh yeah I didn't think of that. That will potentially create a decent amount of churn on code that was manually freezing. It would be so much easier if frozen regular object would automatically pass trapping checks, but that would effectively transform non-trapping into a "is proxy" predicate.
| * non-trapping integrity trait, we have to `freeze` here but not `harden`. | ||
| * However, once we do have that support, and `passStyleOf` checks that | ||
| * its argument is also non-trapping, we still need to avoid `harden` | ||
| * because that would also hardden `__proto__`. So we will need to |
There was a problem hiding this comment.
| * because that would also hardden `__proto__`. So we will need to | |
| * because that would also harden `__proto__`. So we will need to |
| /** | ||
| * TODO In order to run this test before we have explicit support for a | ||
| * non-trapping integrity trait, we have to `freeze` here but not `harden`. | ||
| * However, once we do have that support, and `passStyleOf` checks that | ||
| * its argument is also non-trapping, we still need to avoid `harden` | ||
| * because that would also hardden `__proto__`. So we will need to | ||
| * explicitly make this non-trapping, which we cannot yet express. | ||
| * @see https://github.com/endojs/endo/blob/master/packages/ses/docs/preparing-for-stabilize.md |
There was a problem hiding this comment.
Ok I was confused at first, but this is a test that explicitly verifies that an object with a non-hardened proto is not passable. Can we make that more clear in the prelude of this comment?
| const tagRecord3 = Object.freeze( | ||
| makeTagishRecord('Alleged: both manually frozen'), | ||
| ); | ||
| const tagRecord3 = harden(makeTagishRecord('Alleged: both manually frozen')); | ||
| /** @type {any} UNTIL https://github.com/microsoft/TypeScript/issues/38385 */ | ||
| const farObj3 = Object.freeze({ | ||
| __proto__: tagRecord3, | ||
| }); | ||
| const farObj3 = harden({ __proto__: tagRecord3 }); |
There was a problem hiding this comment.
I think this should get a TODO to be switched to both manually "non-trapping" instead of using harden.
| get(_shadow, prop) { | ||
| Fail`Please report unexpected scope handler trap: ${q(String(prop))}`; |
There was a problem hiding this comment.
So thinking about this, unless we do something wrong inside ses itself, we should never trigger this for the isNonTrapping trap, right? And even if we did, it would be a safe failure.
Closes: #XXXX
Refs: Agoric/agoric-sdk#10795
Description
Prepare for anticipated introduction and use of the non-trapping integrity trait as explained at https://github.com/endojs/endo/blob/b12eb434b6672f0ceae41be55aac7f24c4562b7b/packages/ses/docs/preparing-for-stabilize.md
These preparations must work now, before these traits are introduced, and should continue to work after these traits are introduced and used.
Security Considerations
Some things that had been deeply frozen automatically by
hardenare now manually frozen by explicit calls tofreeze. We need to review these carefully to ensure that nothing has inadvertently be left unfrozen as a result of the changes in this PR.Some proxies will become unhardenable, but they will still be hardenable as of now, so mistaken hardenings will not be detected.
Scaling Considerations
For this PR by itself, none. Using the shim-based implementation of the non-trapping trait will have scaling consequences: #2675
Documentation Considerations
https://github.com/endojs/endo/blob/b12eb434b6672f0ceae41be55aac7f24c4562b7b/packages/ses/docs/preparing-for-stabilize.md will need to be reflected in developer docs.
Testing Considerations
Since this PR by itself should be a pure refactor with no observable changes, there is nothing to test at this stage. The testing burden will come with #2675 to see how adequate these preparations were.
Compatibility Considerations
The point. This changes to coding patterns that should be compat both with the current status quo and with #2675
Upgrade Considerations
As a pure refactor, none.