Shiwoslj patch 1 #5905
Shiwoslj patch 1 #5905
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@shiwoslj is attempting to deploy a commit to the NextChat Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes made to the WebDAV handler in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WebDAVHandler
participant MakeRequest
participant Logger
Client->>WebDAVHandler: Send Request
WebDAVHandler->>Logger: Log Request Details
WebDAVHandler->>MakeRequest: Process Request
MakeRequest-->>WebDAVHandler: Return Response
WebDAVHandler->>Logger: Log Response Details
WebDAVHandler-->>Client: Send Response
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 8
🧹 Outside diff range and nitpick comments (2)
app/api/webdav/[...path]/route.ts (2)
6-14: Expand 'ALLOWED_HEADERS' to include necessary headers for full WebDAV functionalityThe current
ALLOWED_HEADERSmay exclude headers required for certain WebDAV operations, potentially causing issues with client interactions.Consider adding additional headers to
ALLOWED_HEADERS:const ALLOWED_HEADERS = [ "authorization", "content-type", "accept", "depth", "destination", "overwrite", "content-length", + "range", + "if-match", + "if-none-match", + "if-modified-since", + "if-unmodified-since", + "content-disposition", + "user-agent", + "cookie", ];
4-195: Maintain consistency by translating comments to EnglishThe code comments are currently in Chinese, which may hinder understanding for non-Chinese-speaking team members and contributors. Adopting English for comments promotes inclusivity and uniformity across the codebase.
Consider translating the comments to English for better collaboration.
| { error: error.message || "Internal Server Error" }, | ||
| { status, headers: CORS_HEADERS } | ||
| ); |
There was a problem hiding this comment.
Prevent exposure of internal error messages in responses
Returning internal error messages to clients can expose sensitive information. It's safer to return a generic error message and log the detailed error server-side.
Apply this diff to use a generic error message:
return NextResponse.json(
- { error: error.message || "Internal Server Error" },
+ { error: "Internal Server Error" },
{ status, headers: CORS_HEADERS }
);Ensure that detailed errors are logged internally:
console.error("[Proxy Error]", error);
+// Detailed error logged above📝 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.
| { error: error.message || "Internal Server Error" }, | |
| { status, headers: CORS_HEADERS } | |
| ); | |
| { error: "Internal Server Error" }, | |
| { status, headers: CORS_HEADERS } | |
| ); |
| headers.set('transfer-encoding', 'chunked'); | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid manually setting 'transfer-encoding' header
Manually setting the transfer-encoding header can lead to unexpected behavior, as the underlying fetch implementation manages this automatically when streaming. It's better to let the system handle encoding.
Apply this diff to remove the manual header setting:
requestBody = req.body;
- headers.set('transfer-encoding', 'chunked');Committable suggestion skipped: line range outside the PR's diff.
| console.log(`[Proxy Request] ${method} ${req.url}`); | ||
| console.log("Request Headers:", Object.fromEntries(req.headers)); |
There was a problem hiding this comment.
Avoid logging full request headers to prevent sensitive data exposure
Logging complete request headers may inadvertently expose sensitive information such as authentication tokens or personal data. It's important to sanitize or omit sensitive headers to enhance security.
Apply this diff to sanitize or remove sensitive headers from logs:
// Log request method and URL
console.log(`[Proxy Request] ${method} ${req.url}`);
- console.log("Request Headers:", Object.fromEntries(req.headers));
+ // Optionally, log non-sensitive headers if necessary
+ // const loggedHeaders = { ... };
+ // console.log("Request Headers:", loggedHeaders);Committable suggestion skipped: line range outside the PR's diff.
app/api/webdav/[...path]/route.ts
Outdated
|
|
||
| // 构建目标 URL | ||
| const targetUrlObj = new URL(ENDPOINT); | ||
| targetUrlObj.pathname = path.join(targetUrlObj.pathname, ...(params.path || [])); |
There was a problem hiding this comment.
Use URL methods instead of 'path.join' for URL paths
The path.join function is intended for filesystem paths and may not handle URL paths correctly, especially on different operating systems. It's safer to manipulate URL paths using URL methods or string operations.
Apply this diff to correctly construct the target URL path:
// Build the target URL
const targetUrlObj = new URL(ENDPOINT);
- targetUrlObj.pathname = path.join(targetUrlObj.pathname, ...(params.path || []));
+ targetUrlObj.pathname = `${targetUrlObj.pathname}${params.path ? '/' + params.path.join('/') : ''}`;
const targetUrl = targetUrlObj.toString();Alternatively, you can use the URL constructor for more robust handling:
const targetUrl = new URL((params.path || []).join('/'), ENDPOINT).toString();📝 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.
| targetUrlObj.pathname = path.join(targetUrlObj.pathname, ...(params.path || [])); | |
| targetUrlObj.pathname = `${targetUrlObj.pathname}${params.path ? '/' + params.path.join('/') : ''}`; |
| { error: "Internal Server Error", details: error.message }, | ||
| { status: 500, headers: CORS_HEADERS } |
There was a problem hiding this comment.
Prevent exposure of internal error details in responses
Including error.message in the API response may leak internal details. Return a generic message instead.
Apply this diff to remove internal error details from the response:
return NextResponse.json(
- { error: "Internal Server Error", details: error.message },
+ { error: "Internal Server Error" },
{ status: 500, headers: CORS_HEADERS }
);Ensure that the error is logged for debugging purposes:
console.error("[Global Error]", error);
+// Detailed error logged aboveCommittable suggestion skipped: line range outside the PR's diff.
app/api/webdav/[...path]/route.ts
Outdated
| async function makeRequest(url: string, options: RequestInit, retries = 3) { | ||
| const controller = new AbortController(); | ||
| const timeoutId = setTimeout(() => controller.abort(), TIMEOUT); | ||
|
|
||
| for (let i = 0; i < retries; i++) { | ||
| try { | ||
| const response = await fetch(url, { | ||
| ...options, | ||
| signal: controller.signal | ||
| }); | ||
| clearTimeout(timeoutId); | ||
| return response; | ||
| } catch (error) { | ||
| console.error(`Attempt ${i + 1} failed:`, error); | ||
| if (error.name === 'AbortError') { | ||
| throw new Error('Request Timeout'); | ||
| } | ||
| if (i === retries - 1) throw error; | ||
| await new Promise(resolve => setTimeout(resolve, 1000 * (i + 1))); | ||
| } | ||
| } | ||
| }; | ||
| throw new Error('Max retries reached'); | ||
| } |
There was a problem hiding this comment.
Fix the retry logic in makeRequest to re-instantiate AbortController and timeout for each retry
Currently, the AbortController and timeout are initialized outside the retry loop. If a timeout occurs, the controller's signal remains aborted, causing immediate failure on subsequent retries. To ensure each retry operates correctly, instantiate a new AbortController and set a new timeout within each iteration of the loop.
Apply this diff to correct the retry logic:
-async function makeRequest(url: string, options: RequestInit, retries = 3) {
- const controller = new AbortController();
- const timeoutId = setTimeout(() => controller.abort(), TIMEOUT);
for (let i = 0; i < retries; i++) {
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), TIMEOUT);
try {
const response = await fetch(url, {
...options,
signal: controller.signal
});
clearTimeout(timeoutId);
return response;
} catch (error) {
console.error(`Attempt ${i + 1} failed:`, error);
if (error.name === 'AbortError') {
throw new Error('Request Timeout');
}
if (i === retries - 1) throw error;
await new Promise(resolve => setTimeout(resolve, 1000 * (i + 1)));
+ } finally {
+ clearTimeout(timeoutId);
}
}
throw new Error('Max retries reached');
}📝 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.
| async function makeRequest(url: string, options: RequestInit, retries = 3) { | |
| const controller = new AbortController(); | |
| const timeoutId = setTimeout(() => controller.abort(), TIMEOUT); | |
| for (let i = 0; i < retries; i++) { | |
| try { | |
| const response = await fetch(url, { | |
| ...options, | |
| signal: controller.signal | |
| }); | |
| clearTimeout(timeoutId); | |
| return response; | |
| } catch (error) { | |
| console.error(`Attempt ${i + 1} failed:`, error); | |
| if (error.name === 'AbortError') { | |
| throw new Error('Request Timeout'); | |
| } | |
| if (i === retries - 1) throw error; | |
| await new Promise(resolve => setTimeout(resolve, 1000 * (i + 1))); | |
| } | |
| } | |
| }; | |
| throw new Error('Max retries reached'); | |
| } | |
| async function makeRequest(url: string, options: RequestInit, retries = 3) { | |
| for (let i = 0; i < retries; i++) { | |
| const controller = new AbortController(); | |
| const timeoutId = setTimeout(() => controller.abort(), TIMEOUT); | |
| try { | |
| const response = await fetch(url, { | |
| ...options, | |
| signal: controller.signal | |
| }); | |
| clearTimeout(timeoutId); | |
| return response; | |
| } catch (error) { | |
| console.error(`Attempt ${i + 1} failed:`, error); | |
| if (error.name === 'AbortError') { | |
| throw new Error('Request Timeout'); | |
| } | |
| if (i === retries - 1) throw error; | |
| await new Promise(resolve => setTimeout(resolve, 1000 * (i + 1))); | |
| } finally { | |
| clearTimeout(timeoutId); | |
| } | |
| } | |
| throw new Error('Max retries reached'); | |
| } |
| response = await makeRequest(targetUrl, fetchOptions); | ||
| } catch (error) { | ||
| console.error("[Proxy Error]", error); | ||
| const status = error.message === 'Request Timeout' ? 504 : 500; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use error types instead of messages for error handling logic
Relying on error messages for control flow is fragile and can lead to bugs if messages change. Using custom error classes or properties provides a more reliable way to handle specific errors.
Consider defining a custom error class for timeouts and updating the error handling:
+// Define a custom error class
+class RequestTimeoutError extends Error {
+ constructor(message: string) {
+ super(message);
+ this.name = 'RequestTimeoutError';
+ }
+}
// In makeRequest function
if (error.name === 'AbortError') {
- throw new Error('Request Timeout');
+ throw new RequestTimeoutError('Request Timeout');
}
// In the handler function
try {
// ...
} catch (error) {
console.error("[Proxy Error]", error);
- const status = error.message === 'Request Timeout' ? 504 : 500;
+ const status = error instanceof RequestTimeoutError ? 504 : 500;
return NextResponse.json(
{ error: "Internal Server Error" },
{ status, headers: CORS_HEADERS }
);
}Committable suggestion skipped: line range outside the PR's diff.
| console.log(`[Proxy Response] ${response.status} ${response.statusText}`); | ||
| console.log("Response Headers:", Object.fromEntries(responseHeaders)); |
There was a problem hiding this comment.
Avoid logging full response headers to prevent sensitive data exposure
Logging all response headers could expose sensitive information. It's advisable to sanitize or avoid logging headers that may contain confidential data.
Apply this diff to sanitize or remove sensitive headers from logs:
// Log response status
console.log(`[Proxy Response] ${response.status} ${response.statusText}`);
- console.log("Response Headers:", Object.fromEntries(responseHeaders));
+ // Optionally, log non-sensitive headers if necessary
+ // const loggedHeaders = { ... };
+ // console.log("Response Headers:", loggedHeaders);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
app/api/webdav/[...path]/route.ts (2)
36-58: Consider implementing circuit breaker patternWhile the retry mechanism is good, it could benefit from a circuit breaker to prevent overwhelming failing systems.
Consider implementing a circuit breaker:
class CircuitBreaker { private failures = 0; private lastFailure: number = 0; private readonly threshold = 5; private readonly resetTimeout = 60000; // 1 minute async execute<T>(fn: () => Promise<T>): Promise<T> { if (this.isOpen()) { throw new Error('Circuit breaker is open'); } try { const result = await fn(); this.reset(); return result; } catch (error) { this.recordFailure(); throw error; } } private isOpen(): boolean { if (this.failures >= this.threshold) { const shouldReset = Date.now() - this.lastFailure >= this.resetTimeout; if (shouldReset) { this.reset(); return false; } return true; } return false; } private recordFailure(): void { this.failures++; this.lastFailure = Date.now(); } private reset(): void { this.failures = 0; this.lastFailure = 0; } }
181-187: Enhance type safety for HTTP methodsConsider using TypeScript enums or constants to ensure type safety for HTTP methods.
Add type definitions:
enum WebDAVMethod { GET = 'GET', POST = 'POST', PUT = 'PUT', DELETE = 'DELETE', OPTIONS = 'OPTIONS', PROPFIND = 'PROPFIND', MKCOL = 'MKCOL' } // Then use it in your ALLOWED_METHODS const ALLOWED_METHODS = Object.values(WebDAVMethod); // Type your handler async function handler( req: NextRequest & { method: WebDAVMethod }, { params }: { params: { path: string[] } } )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/api/webdav/[...path]/route.ts(1 hunks)
🔇 Additional comments (1)
app/api/webdav/[...path]/route.ts (1)
22-23: Validate WebDAV endpoint configuration
The fallback to localhost could lead to unexpected behavior in production.
Let's verify the environment configuration:
| function joinPaths(...parts: string[]): string { | ||
| return parts | ||
| .map(part => part.replace(/^\/+|\/+$/g, '')) | ||
| .filter(Boolean) | ||
| .join('/'); | ||
| } |
There was a problem hiding this comment.
Add path traversal protection to joinPaths function
The current implementation doesn't prevent directory traversal attacks using ../ sequences.
Add path normalization:
function joinPaths(...parts: string[]): string {
+ // Normalize and validate path segments
+ const normalizedParts = parts.map(part => {
+ const cleaned = part.replace(/^\/+|\/+$/g, '');
+ if (cleaned.includes('../') || cleaned === '..') {
+ throw new Error('Path traversal not allowed');
+ }
+ return cleaned;
+ });
+
return parts
- .map(part => part.replace(/^\/+|\/+$/g, ''))
.filter(Boolean)
.join('/');
}Committable suggestion skipped: line range outside the PR's diff.
| const CORS_HEADERS = { | ||
| "Access-Control-Allow-Origin": "*", | ||
| "Access-Control-Allow-Methods": ALLOWED_METHODS.join(","), | ||
| "Access-Control-Allow-Headers": ALLOWED_HEADERS.join(", "), | ||
| "Access-Control-Max-Age": "86400", |
There was a problem hiding this comment.
Restrict CORS configuration for production environments
The current CORS configuration allows requests from any origin ("*"), which could expose your WebDAV endpoint to unauthorized access.
Consider implementing environment-specific CORS:
const CORS_HEADERS = {
- "Access-Control-Allow-Origin": "*",
+ "Access-Control-Allow-Origin": process.env.ALLOWED_ORIGINS || "*",
"Access-Control-Allow-Methods": ALLOWED_METHODS.join(","),
"Access-Control-Allow-Headers": ALLOWED_HEADERS.join(", "),
"Access-Control-Max-Age": "86400",
};Committable suggestion skipped: line range outside the PR's diff.
| let requestBody: BodyInit | null = null; | ||
| if (["POST", "PUT"].includes(method)) { | ||
| try { | ||
| const contentLength = req.headers.get('content-length'); | ||
| if (contentLength && parseInt(contentLength) > 10 * 1024 * 1024) { | ||
| requestBody = req.body; | ||
| headers.set('transfer-encoding', 'chunked'); | ||
| } else { | ||
| requestBody = await req.blob(); | ||
| } | ||
| } catch (error) { | ||
| console.error("[Request Body Error]", error); | ||
| return NextResponse.json( | ||
| { error: "Invalid Request Body" }, | ||
| { status: 400, headers: CORS_HEADERS } | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add content type validation for uploads
The current implementation doesn't validate content types for uploaded files.
Add content type validation:
let requestBody: BodyInit | null = null;
if (["POST", "PUT"].includes(method)) {
try {
+ const contentType = req.headers.get('content-type');
+ if (contentType) {
+ // Add your allowed content types
+ const allowedTypes = ['application/octet-stream', 'text/plain'];
+ if (!allowedTypes.some(type => contentType.includes(type))) {
+ return NextResponse.json(
+ { error: "Unsupported content type" },
+ { status: 415, headers: CORS_HEADERS }
+ );
+ }
+ }
const contentLength = req.headers.get('content-length');Committable suggestion skipped: line range outside the PR's diff.
| const contentLength = req.headers.get('content-length'); | ||
| if (contentLength && parseInt(contentLength) > 10 * 1024 * 1024) { | ||
| requestBody = req.body; | ||
| headers.set('transfer-encoding', 'chunked'); | ||
| } else { | ||
| requestBody = await req.blob(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve large file handling configuration
The large file threshold is hardcoded and the handling logic could be improved.
Consider these improvements:
+const MAX_MEMORY_FILE_SIZE = 10 * 1024 * 1024; // 10MB
+
const contentLength = req.headers.get('content-length');
-if (contentLength && parseInt(contentLength) > 10 * 1024 * 1024) {
+const size = contentLength ? parseInt(contentLength) : 0;
+if (size > MAX_MEMORY_FILE_SIZE) {
requestBody = req.body;
- headers.set('transfer-encoding', 'chunked');
+ // Let the underlying fetch implementation handle transfer encoding
} else {
- requestBody = await req.blob();
+ // For smaller files, use ArrayBuffer for better memory efficiency
+ requestBody = await req.arrayBuffer();
}📝 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 contentLength = req.headers.get('content-length'); | |
| if (contentLength && parseInt(contentLength) > 10 * 1024 * 1024) { | |
| requestBody = req.body; | |
| headers.set('transfer-encoding', 'chunked'); | |
| } else { | |
| requestBody = await req.blob(); | |
| } | |
| const MAX_MEMORY_FILE_SIZE = 10 * 1024 * 1024; // 10MB | |
| const contentLength = req.headers.get('content-length'); | |
| const size = contentLength ? parseInt(contentLength) : 0; | |
| if (size > MAX_MEMORY_FILE_SIZE) { | |
| requestBody = req.body; | |
| // Let the underlying fetch implementation handle transfer encoding | |
| } else { | |
| // For smaller files, use ArrayBuffer for better memory efficiency | |
| requestBody = await req.arrayBuffer(); | |
| } |
💻 变更类型 | Change Type
🔀 变更说明 | Description of Change
📝 补充信息 | Additional Information
Summary by CodeRabbit
New Features
Bug Fixes
Documentation