Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your server implementation is well on its way, and the security checks to prevent path traversal are excellent. However, changes are needed because a key requirement is not currently met.
The main issue is that a request to /file incorrectly returns a hint message instead of serving index.html. According to the requirements, /file should be treated the same as /file/. Please adjust the condition that checks the request path to allow /file to be handled by the file-serving logic.
Additionally, when handling this kind of invalid path, the server responds with a 200 OK status. This should be a client error status, such as 400 Bad Request, to accurately reflect that the request was malformed. Once these corrections are made, your solution will be ready!
✨ 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
| return; | ||
| } | ||
|
|
||
| if (!url.startsWith('/file/')) { |
There was a problem hiding this comment.
This condition incorrectly blocks requests to /file. According to the requirements, /file should be treated the same as /file/ and serve index.html. This check should be adjusted to allow the /file path to be handled by the file-serving logic below.
| } | ||
|
|
||
| if (!url.startsWith('/file/')) { | ||
| res.statusCode = 200; |
There was a problem hiding this comment.
A 200 OK status code is not appropriate here, as you're handling a request for an invalid path. This should be a client error status code, like 400 Bad Request, to be consistent with your other error handling (e.g., on line 14).
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because a core requirement from the previous review has not been addressed. While you've made good progress on handling most file requests, the server still does not correctly handle requests for the /file endpoint.
According to the requirements, a request to /file must serve public/index.html, but your current implementation returns a hint message instead. This is because the condition checking the URL path is too strict. Additionally, the hint message for this case is being sent with a 200 OK status, but it should be a client error code like 400 Bad Request.
Please adjust the logic to ensure that requests to /file are handled the same way as /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
| if (!url.startsWith('/file/')) { | ||
| res.writeHead(200, { 'Content-Type': 'text/plain' }); | ||
|
|
||
| return res.end('Use /file/<filename> to load files'); | ||
| } |
There was a problem hiding this comment.
This condition incorrectly handles requests for /file. According to the requirements, /file must serve public/index.html, but this logic returns a hint message. This issue was pointed out in the previous review.
Additionally, the response uses a 200 OK status code, which is not appropriate for a hint about incorrect usage. A client error code like 400 Bad Request should be used here.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work addressing the previous feedback! The server is now correctly handling requests for /file and /file/ and using the correct status codes.
I am approving this submission as an educational override to help you progress, but there's one important detail to correct. When serving index.html, the Content-Type header is currently set to text/plain. This causes the browser to display the HTML source code instead of rendering the webpage. To ensure the page displays correctly, this header should be changed to text/html.
Great progress, keep it up!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
|
|
||
| if (pathname === publicPathPrefix || pathname === `${publicPathPrefix}/`) { | ||
| res.statusCode = 200; | ||
| res.setHeader('Content-Type', 'text/plain'); |
There was a problem hiding this comment.
When serving index.html, the Content-Type header should be text/html. Setting it to text/plain will cause browsers to display the HTML source code as plain text instead of rendering it as a webpage.
3 participants
No description provided.