Feat: Api key enable - #237
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA new NestJS interceptor, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ScoresController
participant ValidateApiKeyInterceptor
participant AuthService
Client->>ScoresController: HTTP Request (e.g., POST /updateLearnerProfile/ta)
ScoresController->>ValidateApiKeyInterceptor: Request intercepted
ValidateApiKeyInterceptor->>ValidateApiKeyInterceptor: Check env for API key validation
alt Validation enabled
ValidateApiKeyInterceptor->>ValidateApiKeyInterceptor: Extract 'api-key' header
ValidateApiKeyInterceptor->>AuthService: POST /validate with API key
AuthService-->>ValidateApiKeyInterceptor: Validation response
alt Key valid
ValidateApiKeyInterceptor->>ScoresController: Proceed to controller logic
else Key invalid
ValidateApiKeyInterceptor-->>Client: 401 Unauthorized
end
else Validation disabled
ValidateApiKeyInterceptor->>ScoresController: Proceed to controller logic
end
ScoresController-->>Client: Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/middlewares/verify.key.ts (3)
17-19: Consider sanitizing the API key input.While type checking is good, consider additional validation to prevent potential injection attacks or malformed keys.
if (!clientApiKey || typeof clientApiKey !== 'string') { throw new HttpException('API key missing or invalid', HttpStatus.UNAUTHORIZED); } +if (clientApiKey.length > 256 || !/^[a-zA-Z0-9\-_\.]+$/.test(clientApiKey)) { + throw new HttpException('API key format invalid', HttpStatus.UNAUTHORIZED); +}
20-26: Consider implementing API key caching for better performance.Each request triggers an external API call, which could impact performance and reliability. Consider caching valid API keys with TTL.
You could implement a simple in-memory cache with TTL or use Redis for distributed caching:
private cache = new Map<string, { isValid: boolean; expires: number }>(); private isValidCached(apiKey: string): boolean | null { const cached = this.cache.get(apiKey); if (cached && cached.expires > Date.now()) { return cached.isValid; } return null; }
37-37: Consider removing redundant default export.The class is already exported as a named export. The default export might be unnecessary unless specifically required by the consuming code.
-export default ValidateApiKeyInterceptor;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/middlewares/verify.key.ts(1 hunks)src/mongodb/scores.controller.ts(14 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/mongodb/scores.controller.ts (1)
Learnt from: DevendraPPatil
PR: #238
File: src/mongodb/scores.controller.ts:6470-6499
Timestamp: 2025-07-28T08:57:10.283Z
Learning: For the project Sunbird-ALL/all-learner-ai-services, API documentation (Swagger decorators like @ApiBody, @apiresponse, @apioperation) is not required for new endpoints in the scores controller.
🔇 Additional comments (7)
src/middlewares/verify.key.ts (2)
1-6: LGTM! Proper NestJS interceptor setup.The imports and class declaration follow NestJS conventions correctly with appropriate dependencies imported.
7-34: Audit API Key Interceptor Coverage Across ControllersBased on the grep results:
- The
ValidateApiKeyInterceptoris applied on all routes insrc/mongodb/scores.controller.ts.- No interceptor is used in
src/mysql/scores.controller.ts.- The health‐check (
app.controller.ts) and root (getHello) endpoints are unprotected.Please verify:
• Which controllers/endpoints should enforce API-key validation (e.g., MySQL scores routes)?
• If protection is required, add@UseInterceptors(ValidateApiKeyInterceptor)at the class or method level.
• Ensure your deployment or.envfile definesAPI_KEY_ENABLE='true'and a validAUTH_SERVICE_APIURL.src/mongodb/scores.controller.ts (5)
14-14: LGTM! Import statement correctly added.The
UseInterceptorsimport is properly added to support the new API key validation functionality.
38-38: LGTM! ValidateApiKeyInterceptor import is correct.The interceptor is imported from the expected location and follows standard NestJS import patterns.
114-114: Consistent API key validation applied to all updateLearnerProfile endpoints.The
@UseInterceptors(ValidateApiKeyInterceptor)decorator has been systematically applied to all seven language-specific POST endpoints for updating learner profiles (Tamil, Gujarati, Oriya, Hindi, Kannada, English, Telugu). This ensures consistent security enforcement across all profile update operations.Also applies to: 480-480, 846-846, 1206-1206, 1579-1579, 2412-2412, 2973-2973
3962-3962: API key validation properly applied to GetContent endpoints.The interceptor is correctly applied to all four GET endpoints that fetch user-specific content by different types (char, word, sentence, paragraph). This maintains consistent security for content retrieval operations.
Also applies to: 4140-4140, 4303-4303, 4476-4476
4691-4691: LGTM! getSetResult endpoint properly secured.The
@UseInterceptors(ValidateApiKeyInterceptor)decorator is correctly applied to the POST/getSetResultendpoint, which handles session result calculations and milestone updates. This is appropriate given the critical nature of this endpoint.
| const apiKeyEnabled = process.env.API_KEY_ENABLE === 'true'; | ||
| const clientApiKey = request.headers['api-key']; | ||
| const validateUrl = process.env.AUTH_SERVICE_API || ''; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for AUTH_SERVICE_API environment variable.
The validateUrl could be empty if AUTH_SERVICE_API is not set, which would cause the axios request to fail unexpectedly.
const apiKeyEnabled = process.env.API_KEY_ENABLE === 'true';
const clientApiKey = request.headers['api-key'];
-const validateUrl = process.env.AUTH_SERVICE_API || '';
+const validateUrl = process.env.AUTH_SERVICE_API;
if (!apiKeyEnabled) {
return next.handle();
}
+if (!validateUrl) {
+ throw new HttpException('AUTH_SERVICE_API not configured', HttpStatus.INTERNAL_SERVER_ERROR);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const apiKeyEnabled = process.env.API_KEY_ENABLE === 'true'; | |
| const clientApiKey = request.headers['api-key']; | |
| const validateUrl = process.env.AUTH_SERVICE_API || ''; | |
| const apiKeyEnabled = process.env.API_KEY_ENABLE === 'true'; | |
| const clientApiKey = request.headers['api-key']; | |
| - const validateUrl = process.env.AUTH_SERVICE_API || ''; | |
| + const validateUrl = process.env.AUTH_SERVICE_API; | |
| if (!apiKeyEnabled) { | |
| return next.handle(); | |
| } | |
| + if (!validateUrl) { | |
| + throw new HttpException( | |
| + 'AUTH_SERVICE_API not configured', | |
| + HttpStatus.INTERNAL_SERVER_ERROR | |
| + ); | |
| + } |
🤖 Prompt for AI Agents
In src/middlewares/verify.key.ts around lines 10 to 12, the AUTH_SERVICE_API
environment variable is used without validation, which can lead to an empty URL
and cause axios requests to fail. Add a check to ensure AUTH_SERVICE_API is set
and not empty before using it; if it is missing, handle the error appropriately
by logging or throwing an error to prevent making a request with an invalid URL.
| if (!clientApiKey || typeof clientApiKey !== 'string') { | ||
| throw new HttpException('API key missing or invalid', HttpStatus.UNAUTHORIZED); | ||
| } | ||
| const responseFromAuth = await axios.post(validateUrl, { apiKey: clientApiKey }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout and error handling for external API call.
The axios request lacks timeout configuration and could hang indefinitely, causing request timeouts. Also consider implementing retry logic for transient failures.
-const responseFromAuth = await axios.post(validateUrl, { apiKey: clientApiKey });
+const responseFromAuth = await axios.post(validateUrl, { apiKey: clientApiKey }, {
+ timeout: 5000, // 5 second timeout
+ validateStatus: (status) => status < 500, // Don't throw for 4xx errors
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const responseFromAuth = await axios.post(validateUrl, { apiKey: clientApiKey }); | |
| const responseFromAuth = await axios.post(validateUrl, { apiKey: clientApiKey }, { | |
| timeout: 5000, // 5 second timeout | |
| validateStatus: (status) => status < 500, // Don't throw for 4xx errors | |
| }); |
🤖 Prompt for AI Agents
In src/middlewares/verify.key.ts at line 20, the axios.post call to the external
API lacks a timeout setting and error handling, which can cause the request to
hang indefinitely. Add a timeout option to the axios request configuration to
limit wait time, and wrap the call in a try-catch block to handle errors
gracefully. Optionally, implement retry logic for transient failures by retrying
the request a few times before failing.
| if (responseFromAuth.data.isValid === true) { | ||
| return next.handle(); | ||
| } else { | ||
| throw new HttpException('Unauthorized: API key invalid', HttpStatus.UNAUTHORIZED); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add response structure validation.
The code assumes the response has a specific structure without validation, which could cause runtime errors.
-if (responseFromAuth.data.isValid === true) {
+if (responseFromAuth.data && typeof responseFromAuth.data.isValid === 'boolean' && responseFromAuth.data.isValid === true) {
return next.handle();
} else {
throw new HttpException('Unauthorized: API key invalid', HttpStatus.UNAUTHORIZED);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (responseFromAuth.data.isValid === true) { | |
| return next.handle(); | |
| } else { | |
| throw new HttpException('Unauthorized: API key invalid', HttpStatus.UNAUTHORIZED); | |
| } | |
| if (responseFromAuth.data && typeof responseFromAuth.data.isValid === 'boolean' && responseFromAuth.data.isValid === true) { | |
| return next.handle(); | |
| } else { | |
| throw new HttpException('Unauthorized: API key invalid', HttpStatus.UNAUTHORIZED); | |
| } |
🤖 Prompt for AI Agents
In src/middlewares/verify.key.ts around lines 22 to 26, the code assumes
responseFromAuth.data has an isValid property without checking if
responseFromAuth or responseFromAuth.data exist. Add validation to confirm
responseFromAuth and responseFromAuth.data are defined and that isValid is a
boolean before accessing it. If the structure is invalid, handle it gracefully
by throwing an appropriate HttpException or error.
| throw new HttpException('Unauthorized: API key invalid', HttpStatus.UNAUTHORIZED); | ||
| } | ||
| } catch (err) { | ||
| console.error('API key validation error:', err); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid logging sensitive information.
The error logging could potentially expose API keys or other sensitive data from the error object.
-console.error('API key validation error:', err);
+console.error('API key validation error:', err.message);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.error('API key validation error:', err); | |
| console.error('API key validation error:', err.message); |
🤖 Prompt for AI Agents
In src/middlewares/verify.key.ts at line 28, the current error logging statement
outputs the entire error object, which may contain sensitive information like
API keys. Modify the logging to avoid printing the full error object; instead,
log a generic error message or sanitize the error details to exclude sensitive
data before logging.
|
2 participants
Feat: Api key enable
Summary by CodeRabbit
New Features
Bug Fixes