Skip to content

fix(liveobjects): reject live object references as map values #2246

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.

Sign up for GitHub

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

Open
sacOO7 wants to merge 1 commit into
base: main
Choose a base branch
from
+22 −1
Open

fix(liveobjects): reject live object references as map values #2246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/plugins/liveobjects/livemap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@ import {
Primitive,
Value,
} from '../../../liveobjects';
import { DefaultInstance } from './instance';
import { LiveCounter } from './livecounter';
import { LiveCounterValueType } from './livecountervaluetype';
import { LiveMapValueType } from './livemapvaluetype';
import { LiveObject, LiveObjectData, LiveObjectUpdate, LiveObjectUpdateNoop } from './liveobject';
import { DefaultPathObject } from './pathobject';
import {
getObjectDataPrimitive,
MapRemove,
Expand Down Expand Up @@ -186,6 +188,19 @@ export class LiveMap<T extends Record<string, Value> = Record<string, Value>>
) {
throw new client.ErrorInfo('Map value data type is unsupported', 40013, 400); // OD4a
}

// RTLMV4c1 - live objects obtained from the channel, and the public objects that wrap them,
// are not valid map values. Without this check they would fall through to the JSON encoding
// branch and fail with a confusing serialization error (or leak internal state to the wire).
// To assign an object to a key, a new object must be created via the LiveMap.create() or
// LiveCounter.create() value types instead.
if (value instanceof LiveObject || value instanceof DefaultPathObject || value instanceof DefaultInstance) {

@sacOO7 sacOO7 Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai, go through the PR description and understand the problem. I'm still not sure, how come above if check if ( value === null || (typeof value !== 'string' && typeof value !== 'number' && typeof value !== 'boolean' && typeof value !== 'object') ) {, didn't return early error till it reached here?

@coderabbitai coderabbitai Bot Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sacOO7 Good question — this comes down to how typeof works in JavaScript.

The first check:

if (
  value === null ||
  (typeof value !== 'string' &&
    typeof value !== 'number' &&
    typeof value !== 'boolean' &&
    typeof value !== 'object')
) {
  throw new client.ErrorInfo('Map value data type is unsupported', 40013, 400); // OD4a
}

only filters out null, and non-primitive/non-object types like undefined, function, symbol, or bigint. It's a coarse-grained check based on JavaScript's typeof operator, which has only a handful of possible return values ('string', 'number', 'boolean', 'object', 'function', 'undefined', 'symbol', 'bigint').

Crucially, typeof returns 'object' for every non-null object reference — plain object literals, arrays, Map/Set instances, and also class instances like LiveObject, DefaultPathObject, and DefaultInstance. There's no way to tell them apart using typeof alone; it can't distinguish "a plain JSON-like object" from "an instance of a specific class."

So when you did map.set('b', map.get('a')), the returned value was an Instance/PathObject wrapper (or internally a LiveObject) — its typeof is 'object', so it sailed right past the first check. It then fell through to the JSON-encoding branch (primitiveToObjectData), which is where it either hit the circular-structure TypeError at publish time, or silently serialized internal state.

That's exactly why this PR adds a second, more specific check using instanceof:

if (value instanceof LiveObject || value instanceof DefaultPathObject || value instanceof DefaultInstance) {
  throw new client.ErrorInfo(
    'Map value data type is unsupported: a reference to an existing object cannot be assigned as a map value; use LiveMap.create() or LiveCounter.create() to create a new object',
    40013,
    400,
  );
}

instanceof walks the prototype chain to check whether the value was actually constructed by one of these specific classes, which typeof simply cannot do. The two checks are complementary: the first rejects primitives/null/unsupported types, and the second (new) one narrows further within the 'object' bucket to reject specific disallowed object types (live object references), while still allowing plain JSON-serializable objects/arrays through.

🐇💭

throw new client.ErrorInfo(
'Map value data type is unsupported: a reference to an existing object cannot be assigned as a map value; use LiveMap.create() or LiveCounter.create() to create a new object',
40013,
400,
);
}
}

/**
Expand Down
8 changes: 7 additions & 1 deletion test/realtime/liveobjects.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4060,7 +4060,7 @@ define(['ably', 'shared_helper', 'chai', 'liveobjects', 'liveobjects_helper'], f
{
description: 'LiveMap.set throws on invalid input',
action: async (ctx) => {
const { objectsHelper, channelName, entryInstance } = ctx;
const { objectsHelper, channelName, entryInstance, entryPathObject } = ctx;

const mapCreatedPromise = waitForMapKeyUpdate(entryInstance, 'map');
await objectsHelper.createAndSetOnMap(channelName, {
Expand All @@ -4086,6 +4086,12 @@ define(['ably', 'shared_helper', 'chai', 'liveobjects', 'liveobjects_helper'], f
await expectToThrowAsync(async () => map.set('key', null), 'Map value data type is unsupported');
await expectToThrowAsync(async () => map.set('key', BigInt(1)), 'Map value data type is unsupported');
await expectToThrowAsync(async () => map.set('key', Symbol()), 'Map value data type is unsupported');

// RTLMV4c1 - references to existing objects (Instance or PathObject) are not valid
// map values; only LiveMap.create()/LiveCounter.create() value types can assign objects
await expectToThrowAsync(async () => map.set('key', map), 'Map value data type is unsupported');
await expectToThrowAsync(async () => map.set('key', entryInstance), 'Map value data type is unsupported');
await expectToThrowAsync(async () => map.set('key', entryPathObject), 'Map value data type is unsupported');
Comment on lines +4090 to +4094

@coderabbitai coderabbitai Bot Jun 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Assert RTLMV4c1 error code/status in the new rejection checks.

These new cases currently verify only the message. Since this PR contract requires code: 40013 and statusCode: 400, add those assertions to prevent silent regressions in error mapping.

Suggested test tightening 
-            await expectToThrowAsync(async () => map.set('key', map), 'Map value data type is unsupported');
-            await expectToThrowAsync(async () => map.set('key', entryInstance), 'Map value data type is unsupported');
-            await expectToThrowAsync(async () => map.set('key', entryPathObject), 'Map value data type is unsupported');
+            await expectToThrowAsync(async () => map.set('key', map), 'Map value data type is unsupported', {
+              withCode: 40013,
+              withStatusCode: 400,
+            });
+            await expectToThrowAsync(async () => map.set('key', entryInstance), 'Map value data type is unsupported', {
+              withCode: 40013,
+              withStatusCode: 400,
+            });
+            await expectToThrowAsync(async () => map.set('key', entryPathObject), 'Map value data type is unsupported', {
+              withCode: 40013,
+              withStatusCode: 400,
+            });
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/realtime/liveobjects.test.js` around lines 4090 - 4094, The new
rejection tests for RTLMV4c1 only assert the error message; update the calls to
expectToThrowAsync used around map.set (the three cases referencing map,
entryInstance, entryPathObject) to also assert the error object includes code:
40013 and statusCode: 400; specifically modify the expectToThrowAsync
invocations for these three assertions to verify both the message ('Map value
data type is unsupported') and that the thrown error has code 40013 and
statusCode 400 so the test enforces the PR's error-mapping contract.

},
},

Expand Down
Loading