-
-
Notifications
You must be signed in to change notification settings - Fork 151
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
Comments
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 👍 |
There are two potential error cases at present:
null
is returned rather thanstat
. This would cause an unhandled exception if it were ever encountered (i.e. the file is deleted betweenfs.readdir
andfs.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.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 youfs.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:
(or maybe something more conventional like
type: 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.
The text was updated successfully, but these errors were encountered: