database/project collabs: decaff _user_set_query_project_users, fix TODO, add simple tests

haraldschilly · haraldschilly · commit 970bf8474095 · 2025-12-15T12:48:51.000+01:00
diff --git a/src/packages/database/postgres-user-queries.coffee b/src/packages/database/postgres-user-queries.coffee
@@ -26,11 +26,12 @@ lodash       = require('lodash')
 {defaults} = misc = require('@cocalc/util/misc')
 required = defaults.required
 
-{PROJECT_UPGRADES, SCHEMA, OPERATORS, isToOperand} = require('@cocalc/util/schema')
+{SCHEMA, OPERATORS, isToOperand} = require('@cocalc/util/schema')
 {queryIsCmp, userGetQueryFilter} = require("./user-query/user-get-query")
 
 {updateRetentionData} = require('./postgres/retention')
 {sanitizeManageUsersOwnerOnly} = require('./postgres/project/manage-users-owner-only')
+{sanitizeUserSetQueryProjectUsers} = require('./postgres/project/user-set-query-project-users')
 
 { checkProjectName } = require("@cocalc/util/db-schema/name-rules");
 {callback2} = require('@cocalc/util/async-utils')
@@ -794,51 +795,7 @@ exports.extend_PostgreSQL = (ext) -> class PostgreSQL extends ext
                                     y[k0] = v0
 
     _user_set_query_project_users: (obj, account_id) =>
-        dbg = @_dbg("_user_set_query_project_users")
-        if not obj.users?
-            # nothing to do -- not changing users.
-            return
-        ##dbg("disabled")
-        ##return obj.users
-        #   - ensures all keys of users are valid uuid's (though not that they are valid users).
-        #   - and format is:
-        #          {group:'owner' or 'collaborator', hide:bool, upgrades:{a map}}
-        #     with valid upgrade fields.
-        upgrade_fields = PROJECT_UPGRADES.params
-        users = {}
-        # TODO: we obviously should check that a user is only changing the part
-        # of this object involving themselves... or adding/removing collaborators.
-        # That is not currently done below.  TODO TODO TODO  SECURITY.
-        for id, x of obj.users
-            if misc.is_valid_uuid_string(id)
-                for key in misc.keys(x)
-                    if key not in ['group', 'hide', 'upgrades', 'ssh_keys']
-                        throw Error("unknown field '#{key}")
-                if x.group? and (x.group not in ['owner', 'collaborator'])
-                    throw Error("invalid value for field 'group'")
-                if x.hide? and typeof(x.hide) != 'boolean'
-                    throw Error("invalid type for field 'hide'")
-                if x.upgrades?
-                    if not misc.is_object(x.upgrades)
-                        throw Error("invalid type for field 'upgrades'")
-                    for k,_ of x.upgrades
-                        if not upgrade_fields[k]
-                            throw Error("invalid upgrades field '#{k}'")
-                if x.ssh_keys
-                    # do some checks.
-                    if not misc.is_object(x.ssh_keys)
-                        throw Error("ssh_keys must be an object")
-                    for fingerprint, key of x.ssh_keys
-                        if not key # deleting
-                            continue
-                        if not misc.is_object(key)
-                            throw Error("each key in ssh_keys must be an object")
-                        for k, v of key
-                            # the two dates are just numbers not actual timestamps...
-                            if k not in ['title', 'value', 'creation_date', 'last_use_date']
-                                throw Error("invalid ssh_keys field '#{k}'")
-                users[id] = x
-        return users
+        return sanitizeUserSetQueryProjectUsers(obj, account_id)
 
     _user_set_query_project_manage_users_owner_only: (obj, account_id) =>
         # This hook is called from the schema functional substitution to validate
