Implement secure static files server with traversal protection#340
Implement secure static files server with traversal protection#340S-Khraban wants to merge 3 commits intomate-academy:masterfrom
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on the server implementation! You've successfully covered most of the requirements, including the important security checks to prevent directory traversal. However, this requires changes because one of the core requirements is not met. Your server correctly handles paths that start with /file/, but it fails to serve public/index.html for a request to the exact /file path, returning the hint message instead. Please review the condition in src/createServer.js that checks the request path; it needs to be adjusted to handle both /file and paths starting with /file/.
✨ 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
src/createServer.js
Outdated
| const rawUrl = req.url || ''; | ||
| const rawPath = rawUrl.split('?')[0]; | ||
|
|
||
| if (!rawPath.startsWith('/file/')) { |
There was a problem hiding this comment.
This condition correctly handles paths that start with /file/, but it doesn't account for the case where the path is exactly /file. According to the requirements, a request to /file should also serve the public/index.html file.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great effort on the updates! Your code correctly handles most requirements, including the security checks to prevent directory traversal. However, this requires changes because a core requirement is still not implemented correctly. A request to the exact path /file must serve the public/index.html file, but your current implementation returns a hint message instead. Please adjust the logic for the /file case so that it serves index.html, similar to how you handle a request to /file/. Once that's fixed, you'll be all set!
✨ 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
src/createServer.js
Outdated
| if (rawPath === '/file') { | ||
| send( | ||
| res, | ||
| 200, | ||
| 'Use /file/<path> to load static files from public folder', | ||
| ); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
According to the requirements, a request to /file should serve the public/index.html file. This block currently sends a hint message instead. The logic should be changed so that a request to /file is handled similarly to a request to /file/, which correctly serves the index.html file.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job getting the server to handle the /file route correctly! I am approving this solution as an educational override to help you move forward, but there's one important detail to address. The forcePlain = true logic you added causes index.html to be served with a Content-Type of text/plain, which makes the browser display the raw code instead of the webpage. To fix this, please remove that logic so the server can correctly determine the Content-Type as text/html from the file extension. You're doing great work, keep it up!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| if (rawPath === '/file') { | ||
| forcePlain = true; |
There was a problem hiding this comment.
This if statement correctly routes requests for /file to the file-serving logic, which is a great improvement. However, setting forcePlain = true here causes an issue later on (line 94), forcing the Content-Type to text/plain. This means index.html is sent as plain text instead of a renderable HTML page.
Please remove the forcePlain logic to ensure index.html is always served with its correct text/html MIME type.
2 participants
No description provided.