Develop by shatskov-artur · Pull Request #343 · mate-academy/node_static-files-server

shatskov-artur · 2026-02-07T14:08:10Z

Solution

mateacademy-ai-mentor

Review Summary

Good work on setting up the server. I am requesting a few changes before this can be approved, as there is a critical security vulnerability and a couple of unmet functional requirements.

Specifically, you'll need to fix these issues:

Security: The server is vulnerable to path traversal attacks (e.g., /file/../../package.json). You must sanitize the file path to ensure it always stays within the public directory.
Functionality: Requests to /file and /file/ should serve public/index.html, but currently they return a 'File Not Found' error.
Content-Type: The Content-Type header is hardcoded to text/plain, which prevents browsers from rendering HTML or CSS. This should be set dynamically based on the file extension.

Addressing these items will make your server both secure and fully functional according to the requirements.

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-02-07T14:09:45Z

src/createServer.js

+    if (!relativeFilePath) {
+      res.writeHead(200, { 'Content-Type': 'text/plain' });
+      res.end('File Not Found');
+
+      return;
+    }


According to the requirements, requests for /file/ or /file should return public/index.html. This logic currently returns a 'File Not Found' message. You should modify this logic to serve index.html when relativeFilePath is empty.

mateacademy-ai-mentor · 2026-02-07T14:09:45Z

src/createServer.js

+      return;
+    }
+
+    fs.readFile(`public/${relativeFilePath}`, (err, data) => {


This line is vulnerable to path traversal attacks. A request like /file/../../package.json could allow access to files outside the public directory. You should sanitize the relativeFilePath to ensure it does not contain elements like .. and that the final resolved path is still within the public folder. The built-in path module can be very helpful for this.

mateacademy-ai-mentor · 2026-02-07T14:09:45Z

src/createServer.js

+
+        return;
+      }
+      res.writeHead(200, { 'Content-Type': 'text/plain' });


The Content-Type is hardcoded to text/plain. This will prevent browsers from correctly rendering file types like HTML or CSS. The content type should be determined dynamically based on the file's extension (e.g., text/html for .html, text/css for .css).

mateacademy-ai-mentor

Review Summary

Great job fixing the routing for /file and /file/! However, a couple of critical issues remain that need to be addressed.

Security Vulnerability: The server is still vulnerable to path traversal attacks. For example, a request to /file/../createServer.js can expose your source code. You must resolve the requested file path and verify that it stays within the public directory. The built-in path module is perfect for this.
Incorrect Content-Type: The Content-Type header is still hardcoded to text/plain, which prevents browsers from rendering HTML or applying CSS. This needs to be set dynamically based on the file's extension (e.g., .html files should have Content-Type: text/html).

Please focus on resolving these two points to make your server secure and functional.

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-02-07T14:14:36Z

src/createServer.js

+
+          return;
+        }
+        res.writeHead(200, { 'Content-Type': 'text/plain' });


The Content-Type is still hardcoded as text/plain. For the server to be useful, this should be set dynamically based on the file's extension. For instance, .html files should have Content-Type: text/html, and .css files should have Content-Type: text/css.

mateacademy-ai-mentor · 2026-02-07T14:14:36Z

src/createServer.js

+      return;
+    }
+
+    fs.readFile(`public/${relativeFilePath}`, (err, data) => {


This implementation is still vulnerable to path traversal attacks. For example, a request to /file/../createServer.js could expose your server's source code. You need to resolve the full file path and verify that it is still inside the public directory before reading the file. The built-in path module can be very helpful here.

mateacademy-ai-mentor · 2026-02-07T14:14:36Z

src/createServer.js

+
+        return;
+      }
+      res.writeHead(200, { 'Content-Type': 'text/plain' });


Similar to the other file-serving block, the Content-Type here is hardcoded. This needs to be determined from the file extension of the relativeFilePath to ensure browsers can correctly interpret the content.

mateacademy-ai-mentor

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
_{[CODE: NCM]}

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

mateacademy-ai-mentor

Review Summary

I am approving this step to help you move forward, but please note there are still a few critical issues to address. Great job adding the check for // in the path!

However, the server remains vulnerable to path traversal attacks. It's crucial to use the path module to safely build and validate file paths, ensuring they always stay within the public directory. Also, the Content-Type header is still hardcoded as text/plain, which prevents browsers from correctly rendering HTML or CSS. Lastly, remember that requests to /file and /file/ should serve public/index.html.

Keep these points in mind for the next steps. You're making good progress!

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Rate AI review example ↩

mateacademy-ai-mentor · 2026-02-07T15:41:19Z

src/createServer.js

+    if (!relativeFilePath) {
+      res.writeHead(200, { 'Content-Type': 'text/plain' });
+      res.end('File Not Found');
+
+      return;
+    }


The current logic returns "File Not Found" for requests to /file or /file/. According to the requirements, these paths should serve public/index.html. You should adjust this logic to handle an empty relativeFilePath by defaulting it to index.html.

mateacademy-ai-mentor · 2026-02-07T15:41:19Z

src/createServer.js

+      return;
+    }
+
+    fs.readFile(`public/${relativeFilePath}`, (err, data) => {


This line is still vulnerable to path traversal attacks. A request like /file/../createServer.js could expose your server's source code. You need to use the path module to create a secure file path and verify that the final path remains inside the public directory. Concatenating strings to form a path is not safe.

mateacademy-ai-mentor · 2026-02-07T15:41:19Z

src/createServer.js

+
+        return;
+      }
+      res.writeHead(200, { 'Content-Type': 'text/plain' });


The Content-Type is still hardcoded to text/plain. This prevents browsers from correctly rendering HTML or applying CSS. You need to set this header dynamically based on the file extension of the requested file (e.g., text/html for .html files, text/css for .css files).

Artur Shatskov added 2 commits February 7, 2026 13:10

Load local changes

37522e1

Solution

6e39ea5

mateacademy-ai-mentor suggested changes Feb 7, 2026

View reviewed changes

Solution

39aebdb

shatskov-artur requested a review from mateacademy-ai-mentor February 7, 2026 14:13

mateacademy-ai-mentor suggested changes Feb 7, 2026

View reviewed changes

Solution3

fec4001

shatskov-artur requested a review from mateacademy-ai-mentor February 7, 2026 14:26

mateacademy-ai-mentor reviewed Feb 7, 2026

View reviewed changes

Solution3

7acb1c7

shatskov-artur requested a review from mateacademy-ai-mentor February 7, 2026 14:34

mateacademy-ai-mentor approved these changes Feb 7, 2026

View reviewed changes

Conversation

shatskov-artur commented Feb 7, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants