Fix false conflicts when serverOnlyField is absent from stored doc

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 ### Bug Fixes
 
+- Fix false conflicts during replication push when a `serverOnlyField` is absent from the stored server document (e.g. because the field is optional and was never set). `mergeServerDocumentFieldsMonad` previously wrote a `null` value for the missing field onto the merged `assumedMasterState`, so the extra key caused `masterWrite`'s `isEqual` check to report a conflict that did not actually exist, silently reverting the client's update. The helper now deletes the property in that case so the merged row matches the stored master state.
 - Fix REST `/set` endpoint allowing a client to overwrite documents they do not own. When a `queryModifier` was configured, the handler only validated that the client-provided (new) document state matched the modifier but never checked the existing server document. An authenticated user could therefore take over a foreign document by sending a write whose new state matched the modifier while targeting another user's primary key. The handler now also runs the query matcher against the existing server document and rejects the request with 403 Forbidden if it does not match, aligning the behavior with the replication `/push` endpoint.
 - Fix invalid CORS response when the server is configured with the default `cors: '*'`. The express adapter always sends `Access-Control-Allow-Credentials: true`, but combining that with `Access-Control-Allow-Origin: *` is rejected by browsers per the CORS spec, so credentialed (cookie/auth-header) requests from any cross-origin client would fail. The adapter now reflects the request `Origin` back when `cors` is `'*'`, keeping the "allow from anywhere" semantics while staying compatible with credentials.
 - Fix REST endpoint `/set` allowing clients to populate `serverOnlyFields` when inserting NEW documents. Updates to existing documents already stripped client-supplied values for these fields, but the insert path passed the client document straight to `RxCollection.insert()`, so a client could persist arbitrary values into fields that are documented as server-only. The handler now strips server-only fields from the client document before inserting, matching the documented contract that clients cannot do writes where one of the `serverOnlyFields` is set.

src/plugins/server/helper.ts

@@ -144,9 +144,20 @@ export function mergeServerDocumentFieldsMonad<RxDocType>(serverOnlyFields: stri
             return ret;
         }
         useFields.forEach(field => {
-            // Only copy if field exists on serverDoc to avoid creating
-            // properties with undefined value (which break deepEqual key count)
-            (ret as any)[field] = field in (serverDoc as any) ? (serverDoc as any)[field] : null;
+            /**
+             * If the field exists on the serverDoc, copy its value so that
+             * the server-side value always wins over anything the client
+             * sent. If it does not exist on the serverDoc, delete it from
+             * the merged document so the key is truly absent — using `null`
+             * here would make the merged row differ from the stored master
+             * state during the isEqual check inside masterWrite and
+             * produce a false conflict that silently reverts the write.
+             */
+            if (field in (serverDoc as any)) {
+                (ret as any)[field] = (serverDoc as any)[field];
+            } else {
+                delete (ret as any)[field];
+            }
         });
         return ret;
     }

test/unit/endpoint-replication.test.ts

