-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
SharedDataMiddleware fails with IsADirectory #1599
Comments
This comment has been minimized.
This comment has been minimized.
This seems to be expected, and it happens even with a This should probably return a 404 though. Also, we should be depending on importlib_resources, not pkg_resources, for this. |
That would mean that packages and normal filesystem paths have different locations - the latter do support pointing to a directory and I think it makes perfect sense that you point this middleware to a directory and not a file... |
You do point at a directory, but it only serves files. If you go to the top level, you don't get a directory listing, you need to go to an actual file within the directory. What do you expect to get when visiting |
OK, after testing it again now I remember the exact issue: Let's take this example: from flask import Flask
from werkzeug.middleware.shared_data import SharedDataMiddleware
app = Flask(__name__)
app.wsgi_app = SharedDataMiddleware(app.wsgi_app, {
'/': ('flask', 'json'),
'/test/': ('flask', 'json'),
'/fs/': '/var/empty/',
})
IMO the first two options should 404 as well to be consistent and not cause an actual exception simply due to a client requesting that URL. 403 would probably be even more suitable for all cases since AFAIK that's what real webserver usually do when you attempt to access a directory that has no directory listing enabled. |
Definitely agree they should raise 403/4 instead of 500, thanks for clarifying. |
Due to the way the middleware works, we can't return 403 as unhandled paths are intended to pass through to the app, which is what's actually raising the 404. Would be too big a rewrite at this point to support 403. |
Minimal example:
This only seems to happen when using
/
with the tuple syntax to reference a package. When using another mapping like/test/
it works fine.The text was updated successfully, but these errors were encountered: