fix(exo): allow richer behaviorMethods by erights · Pull Request #1809 · endojs/endo

erights · 2023-10-06T15:13:49Z

closes: #1817
refs: #1815 #1808 #1773 #1766

Description

The intention of the exo-making methods was to be able to use a JS class' .prototype as a source of raw methods. This PR

fixes the bugs reported in Exo classes need to handle non-enumerable raw methods correctly. #1817 that prevented use of JS classes as a source of raw methods for an exo class.
additionally accommodates JS class inheritance so that superclasses can also provide raw classes.
provides tests that show use of JS classes to construct Exo classes.
these tests also provide hypothetical helper abstractions for better supporting such use of classes.
- These helpers remain in tests because this PR merely demonstrates that such support is possible, without committing to the particular helpers to actually provide. We need at least to bikeshed from the currently awful names (defineExoClassFromJSClass).
these tests demonstrate the use of both super and late-bound self in inheritance among these methods, with super calls bypassing guards, while self calls are guarded.

This PR also provides tests demonstrating the use of JS classes to directly provide unguarded behavior at the @endo/pass-style / Far level of abstraction. This PR makes no changes to the src/ code at this level, demonstrating what support is already possible. Unlike the above exo-level support, we do not recommend the coding style shown in these pass-style level tests. However, neither can we reasonably prevent it, so we need to understand it.

Security Considerations

Corresponding examples at

endo/packages/pass-style/test/test-far-wobbly-point.js

Lines 132 to 144 in d50a60b

    
           // This test, though correct, demonstrates a sucurity weakness of 
        
           // this approach to JS class inheritance at this 
        
           // `@endo/pass-style` / `Far` level of abstraction. The weakness 
        
           // is that the overridden method from a superclass can nevertheless 
        
           // be directly applied to an instance of the subclass. The 
        
           // subclass override did not suppress the use of the overridden 
        
           // method as, effectively, part of the subclass' instance's public 
        
           // API. 
        
           // 
        
           // See the corresponding example at 
        
           // `test-exo-wobbly-point.js` to see the absence of this vulnerability 
        
           // at the Exo level of abstraction. 
        
           t.is(apply(otherPt.getX, otherWpt, []), 3);

and

endo/packages/exo/test/test-exo-wobbly-point.js

Lines 183 to 193 in d50a60b

    
           // This error behavior shows the absence of the security vulnerability 
        
           // explained at the corresponding example in `test-far-wobbly-point.js` 
        
           // for the `@endo/pass-style` / `Far` level of abstraction. At the exo level 
        
           // of abstraction, a raw-method subclass overriding an inherited superclass 
        
           // method denies that method to clients of instances of the subclass. 
        
           // At the same time, this overridden method remains available within 
        
           // the overriding subclass via unguarded `super` calls. 
        
           t.throws(() => apply(otherPt.getX, otherWpt, []), { 
        
             message: 
        
               '"In \\"getX\\" method of (ExoPoint)" may only be applied to a valid instance: "[Alleged: ExoWobblyPoint]"', 
        
           });

demonstrates that the demonstrated non-recommended use of class inheritance at the far level of abstraction has a security vulnerability absent from the use of class inheritance at the exo level of abstraction. This is one reason why we do not recommend the former but may recommend the latter.

Scaling Considerations

If classes had large number of methods and class inheritance chains were deep, this has somewhat worse scaling than a JS class programmer would normally expect. However, if either of these are reasonably small, there should not be any significant scaling problem.

Documentation Considerations

If we decide that we like and wish to promote the use of JS classes demonstrated by this PR, we should move some support (defineExoClassFromJSClass) into exported sources and we should document the whole approach. The purpose of this PR is not to establish such practice or decide on its exact shape, but merely to demonstrate that this PR makes such JS class-based practice possible.

In particular, we may be able to improve of the usability shown in this PR by using JS class decorators. It is a worthy experiment. (attn @mhofman )

Testing Considerations

The src changes that this PR makes are only to exo-tools.js code shared among heap, virtual, and durable exos. However, the tests only test the behavior of heap exos. Once agoric-sdk is upgraded to depend on the changes from this PR, we should reproduce these tests for virtual and durable exos. Ideally, we would use Zones to write the tests once and then reuse it to test all three types of zone.

This PR makes no effort to test JS classes used for facets of exo kits. Nor does it provide any hypothetical helper functions to help write such exo kits. If we decide that this code pattern for using JS classes is attractive, a later PR should indeed extend the approach to exo kits.

Upgrade Considerations

This PR makes no changes to data. For existing exo-making code that only uses object literals for own enumerable methods, this PR should not result in any behavior change or even internal representation change. The only code it should make a difference for is code that previously did not work because it needed the fixes and features provided by this PR. We are not aware of any such code prior to this PR.

