expressjs/serve-index

How to handle fs.stat error on a file?

Opened this issue · 4 comments

There are two potential error cases at present:

  • ENOENT: null is returned rather than stat. This would cause an unhandled exception if it were ever encountered (i.e. the file is deleted between fs.readdir and fs.stat). However, it is unlikely that any one will ever encounter this error and much less likely that they will be able to repeat it to know what happened and submit a bug.
  • Other errors (EPERM, etc). These are currently handled by passing the error to express and causing a 500 error.

ENOENT

In the first case I think we should just filter out the file.

If it's been deleted (or is otherwise a special type of file, such as a virtual file on fuse fs, which is returned from fs.readdir() but doesn't exist when you fs.stat()), why bother to show it? And why error out?

Other Erorrs

I disagree with the current behavior. For one, it's inconsistent between text/plain and application/json responses (work) and text/html responses (fail).

I think we should instead provide a stat object that looks like this:

{ "name": "foo.txt"
, "size": 0
, "lastModified": "1970-01-01T00:00:00.000Z"
, "type": "error/EPERM"
}

(or maybe something more conventional liketype: application/vnd.eperm+x-error)

Or perhaps omit the file.

In any case, I don't think that the current behavior of throwing a 500 on any single potential permission error is good behavior.

Yea, I believe there were some issues opened up about this in the past but no PRs ever materialized. I would except it to do the same thing that the Apache HTTPD index view does in this case, which I assume is either hide it or display it without additional metadata.

And also agree that all the three views should behave in the same way.

I don't know how Apache handles it, so how about we just hide it for now (most secure) and circle back around to it if there's a complaint.

Once we get #74 taken care of I'll come back around to this and update the PR.

We have time now to do it right :) waiting until later is unnecessarily churn. I can look at apache httpd and report back to see what it does and then we can decide how to love forward 👍

Bringing up security means it is even more imparitive to validate our implementation against one that has been reviewed many times, or alternatively we can see if a security researcher can take a look at our design here 👍