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 setting up the static file server! The core logic for serving files from the public directory and handling 404 errors is correctly implemented.
However, changes are needed for two critical issues:
- Failing Security Test: The tests are failing because the server doesn't prevent path traversal. A request like
/file/../somefile.jsshould be blocked with a400 Bad Requeststatus. You'll need to validate that the resolved file path stays within thepublicdirectory. - Incorrect Root File Handling: According to the requirements, requests to
/fileand/file/should serve thepublic/index.htmlfile, but they currently return a text hint. Please adjust your logic to handle these specific cases.
Once these two issues are addressed, your solution will be complete. Keep up the great effort!
✨ 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
| if (pathname === '/file' || pathname === '/file/') { | ||
| res.statusCode = 200; | ||
| res.setHeader('Content-Type', 'text/plain'); | ||
| res.end('To upload a file, use the path /file/<file_name>'); | ||
|
|
||
| return; |
There was a problem hiding this comment.
According to the requirements, requests to /file and /file/ should return the public/index.html file. This implementation returns a hint message instead.
|
учёл замечание: |
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job addressing the previous feedback and correctly handling the root paths like /file and /file/. We're very close to a complete solution!
However, there are a couple of critical issues that need to be fixed.
- Path Traversal: The security check for path traversal is not working correctly, which is causing the tests to fail. A request like
/file/../somefile.jsmust be blocked with a400 Bad Requeststatus. You can fix this by adding a check to see if the requested path contains..before trying to serve the file. - Incorrect Content-Type: When serving
index.htmlfor the root paths, theContent-Typeis hardcoded astext/plain. This causes the browser to show the HTML code instead of rendering the page. Please use yourgetContentTypehelper function to set the correct type, which should betext/html.
Once these two issues are resolved, your server should meet all the requirements. Keep up the great work!
✨ 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
| ); | ||
| const newPath = path.join(pathTopublicDir, 'index.html'); | ||
|
|
||
| readFile(newPath, res, 'text/plain'); |
There was a problem hiding this comment.
The content type for index.html is hardcoded to text/plain. This will cause the browser to display the HTML source code instead of rendering the page. You should use the correct MIME type, which you can get by using your getContentType helper function or by specifying 'text/html' directly.
|
Чтобы проверить работы теста, проверяющего наличие '..' в адресной строке, прописываю в адресе путь /file/../app.js и браузер её сразу же нормализует сам и в консоль выводится путь без '..'. Возможно из-за этого тест и не отлавливает эту ошибку. В тесте, где проверяется возврат index.html при /file и /file/, тест показывает: |
3 participants
No description provided.