erights · 2023-10-29T21:50:10Z

@michaelfig , reviewers,

I rebased and resolved merge conflicts with #1831

packages/exo/src/exo-tools.js

packages/exo/package.json

packages/exo/test/test-exo-class-js-class.js

packages/exo/test/test-exo-wobbly-point.js

packages/exo/test/test-non-enumerable-methods.js

packages/pass-style/package.json

erights · 2023-11-06T23:12:38Z

At #1809 (review) @michaelfig writes:

Looking good, just some quibbles. I'd approve based on their resolution,

All quibbles resolved. PTAL.

but I have a feeling that some of the other reviewers you tagged should weigh in first.

@turadg @mhofman , Care to? Thanks.

erights · 2023-11-08T03:30:51Z

Reviewers,
In case @kriskowal is able, before the hackathon, to do a new endo release & upgrade agoric-sdk to depend on it, I'd really like to be able to get this in so I could build on it. Given the reviewing @michaelfig has already done, you may find the remaining review work could be done quickly, perhaps? Just sayin ;) . Thanks.

turadg

I raised some issues at the periphery with assertions and documentation.

We need at least to bikeshed from the currently awful names (defineExoClassFromJSClass).

I think "bikeshed" here is facetious but I want to point out that API naming in Endo merits careful consideration. They will stick around a long time and we want to avoid developers being confused by them or having misconceptions about what they do.

That said, I don't think defineExoClassFromJSClass is awful. It's long but it says what it does. It's probably also not the end state for the dev-facing API because I expect defining the class with decorators will be the most common.

packages/exo/src/exo-tools.js

packages/exo/test/test-exo-class-js-class.js

packages/pass-style/src/make-far.js

erights · 2023-11-10T23:18:26Z

We need at least to bikeshed from the currently awful names (defineExoClassFromJSClass).

I think "bikeshed" here is facetious but I want to point out that API naming in Endo merits careful consideration. They will stick around a long time and we want to avoid developers being confused by them or having misconceptions about what they do.

I agree completely, which is why these awful names --- as well as the entirety of the defineExoClassFromJSClass helper abstractions --- are in a test/ file rather than a src/ file. I want to put a lot more thought into them before providing them for actual usage. Their purpose in the test is to establish the that src/ changes of this PR enable such things to be possible.

That said, even as a test/ POC of possibility, I'm happy to consider suggested improvements.

turadg

No blocking concerns. I do think the underscores are worth considering before merging.

I'd also suggest cleaning up the commits to reduce noise in the changelog. Most of the "fix" are really "fixup" that should be squashed into noteworthy commits.

Btw, if a commit shouldn't appear in the changelog, it shouldn't have conventional commit message. Noise in the commit history isn't as bad as noise in the changelog.

packages/pass-style/src/make-far.js

erights · 2023-11-12T02:28:05Z

I'd also suggest cleaning up the commits to reduce noise in the changelog. Most of the "fix" are really "fixup" that should be squashed into noteworthy commits.

Btw, if a commit shouldn't appear in the changelog, it shouldn't have conventional commit message. Noise in the commit history isn't as bad as noise in the changelog.

Manually squashed those away before merging. Done.

erights self-assigned this Oct 6, 2023

erights force-pushed the markm-objectMetaMap branch 2 times, most recently from 55c660a to ebf8a11 Compare October 6, 2023 23:02

erights changed the title ~~feat(patterns): objectMetaMap~~ fix(exo): allow richer behaviorMethods Oct 6, 2023

erights force-pushed the markm-objectMetaMap branch 3 times, most recently from 61b2bb5 to 82f1d2c Compare October 7, 2023 04:08

This was referenced Oct 7, 2023

fix(exo): allow non-enumerable methods #1808

Closed

fix(exo): methods might be non-enumerable #1773

Closed

erights force-pushed the markm-objectMetaMap branch from 82f1d2c to d113f32 Compare October 7, 2023 04:20

erights mentioned this pull request Oct 7, 2023

feat(pass-style): far class instances #1766

Closed

erights force-pushed the markm-objectMetaMap branch from d113f32 to 87f1cf8 Compare October 7, 2023 14:33

erights changed the base branch from master to markm-meta-ops October 7, 2023 14:34

erights force-pushed the markm-objectMetaMap branch 4 times, most recently from 4aa969f to aa5925a Compare October 9, 2023 20:38

erights changed the base branch from markm-meta-ops to master October 9, 2023 20:38

erights force-pushed the markm-objectMetaMap branch 2 times, most recently from 5776030 to d50a60b Compare October 9, 2023 21:38

erights requested review from mhofman, michaelfig and turadg October 9, 2023 22:05

erights marked this pull request as ready for review October 9, 2023 22:08

