search

expressjs / serve-static

Serve static files
MIT License
1.4k stars 228 forks source link

don't ignore .well-known by default #54

Closed coolaj86 closed 8 years ago

coolaj86 commented 8 years ago

Unfortunately the RFC IETF or whoever decided it was a good idea to standardize on a directory that is, by default, hidden. Right?

So when I'm tearing my hair out wondering why my Let's Encrypt certs aren't getting validated....

It's the darn .well-known givin' me all the troubles.

I'd like to suggest that .well-known (and any other RFC / IETF spec'd dirs) not be ignored by default.

Perhaps the default should be the array ['.well-known'] and you can override that value with true, false, or [] (ignore)?

dougwilson commented 8 years ago

I'm not sure why you think there is a bug in this module. By default, we do allow that to work, unless you change it to do otherwise. You can even see it working just fine using the simple example at https://github.com/expressjs/serve-static#serve-files-with-vanilla-nodejs-http-server and serving up the file in a directory called .well-known (I removed the index option and changed the root path to simplify):

$ npm i finalhandler serve-static
finalhandler@0.4.1 node_modules\finalhandler
├── escape-html@1.0.3
├── unpipe@1.0.0
├── on-finished@2.3.0 (ee-first@1.1.1)
└── debug@2.2.0 (ms@0.7.1)
serve-static@1.10.0 node_modules\serve-static
├── escape-html@1.0.2
├── parseurl@1.3.0
└── send@0.13.0 (destroy@1.0.3, fresh@0.3.0, range-parser@1.0.3, ms@0.7.1, etag@1.7.0, statuses@1.2.1, debug@2.2.0, mime@1.3.4, depd@1.0.1, on-finished@2.3.0, http-errors@1.3.1)

$ cat app.js
// This is the example from the README
var finalhandler = require('finalhandler')
var http = require('http')
var serveStatic = require('serve-static')

// Serve up public folder
var serve = serveStatic('public')

// Create server
var server = http.createServer(function(req, res){
  var done = finalhandler(req, res)
  serve(req, res, done)
})

// Listen
server.listen(3000)

$ mkdir public

$ mkdir public/.well-known

$ mkdir public/.well-known/acme-challenge

$ echo 'hello!' > public/.well-known/acme-challenge/xxxxxxxxxxxxxxx

$ node app &
[1] 12984

$ curl http://127.0.0.1:3000/.well-known/acme-challenge/xxxxxxxxxxxxxxx
hello!

If you still think there is an issue, please build upon the reproduction steps I have above and paste the way to reproduce here so I can try it out :)

coolaj86 commented 8 years ago

I did pretty much exactly what you did there (@1.9.2) and once I added { dotfiles: 'allow' }, it worked. But it did not work with the default setting.

(And I agree with the default setting that things like .git, .ssh, etc are better off to ignore, just that .well-known should be let to pass through.)

The default value is 'ignore'.

^^

dotfiles

Set how "dotfiles" are treated when encountered. A dotfile is a file or directory that begins with a dot ("."). Note this check is done on the path itself without checking if the path actually exists on the disk. If root is specified, only the dotfiles above the root are checked (i.e. the root itself can be within a dotfile when set to "deny").

The default value is 'ignore'.

  • 'allow' No special treatment for dotfiles.
  • 'deny' Deny a request for a dotfile and 403/next().
  • 'ignore' Pretend like the dotfile does not exist and 404/next().

so if that does work by default, then the documentation is incorrect

coolaj86 commented 8 years ago

Anyway, looking through the code I found that serve static does'n actually handle this, it's passed on to send without modification.

dougwilson commented 8 years ago

Hi @coolaj86 , did you try my reproduce steps and did it work? I tried to reproduce your issue but was unsuccessful, so without me being able to reproduce the issue, I have no idea what I'm supposed to be changing.

Can you provide me complete code & instructions on how to reproduce the issue? You can use a format similar to https://github.com/expressjs/serve-static/issues/54#issuecomment-164090223 .

At this point, unless you can provide me with a way to reproduce the issue or a pull request with the fix and tests, then I'm not really sure what I'm supposed to do. All I can say is that we definitely allow access to dotfolders out of the box, only if you alter the dotfiles option will it be different. You can see in my steps that works above, I did not specify the dotfiles option, which would be the out of box experience with this module.

