chore: add logging to signed url request #1665
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes introduce authentication for obtaining presigned S3 URLs by requiring a bearer token in API requests, sourced from an environment variable. Both the client and server now handle authorization and provide improved, more specific error messages. Server-side error logging has also been enhanced for better traceability. Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🪧 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
lib/files/get-file.ts (1)
44-45: Improved error handling with specific error messages.The error handling has been enhanced to extract and propagate the specific error message from the server response, which provides better context for debugging and user feedback.
Consider adding a fallback for cases where the response might not be in JSON format or doesn't have a message property:
- const error = await response.json(); - throw new Error(error.message); + try { + const error = await response.json(); + throw new Error(error.message || `Failed to get file from S3: ${response.status}`); + } catch (e) { + // Handle case where response is not JSON + throw new Error(`Failed to get file from S3: ${response.status}`); + }pages/api/file/s3/get-presigned-get-url.ts (2)
21-29: Added authorization check for API endpoint security.Authentication has been implemented to secure the API endpoint, which is a good security practice. The code checks for a bearer token in the Authorization header and validates it against an environment variable.
Consider handling the case where the Authorization header might be missing or malformed:
// Extract the API Key from the Authorization header const authHeader = req.headers.authorization; - const token = authHeader?.split(" ")[1]; // Assuming the format is "Bearer [token]" + const token = authHeader && authHeader.startsWith('Bearer ') ? authHeader.split(" ")[1] : null; // Check if the API Key matches - if (token !== process.env.INTERNAL_API_KEY) { + if (!token || token !== process.env.INTERNAL_API_KEY) { res.status(401).json({ message: "Unauthorized" }); return; }
61-67: Enhanced error logging and response.The error handling has been improved to log detailed information about errors and provide a more specific error message in the response. This will aid in debugging and provide better feedback.
Consider sanitizing sensitive information in error logs and adding more structure to the error response:
log({ - message: `Error getting presigned get url for ${key} \n\n ${error}`, + message: `Error getting presigned get url for ${key.substring(0, 20)}... \n\n ${error.toString().substring(0, 200)}...`, type: "error", }); return res .status(500) - .json({ error: "AWS Cloudfront Signed URL Error", message: error }); + .json({ + error: "AWS Cloudfront Signed URL Error", + message: error.toString(), + requestId: req.headers['x-request-id'] || Date.now().toString() // For traceability + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
lib/files/get-file.ts(1 hunks)pages/api/file/s3/get-presigned-get-url.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
pages/api/file/s3/get-presigned-get-url.ts (1)
lib/utils.ts (1)
log(52-112)
🔇 Additional comments (1)
pages/api/file/s3/get-presigned-get-url.ts (1)
9-9: Added logging utility import.The import for the
logutility has been added to support enhanced error logging.
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| Authorization: `Bearer ${process.env.INTERNAL_API_KEY}`, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Authentication added for presigned URL requests.
A bearer token authentication has been added to secure the S3 presigned URL endpoint. This is a good security practice.
Consider adding a check to handle cases where the environment variable might not be set:
- Authorization: `Bearer ${process.env.INTERNAL_API_KEY}`,
+ Authorization: process.env.INTERNAL_API_KEY ? `Bearer ${process.env.INTERNAL_API_KEY}` : '',Also, verify that this environment variable is properly documented and set in all deployment environments:
🏁 Script executed:
#!/bin/bash
echo "Checking if INTERNAL_API_KEY is defined in environment files..."
grep -r "INTERNAL_API_KEY" .env* --include=".env*" || echo "INTERNAL_API_KEY not found in any .env files"Length of output: 293
Ensure INTERNAL_API_KEY is guarded and documented
The INTERNAL_API_KEY lookup currently assumes the variable is set. Our check shows it’s not defined in any .env* files, so we need to:
• In lib/files/get-file.ts, guard against a missing key (e.g. throw an error or use an empty string/fallback).
• In your environment files (e.g. .env.example), add INTERNAL_API_KEY with a placeholder and update any deployment docs.
Suggested code change:
- Authorization: `Bearer ${process.env.INTERNAL_API_KEY}`,
+ Authorization: (() => {
+ const key = process.env.INTERNAL_API_KEY;
+ if (!key) {
+ throw new Error('Missing INTERNAL_API_KEY environment variable');
+ }
+ return `Bearer ${key}`;
+ })(),And in .env.example (or your docs):
# API key for internal S3 presigned URL endpoint
INTERNAL_API_KEY=your-internal-api-key-here🤖 Prompt for AI Agents
In lib/files/get-file.ts at line 37, the code uses process.env.INTERNAL_API_KEY
without checking if it is set, which can cause runtime errors or insecure
behavior. Add a guard to verify that INTERNAL_API_KEY is defined before using
it; if it is missing, throw a clear error to prevent proceeding without
authentication. Additionally, update the .env.example file to include
INTERNAL_API_KEY with a placeholder value and ensure deployment documentation
mentions this variable so it is properly set in all environments.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pages/api/file/s3/get-presigned-get-url.ts (1)
13-41: Consider implementing rate limiting for this endpoint.This endpoint now has authentication, which is great. For additional security, consider implementing rate limiting to prevent potential abuse or brute force attempts against the API key.
You could utilize a package like
express-rate-limitor implement a simple in-memory rate limiting solution based on the requester's IP address.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
lib/files/get-file.ts(1 hunks)pages/api/file/s3/get-presigned-get-url.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/files/get-file.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
pages/api/file/s3/get-presigned-get-url.ts (1)
lib/utils.ts (1)
log(52-112)
🔇 Additional comments (2)
pages/api/file/s3/get-presigned-get-url.ts (2)
9-9: LGTM! Good addition of logging capability.The addition of the logging utility import is appropriate for improving error visibility in the API handler.
32-36: Good proactive logging for configuration issues.The logging of missing environment variables is valuable for diagnosing deployment issues. This helps provide visibility when the service is not configured correctly.
| log({ | ||
| message: `Error getting presigned get url for ${key} \n\n ${error}`, | ||
| type: "error", | ||
| }); | ||
| return res | ||
| .status(500) | ||
| .json({ error: "AWS Cloudfront Signed URL Error", message: error }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improved error handling and logging, but consider response format.
Adding error logging with context about the requested key is excellent for debugging. However, directly passing the error object in the response might expose sensitive information or fail to serialize properly.
log({
message: `Error getting presigned get url for ${key} \n\n ${error}`,
type: "error",
});
return res
.status(500)
- .json({ error: "AWS Cloudfront Signed URL Error", message: error });
+ .json({
+ error: "AWS Cloudfront Signed URL Error",
+ message: error instanceof Error ? error.message : String(error)
+ });This ensures that only the error message (not the full error object with potentially sensitive stack traces) is returned to the client.
📝 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.
| log({ | |
| message: `Error getting presigned get url for ${key} \n\n ${error}`, | |
| type: "error", | |
| }); | |
| return res | |
| .status(500) | |
| .json({ error: "AWS Cloudfront Signed URL Error", message: error }); | |
| log({ | |
| message: `Error getting presigned get url for ${key} \n\n ${error}`, | |
| type: "error", | |
| }); | |
| return res | |
| .status(500) | |
| .json({ | |
| error: "AWS Cloudfront Signed URL Error", | |
| message: error instanceof Error ? error.message : String(error) | |
| }); |
🤖 Prompt for AI Agents
In pages/api/file/s3/get-presigned-get-url.ts around lines 73 to 79, the error
response currently includes the full error object, which may expose sensitive
information or cause serialization issues. Modify the response to only include
the error message string instead of the entire error object by accessing
error.message or converting the error to a safe string. This ensures the client
receives a clear error message without sensitive details.
| // Extract the API Key from the Authorization header | ||
| const authHeader = req.headers.authorization; | ||
| if (!authHeader || !authHeader.startsWith("Bearer ")) { | ||
| return res | ||
| .status(401) | ||
| .json({ message: "Unauthorized: Invalid Authorization header" }); | ||
| } | ||
| const token = authHeader?.split(" ")[1]; // Assuming the format is "Bearer [token]" | ||
|
|
||
| // Check if the API Key matches | ||
| if (!process.env.INTERNAL_API_KEY) { | ||
| log({ | ||
| message: "INTERNAL_API_KEY environment variable is not set", | ||
| type: "error", | ||
| }); | ||
| return res.status(500).json({ message: "Server configuration error" }); | ||
| } | ||
| if (token !== process.env.INTERNAL_API_KEY) { | ||
| res.status(401).json({ message: "Unauthorized" }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement proper authentication for presigned URL requests.
The added authentication logic properly secures the endpoint by validating the bearer token against the INTERNAL_API_KEY environment variable, which is a good security practice.
However, there are a few minor issues to address:
- Line 28 uses optional chaining (
?.) onauthHeadereven though its existence is already verified on line 23 - No validation for empty token after Bearer prefix
- Inconsistent response format between different error scenarios
Consider this revised implementation:
// Extract the API Key from the Authorization header
const authHeader = req.headers.authorization;
if (!authHeader || !authHeader.startsWith("Bearer ")) {
return res
.status(401)
.json({ message: "Unauthorized: Invalid Authorization header" });
}
- const token = authHeader?.split(" ")[1]; // Assuming the format is "Bearer [token]"
+ const token = authHeader.split(" ")[1]; // Assuming the format is "Bearer [token]"
+
+ if (!token) {
+ return res.status(401).json({ message: "Unauthorized: Missing token" });
+ }
// Check if the API Key matches
if (!process.env.INTERNAL_API_KEY) {
log({
message: "INTERNAL_API_KEY environment variable is not set",
type: "error",
});
return res.status(500).json({ message: "Server configuration error" });
}
if (token !== process.env.INTERNAL_API_KEY) {
- res.status(401).json({ message: "Unauthorized" });
- return;
+ return res.status(401).json({ message: "Unauthorized: Invalid token" });
}📝 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.
| // Extract the API Key from the Authorization header | |
| const authHeader = req.headers.authorization; | |
| if (!authHeader || !authHeader.startsWith("Bearer ")) { | |
| return res | |
| .status(401) | |
| .json({ message: "Unauthorized: Invalid Authorization header" }); | |
| } | |
| const token = authHeader?.split(" ")[1]; // Assuming the format is "Bearer [token]" | |
| // Check if the API Key matches | |
| if (!process.env.INTERNAL_API_KEY) { | |
| log({ | |
| message: "INTERNAL_API_KEY environment variable is not set", | |
| type: "error", | |
| }); | |
| return res.status(500).json({ message: "Server configuration error" }); | |
| } | |
| if (token !== process.env.INTERNAL_API_KEY) { | |
| res.status(401).json({ message: "Unauthorized" }); | |
| return; | |
| } | |
| // Extract the API Key from the Authorization header | |
| const authHeader = req.headers.authorization; | |
| if (!authHeader || !authHeader.startsWith("Bearer ")) { | |
| return res | |
| .status(401) | |
| .json({ message: "Unauthorized: Invalid Authorization header" }); | |
| } | |
| const token = authHeader.split(" ")[1]; // Assuming the format is "Bearer [token]" | |
| if (!token) { | |
| return res.status(401).json({ message: "Unauthorized: Missing token" }); | |
| } | |
| // Check if the API Key matches | |
| if (!process.env.INTERNAL_API_KEY) { | |
| log({ | |
| message: "INTERNAL_API_KEY environment variable is not set", | |
| type: "error", | |
| }); | |
| return res.status(500).json({ message: "Server configuration error" }); | |
| } | |
| if (token !== process.env.INTERNAL_API_KEY) { | |
| return res.status(401).json({ message: "Unauthorized: Invalid token" }); | |
| } |
🤖 Prompt for AI Agents
In pages/api/file/s3/get-presigned-get-url.ts around lines 21 to 41, remove the
unnecessary optional chaining on authHeader when extracting the token since its
presence is already checked. Add a validation step to ensure the token extracted
after "Bearer " is not empty or missing. Standardize the error response format
across all authentication failure cases to maintain consistency, returning JSON
with a message field for all errors.
Summary by CodeRabbit
New Features
Bug Fixes