Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

Do not serve files when path ends with / #224

Merged
merged 4 commits into from
Jul 26, 2024
Merged

Do not serve files when path ends with / #224

merged 4 commits into from
Jul 26, 2024

Conversation

rmhaiderali
Copy link
Contributor

@rmhaiderali rmhaiderali commented Jan 20, 2024 •
edited
Loading

When index option is set to false and the environment is Windows, requests that end with a / might result in serving files. This happens because in Windows, when accessing a file or directory, if the path ends with /, the system first checks for a directory. If no directory is found, it then checks if a file with the same name exists, ignoring the / at the end of the path. This enables access to a file unexpectedly, as if the path did not conclude with /.

The proposed fix addresses this issue by introducing a check to ensure that the path does not ends with / before serving a file. For example, in the given case, file is being accessed when it should not be, and the proposed change aims to rectify this behavior.

index.js

var http = require("http");
var send = require("send");

var server = http.createServer(function onRequest(req, res) {
  send(req, "./index.html/", { index: false }).pipe(res);
});

server.listen(3000);

index.html

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
    <title>Document</title>
  </head>
  <body>
    HTML File
  </body>
</html>

@wesleytodd
Copy link
Member

Hey @rmhaiderali, would you be able to add some tests and docs for this?

@rmhaiderali
Copy link
Contributor Author

Hi @wesleytodd, I've included the test case, but I'm unable to come up with any documentation content since it's more of a bug fix. If you have any suggestions please let me know.

@wesleytodd
Copy link
Member

I was thinking that maybe a call out for the behavior on windows and how it differs from *nix systems would be good to document. These sort of cases require either docs or someone who develops on windows around to ensure the context is not lost, so just wanted to make sure the reason was well documented since I dont think anyone who maintains these packages develops on windows.

@wesleytodd wesleytodd changed the base branch from master to 1.0 July 23, 2024 01:39
@wesleytodd
Copy link
Member

I am working to prepare the 1.0 release so I re-targeted this PR to that. I understand the reason you can't see a good doc to write and now that it is going out with a major it is less likely to cause churn. Would you be willing to rebase this onto 1.0? If not I can do that tomorrow before merging.

@wesleytodd wesleytodd changed the base branch from 1.0 to master July 26, 2024 20:35
@wesleytodd wesleytodd merged commit 9774100 into pillarjs:master Jul 26, 2024
4 checks passed
@wesleytodd wesleytodd mentioned this pull request Jul 26, 2024
Closed
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
needs docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rmhaiderali @wesleytodd