Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This pull request adds support for HTTP Basic Authentication as a method for clients to provide credentials when requesting OAuth tokens, in addition to the existing form body method. This allows clients to send client_id and client_secret via the Authorization: Basic header instead of in the request body.
Changes:
- Added
parseBasicAuthutility function to extract and decode client credentials from Authorization headers - Integrated Basic Auth support into the OAuth token endpoint for all grant types
- Added comprehensive unit tests and e2e tests for Basic Auth functionality
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| lib/oauth/basic-auth.ts | New utility function to parse and extract credentials from Basic Auth headers |
| lib/oauth/basic-auth.test.ts | Comprehensive unit tests for the Basic Auth parser covering valid/invalid cases |
| app/api/oauth/token/route.ts | Integration of Basic Auth support into token endpoint with body credentials taking precedence |
| e2e/oauth/client-credentials.spec.ts | End-to-end tests validating Basic Auth works for client credentials grant type |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const credentials = Buffer.from(base64Credentials, "base64").toString( | ||
| "utf-8", |
There was a problem hiding this comment.
The Buffer.from() operation on untrusted base64 input could potentially cause issues with malformed input. While the try-catch block handles this, consider adding explicit validation of the base64 string format before decoding to fail fast on obviously malformed input. Additionally, consider adding a maximum length check on the base64Credentials string to prevent potential DoS attacks with extremely large authorization headers.
| clientSecret: "secret%3Dvalue", | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing test case for invalid base64 encoding. The function should handle cases where the Authorization header contains "Basic" followed by invalid base64 characters (e.g., "Basic !!!invalid!!!"). This would help ensure the error handling in the try-catch block is properly tested.
| clientSecret: "secret%3Dvalue", | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing test case for very long credentials that could test buffer limits. Consider adding a test with extremely long clientId or clientSecret values to ensure the function handles them appropriately without performance degradation or memory issues.
| // Only use header values if not already in body | ||
| if (!body.client_id) { | ||
| body.client_id = basicAuth.clientId; | ||
| } | ||
| if (!body.client_secret) { | ||
| body.client_secret = basicAuth.clientSecret; | ||
| } |
There was a problem hiding this comment.
According to RFC 6749 Section 2.3.1, including client credentials in both the Authorization header and request body is not permitted. The current implementation silently prefers body credentials, but it should instead return an error if credentials are provided in both locations. This prevents ambiguous authentication attempts and follows the OAuth 2.0 specification more strictly.
| // Only use header values if not already in body | |
| if (!body.client_id) { | |
| body.client_id = basicAuth.clientId; | |
| } | |
| if (!body.client_secret) { | |
| body.client_secret = basicAuth.clientSecret; | |
| } | |
| // RFC 6749 Section 2.3.1: client credentials MUST NOT be included | |
| // in more than one location (e.g., Authorization header and body). | |
| if (body.client_id !== undefined || body.client_secret !== undefined) { | |
| return NextResponse.json( | |
| { | |
| error: "invalid_request", | |
| error_description: | |
| "Client credentials must not be provided in multiple locations", | |
| }, | |
| { status: 400 }, | |
| ); | |
| } | |
| body.client_id = basicAuth.clientId; | |
| body.client_secret = basicAuth.clientSecret; |
| test("should prefer body credentials over header credentials", async ({ | ||
| request, | ||
| }) => { | ||
| // Send wrong credentials in header but correct in body | ||
| const wrongCredentials = Buffer.from("wrong:wrong").toString("base64"); | ||
|
|
||
| const response = await request.post("/api/oauth/token", { | ||
| headers: { | ||
| Authorization: `Basic ${wrongCredentials}`, | ||
| }, | ||
| form: { | ||
| grant_type: "client_credentials", | ||
| client_id: clientId, | ||
| client_secret: clientSecret, | ||
| scope: "read:profile", | ||
| }, | ||
| }); | ||
|
|
||
| // Should succeed because body credentials take precedence | ||
| expect(response.ok()).toBeTruthy(); | ||
| const tokens = await response.json(); | ||
| expect(tokens.access_token).toBeTruthy(); | ||
| }); |
There was a problem hiding this comment.
This test validates behavior that conflicts with RFC 6749 Section 2.3.1. According to the OAuth 2.0 specification, a client MUST NOT use more than one authentication method in each request. Rather than allowing body credentials to take precedence, the endpoint should return an error when credentials are provided in both the Authorization header and request body. This test should be updated to expect a 400 or 401 error response instead of success.
| const tokens = await response.json(); | ||
| expect(tokens.access_token).toBeTruthy(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing e2e test case for malformed Basic auth headers. Consider adding tests for edge cases such as: 1) Authorization header with "Basic" but no space or credentials, 2) Authorization header with invalid base64 encoding, 3) Authorization header with "Basic" followed by empty string. These cases help ensure robust error handling in production.
| // Support client credentials from Authorization header (Basic auth) | ||
| const authHeader = request.headers.get("authorization"); | ||
| const basicAuth = parseBasicAuth(authHeader); | ||
| if (basicAuth) { | ||
| // Only use header values if not already in body | ||
| if (!body.client_id) { | ||
| body.client_id = basicAuth.clientId; | ||
| } | ||
| if (!body.client_secret) { | ||
| body.client_secret = basicAuth.clientSecret; | ||
| } | ||
| } |
There was a problem hiding this comment.
For consistency with the token endpoint, consider also supporting Basic Auth credentials in the revoke endpoint (app/api/oauth/revoke/route.ts). The OAuth 2.0 Token Revocation spec (RFC 7009) allows the same client authentication methods as the token endpoint, so clients expecting Basic Auth support would expect it to work for revocation as well.
1 participant
No description provided.