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

How to handle fs.stat error on a file? #76

Open
coolaj86 opened this issue Aug 13, 2018 · 4 comments
Open

How to handle fs.stat error on a file? #76

coolaj86 opened this issue Aug 13, 2018 · 4 comments
Labels

Comments

@coolaj86
Copy link

coolaj86 commented Aug 13, 2018

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.

@coolaj86 coolaj86 changed the title How to handle files that cannot be read How to handle fs.stat error on a file? Aug 13, 2018
coolaj86 pushed a commit to coolaj86/serve-index that referenced this issue Aug 13, 2018
@dougwilson
Copy link
Contributor

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.

@dougwilson
Copy link
Contributor

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

@coolaj86
Copy link
Author

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.

@dougwilson
Copy link
Contributor

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 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants