From dfffb0502be5dd9ab5598e785b9988b1f4000227 Mon Sep 17 00:00:00 2001
From: Denis Badurina <badurinadenis@gmail.com>
Date: Thu, 1 Oct 2020 23:53:02 +0200
Subject: [PATCH] fix(server): `subscription` operations are distinct on the
 message ID (#24)

* docs: specify what happens on duplicate ids

* docs: smaller wording changes

* fix(server): prevent duplicate subscription operations
---
 PROTOCOL.md         |  4 +++-
 src/server.ts       |  7 ++++++
 src/tests/server.ts | 58 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/PROTOCOL.md b/PROTOCOL.md
index 05a6ff59..e0ac166a 100644
--- a/PROTOCOL.md
+++ b/PROTOCOL.md
@@ -73,7 +73,9 @@ The client is now **ready** to request subscription operations.
 
 Direction: **Client -> Server**
 
-Requests a operation specified in the message `payload`. This message leverages the unique ID field to connect future server messages to the operation started by this message.
+Requests a operation specified in the message `payload`. This message provides a unique ID field to connect future server messages to the operation started by this message.
+
+If there is already an active subscriber for a `subscription` operation matching the provided ID, the server will close the socket immediately with the event `4409: Subscriber for <unique-operation-id> already exists`. Since `query` and `mutation` resolve to a single emitted value, their subscription does not require reservations for additional future events. Having this in mind, the server may not assert this rule for these operations.
 
 ```typescript
 import { DocumentNode } from 'graphql';
diff --git a/src/server.ts b/src/server.ts
index f19a064e..18546ead 100644
--- a/src/server.ts
+++ b/src/server.ts
@@ -461,6 +461,13 @@ export function createServer(
             if (operationAST.operation === 'subscription') {
               const subscriptionOrResult = await subscribe(execArgs);
               if (isAsyncIterable(subscriptionOrResult)) {
+                // iterable subscriptions are distinct on ID
+                if (ctx.subscriptions[message.id]) {
+                  return ctx.socket.close(
+                    4409,
+                    `Subscriber for ${message.id} already exists`,
+                  );
+                }
                 ctx.subscriptions[message.id] = subscriptionOrResult;
 
                 try {
diff --git a/src/tests/server.ts b/src/tests/server.ts
index 9764ca19..6a4adf03 100644
--- a/src/tests/server.ts
+++ b/src/tests/server.ts
@@ -819,6 +819,64 @@ describe('Subscribe', () => {
 
     expect(onMessageFn).toBeCalledTimes(3); // ack, next, complete
   });
+
+  it('should close the socket on duplicate `subscription` operation subscriptions request', async () => {
+    expect.assertions(3);
+
+    await makeServer();
+
+    const client = new WebSocket(url, GRAPHQL_TRANSPORT_WS_PROTOCOL);
+    client.onopen = () => {
+      client.send(
+        stringifyMessage<MessageType.ConnectionInit>({
+          type: MessageType.ConnectionInit,
+        }),
+      );
+    };
+
+    client.onmessage = ({ data }) => {
+      const message = parseMessage(data);
+      if (message.type === MessageType.ConnectionAck) {
+        client.send(
+          stringifyMessage<MessageType.Subscribe>({
+            id: 'not-unique',
+            type: MessageType.Subscribe,
+            payload: {
+              query: `subscription {
+              boughtBananas {
+                name
+              }
+            }`,
+            },
+          }),
+        );
+
+        // try subscribing with a live subscription id
+        setTimeout(() => {
+          client.send(
+            stringifyMessage<MessageType.Subscribe>({
+              id: 'not-unique',
+              type: MessageType.Subscribe,
+              payload: {
+                query: `subscription {
+                  greetings
+                }`,
+              },
+            }),
+          );
+        }, 10);
+      }
+    };
+
+    await new Promise((resolve) => {
+      client.onclose = (event) => {
+        expect(event.code).toBe(4409);
+        expect(event.reason).toBe('Subscriber for not-unique already exists');
+        expect(event.wasClean).toBeTruthy();
+        resolve(); // done
+      };
+    });
+  });
 });
 
 describe('Keep-Alive', () => {