Hardening of scope validation (#12490)

app/scenes/Login/OAuthScopeHelper.test.ts

@@ -0,0 +1,98 @@
+import type { TFunction } from "i18next";
+import { OAuthScopeHelper } from "./OAuthScopeHelper";
+
+const t = ((key: string) => key) as unknown as TFunction;
+
+describe("OAuthScopeHelper", () => {
+  describe("normalizeScopes", () => {
+    it("renders the full wildcard as Full access", () => {
+      expect(OAuthScopeHelper.normalizeScopes(["*"], t)).toEqual([
+        "Full access",
+      ]);
+    });
+
+    it("renders the equivalent /api/*.* route scope as Full access", () => {
+      expect(OAuthScopeHelper.normalizeScopes(["/api/*.*"], t)).toEqual([
+        "Full access",
+      ]);
+    });
+
+    it("renders global access scopes", () => {
+      expect(OAuthScopeHelper.normalizeScopes(["read"], t)).toEqual([
+        "Read all data",
+      ]);
+      expect(OAuthScopeHelper.normalizeScopes(["write"], t)).toEqual([
+        "Write all data",
+      ]);
+      expect(OAuthScopeHelper.normalizeScopes(["create"], t)).toEqual([
+        "Create all data",
+      ]);
+    });
+
+    it("renders route scopes with both namespace and method", () => {
+      expect(
+        OAuthScopeHelper.normalizeScopes(["/api/documents.list"], t)
+      ).toEqual(["Read documents"]);
+      expect(
+        OAuthScopeHelper.normalizeScopes(["/api/documents.create"], t)
+      ).toEqual(["Create documents"]);
+      expect(
+        OAuthScopeHelper.normalizeScopes(["/api/collections.update"], t)
+      ).toEqual(["Write collections"]);
+    });
+
+    it("renders wildcard methods", () => {
+      expect(OAuthScopeHelper.normalizeScopes(["/api/documents.*"], t)).toEqual(
+        ["Read and write documents"]
+      );
+    });
+
+    it("renders wildcard namespaces", () => {
+      expect(OAuthScopeHelper.normalizeScopes(["/api/*.list"], t)).toEqual([
+        "Read workspace",
+      ]);
+    });
+
+    it("renders namespaced access scopes", () => {
+      expect(OAuthScopeHelper.normalizeScopes(["documents:read"], t)).toEqual([
+        "Read documents",
+      ]);
+      expect(OAuthScopeHelper.normalizeScopes(["documents:write"], t)).toEqual([
+        "Write documents",
+      ]);
+    });
+
+    it("translates known namespaces", () => {
+      // `capitalize` lowercases the rest of the string, so "API keys" becomes
+      // "api keys" after composition with the verb. This matches what users
+      // see today.
+      expect(
+        OAuthScopeHelper.normalizeScopes(["/api/apiKeys.list"], t)
+      ).toEqual(["Read api keys"]);
+    });
+
+    it("falls back to the raw namespace when unknown", () => {
+      expect(
+        OAuthScopeHelper.normalizeScopes(["/api/widgets.list"], t)
+      ).toEqual(["Read widgets"]);
+    });
+
+    it("deduplicates equivalent scopes", () => {
+      expect(
+        OAuthScopeHelper.normalizeScopes(
+          ["/api/documents.list", "/api/documents.info", "documents:read"],
+          t
+        )
+      ).toEqual(["Read documents"]);
+    });
+
+    it("renders legacy malformed mixed scopes by their wildcard prefix", () => {
+      // These scopes can no longer be saved but may exist in older rows.
+      // The trailing access level is dropped and the result reflects the
+      // effective `*.*` access that enforcement granted.
+      expect(OAuthScopeHelper.normalizeScopes(["/api/*.*:read"], t)).toEqual([
+        "Read and write workspace",
+      ]);
+    });
+  });
+});

app/scenes/Login/OAuthScopeHelper.ts

@@ -9,7 +9,7 @@ export class OAuthScopeHelper {
       info: t("read"),
       read: t("read"),
       write: t("write"),
-      create: t("write"),
+      create: t("create"),
       update: t("write"),
       delete: t("write"),
       "*": t("read and write"),
@@ -34,12 +34,18 @@ export class OAuthScopeHelper {
     };
 
     const normalizedScopes = scopes.map((scope) => {
+      if (scope === "*" || scope === "/api/*.*") {
+        return t("Full access");
+      }
       if (scope === Scope.Read) {
         return t("Read all data");
       }
       if (scope === Scope.Write) {
         return t("Write all data");
       }
+      if (scope === Scope.Create) {
+        return t("Create all data");
+      }
 
       const [namespace, method] = scope.replace("/api/", "").split(/[:.]/g);
       const readableMethod =

server/models/ApiKey.ts

@@ -54,8 +54,9 @@ class ApiKey extends ParanoidModel<
   name: string;
 
   /** A list of scopes that this API key has access to */
-  @Matches(/[/.\w\s]*/, {
+  @Matches(AuthenticationHelper.scopeGrammarRegex, {
     each: true,
+    message: "Scope must be a valid API scope",
   })
   @Column(DataType.ARRAY(DataType.STRING))
   scope: string[] | null;

server/models/oauth/OAuthAuthentication.ts

@@ -83,8 +83,9 @@ class OAuthAuthentication extends ParanoidModel<
   grantId: string | null;
 
   /** A list of scopes that this authentication has access to */
-  @Matches(/[/.\w\s]*/, {
+  @Matches(AuthenticationHelper.scopeGrammarRegex, {
     each: true,
+    message: "Scope must be a valid API scope",
   })
   @Column(DataType.ARRAY(DataType.STRING))
   scope: string[];

server/models/oauth/OAuthAuthorizationCode.ts

@@ -8,6 +8,7 @@ import {
   Table,
   Length,
 } from "sequelize-typescript";
+import AuthenticationHelper from "@shared/helpers/AuthenticationHelper";
 import { OAuthClientValidation } from "@shared/validations";
 import env from "@server/env";
 import User from "@server/models/User";
@@ -57,8 +58,9 @@ class OAuthAuthorizationCode extends IdModel<
   grantId: string | null;
 
   /** A list of scopes that this authorization code has access to */
-  @Matches(/[/.\w\s]*/, {
+  @Matches(AuthenticationHelper.scopeGrammarRegex, {
     each: true,
+    message: "Scope must be a valid API scope",
   })
   @Column(DataType.ARRAY(DataType.STRING))
   scope: string[];

server/utils/oauth/OAuthInterface.test.ts

@@ -353,5 +353,12 @@ describe("OAuthInterface", () => {
       const result = await OAuthInterface.validateScope(user, client, scope);
       expect(result).toBe(false);
     });
+
+    it("should reject root wildcard route scopes", async () => {
+      const result = await OAuthInterface.validateScope(user, client, [
+        "/api/*.*",
+      ]);
+      expect(result).toBe(false);
+    });
   });
 });

server/utils/oauth/OAuthInterface.ts

@@ -5,7 +5,7 @@ import type {
   User as OAuthUser,
 } from "@node-oauth/oauth2-server";
 import type { Required } from "utility-types";
-import { Scope } from "@shared/types";
+import AuthenticationHelper from "@shared/helpers/AuthenticationHelper";
 import { isUrl } from "@shared/utils/urls";
 import {
   OAuthClient,
@@ -393,29 +393,11 @@ export const OAuthInterface: RefreshTokenModel &
     }
 
     const scopes = Array.isArray(scope) ? scope : [scope];
-    const validAccessScopes = Object.values(Scope);
 
-    return scopes.every((s: string) => {
-      if (validAccessScopes.includes(s as Scope)) {
-        return true;
-      }
-
-      const periodCount = (s.match(/\./g) || []).length;
-      const colonCount = (s.match(/:/g) || []).length;
-
-      if (periodCount === 1 && colonCount === 0) {
-        return true;
-      }
-
-      if (
-        colonCount === 1 &&
-        validAccessScopes.includes(s.split(":")[1] as Scope)
-      ) {
-        return true;
-      }
-
-      return false;
-    })
+    // OAuth clients cannot request scopes that grant unrestricted access.
+    return scopes.every((s: string) =>
+      AuthenticationHelper.isValidScope(s, { allowRootWildcard: false })
+    )
       ? scopes
       : false;
   },

shared/helpers/AuthenticationHelper.test.ts

@@ -146,5 +146,125 @@ describe("AuthenticationHelper", () => {
         expect(canAccess("/api/users.info", scopes)).toBe(false);
       });
     });
+
+    describe("malformed scopes", () => {
+      it("should reject mixed route and namespaced scopes", async () => {
+        // Previously these were silently parsed as `/api/*.*` and granted
+        // access to every route.
+        expect(canAccess("/api/documents.list", ["/api/*.*:read"])).toBe(false);
+        expect(canAccess("/api/documents.create", ["/api/*.*:read"])).toBe(
+          false
+        );
+        expect(
+          canAccess("/api/documents.list", ["/api/documents.list:read"])
+        ).toBe(false);
+      });
+
+      it("should reject route scopes missing a method or namespace", async () => {
+        expect(canAccess("/api/documents.list", ["/api/documents"])).toBe(
+          false
+        );
+        expect(canAccess("/api/documents.list", ["/api/.list"])).toBe(false);
+        expect(canAccess("/api/documents.list", ["/api/documents."])).toBe(
+          false
+        );
+      });
+
+      it("should reject namespaced scopes with unknown access level", async () => {
+        expect(canAccess("/api/documents.list", ["documents:foo"])).toBe(false);
+        expect(canAccess("/api/documents.list", ["documents:"])).toBe(false);
+      });
+
+      it("should reject unprefixed namespace.method scopes", async () => {
+        expect(canAccess("/api/documents.list", ["documents.list"])).toBe(
+          false
+        );
+      });
+
+      it("should reject empty scopes", async () => {
+        expect(canAccess("/api/documents.list", [""])).toBe(false);
+      });
+
+      it("should still grant access via other valid scopes alongside a malformed one", async () => {
+        expect(
+          canAccess("/api/documents.list", [
+            "/api/*.*:read",
+            "/api/documents.list",
+          ])
+        ).toBe(true);
+      });
+    });
+  });
+
+  describe("isValidScope", () => {
+    it("accepts the full wildcard", () => {
+      expect(AuthenticationHelper.isValidScope("*")).toBe(true);
+    });
+
+    it("accepts global access scopes", () => {
+      expect(AuthenticationHelper.isValidScope("read")).toBe(true);
+      expect(AuthenticationHelper.isValidScope("write")).toBe(true);
+      expect(AuthenticationHelper.isValidScope("create")).toBe(true);
+    });
+
+    it("accepts namespaced access scopes", () => {
+      expect(AuthenticationHelper.isValidScope("documents:read")).toBe(true);
+      expect(AuthenticationHelper.isValidScope("documents:write")).toBe(true);
+      expect(AuthenticationHelper.isValidScope("documents:create")).toBe(true);
+    });
+
+    it("accepts route scopes with wildcards", () => {
+      expect(AuthenticationHelper.isValidScope("/api/documents.list")).toBe(
+        true
+      );
+      expect(AuthenticationHelper.isValidScope("/api/documents.*")).toBe(true);
+      expect(AuthenticationHelper.isValidScope("/api/*.list")).toBe(true);
+      expect(AuthenticationHelper.isValidScope("/api/*.*")).toBe(true);
+    });
+
+    it("rejects mixed route and namespaced scopes", () => {
+      expect(AuthenticationHelper.isValidScope("/api/*.*:read")).toBe(false);
+      expect(
+        AuthenticationHelper.isValidScope("/api/documents.list:read")
+      ).toBe(false);
+    });
+
+    it("rejects malformed scopes", () => {
+      expect(AuthenticationHelper.isValidScope("")).toBe(false);
+      expect(AuthenticationHelper.isValidScope("documents")).toBe(false);
+      expect(AuthenticationHelper.isValidScope("documents.list")).toBe(false);
+      expect(AuthenticationHelper.isValidScope("documents:foo")).toBe(false);
+      expect(AuthenticationHelper.isValidScope("/api/documents")).toBe(false);
+      expect(AuthenticationHelper.isValidScope("/api/documents.")).toBe(false);
+      expect(AuthenticationHelper.isValidScope("/api/.list")).toBe(false);
+    });
+
+    describe("with allowRootWildcard: false", () => {
+      const opts = { allowRootWildcard: false };
+
+      it("rejects scopes that grant access to every route", () => {
+        expect(AuthenticationHelper.isValidScope("*", opts)).toBe(false);
+        expect(AuthenticationHelper.isValidScope("/api/*.*", opts)).toBe(false);
+      });
+
+      it("still accepts wildcards on a single dimension", () => {
+        expect(AuthenticationHelper.isValidScope("/api/*.list", opts)).toBe(
+          true
+        );
+        expect(
+          AuthenticationHelper.isValidScope("/api/documents.*", opts)
+        ).toBe(true);
+      });
+
+      it("still accepts other valid scopes", () => {
+        expect(AuthenticationHelper.isValidScope("read", opts)).toBe(true);
+        expect(AuthenticationHelper.isValidScope("documents:read", opts)).toBe(
+          true
+        );
+        expect(
+          AuthenticationHelper.isValidScope("/api/documents.list", opts)
+        ).toBe(true);
+      });
+    });
   });
 });