diff --git a/src/packages/database/postgres/project/user-set-query-project-users.test.ts b/src/packages/database/postgres/project/user-set-query-project-users.test.ts
@@ -0,0 +1,70 @@
+/*
+ *  This file is part of CoCalc: Copyright © 2025 Sagemath, Inc.
+ *  License: MS-RSL – see LICENSE.md for details
+ */
+
+import { uuid } from "@cocalc/util/misc";
+import { sanitizeUserSetQueryProjectUsers } from "./user-set-query-project-users";
+
+describe("_user_set_query_project_users sanitizer", () => {
+  const accountId = uuid();
+  const otherId = uuid();
+
+  test("returns undefined when users is not provided", () => {
+    const value = sanitizeUserSetQueryProjectUsers({}, accountId);
+    expect(value).toBeUndefined();
+  });
+
+  test("allows updating own hide and upgrades", () => {
+    const value = sanitizeUserSetQueryProjectUsers(
+      {
+        users: {
+          [accountId]: { hide: true, upgrades: { memory: 1024 } },
+        },
+      },
+      accountId,
+    );
+    expect(value).toEqual({
+      [accountId]: { hide: true, upgrades: { memory: 1024 } },
+    });
+  });
+
+  test("rejects modifying another account", () => {
+    expect(() =>
+      sanitizeUserSetQueryProjectUsers(
+        {
+          users: {
+            [otherId]: { hide: true },
+          },
+        },
+        accountId,
+      ),
+    ).toThrow("users set queries may only modify the requesting account");
+  });
+
+  test("rejects group changes", () => {
+    expect(() =>
+      sanitizeUserSetQueryProjectUsers(
+        {
+          users: {
+            [accountId]: { group: "owner" },
+          },
+        },
+        accountId,
+      ),
+    ).toThrow("changing collaborator group via user_set_query is not allowed");
+  });
+
+  test("rejects invalid upgrade field", () => {
+    expect(() =>
+      sanitizeUserSetQueryProjectUsers(
+        {
+          users: {
+            [accountId]: { upgrades: { invalidQuota: 1 } },
+          },
+        },
+        accountId,
+      ),
+    ).toThrow("invalid upgrades field 'invalidQuota'");
+  });
+});
diff --git a/src/packages/database/postgres/project/user-set-query-project-users.ts b/src/packages/database/postgres/project/user-set-query-project-users.ts
@@ -0,0 +1,126 @@
+/*
+ *  This file is part of CoCalc: Copyright © 2025 Sagemath, Inc.
+ *  License: MS-RSL – see LICENSE.md for details
+ */
+
+import { PROJECT_UPGRADES } from "@cocalc/util/schema";
+import {
+  assert_valid_account_id,
+  is_object,
+  is_valid_uuid_string,
+} from "@cocalc/util/misc";
+
+type AllowedUserFields = {
+  hide?: boolean;
+  upgrades?: Record<string, unknown>;
+  ssh_keys?: Record<string, Record<string, unknown> | undefined>;
+};
+
+function ensureAllowedKeys(user: Record<string, unknown>): void {
+  const allowed = new Set(["hide", "upgrades", "ssh_keys"]);
+  for (const key of Object.keys(user)) {
+    if (key === "group") {
+      throw Error(
+        "changing collaborator group via user_set_query is not allowed",
+      );
+    }
+    if (!allowed.has(key)) {
+      throw Error(`unknown field '${key}'`);
+    }
+  }
+}
+
+function sanitizeUpgrades(upgrades: unknown): Record<string, unknown> {
+  if (!is_object(upgrades)) {
+    throw Error("invalid type for field 'upgrades'");
+  }
+  const allowedUpgrades = PROJECT_UPGRADES.params;
+  for (const key of Object.keys(upgrades)) {
+    if (!Object.prototype.hasOwnProperty.call(allowedUpgrades, key)) {
+      throw Error(`invalid upgrades field '${key}'`);
+    }
+  }
+  return upgrades as Record<string, unknown>;
+}
+
+function sanitizeSshKeys(
+  ssh_keys: unknown,
+): Record<string, Record<string, unknown> | undefined> {
+  if (!is_object(ssh_keys)) {
+    throw Error("ssh_keys must be an object");
+  }
+  const sanitized: Record<string, Record<string, unknown> | undefined> = {};
+  for (const fingerprint of Object.keys(ssh_keys)) {
+    const key = (ssh_keys as Record<string, unknown>)[fingerprint];
+    if (!key) {
+      sanitized[fingerprint] = undefined;
+      continue;
+    }
+    if (!is_object(key)) {
+      throw Error("each key in ssh_keys must be an object");
+    }
+    for (const field of Object.keys(key)) {
+      if (
+        !["title", "value", "creation_date", "last_use_date"].includes(field)
+      ) {
+        throw Error(`invalid ssh_keys field '${field}'`);
+      }
+    }
+    sanitized[fingerprint] = key as Record<string, unknown>;
+  }
+  return sanitized;
+}
+
+/**
+ * Sanitize and security-check project user mutations submitted via user set query.
+ *
+ * Only permits modifying the requesting user's own entry (hide/upgrades/ssh_keys).
+ * Collaborator role changes must use dedicated APIs that enforce ownership rules.
+ */
+export function sanitizeUserSetQueryProjectUsers(
+  obj: { users?: unknown } | undefined,
+  account_id: string,
+): Record<string, AllowedUserFields> | undefined {
+  if (obj?.users == null) {
+    return undefined;
+  }
+  assert_valid_account_id(account_id);
+  if (!is_object(obj.users)) {
+    throw Error("users must be an object");
+  }
+
+  const sanitized: Record<string, AllowedUserFields> = {};
+  const usersInput = obj.users as Record<string, unknown>;
+
+  for (const id of Object.keys(usersInput)) {
+    if (!is_valid_uuid_string(id)) {
+      throw Error(`invalid account_id '${id}'`);
+    }
+    if (id !== account_id) {
+      throw Error("users set queries may only modify the requesting account");
+    }
+    const user = usersInput[id];
+    if (!is_object(user)) {
+      throw Error("user entry must be an object");
+    }
+
+    ensureAllowedKeys(user as Record<string, unknown>);
+
+    const entry: AllowedUserFields = {};
+    if ("hide" in user) {
+      if (typeof (user as any).hide !== "boolean") {
+        throw Error("invalid type for field 'hide'");
+      }
+      entry.hide = (user as any).hide;
+    }
+    if ("upgrades" in user) {
+      entry.upgrades = sanitizeUpgrades((user as any).upgrades);
+    }
+    if ("ssh_keys" in user) {
+      entry.ssh_keys = sanitizeSshKeys((user as any).ssh_keys);
+    }
+    sanitized[id] = entry;
+  }
+
+  return sanitized;
+}
diff --git a/src/packages/database/postgres/types.ts b/src/packages/database/postgres/types.ts
@@ -161,6 +161,11 @@ export interface PostgreSQL extends EventEmitter {
     cb: CB;
   }): void;
 
+  _user_set_query_project_users(
+    obj: any,
+    account_id: string,
+  ): Record<string, unknown> | undefined;
+
   user_is_in_project_group(opts: {
     account_id: string;
     project_id: string;
