Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: disable root path check when serve is false #467

Closed

Conversation

nimesh0505
Copy link

@nimesh0505 nimesh0505 commented Aug 30, 2024

Summary

This MR addresses the issue #460 where the checkRootPathForErrors function still requires a valid path to root even when serve: false is set.

Changes

  • Disabled checkRootPathForErrors when serve: false.
  • Added a unit test to confirm that the root path check is bypassed when serve: false.

Tests

  • Added a test in test/static.js to verify that checkRootPathForErrors is not called when serve: false.

Checklist

@nimesh0505 nimesh0505 force-pushed the feature-disable-rootpath-check branch from 7efb216 to 8c39dca Compare September 2, 2024 08:08
@nimesh0505 nimesh0505 changed the title fix: disable root path check when serve is false feature: disable root path check when serve is false Sep 2, 2024
@nimesh0505 nimesh0505 marked this pull request as ready for review September 2, 2024 08:17
Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think simply disable the check is the solution.

  1. It may fails to serve file because there is no root provided. You can see the root path is passing everywhere inside the codebase. You must provide test to ensure it will works.
  2. It can leads to security issue because it means by default the serving of files can be escaped to everywhere. You should at least default the root to CWD and update the read me.

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 2, 2024

My remark from three days ago was still not addressed.

Imho this PR does not take in account, that if no general root path is defined, that rootPath parameter of sendFile becomes mandatory.

root || sendOptions.root,

@climba03003
Copy link
Member

Note that please do not open another PR for updates or address comment.
It is hard to follows.

You should push commit to your branch to address the comments.

@nimesh0505
Copy link
Author

@climba03003 Can you review this PR?

README.md Outdated Show resolved Hide resolved
@nimesh0505 nimesh0505 requested a review from Uzlopak September 10, 2024 08:45
@@ -408,7 +413,7 @@ function normalizeRoot (root) {
return root
}

function checkRootPathForErrors (fastify, rootPath) {
function checkRootPathForErrors (fastify, rootPath, skipExistenceCheck) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am think is it necessary to skip the check of root path?
You should never use a invalid root path, defaulting to cwd is a trade-off for security reason.

All of your served file should be contained within to the root and prevent path escape.

Copy link
Author

@nimesh0505 nimesh0505 Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@climba03003 Thank you for your feedback on the security implications. Before implementing changes, I would like to confirm the approach that we remove the skipExistenceCheck parameter from the checkPath and checkRootPathForErrors functions since we need perform full validation of the path.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can skip for cwd, but you must provide a valid root.
No matter other setting.

@mcollina
Copy link
Member

@nimesh0505 @simoneb ping

@simoneb
Copy link
Contributor

simoneb commented Oct 12, 2024

Unfortunately we cannot progress on this PR, so I'm closing it.

@simoneb simoneb closed this Oct 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants