Client secrets should not be removed, but expiry checked in middleware

Author: jspahrsummers

src/server/auth/clients.ts

@@ -12,7 +12,7 @@ export interface OAuthRegisteredClientsStore {
   /**
    * Registers a new client with the server. The client ID and secret will be automatically generated by the library. A modified version of the client information can be returned to reflect specific values enforced by the server.
    * 
-   * NOTE: Implementations must ensure that client secrets, if present, are expired in accordance with the `client_secret_expires_at` field.
+   * NOTE: Implementations should NOT delete expired client secrets in-place. Auth middleware provided by this library will automatically check the `client_secret_expires_at` field and reject requests with expired secrets. Any custom logic for authenticating clients should check the `client_secret_expires_at` field as well.
    * 
    * If unimplemented, dynamic client registration is unsupported.
    */

src/server/auth/middleware/clientAuth.test.ts

@@ -15,11 +15,18 @@ describe('clientAuth middleware', () => {
           redirect_uris: ['https://example.com/callback']
         };
       } else if (clientId === 'expired-client') {
-        // Client with expired secret
+        // Client with no secret
         return {
           client_id: 'expired-client',
           redirect_uris: ['https://example.com/callback']
-          // No client_secret due to expiration
+        };
+      } else if (clientId === 'client-with-expired-secret') {
+        // Client with an expired secret
+        return {
+          client_id: 'client-with-expired-secret',
+          client_secret: 'expired-secret',
+          client_secret_expires_at: Math.floor(Date.now() / 1000) - 3600, // Expired 1 hour ago
+          redirect_uris: ['https://example.com/callback']
         };
       }
       return undefined;
@@ -101,9 +108,22 @@ describe('clientAuth middleware', () => {
         client_id: 'expired-client'
       });
 
-    // Since the expired client has no secret, this should pass without providing one
+    // Since the client has no secret, this should pass without providing one
     expect(response.status).toBe(200);
   });
+  
+  it('rejects request when client secret has expired', async () => {
+    const response = await supertest(app)
+      .post('/protected')
+      .send({
+        client_id: 'client-with-expired-secret',
+        client_secret: 'expired-secret'
+      });
+
+    expect(response.status).toBe(400);
+    expect(response.body.error).toBe('invalid_client');
+    expect(response.body.error_description).toBe('Client secret has expired');
+  });
 
   it('handles malformed request body', async () => {
     const response = await supertest(app)

src/server/auth/middleware/clientAuth.ts

@@ -39,8 +39,22 @@ export function authenticateClient({ clientsStore }: ClientAuthenticationMiddlew
         throw new InvalidClientError("Invalid client_id");
       }
 
-      if (client.client_secret !== client_secret) {
-        throw new InvalidClientError("Invalid client_secret");
+      // If client has a secret, validate it
+      if (client.client_secret) {
+        // Check if client_secret is required but not provided
+        if (!client_secret) {
+          throw new InvalidClientError("Client secret is required");
+        }
+
+        // Check if client_secret matches
+        if (client.client_secret !== client_secret) {
+          throw new InvalidClientError("Invalid client_secret");
+        }
+
+        // Check if client_secret has expired
+        if (client.client_secret_expires_at && client.client_secret_expires_at < Math.floor(Date.now() / 1000)) {
+          throw new InvalidClientError("Client secret has expired");
+        }
       }
 
       req.client = client;