fix!: only string named methods by erights · Pull Request #2792 · endojs/endo

erights · 2025-05-05T19:10:47Z

Staged on #2797

Description

This PR prohibits remotables from having symbol named methods, or methods named 'then' or 'constructor'. IOW, it only allows string-named methods other than 'then' or 'constructor'. If a 'constructor' is provided on the input object to making a remotable, it is ignored, so that class prototypes can be used as such input.

Security Considerations

Narrower attack surface. Fewer cases.

Scaling Considerations

none

Documentation Considerations

Need to document what we allow as method names of remotables.

Testing Considerations

Fixed the tests, so that they pass. Having read them while fixing them, I think these fixed original tests are adequate.

Compatibility Considerations

This is a compat break. That why fix! rather than fix:. A client could not adopt this until after they had purged all prohibited remotable method names from their code.

On the flip side, this is a huge step towards compat with ocapn, which by convention uses only one namespace of string method names. Ironically, in ocapn the convention is that the method names on the wire are only symbols. An endo object will always convert the method name to a string.

Our prohibition of 'then' and 'constructor' are currently remaining incompats with ocapn. For use to meet ocapn here, we'll have to do some kind of hilbert hotel, lifting, for example, a protocol 'then' into a JS '__then__' or something.

Upgrade Considerations

It is not clear how to stage this so that code running from before this change and code running after this change can talk to each other.

Update NEWS.md for user-facing changes.

packages/marshal/test/marshal-capdata.test.js

packages/pass-style/src/types.d.ts

packages/pass-style/test/safe-promise.test.js

packages/patterns/src/keys/compareKeys.js

erights · 2025-05-05T19:32:05Z

packages/patterns/src/patterns/getGuardPayloads.js

-    // There is no need to accommodate LegacyMethodGuardShape in
-    // this position, since `symbolMethodGuards happened
-    // after https://github.com/endojs/endo/pull/1712
-    symbolMethodGuards: M.mapOf(M.symbol(), MethodGuardShape),


Need to figure out how to retire symbolMethodGuards with some kind of deprecation dance instead.

packages/patterns/test/copyMap.test.js

erights · 2025-05-06T00:32:04Z

packages/exo/src/exo-tools.js

-      ...(symbolMethodGuards &&
-        fromEntries(getCopyMapEntries(symbolMethodGuards))),


Need deprecation dance

erights · 2025-05-06T00:34:15Z

packages/patterns/src/patterns/patternMatchers.js

  {
    defaultGuards: M.or(M.undefined(), 'passable', 'raw'),
    sloppy: M.boolean(),
-    symbolMethodGuards: M.mapOf(M.symbol(), MethodGuardShape),


Need deprecation dance

erights · 2025-05-06T00:34:38Z

packages/patterns/src/types.js

 *   interfaceName: string,
 *   methodGuards:
 *     Omit<T, symbol> & Partial<{ [K in Extract<keyof T, symbol>]: never }>,
- *   symbolMethodGuards?:


Need deprecation dance

packages/pass-style/test/passStyleOf.test.js

erights · 2025-05-07T20:26:56Z

Putting this back into Draft until I make some changes:

Split the PR into two staged PRs.
1. change all remotable creation to avoid symbol-named methods, with the possible exception of Symbol.asyncIterator.
2. enforce that remotables cannot have symbol-named methods
- With a possible special carve-out to grandfather in Symbol.asyncIterator with its current wire representation, while enforcing that a remotable cannot have both a method named with the string @@asyncIterator.

The reason for the split is that the first still does not create any compat problems, assuming that none of the prohibited symbols were actually sent in production. This helps phase the coordinated changes to agoric-sdk.

We would only do the special case for Symbol.asyncIterator if we're actually sending it between vats in production in either endo or agoric-sdk.

kriskowal

Requesting changes to clear out my review inbox. Positions are already stated and I agree with the broad strokes of the proposed plan to converge on the simplest practically-non-breaking single category for method names.

Ideally, method names are only strings in JavaScript memory (and OCapN symbols on the wire; for CapTP, method names are not directly passable data). But, this ideal conflicts with safety, since the method names constructor, prototype, and then create attack surface.

Ideally, all method names are valid on the wire. We might be able to cross-over certain string names like constructor, prototype, and then to Symbol.for('constructor'), Symbol.for('prototype'), and Symbol.for('then') when declaring remotables or exos. I find it unlikely that any extant exos use these method names. I do not think we need to validate this change against chain transcripts because it should be equally satisfying to statically analyze existing makeExo.

Ideally we would just drop support for Symbol.asyncIterator entirely. To do so, we must validate the change against chain transcript replay. To avoid the cost of validation against chain transcript relay, we have the option of making a (permanent) legacy exception for Symbol.asyncIterator by similarly swapping the method name "@@asyncIterator" for Symbol.asyncIterator when converting the string on the wire to an in-memory method name and vice versa. Honestly, I find this wart tolerable.

erights · 2025-06-03T21:02:40Z

since the method names constructor, prototype, and then create attack surface.

Yes for constructor and then. What's the problem with a remotable method named prototype? (I admit it is weird, but I don't see a problem.)

