`Plug.Static` should not raise error when an invalid path is received

[waj]

I understand that Plug.Static validates the path received to avoid directory traversals outside the specified directory. However I think it would be enough to discard the request and let other plugs to evaluate if they want to handle it.

For example, I have a Phoenix app where a controller receives one of the parameters that includes a slash (yes, my entity id contains slashes...). These slashes are properly escaped into the URL but the Plug.Static prevents these routes to reach the Phoenix router because it raises the InvalidPathError exception.

If there is not a better reason to continue raising this error, I could send a PR with the fix.

[josevalim]

Couldn't you use the :only option in Plug.Static to ensure it only activates for certain paths? This way we will no longer check all paths and you should be able to handle them in your app.

[waj]

That would work as a workaround but I have to be exhaustive and is quite error prone when new assets are added to priv/static.

Still, I don't understand why Plug.Static have to raise the error not allowing other plugs to handle the request. InvalidPathError is misleading because there is nothing inherently wrong with the URL.

[josevalim]

That would work as a workaround but I have to be exhaustive and is quite error prone when new assets are added to priv/static.

That is how Phoenix handles it and adding new assets are not quite likely. Some code comments will help users remember the configuration needs to be set.

I would prefer Plug.Static to raise because that yields the better experience, especially during development, on the cases where you actually expect Plug.Static to handle that path. The behaviour you propose makes sense only when cascading and that is more uncommon, especially when you can limit the paths Plug.Static act on.

[waj]

@josevalim I have big respect for you and all the work you do, and I always greatly appreciate how quickly you respond to issues and comments in any of the projects you're involved. Nothing of that will change no matter what you do with this quite trivial problem. However, I found a little bit rude that you close issues like this without any further consideration.

Of course it's your project and at the end you can do whatever you want but in this case I find most of your statements false.

adding new assets are not quite likely

I've been working on software development and specially web development for quite many years and I find this is totally the opposite. And having to declare in a list all the assets that must be served feels really odd.

Some code comments will help users remember the configuration needs to be set.

Yup... maybe right after two hours of perplexity trying to understand why the server returns a 404 for an asset that's clearly in the static directory.

that yields the better experience, especially during development

I think the feature that prevents directory traversals while looking for static assets is more related to production environment than development. What's the case where a developer would be trying to retrieve an asset with an "invalid" path?

The behaviour you propose makes sense only when cascading

And that's exactly what happens in Phoenix. For example if I request the path /api/controller/odd%2Fparam%2Fwith%2Fslashes I expect the controller to handle it, but before that it has to pass through the Plug.Static handler without raising.

[josevalim]

adding new assets are not quite likely

Sorry. Let me clarity this: it is most common to organize the assets in directories and :only requires you to list only the top level directories, not every asset. So what I meant above is that adding new parent directories to work as assets is not quite likely. Or, in other words, adding new entries to the :only list is uncommon.

[waj]

What about having a new option :invalid_path_handling that defaults to :raise but could also be configured to :ignore?

[josevalim]

That's fine by me, tks.