erights force-pushed the markm-objectMetaMap branch from d50a60b to 1522ef7 Compare October 13, 2023 22:36

erights force-pushed the markm-objectMetaMap branch 3 times, most recently from 992248d to 7460a13 Compare October 29, 2023 21:38

michaelfig reviewed Oct 31, 2023

View reviewed changes

erights force-pushed the markm-objectMetaMap branch from de5766c to 6dc17f6 Compare November 6, 2023 22:15

erights requested a review from michaelfig November 6, 2023 22:18

erights force-pushed the markm-objectMetaMap branch 4 times, most recently from fa32908 to a315a7c Compare November 6, 2023 23:05

erights force-pushed the markm-objectMetaMap branch from a315a7c to 2e6ed72 Compare November 7, 2023 23:16

erights force-pushed the markm-objectMetaMap branch from 2e6ed72 to f35d7de Compare November 8, 2023 23:37

turadg reviewed Nov 9, 2023

View reviewed changes

packages/exo/src/exo-tools.js Outdated Show resolved Hide resolved

packages/exo/test/test-exo-class-js-class.js Show resolved Hide resolved

packages/pass-style/src/make-far.js Outdated Show resolved Hide resolved

packages/pass-style/src/make-far.js Outdated Show resolved Hide resolved

kriskowal added the next-release label Nov 9, 2023

erights force-pushed the markm-objectMetaMap branch from f35d7de to d3b9abb Compare November 10, 2023 23:10

erights force-pushed the markm-objectMetaMap branch from f2dbf29 to 25633d6 Compare November 10, 2023 23:44

erights requested a review from turadg November 10, 2023 23:48

turadg approved these changes Nov 11, 2023

View reviewed changes

packages/pass-style/src/make-far.js Outdated Show resolved Hide resolved

fix(exo): allow richer behaviorMethods

fde26da

erights force-pushed the markm-objectMetaMap branch 3 times, most recently from a9db3d8 to 336b138 Compare November 12, 2023 02:27

erights added 2 commits November 11, 2023 18:35

feat(pass-style): Far GET_METHOD_NAMES meta method

b079812

chore(eventual-send)!: restrict exports

1f29d4a

erights force-pushed the markm-objectMetaMap branch from 336b138 to 1f29d4a Compare November 12, 2023 02:35

erights merged commit e1c63bf into master Nov 12, 2023

erights deleted the markm-objectMetaMap branch November 12, 2023 03:00

erights mentioned this pull request Dec 11, 2023

fix(exo)!: reject extra args by default #1890

Merged

kriskowal removed the next-release label Jul 24, 2024

erights mentioned this pull request May 6, 2025

fix!: only string named methods #2792

Draft

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(exo): allow richer behaviorMethods#1809

fix(exo): allow richer behaviorMethods#1809
erights merged 3 commits intomasterfrom
markm-objectMetaMap

erights commented Oct 6, 2023 •

edited

Loading

Uh oh!

erights commented Oct 29, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erights commented Nov 6, 2023

Uh oh!

erights commented Nov 8, 2023

Uh oh!

turadg left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erights commented Nov 10, 2023

Uh oh!

turadg left a comment

Uh oh!

Uh oh!

erights commented Nov 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	// This test, though correct, demonstrates a sucurity weakness of
	// this approach to JS class inheritance at this
	// `@endo/pass-style` / `Far` level of abstraction. The weakness
	// is that the overridden method from a superclass can nevertheless
	// be directly applied to an instance of the subclass. The
	// subclass override did not suppress the use of the overridden
	// method as, effectively, part of the subclass' instance's public
	// API.
	//
	// See the corresponding example at
	// `test-exo-wobbly-point.js` to see the absence of this vulnerability
	// at the Exo level of abstraction.
	t.is(apply(otherPt.getX, otherWpt, []), 3);

	// This error behavior shows the absence of the security vulnerability
	// explained at the corresponding example in `test-far-wobbly-point.js`
	// for the `@endo/pass-style` / `Far` level of abstraction. At the exo level
	// of abstraction, a raw-method subclass overriding an inherited superclass
	// method denies that method to clients of instances of the subclass.
	// At the same time, this overridden method remains available within
	// the overriding subclass via unguarded `super` calls.
	t.throws(() => apply(otherPt.getX, otherWpt, []), {
	message:
	'"In \\"getX\\" method of (ExoPoint)" may only be applied to a valid instance: "[Alleged: ExoWobblyPoint]"',
	});

Conversation

erights commented Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

Uh oh!

erights commented Oct 29, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erights commented Nov 6, 2023

Uh oh!

erights commented Nov 8, 2023

Uh oh!

turadg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erights commented Nov 10, 2023

Uh oh!

turadg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erights commented Nov 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

erights commented Oct 6, 2023 •

edited

Loading