The Solution#373
Conversation
There was a problem hiding this comment.
Pull request overview
Implements the static files server behavior for serving files from public/ via /file/..., adds shared response helpers, and updates tooling/CI configuration.
Changes:
- Added response helpers for 404/400 and hint responses.
- Added a file streaming handler with basic MIME type selection.
- Implemented
createServer()routing logic and updated@mate-academy/scriptsversion; added a GitHub Actions workflow template.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/helpers/responses.js | Introduces helpers for consistent plain-text error/hint responses. |
| src/handlers/fileHandler.js | Adds a streaming file responder with basic MIME mapping. |
| src/createServer.js | Implements request routing, decoding, and validation for /file/... paths. |
| package.json | Bumps @mate-academy/scripts dev dependency. |
| package-lock.json | Locks updated @mate-academy/scripts dependency metadata. |
| .github/workflows/test.yml-template | Adds a CI workflow template for running tests on PRs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (decodedUrl.includes('..') || !decodedUrl.startsWith('/file')) { | ||
| return badRequest(res); | ||
| } | ||
|
|
| if (decodedUrl.includes('..') || !decodedUrl.startsWith('/file')) { | ||
| return badRequest(res); | ||
| } | ||
|
|
||
| const urlArr = decodedUrl.split('/').filter(Boolean); | ||
|
|
| return hint(res); | ||
| } | ||
|
|
||
| const filename = 'public/' + urlArr.slice(1).join('/'); |
| const filename = 'public/' + urlArr.slice(1).join('/'); | ||
|
|
||
| fileOutput(res, filename); |
|
|
||
| on: | ||
| pull_request: | ||
| branches: [ master ] |
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
There are critical requirement violations that need to be fixed. According to checklist item #3, paths that don't start with /file/ should return hint messages (200), but your code incorrectly returns badRequest (400). This issue exists in both src/helpers/responses.js (line 22) and src/handlers/fileHandler.js (line 22). Additionally, the hint logic in fileHandler.js (lines 28-30) is unreachable because line 22 catches non-/file paths first and returns the wrong status code. Finally, src/createServer.js checks for includes('..') but the requirement specifically mentions ../ — this over-restrictive check would incorrectly block valid filenames like co..perate.txt. The path traversal check should only look for the ../ sequence.
✨ 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
| notFound, | ||
| badRequest, | ||
| hint, | ||
| }; |
There was a problem hiding this comment.
This line combines two separate conditions that should be handled independently. includes('..') correctly returns 400 (checklist item #5), but !decodedUrl.startsWith('/file') should return hint (200) per checklist item #3, not badRequest (400). Consider splitting: first check for .. returning 400, then check if path starts with /file for hint.
| stream.pipe(res); | ||
| }); | ||
|
|
There was a problem hiding this comment.
The condition !decodedUrl.startsWith('/file') returns badRequest (400), but requirement #3 specifies that paths NOT starting with /file/ should return a hint message (200). For example, /anything should return hint, not 400. This check should only return badRequest for .. (path traversal), not for missing /file/ prefix.
| } | ||
| notFound(res); | ||
| }); |
There was a problem hiding this comment.
The hint logic at line 28-30 is unreachable for paths like /anything because line 22 catches them first. According to requirements, /anything should return hint (200), not badRequest (400). The startsWith('/file') check needs to be moved after the .. check and return hint instead of badRequest.
| if (decodedUrl.includes('//')) { | ||
| return notFound(res); | ||
| } | ||
|
|
There was a problem hiding this comment.
This checks for .. anywhere in the URL, but the requirement states 'If the pathname contains ../'. The current implementation would block filenames like ..hidden or index..html even though they don't contain the path traversal sequence ../. Consider checking for ../ specifically instead.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The request meets the required criteria.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| } | ||
| notFound(res); | ||
| }); | ||
| } | ||
|
|
||
| module.exports = { |
There was a problem hiding this comment.
When the URL is /file/, urlArr.slice(1) produces an empty array, resulting in filename = 'public/'. The code then tries to read a directory instead of public/index.html. According to the requirements, /file/ should return public/index.html.
| const contentType = mimeTypes[ext] || 'text/plain'; | ||
|
|
||
| res.writeHead(200, { 'Content-Type': contentType }); | ||
| stream.pipe(res); |
There was a problem hiding this comment.
The condition || decodedUrl === '/app.js' is not part of the requirements. The task only requires blocking paths containing ../. Consider removing this extra check.
No description provided.