Perhaps it's just the documentation needs to be updated?

coolaj86 commented 8 years ago

@dougwilson I'm rerunning right now and trying to figure out why my experience is different from yours.

I take your example (on the same machine with the same version of code that I was running) and it runs in the way that you describe - which is different from the documentation.

I have my code (which began working after adding { dotfiles: 'allow' }) which is working the way that the documentation describes, but not the way that you are explaining.

I'm trying to find the discrepancy. I have npm 3 so I wouldn't think that I have a hidden old version somewhere, but I'm going to dig a little to find out and report back.

dougwilson commented 8 years ago

I did pretty much exactly what you did there (@1.9.2) and once I added { dotfiles: 'allow' }, it worked. But it did not work with the default setting.

Um, version 1.9.2 works just fine with .well-known as well. Here are the exact steps I did to test it. Perhaps, if you have more code in your app, you have a bug somewhere in your code? Can you explain how you're sure that this module is not working with .well-known, exactly? Can you provide me with full code & steps for me to reproduce? I would be happy to look into it, but unless you can tell me how to reproduce, I'm not sure what I can do. I tried hard to reproduce your issue but am unable.

$ npm i finalhandler serve-static@1.9.2
finalhandler@0.4.1 node_modules\finalhandler
├── escape-html@1.0.3
├── unpipe@1.0.0
├── on-finished@2.3.0 (ee-first@1.1.1)
└── debug@2.2.0 (ms@0.7.1)

serve-static@1.9.2 node_modules\serve-static
├── utils-merge@1.0.0
├── escape-html@1.0.1
├── parseurl@1.3.0
└── send@0.12.2 (destroy@1.0.3, range-parser@1.0.3, fresh@0.2.4, ms@0.7.0, mime@1.3.4, depd@1.0.1, debug@2.1.3, on-finished@2.2.1, etag@1.5.1)

$ cat app.js
// This is the example from the README
var finalhandler = require('finalhandler')
var http = require('http')
var serveStatic = require('serve-static')

// Serve up public/ folder
var serve = serveStatic('public')

// Create server
var server = http.createServer(function(req, res){
  var done = finalhandler(req, res)
  serve(req, res, done)
})

// Listen
server.listen(3000)

$ mkdir public

$ mkdir public/.well-known

$ mkdir public/.well-known/acme-challenge

$ echo 'hello!' > public/.well-known/acme-challenge/xxxxxxxxxxxxxxx

$ node app &
[1] 13956

$ curl http://127.0.0.1:3000/.well-known/acme-challenge/xxxxxxxxxxxxxxx
hello!
dougwilson commented 8 years ago

which is different from the documentation.

I'm fixing the documentation now. It should have pretty much said the same thing the send documentation says for the dotfiles option.

coolaj86 commented 8 years ago

Something weird is going on. Check this out:

Here's the little segment of my code:

screen shot 2015-12-11 at 6 57 14 pm

Here's what happens with the option commented out, effectively ignore:

default (not the same as ignore): 200's non-dotfiles in dotdirecotry, 404's dotfiles in dotdirectory

screen shot 2015-12-11 at 6 57 27 pm

ignore:

screen shot 2015-12-11 at 7 12 39 pm

allow: 200's everything

screen shot 2015-12-11 at 6 58 39 pm

deny: 404's the dotfile, but 403's the non-dotfile in the dotdirectory

screen shot 2015-12-11 at 7 01 03 pm

directory structure:

screen shot 2015-12-11 at 7 01 42 pm

coolaj86 commented 8 years ago

The default value is similar to 'ignore', with the exception that this default will not ignore the files within a directory that begins with a dot, for backward-compatibility.

Ah...

I'll need to try that.

coolaj86 commented 8 years ago

default:

ignore:

allow:

deny:

dougwilson commented 8 years ago

For your deny example, I get 403 for all 4 cases you provided above. Can you please share your code so I can reproduce? They should all be 403 and they all are for me.

coolaj86 commented 8 years ago

As for the original issue: I was obviously doing something differently from what I thought I was doing because reverting what I thought was the fixing change yesterday did not cause the code to break today.

Thanks for the support. I'll leave this thread alone now.