Solution#374
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation has critical gaps that must be addressed before approval. First, the Content-Type header is always set to text/plain, but the requirements explicitly state that .html files should return text/html and .css files should return text/css. Use path.extname() to determine the correct MIME type based on file extension. Second, there's no explicit check for the ../ substring in the pathname - the requirements specifically require returning a 400 status when ../ is detected in the pathname itself. Add a simple check like if (pathname.includes('../')) that returns a 400 response before any path processing occurs. Additionally, when the pathname is /file/ (with trailing slash), ensure it falls back to serving index.html from the public folder rather than attempting to read the directory itself. These corrections are essential to meet the task specifications.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| res.statusCode = 200; | ||
| res.setHeader('Content-Type', 'text/plain'); |
There was a problem hiding this comment.
The Content-Type is always set to text/plain. According to requirements, it should be text/html for .html files and text/css for .css files. Consider determining the content type based on the file extension.
|
|
||
| const relativePath = fileName.replace('/file/', ''); | ||
|
|
||
| const publicDir = path.resolve('./public'); | ||
| const resolvedPath = path.resolve(`./public/${relativePath}`); | ||
|
|
||
| if ( | ||
| !resolvedPath.startsWith(publicDir + path.sep) && | ||
| resolvedPath !== publicDir | ||
| ) { | ||
| res.statusCode = 400; | ||
| res.setHeader('Content-Type', 'text/plain'); | ||
| res.end("You can't go outside /file folder"); | ||
|
|
||
| return; |
There was a problem hiding this comment.
The requirements specify that if pathname contains ../ it should return 400 status. Currently there's no explicit check for ../ substring. The path traversal check handles security, but the explicit requirement is to detect ../ in the pathname itself.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation is close to complete but has one critical issue remaining. When the pathname is /file/ (with a trailing slash), the relativePath variable becomes an empty string, causing the server to attempt reading ./public/ which fails and returns a 404. According to the requirements, this should serve public/index.html instead. Add a fallback check before the fs.readFile call, such as: const fileToRead = relativePath || 'index.html'; and use this variable for the file path. All other requirements pass, including Content-Type handling and ../ security checks, so you're very close to a working solution.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
|
|
||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
When pathname is /file/ (with trailing slash), relativePath becomes empty string. The code then tries to read ./public/ which fails. According to requirements, it should serve public/index.html instead. Add a check like: if (!relativePath) { relativePath = 'index.html'; } before the fs.readFile call.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This is an educational approval to help you progress while learning. You've done an excellent job implementing the path traversal and double slash security checks, as well as the file serving logic for /file/ requests. There's one remaining bug to fix: when serving /file/, the Content-Type defaults to text/plain because path.extname('') returns an empty string. The suggested fix is to add a fallback that checks if fileToRead === 'index.html' and returns 'text/html', or use path.extname(fileToRead) to derive the extension from the resolved filename instead of the empty relativePath. Once you address this small issue, your implementation will be complete!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
|
|
||
| const ext = path.extname(relativePath); | ||
| const contentTypes = { | ||
| '.html': 'text/html', | ||
| '.css': 'text/css', | ||
| }; |
There was a problem hiding this comment.
When relativePath is empty (for /file/ requests), path.extname('') returns an empty string, causing contentTypes[''] to be undefined and defaulting to text/plain. This should be text/html since we're serving index.html. Add a fallback: const contentType = contentTypes[ext] || (fileToRead === 'index.html' ? 'text/html' : 'text/plain');
No description provided.