erights · 2025-06-03T21:10:39Z

Re constructor, then, and @@asyncIterator, very clever mapping these 1-1 to JS-only symbols. I had thought any solution to these would need a Hilbert hotel. But since there's no other way to mention these symbols from OCapN, I think we can avoid finding more rooms in the hotel. Does this make sense, or does this solution still need a Hilbert hotel somewhere?

Double checking:

We would then prohibit remotables from having methods that, as the JS level, are named with the JS strings @@asyncIterator or then, and we would ignore what seems to be a JS method named with the JS string constructor.

Yes?

…2793) Closes: #XXXX Refs: #2792 #2777 ## Description This PR is essentially a pure refactor to set things up for upcoming substantive PRs like #2792 . The main one will change the passable symbol representation, along the lines of #2777 but likely different. This is for symbols used for purposes passing, or test counterexamples to being passable. All other symbol uses should not change. The change from this PR is - change calls to `Symbol.for('foo')` for making a passable symbol, to `passableSymbolForName('foo')` - change calls to `Symbol('foo')` for making a non-passable symbol for testing purposes, to `unpassableSymbolForName('foo')` ### Security Considerations Assuming we changed each symbol-making callsite from the old to the correct new one, and assuming that we did not alter the code that was making symbols for other reasons, none. ### Scaling Considerations Using registered symbols like `Symbol.for('foo')` is a semantically perfect binding to the pass-style and ocapn category `'symbol'`. However its scaling properties are disastrous. In some js systems like v8, unused registered symbols are not garbage collected. This is why we're going to switch passable symbol representation. However this PR does none of that yet. It merely sets the stage. This PR should not cause any change in scaling. ### Documentation Considerations The need to pass symbols is rare and obscure, and most users will not need to be aware of this. However, any reference documentation should clearly explain when to use the new abstract symbol-creation functions, and when not to. ### Testing Considerations As a pure refactor, the main concern is that all the old tests still pass. Any substantive new tests or test changes will be downstream, in the PRs that this PR enables. ### Compatibility Considerations Assuming we changed each symbol-making callsite from the old to the correct new one, and assuming that we did not alter the code that was making symbols for other reasons, none. ### Upgrade Considerations As a pure refactor, none. Beyond that, when we change the JS representation of a passable symbol, we will not change the wire representation in any of our encodings. Thus, durable state saved using the representation will, at that time, revive into the new representation, so still none. Likewise with messages sent from code on one side of the upgrade and received by code on the other side of the upgrade.

@kriskowal

