diff --git a/app/scenes/Login/OAuthScopeHelper.test.ts b/app/scenes/Login/OAuthScopeHelper.test.ts new file mode 100644 index 0000000000..d95a2d4ffe --- /dev/null +++ b/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", + ]); + }); + }); +}); diff --git a/app/scenes/Login/OAuthScopeHelper.ts b/app/scenes/Login/OAuthScopeHelper.ts index 078ee8810e..f63fb73a56 100644 --- a/app/scenes/Login/OAuthScopeHelper.ts +++ b/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 = diff --git a/server/models/ApiKey.ts b/server/models/ApiKey.ts index 4116aa2a20..38e3fd7a54 100644 --- a/server/models/ApiKey.ts +++ b/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; diff --git a/server/models/oauth/OAuthAuthentication.ts b/server/models/oauth/OAuthAuthentication.ts index f75a355aca..aa9f9becf6 100644 --- a/server/models/oauth/OAuthAuthentication.ts +++ b/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[]; diff --git a/server/models/oauth/OAuthAuthorizationCode.ts b/server/models/oauth/OAuthAuthorizationCode.ts index 37767effe9..15311695c2 100644 --- a/server/models/oauth/OAuthAuthorizationCode.ts +++ b/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[]; diff --git a/server/utils/oauth/OAuthInterface.test.ts b/server/utils/oauth/OAuthInterface.test.ts index 93777a4047..b515a2974f 100644 --- a/server/utils/oauth/OAuthInterface.test.ts +++ b/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); + }); }); }); diff --git a/server/utils/oauth/OAuthInterface.ts b/server/utils/oauth/OAuthInterface.ts index 979e690641..e869a46667 100644 --- a/server/utils/oauth/OAuthInterface.ts +++ b/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; }, diff --git a/shared/helpers/AuthenticationHelper.test.ts b/shared/helpers/AuthenticationHelper.test.ts index 9b57342f23..3a659b7a80 100644 --- a/shared/helpers/AuthenticationHelper.test.ts +++ b/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); + }); + }); }); }); diff --git a/shared/helpers/AuthenticationHelper.ts b/shared/helpers/AuthenticationHelper.ts index bf492b40f4..b657b62397 100644 --- a/shared/helpers/AuthenticationHelper.ts +++ b/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 + * - `:(read|write|create)` — namespaced access scope + * - `/api/.` — 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]; diff --git a/shared/i18n/locales/en_US/translation.json b/shared/i18n/locales/en_US/translation.json index b87b45f606..512a1b7515 100644 --- a/shared/i18n/locales/en_US/translation.json +++ b/shared/i18n/locales/en_US/translation.json @@ -1036,6 +1036,7 @@ "You will be redirected to {{ redirectUri }} after authorizing. Make sure you trust this URL.": "You will be redirected to {{ redirectUri }} 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",