shared/helpers/AuthenticationHelper.ts

@@ -21,6 +21,41 @@ export default class AuthenticationHelper {
     export: Scope.Read,
   };
 
+  /**
+   * Matches exactly one of the supported scope grammars:
+   *
+   * - `*` — full wildcard
+   * - `read` | `write` | `create` — global access scope
+   * - `<namespace>:(read|write|create)` — namespaced access scope
+   * - `/api/<namespace>.<method>` — route scope, namespace and method may be `*`
+   */
+  public static scopeGrammarRegex =
+    /^(\*|read|write|create|\w+:(read|write|create)|\/api\/(\*|\w+)\.(\*|\w+))$/;
+
+  /**
+   * Returns whether the given string is a well-formed scope. Scopes that mix
+   * route and namespaced forms (e.g. `/api/documents.list:read`) are rejected
+   * to avoid ambiguity between validation and enforcement.
+   *
+   * @param scope The scope to validate
+   * @param options.allowRootWildcard Whether scopes that grant access to every
+   *   route (`*` and `/api/*.*`) should be considered valid. Defaults to true.
+   * @returns true if the scope conforms to the supported grammar
+   */
+  public static isValidScope = (
+    scope: string,
+    options: { allowRootWildcard?: boolean } = {}
+  ): boolean => {
+    const { allowRootWildcard = true } = options;
+    if (!AuthenticationHelper.scopeGrammarRegex.test(scope)) {
+      return false;
+    }
+    if (!allowRootWildcard && (scope === "*" || scope === "/api/*.*")) {
+      return false;
+    }
+    return true;
+  };
+
   /**
    * Returns whether the given path can be accessed with any of the scopes. We
    * support scopes in the formats of:
@@ -29,6 +64,8 @@ export default class AuthenticationHelper {
    * - `namespace:scope`
    * - `scope`
    *
+   * Malformed scopes that do not match the supported grammar are ignored.
+   *
    * @param path The path to check
    * @param scopes The scopes to check
    * @returns True if the path can be accessed
@@ -46,6 +83,10 @@ export default class AuthenticationHelper {
     const [namespace, method] = resource.split(".");
 
     return scopes.some((scope) => {
+      if (!AuthenticationHelper.isValidScope(scope)) {
+        return false;
+      }
+
       const [scopeNamespace, scopeMethod] = scope.match(/[:.]/g)
         ? scope.replace("/api/", "").split(/[:.]/g)
         : ["*", scope];

shared/i18n/locales/en_US/translation.json

@@ -1036,6 +1036,7 @@
   "You will be redirected to <em>{{ redirectUri }}</em> after authorizing. Make sure you trust this URL.": "You will be redirected to <em>{{ redirectUri }}</em> after authorizing. Make sure you trust this URL.",
   "read": "read",
   "write": "write",
+  "create": "create",
   "read and write": "read and write",
   "API keys": "API keys",
   "attachments": "attachments",
@@ -1052,8 +1053,10 @@
   "users": "users",
   "teams": "teams",
   "workspace": "workspace",
+  "Full access": "Full access",
   "Read all data": "Read all data",
   "Write all data": "Write all data",
+  "Create all data": "Create all data",
   "Any collection": "Any collection",
   "All time": "All time",
   "Past day": "Past day",