feat(oauth): add enhanced scope validation with migration suggestions

Author: bitbeckers

packages/sdk-core/src/auth/OAuthClient.ts

@@ -4,6 +4,7 @@ import type { ATProtoSDKConfig } from "../core/config.js";
 import { AuthenticationError, NetworkError } from "../core/errors.js";
 import { InMemorySessionStore } from "../storage/InMemorySessionStore.js";
 import { InMemoryStateStore } from "../storage/InMemoryStateStore.js";
+import { parseScope, validateScope, ATPROTO_SCOPE } from "./permissions.js";
 
 /**
  * Options for the OAuth authorization flow.
@@ -179,7 +180,7 @@ export class OAuthClient {
    */
   private buildClientMetadata() {
     const clientIdUrl = new URL(this.config.oauth.clientId);
-    return {
+    const metadata = {
       client_id: this.config.oauth.clientId,
       client_name: "ATProto SDK Client",
       client_uri: clientIdUrl.origin,
@@ -193,6 +194,89 @@ export class OAuthClient {
       dpop_bound_access_tokens: true,
       jwks_uri: this.config.oauth.jwksUri,
     } as const;
+
+    // Validate scope before returning metadata
+    this.validateClientMetadataScope(metadata.scope);
+
+    return metadata;
+  }
+
+  /**
+   * Validates the OAuth scope in client metadata and logs warnings/suggestions.
+   *
+   * This method:
+   * 1. Checks if the scope is well-formed using permission utilities
+   * 2. Detects mixing of transitional and granular permissions
+   * 3. Logs warnings for missing `atproto` scope
+   * 4. Suggests migration to granular permissions for transitional scopes
+   *
+   * @param scope - The OAuth scope string to validate
+   * @internal
+   */
+  private validateClientMetadataScope(scope: string): void {
+    // Parse the scope into individual permissions
+    const permissions = parseScope(scope);
+
+    // Validate well-formedness
+    const validation = validateScope(scope);
+    if (!validation.isValid) {
+      this.logger?.error("Invalid OAuth scope detected", {
+        invalidPermissions: validation.invalidPermissions,
+        scope,
+      });
+    }
+
+    // Check for atproto scope
+    const hasAtproto = permissions.includes(ATPROTO_SCOPE);
+    if (!hasAtproto) {
+      this.logger?.warn("OAuth scope missing 'atproto' - basic API access may be limited", {
+        scope,
+        suggestion: "Add 'atproto' to your scope for basic API access",
+      });
+    }
+
+    // Detect transitional scopes
+    const transitionalScopes = permissions.filter((p) => p.startsWith("transition:"));
+    const granularScopes = permissions.filter(
+      (p) =>
+        p.startsWith("account:") ||
+        p.startsWith("repo:") ||
+        p.startsWith("blob") ||
+        p.startsWith("rpc:") ||
+        p.startsWith("identity:") ||
+        p.startsWith("include:"),
+    );
+
+    // Log info about transitional scopes
+    if (transitionalScopes.length > 0) {
+      this.logger?.info("Using transitional OAuth scopes (legacy)", {
+        transitionalScopes,
+        note: "Transitional scopes are supported but granular permissions are recommended",
+      });
+
+      // Suggest migration to granular permissions
+      if (transitionalScopes.includes("transition:email")) {
+        this.logger?.info("Consider migrating 'transition:email' to granular permissions", {
+          suggestion: "Use: account:email?action=read",
+          example: "import { ScopePresets } from '@hypercerts-org/sdk-core'; scope: ScopePresets.EMAIL_READ",
+        });
+      }
+      if (transitionalScopes.includes("transition:generic")) {
+        this.logger?.info("Consider migrating 'transition:generic' to granular permissions", {
+          suggestion: "Use specific permissions like: repo:* account:repo?action=read",
+          example: "import { ScopePresets } from '@hypercerts-org/sdk-core'; scope: ScopePresets.FULL_ACCESS",
+        });
+      }
+    }
+
+    // Warn if mixing transitional and granular
+    if (transitionalScopes.length > 0 && granularScopes.length > 0) {
+      this.logger?.warn("Mixing transitional and granular OAuth scopes", {
+        transitionalScopes,
+        granularScopes,
+        note: "While supported, it's recommended to use either transitional or granular permissions consistently",
+      });
+    }
   }
 
   /**

packages/sdk-core/tests/auth/OAuthClient.test.ts

@@ -161,4 +161,134 @@ describe("OAuthClient", () => {
       expect(logger.logs.length).toBeGreaterThan(0);
     });
   });
+
+  describe("scope validation", () => {
+    it("should log error for invalid scope", async () => {
+      const { MockLogger } = await import("../utils/mocks.js");
+      const logger = new MockLogger();
+      const configWithInvalidScope = await createTestConfigAsync({
+        logger,
+        oauth: {
+          ...config.oauth,
+          scope: "atproto invalid:scope another-bad-scope",
+        },
+      });
+
+      new OAuthClient(configWithInvalidScope);
+
+      // Should have logged an error for invalid permissions
+      const errorLogs = logger.logs.filter((log) => log.level === "error");
+      expect(errorLogs.length).toBeGreaterThan(0);
+      expect(errorLogs[0].message).toContain("Invalid OAuth scope detected");
+    });
+
+    it("should log warning for missing atproto scope", async () => {
+      const { MockLogger } = await import("../utils/mocks.js");
+      const logger = new MockLogger();
+      const configWithoutAtproto = await createTestConfigAsync({
+        logger,
+        oauth: {
+          ...config.oauth,
+          scope: "transition:email",
+        },
+      });
+
+      // Note: The underlying @atproto/oauth-client library will throw during async initialization
+      // because it requires "atproto" scope. However, our validation runs synchronously first and logs the warning.
+      const client = new OAuthClient(configWithoutAtproto);
+
+      // The client initialization promise will reject - we need to handle it to prevent unhandled rejection
+      // We use authorize() to trigger initialization, then catch the rejection
+      try {
+        await client.authorize("test.bsky.social");
+        // eslint-disable-next-line @typescript-eslint/no-unused-vars
+      } catch (error) {
+        // Expected - underlying client initialization will fail due to missing atproto scope
+      }
+
+      // Should have logged a warning for missing atproto during buildClientMetadata()
+      const warnLogs = logger.logs.filter((log) => log.level === "warn");
+      expect(warnLogs.length).toBeGreaterThan(0);
+      expect(warnLogs[0].message).toContain("missing 'atproto'");
+    });
+
+    it("should detect mixed transitional and granular permissions", async () => {
+      const { MockLogger } = await import("../utils/mocks.js");
+      const logger = new MockLogger();
+      const configWithMixedScopes = await createTestConfigAsync({
+        logger,
+        oauth: {
+          ...config.oauth,
+          scope: "atproto transition:email account:email?action=read",
+        },
+      });
+
+      new OAuthClient(configWithMixedScopes);
+
+      // Should have logged a warning about mixing permission models
+      const warnLogs = logger.logs.filter((log) => log.level === "warn");
+      const mixedWarning = warnLogs.find((log) => log.message.includes("Mixing transitional and granular"));
+      expect(mixedWarning).toBeDefined();
+    });
+
+    it("should suggest migration for transition:email", async () => {
+      const { MockLogger } = await import("../utils/mocks.js");
+      const logger = new MockLogger();
+      const configWithTransitionEmail = await createTestConfigAsync({
+        logger,
+        oauth: {
+          ...config.oauth,
+          scope: "atproto transition:email",
+        },
+      });
+
+      new OAuthClient(configWithTransitionEmail);
+
+      // Should have logged info about transitional scopes
+      const infoLogs = logger.logs.filter((log) => log.level === "info");
+      const migrationSuggestion = infoLogs.find((log) => log.message.includes("migrating 'transition:email'"));
+      expect(migrationSuggestion).toBeDefined();
+      expect(migrationSuggestion?.args[0]).toHaveProperty("suggestion");
+    });
+
+    it("should suggest migration for transition:generic", async () => {
+      const { MockLogger } = await import("../utils/mocks.js");
+      const logger = new MockLogger();
+      const configWithTransitionGeneric = await createTestConfigAsync({
+        logger,
+        oauth: {
+          ...config.oauth,
+          scope: "atproto transition:generic",
+        },
+      });
+
+      new OAuthClient(configWithTransitionGeneric);
+
+      // Should have logged info about transitional scopes
+      const infoLogs = logger.logs.filter((log) => log.level === "info");
+      const migrationSuggestion = infoLogs.find((log) => log.message.includes("migrating 'transition:generic'"));
+      expect(migrationSuggestion).toBeDefined();
+      expect(migrationSuggestion?.args[0]).toHaveProperty("suggestion");
+    });
+
+    it("should not log warnings for valid granular permissions with atproto", async () => {
+      const { MockLogger } = await import("../utils/mocks.js");
+      const logger = new MockLogger();
+      const configWithValidScope = await createTestConfigAsync({
+        logger,
+        oauth: {
+          ...config.oauth,
+          scope: "atproto account:email?action=read repo:app.bsky.feed.post?action=create",
+        },
+      });
+
+      new OAuthClient(configWithValidScope);
+
+      // Should not have logged any errors or warnings (only info about granular permissions is OK)
+      const errorLogs = logger.logs.filter((log) => log.level === "error");
+      const warnLogs = logger.logs.filter((log) => log.level === "warn");
+      expect(errorLogs.length).toBe(0);
+      expect(warnLogs.length).toBe(0);
+    });
+  });
 });