@@ -1,8 +1,11 @@
 import assert from 'assert';
 
 import {
+    RxCollection,
     RxDocumentData,
+    RxJsonSchema,
     clone,
+    createRxDatabase,
     randomToken
 } from 'rxdb/plugins/core';
 import {
@@ -864,6 +867,104 @@ describe('endpoint-replication.test.ts', () => {
             await replicationState.cancel();
             await col.database.close();
         });
+        it('should not produce false conflicts when a serverOnlyField is absent from the stored document', async () => {
+            /**
+             * Reproduces a conflict-handling bug:
+             * When a document is stored on the server without the configured
+             * serverOnlyField (e.g. because the field is optional and was
+             * never set), the push endpoint merges a `null` value for that
+             * field into the assumedMasterState before calling masterWrite.
+             * masterWrite's isEqual check then fails because the stored
+             * masterState has no such property, causing a false conflict
+             * that silently reverts the client's update.
+             */
+            type ConflictDocType = {
+                id: string;
+                counter: number;
+                serverSecret?: string;
+            };
+            const conflictSchema: RxJsonSchema<ConflictDocType> = {
+                version: 0,
+                primaryKey: 'id',
+                type: 'object',
+                properties: {
+                    id: { type: 'string', maxLength: 100 },
+                    counter: { type: 'integer', minimum: 0, maximum: 1000, multipleOf: 1 },
+                    serverSecret: { type: 'string', maxLength: 100 }
+                },
+                required: ['id', 'counter']
+            };
+
+            async function createCol(): Promise<RxCollection<ConflictDocType>> {
+                const db = await createRxDatabase<{ items: RxCollection<ConflictDocType> }>({
+                    name: randomToken(10),
+                    storage: config.storage.getStorage(),
+                    multiInstance: false
+                });
+                const cols = await db.addCollections({
+                    items: { schema: conflictSchema }
+                });
+                return cols.items;
+            }
+
+            const serverCol = await createCol();
+            // Insert a document WITHOUT the server-only field.
+            // serverSecret is optional in the schema, so this is a valid write.
+            await serverCol.insert({ id: 'doc1', counter: 0 });
+
+            const port = await nextPort();
+            const server = await createRxServer({
+                adapter: TEST_SERVER_ADAPTER,
+                database: serverCol.database,
+                authHandler,
+                port
+            });
+            const endpoint = await server.addReplicationEndpoint<ConflictDocType>({
+                name: randomToken(10),
+                collection: serverCol,
+                serverOnlyFields: ['serverSecret']
+            });
+            await server.start();
+
+            const clientCol = await createCol();
+            const url = 'http://localhost:' + port + '/' + endpoint.urlPath;
+            const replicationState = replicateServer<ConflictDocType>({
+                collection: clientCol,
+                replicationIdentifier: randomToken(10),
+                url,
+                headers,
+                live: true,
+                push: {},
+                pull: {},
+                eventSource: EventSource
+            });
+            ensureReplicationHasNoErrors(replicationState);
+            await replicationState.awaitInSync();
+
+            // Client has the doc without serverSecret (as expected, it is stripped).
+            const clientDoc = await clientCol.findOne('doc1').exec(true);
+            assert.strictEqual(clientDoc.counter, 0);
+            assert.strictEqual(typeof clientDoc.serverSecret, 'undefined');
+
+            // Client modifies the document. This push must not produce a
+            // false conflict because the only real change is `counter`.
+            await clientDoc.getLatest().patch({ counter: 1 });
+            await replicationState.awaitInSync();
+
+            // The update must be reflected on the server. With the bug,
+            // the false conflict overwrites the client's change, so the
+            // server counter stays at 0.
+            const serverDoc = await serverCol.findOne('doc1').exec(true);
+            assert.strictEqual(
+                serverDoc.counter,
+                1,
+                'server counter should be updated to 1 (false conflict silently reverts client update)'
+            );
+
+            await replicationState.cancel();
+            await serverCol.database.close();
+            await clientCol.database.close();
+        });
     });
 });
 export function customFetchWithFixedHeaders(headers: any) {

test/unit/server.test.ts

@@ -240,9 +240,9 @@ describe('server.test.ts', () => {
             const result = merge(undefined as any, undefined);
             assert.strictEqual(result, undefined);
         });
-        it('should not create undefined properties when serverDoc lacks the field', () => {
+        it('should not add the field when serverDoc lacks it (new-doc-on-server)', () => {
             // Simulates: document was created via push without the server-only field,
-            // stored in DB without it, then a subsequent push references it as serverDoc
+            // stored in DB without it, then a subsequent push references it as serverDoc.
             const serverDoc: any = {
                 _attachments: {},
                 _deleted: false,
@@ -254,12 +254,28 @@ describe('server.test.ts', () => {
             const clientDoc = { id: 'foobar' };
             const result = mergeServerDocumentFieldsMonad<any>(['private'])(clientDoc, serverDoc);
 
-            // Must be null, not undefined
-            assert.strictEqual(result.private, null);
-            // Must survive JSON roundtrip (undefined would be stripped)
-            assert.strictEqual(JSON.parse(JSON.stringify(result)).private, null);
+            // Must be absent so the merged doc matches the stored master state
+            // during masterWrite's isEqual check — a `null` or `undefined` value
+            // would be an extra key and would produce a false conflict.
+            assert.strictEqual('private' in result, false);
         });
-        it('should not cause false conflicts with deepEqual when server-only field is missing from stored doc', () => {
+        it('should strip client-sent values for server-only fields missing on serverDoc', () => {
+            // A malicious or misconfigured client might send a value for a server-only
+            // field on a document that does not have that field server-side. The merge
+            // must drop the client's value so it cannot be persisted.
+            const serverDoc: any = {
+                _attachments: {},
+                _deleted: false,
+                _meta: { lwt: 2000 },
+                _rev: '1-rev',
+                id: 'foobar',
+            };
+            const clientDoc = { id: 'foobar', private: 'hacked' };
+            const result = mergeServerDocumentFieldsMonad<any>(['private'])(clientDoc, serverDoc);
+
+            assert.strictEqual('private' in result, false);
+        });
+        it('should not produce extra keys when serverDoc is missing the field (no false conflict)', () => {
             const merge = mergeServerDocumentFieldsMonad<any>(['private']);
 
             const storedDoc: any = { id: 'foobar', name: 'test', _deleted: false };
@@ -271,17 +287,12 @@ describe('server.test.ts', () => {
             // Simulate what writeDocToDocState returns (the raw stored doc without _meta/_rev)
             const masterState = { id: 'foobar', name: 'test', _deleted: false };
 
-            // BUG: with the old code, this fails because mergedAssumed has
-            // { ..., private: undefined } (4 keys) vs masterState (3 keys)
-            assert.strictEqual(
-                Object.keys(mergedAssumed).length === Object.keys(masterState).length + 1,
-                true,
-                'merged doc should have exactly one extra key (the server-only field)'
-            );
-            assert.strictEqual(
-                Object.values(mergedAssumed).every(v => v !== undefined),
-                true,
-                'no property should be undefined (breaks deepEqual key count)'
+            // The merged doc must have the same key set as the master state,
+            // otherwise deepEqual inside masterWrite would report a conflict
+            // that does not actually exist.
+            assert.deepStrictEqual(
+                Object.keys(mergedAssumed).sort(),
+                Object.keys(masterState).sort()
             );
         });
     });