fix: avoid treating JS protocol hooks as RPC methods#1748
Open
ChargingFoxSec wants to merge 3 commits into
Open
fix: avoid treating JS protocol hooks as RPC methods#1748ChargingFoxSec wants to merge 3 commits into
ChargingFoxSec wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: 19d28b6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 47 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
BundleMonFiles updated (25)
Unchanged files (125)
Total files change -110B -0.02% Final result: ✅ View report in BundleMon website ➡️ |
mcintyre94
reviewed
Jun 24, 2026
mcintyre94
left a comment
mcintyre94
left a comment
Member
There was a problem hiding this comment.
This is great, thankyou! My only concern is the duplication, given that we might need to update these in future. My instinct is to move it to the shared rpc-spec-types, which is already a dependency of both rpc-spec and rpc-subscriptions-spec. WDYT?
| Symbol.asyncIterator, | ||
| Symbol.iterator, | ||
| Symbol.toPrimitive, | ||
| Symbol.toStringTag, |
Member
There was a problem hiding this comment.
I think it'd be worth adding dispose and asyncDispose, just because accidentally doing using rpc = ... is slightly-plausible.
Contributor
Author
|
Yeah, that makes sense. I’ll move the shared non-RPC property list into |
mcintyre94
requested changes
Jun 26, 2026
mcintyre94
left a comment
mcintyre94
left a comment
Member
There was a problem hiding this comment.
Thanks, reducing the duplication is definitely nicer! :)
Comment on lines
117
to
126
| it('does not expose JS protocol hooks as RPC methods', () => { | ||
| expect.assertions(7); | ||
| expect.hasAssertions(); | ||
| const api = createJsonRpcApi<DummyApi>(); | ||
|
|
||
| expect(api).not.toHaveProperty('then'); | ||
| expect(api).not.toHaveProperty('toJSON'); | ||
| expect(getUntypedProperty(api, Symbol.asyncIterator)).toBeUndefined(); | ||
| expect(getUntypedProperty(api, Symbol.for('nodejs.util.inspect.custom'))).toBeUndefined(); | ||
| expect(getUntypedProperty(api, Symbol.iterator)).toBeUndefined(); | ||
| expect(getUntypedProperty(api, Symbol.toPrimitive)).toBeUndefined(); | ||
| expect(getUntypedProperty(api, Symbol.toStringTag)).toBeUndefined(); | ||
| JAVASCRIPT_PROTOCOL_SYMBOLS.forEach(symbol => { | ||
| expect(getUntypedProperty(api, symbol)).toBeUndefined(); | ||
| }); | ||
| }); |
Member
There was a problem hiding this comment.
Nit: a cleaner way to write these tests would be using it.each, for example used here:
This would give us much better feedback if any of the tests fails
Comment on lines
+3
to
+6
| const SymbolWithDispose = Symbol as typeof Symbol & { | ||
| asyncDispose?: symbol; | ||
| dispose?: symbol; | ||
| }; |
Member
There was a problem hiding this comment.
We shouldn't need this - Symbol.dispose and Symbol.asyncDispose should be available in the verison of Typescript we're using, and are available for me on main.
Comment on lines
+106
to
+109
| expect.hasAssertions(); | ||
| JAVASCRIPT_PROTOCOL_SYMBOLS.forEach(symbol => { | ||
| expect(getUntypedProperty(rpc, symbol)).toBeUndefined(); | ||
| }); |
Member
There was a problem hiding this comment.
Same comment re it.each here
Contributor
Author
|
Thanks, updated this. I removed the custom Symbol typing and now use Symbol.dispose / Symbol.asyncDispose directly. Also changed the symbol checks to it.each so failures point at the specific symbol, and mirrored that in the subscription proxy tests. Related node/browser tests, typecheck, lint, and Prettier checks are passing locally. |
mcintyre94
requested changes
Jul 14, 2026
mcintyre94
left a comment
mcintyre94
left a comment
Member
There was a problem hiding this comment.
One last nit and this is ready to go, thankyou!
Comment on lines
+32
to
+41
| export function getNonRpcPropertyValue(propertyName: string | symbol, receiver: unknown): unknown { | ||
| if (JAVASCRIPT_PROTOCOL_PROPERTY_NAMES.has(propertyName)) { | ||
| return undefined; | ||
| } | ||
| return Reflect.get(Object.prototype, propertyName, receiver); | ||
| } | ||
|
|
||
| export function isNonRpcPropertyName(propertyName: string | symbol): boolean { | ||
| return JAVASCRIPT_PROTOCOL_PROPERTY_NAMES.has(propertyName) || OBJECT_PROTOTYPE_PROPERTY_NAMES.has(propertyName); | ||
| } |
Member
There was a problem hiding this comment.
Nit: these are both exported functions. Can you add a docblock for each (just a one liner and reference to the issue is fine), and also @internal? I don't think these should be part of our API surface, but exporting them from here is correct. Sorry, should have flagged that when I suggested moving them here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
RPC and subscription proxy objects currently treat most property access as an RPC method name. After #508,
thenis handled specially, but other JavaScript protocol hooks can still be interpreted as methods.For example,
JSON.stringify(rpc)looks uptoJSON, and primitive/iteration/inspection paths can look up symbols such asSymbol.toPrimitiveandSymbol.iterator.Summary of Changes
Fixes #509.
This adds a narrow non-RPC property check for RPC proxy objects and subscription proxy objects.
toJSON,Symbol.toPrimitive,Symbol.iterator,Symbol.asyncIterator,Symbol.toStringTag, and Node's custom inspect symbol as RPC methods.toStringandhasOwnProperty.Testing
corepack pnpm --filter @solana/rpc-spec test:unit:nodecorepack pnpm --filter @solana/rpc-subscriptions-spec test:unit:nodecorepack pnpm --filter @solana/rpc-spec test:unit:browsercorepack pnpm --filter @solana/rpc-subscriptions-spec test:unit:browsercorepack pnpm --filter @solana/rpc-spec test:typecheckcorepack pnpm --filter @solana/rpc-subscriptions-spec test:typecheckcorepack pnpm --filter @solana/rpc-spec test:lintcorepack pnpm --filter @solana/rpc-subscriptions-spec test:lintcorepack pnpm --filter @solana/rpc-spec test:prettiercorepack pnpm --filter @solana/rpc-subscriptions-spec test:prettiercorepack pnpm --filter @solana/rpc-spec compile:jscorepack pnpm --filter @solana/rpc-subscriptions-spec compile:js