Extracted from #2797 Closes: #XXXX Refs: #2793 #2792 ## Description This PR is the extraction from #2797 of only the safe pure refactor elements of #2797 as suggested by @kriskowal at #2797 (review) . #2797 is then rebased on this one, representing only the elements whose refactor-purity was questionable. This PR should be a pure refactor, to avoid creating remotables with symbol-named methods when there is clearly no possible compat hazard. Non-remotable objects with symbol-named methods are fine. But see Compatibility Considerations below. ### Security Considerations none ### Scaling Considerations none ### Documentation Considerations none ### Testing Considerations Most of the remotable objects with symbol-named methods were in tests. These can clearly be fixed without any compat hazard, so I count these as pure refactors. However, in removing the symbol-ness of some of these tests, I may have accidentally removed the purposes of the tests. They might now be fully redundant with other tests and should be removed. If anyone does investigate this, that cleanup could be in a later PR. ### Compatibility Considerations none. But see Compatibility Considerations of #2797 ### Upgrade Considerations none

Copilot

Pull Request Overview

This PR restricts remotables to only allow string-named methods, excluding 'then' and 'constructor', as part of moving toward compatibility with OCapN. This is a breaking change that removes support for symbol-named methods on remotables.

Removes symbol method support from interface guards and remotable validation
Updates error messages and tests to reflect the new restrictions
Simplifies code by removing symbol method handling logic

Reviewed Changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
packages/patterns/test/copyMap.test.js	Simplifies test by inlining function call
packages/patterns/src/types.js	Removes symbolMethodGuards type definition
packages/patterns/src/patterns/patternMatchers.js	Removes symbol method guard handling logic
packages/patterns/src/patterns/getGuardPayloads.js	Removes symbol method guard support and simplifies method key extraction
packages/pass-style/test/passStyleOf.test.js	Updates test to expect new error for 'then' method names
packages/pass-style/src/symbol.js	Updates documentation example to use passableSymbolForName
packages/pass-style/src/remotable.js	Implements core restriction logic for method names
packages/pass-style/src/iter-helpers.js	Updates comments from "should not" to "cannot"
packages/marshal/test/marshal-far-obj.test.js	Updates tests to expect new symbol method errors
packages/marshal/src/marshal-justin.js	Removes deprecated asyncIterator case
packages/marshal/package.json	Adds wildcard export for tools directory
packages/exo/src/exo-tools.js	Simplifies prototype defense by removing symbol method handling

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-16T08:04:51Z

packages/pass-style/test/passStyleOf.test.js

-    message: /Cannot pass non-promise thenables/,
+  t.throws(() => Far('remote thenable', { then: () => 'thenable' }), {
+    message:
+      'Remotables can only have string-named methods other than "then" and "constuctor": "then"',


There's a typo in the error message: 'constuctor' should be 'constructor'.

Suggested change

'Remotables can only have string-named methods other than "then" and "constuctor": "then"',

'Remotables can only have string-named methods other than "then" and "constructor": "then"',

Copilot · 2025-09-16T08:04:51Z

packages/pass-style/src/remotable.js

-                  CX(check)`A pass-by-remote cannot shadow ${q(PASS_STYLE)}`))))
+                  CX(
+                    check,
+                  )`Remotables can only have string-named methods other than "then" and "constuctor": ${String(key)}`))))


There's a typo in the error message: 'constuctor' should be 'constructor'.

Suggested change

)`Remotables can only have string-named methods other than "then" and "constuctor": ${String(key)}`))))

)`Remotables can only have string-named methods other than "then" and "constructor": ${String(key)}`))))

Copilot · 2025-09-16T08:04:52Z

packages/marshal/test/marshal-far-obj.test.js

  const FAR_ONLYMETH = /cannot serialize Remotables with non-methods/;
  const FAR_EXPLICIT = /Remotables must be explicitly declared/;
+  const FAR_SYMBOL_METHODS =
+    /Remotables can only have string-named methods other than "then" and "constuctor":/;


There's a typo in the error message: 'constuctor' should be 'constructor'.

Suggested change

/Remotables can only have string-named methods other than "then" and "constuctor":/;

/Remotables can only have string-named methods other than "then" and "constructor":/;

changeset-bot · 2026-02-25T22:32:07Z

⚠️ No Changeset found

Latest commit: 2eb80f8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

erights self-assigned this May 5, 2025