apply suggested updates

Author: ochafik

src/client/auth.test.ts

@@ -10,7 +10,7 @@ import {
   auth,
   type OAuthClientProvider,
 } from "./auth.js";
-import { OAuthMetadata } from 'src/shared/auth.js';
+import { OAuthMetadata } from '../shared/auth.js';
 
 // Mock fetch globally
 const mockFetch = jest.fn();
@@ -1615,6 +1615,11 @@ describe("OAuth Authorization", () => {
       token_endpoint_auth_methods_supported: ["none"],
     };
 
+    const metadataWithAllBuiltinMethods = {
+      ...metadataWithBasicOnly,
+      token_endpoint_auth_methods_supported: ["client_secret_basic", "client_secret_post", "none"],
+    };
+
     it("uses HTTP Basic authentication when client_secret_basic is supported", async () => {
       mockFetch.mockResolvedValueOnce({
         ok: true,
@@ -1639,8 +1644,8 @@ describe("OAuth Authorization", () => {
       expect(authHeader).toBe(expected);
 
       const body = request.body as URLSearchParams;
-      expect(body.get("client_id")).toBeNull();     // should not be in body
-      expect(body.get("client_secret")).toBeNull(); // should not be in body
+      expect(body.get("client_id")).toBeNull();
+      expect(body.get("client_secret")).toBeNull();
     });
 
     it("includes credentials in request body when client_secret_post is supported", async () => {
@@ -1669,6 +1674,35 @@ describe("OAuth Authorization", () => {
       expect(body.get("client_secret")).toBe("secret123");
     });
 
+    it("it picks client_secret_basic when all builtin methods are supported", async () => {
+      mockFetch.mockResolvedValueOnce({
+        ok: true,
+        status: 200,
+        json: async () => validTokens,
+      });
+
+      const tokens = await exchangeAuthorization("https://auth.example.com", {
+        metadata: metadataWithAllBuiltinMethods,
+        clientInformation: validClientInfo,
+        authorizationCode: "code123",
+        redirectUri: "http://localhost:3000/callback",
+        codeVerifier: "verifier123",
+      });
+
+      expect(tokens).toEqual(validTokens);
+      const request = mockFetch.mock.calls[0][1];
+
+      // Check Authorization header - should use Basic auth as it's the most secure
+      const authHeader = request.headers.get("Authorization");
+      const expected = "Basic " + btoa("client123:secret123");
+      expect(authHeader).toBe(expected);
+
+      // Credentials should not be in body when using Basic auth
+      const body = request.body as URLSearchParams;
+      expect(body.get("client_id")).toBeNull();
+      expect(body.get("client_secret")).toBeNull();
+    });
+
     it("uses public client authentication when none method is specified", async () => {
       mockFetch.mockResolvedValueOnce({
         ok: true,

src/client/auth.ts

@@ -86,9 +86,10 @@ export interface OAuthClientProvider {
    * - Adding custom headers for proprietary authentication schemes
    * - Implementing client assertion-based authentication (e.g., JWT bearer tokens)
    *
-   * @param url - The token endpoint URL being called
    * @param headers - The request headers (can be modified to add authentication)
    * @param params - The request body parameters (can be modified to add credentials)
+   * @param url - The token endpoint URL being called
+   * @param metadata - Optional OAuth metadata for the server, which may include supported authentication methods
    */
   addClientAuthentication?(headers: Headers, params: URLSearchParams, url: string | URL, metadata?: OAuthMetadata): void | Promise<void>;
 
@@ -110,6 +111,8 @@ export class UnauthorizedError extends Error {
   }
 }
 
+type ClientAuthMethod = 'client_secret_basic' | 'client_secret_post' | 'none';
+
 /**
  * Determines the best client authentication method to use based on server support and client configuration.
  *
@@ -125,8 +128,8 @@ export class UnauthorizedError extends Error {
 function selectClientAuthMethod(
   clientInformation: OAuthClientInformation,
   supportedMethods: string[]
-): string {
-  const hasClientSecret = !!clientInformation.client_secret;
+): ClientAuthMethod {
+  const hasClientSecret = clientInformation.client_secret !== undefined;
 
   // If server doesn't specify supported methods, use RFC 6749 defaults
   if (supportedMethods.length === 0) {
@@ -165,29 +168,26 @@ function selectClientAuthMethod(
  * @throws {Error} When required credentials are missing
  */
 function applyClientAuthentication(
-  method: string,
+  method: ClientAuthMethod,
   clientInformation: OAuthClientInformation,
   headers: Headers,
   params: URLSearchParams
 ): void {
   const { client_id, client_secret } = clientInformation;
 
-  if (method === "client_secret_basic") {
-    applyBasicAuth(client_id, client_secret, headers);
-    return;
-  }
-
-  if (method === "client_secret_post") {
-    applyPostAuth(client_id, client_secret, params);
-    return;
-  }
-
-  if (method === "none") {
-    applyPublicAuth(client_id, params);
-    return;
+  switch (method) {
+    case "client_secret_basic":
+      applyBasicAuth(client_id, client_secret, headers);
+      return;
+    case "client_secret_post":
+      applyPostAuth(client_id, client_secret, params);
+      return;
+    case "none":
+      applyPublicAuth(client_id, params);
+      return;
+    default:
+      throw new Error(`Unsupported client authentication method: ${method}`);
   }
-
-  throw new Error(`Unsupported client authentication method: ${method}`);
 }
 
 /**