Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Good job setting up the basic HTTP server. However, some changes are needed to meet the requirements, particularly regarding security and routing logic.
Here are the critical issues to address:
- Security Vulnerability: Your code is vulnerable to path traversal. A malicious request like
/file/../../package.jsoncan access files outside thepublicdirectory. You must add a check to ensure the final resolved path stays within thepublicfolder. - Incorrect Routing:
- A request to
/fileshould servepublic/index.html, but it currently returns a hint message. - Any path that does not start with
/file/should receive a hint message, but your server incorrectly returns a400 Bad Request.
- A request to
Please address these points to ensure your server is both secure and fully functional according to the requirements.
✨ 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 isFileRequest = pathname.startsWith('/file/'); | ||
|
|
||
| // 2. ВИЗНАЧАЄМО, ЧИ ЦЕ ЗАПИТ НА ПІДКАЗКУ (/file або /) | ||
| const isHintRequest = pathname === '/file' || pathname === '/'; |
There was a problem hiding this comment.
The requirements state that a request to /file should serve public/index.html. This logic incorrectly categorizes it as a hint request, causing it to return a hint message instead.
src/createServer.js
Outdated
| const isHintRequest = pathname === '/file' || pathname === '/'; | ||
|
|
||
| // 3. ЯКЩО ЦЕ НІ ТЕ, НІ ІНШЕ (наприклад, /app.js), ТО ЦЕ 400 | ||
| if (!isFileRequest && !isHintRequest) { |
There was a problem hiding this comment.
According to the requirements, any path that does not start with /file/ should receive a hint message. This conditional block incorrectly returns a 400 Bad Request for such paths instead.
src/createServer.js
Outdated
| // 6. РОБОТА З ФАЙЛАМИ | ||
| const relativePath = pathAfterPrefix || 'index.html'; | ||
| const publicPath = path.resolve('public'); | ||
| const filePath = path.join(publicPath, relativePath); |
There was a problem hiding this comment.
This implementation is vulnerable to path traversal attacks. A malicious path like ../../somefile could be used to access files outside the public directory, which violates a core requirement. You should add a check to ensure that the final resolved filePath is still within the public directory before attempting to read the file.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on securing the server against path traversal and fixing the routing for /file requests! The implementation is much more robust now.
I am requesting one change because the server doesn't yet meet all requirements for handling non-file paths. According to the task, any path that does not start with /file/ should return a specific hint message. Your code currently does this for the root path (/) but returns a 400 Bad Request for other paths like /some-other-path.
Please adjust the logic in src/createServer.js to ensure the hint message is returned for all non-file paths as required. You are very close to completing the task!
✨ 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
| res.statusCode = 400; | ||
| res.setHeader('Content-Type', 'text/plain'); | ||
|
|
||
| return res.end('Bad Request'); |
There was a problem hiding this comment.
According to the requirements, any path that does not start with /file/ should return a hint message. This block correctly handles /, but for other paths (like /some-other-path), it returns a 400 Bad Request instead of the required hint.
The logic in the parent if block (lines 20-37) should be simplified to always send the hint message for any request that isn't a file request.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override to help you progress, as you've made great strides in this submission! Your logic for handling file paths and preventing path traversal is much improved. To make it perfect, please consider a few points for next time. First, the route /file should serve index.html, but the current check pathname.startsWith('/file/') doesn't account for it. Second, the special handling for /app.js contradicts the requirement that all non-file paths should receive the same hint message. Finally, the explicit check for // in the path is unnecessary and can block valid requests, as path.join handles path normalization automatically. You're doing great, keep up the fantastic effort!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| // Return instance of http.Server class | ||
| return http.createServer(async (req, res) => { | ||
| const { pathname } = new URL(req.url, `http://${req.headers.host}`); | ||
| const isFileRoute = pathname.startsWith('/file/'); |
There was a problem hiding this comment.
This check doesn't account for the requirement that a request to /file should also be treated as a file route and serve index.html. Currently, a request to /file will be handled as a non-file route because '/file'.startsWith('/file/') is false.
| if (pathname === '/app.js') { | ||
| res.statusCode = 400; | ||
| res.setHeader('Content-Type', 'text/plain'); | ||
|
|
||
| return res.end('Bad Request'); | ||
| } |
There was a problem hiding this comment.
This special case for /app.js contradicts the task requirement. According to the description, any path that does not start with /file/ should return the hint message. This block should be removed to ensure all non-file paths are handled uniformly as required.
| if (pathAfterPrefix.includes('//')) { | ||
| res.statusCode = 404; | ||
| res.setHeader('Content-Type', 'text/plain'); | ||
|
|
||
| return res.end('File not found'); |
There was a problem hiding this comment.
This check for // might cause incorrect behavior. The requirement is to return 404 for non-existent files. A path like /file/styles//main.css can resolve to a valid, existing file (public/styles/main.css) because path.join normalizes it. By returning 404 here, you might be incorrectly rejecting a valid request.
2 participants
No description provided.