Hello!
I’m trying to get my first application working without issues, and I’ve just noticed an unpleasant behavior. Currently I have resources "/blogs", BlogController set in my router for all operations on blogs. I also have a kind of “require authentication” plug to prevent users from tempering around. The problem is: when I type a blog’s id in the address string, I get the blog with that id shown as expected and if an unauthorized user tries to enter something like /blogs/new he gets “Authorization required” message. But if I hit, say, blogs/delete or anything that is not an integer, instead of “404 Not found” or “Authorization required” I’ve got a bunch of errors in development and “Internal server error” in production, and this is not what I expect. I understand that normally user shouldn’t get to those unspecified routes, but what is someone enters these routes on purpose? Is there any way to prevent such behavior? Maybe a guard clause in router or wildcards? I really don’t like the idea to show users “Internal server error” instead of “You are not welcome here”.
You can use get "/*path" to wildcard any path and match it to an error controller/action. If you’re using an authorization plug in the form of a router pipeline and scoping your routes, then you can create a wildcard match inside the scoped routes, matching to a the error controller/action for logged in users but that will simple render whatever your auth plug renders when accessed by an unlogged user?
But under “resources” I already have blogs/:id that will catch anything before it gets to the /*path. Wouldn’t it be nice if we could use something like get /blogs/:id when is_integer(:id), BlogController, :show? Currently I think about setting a similar guard clause into get_blog!/1 function in the Context module.
each route can be defined with a method. You might want to be explicit in your routing, and not use resources, if You want to separate them between different pipelines.
In your case…
get("/blogs", BlogController, :index)
get("/blogs/:id", BlogController, :show)
pipe_through(:api_auth)
get("/blogs/new", BlogController, :new)
post("/blogs", BlogController, :create)
get("/blogs/:id/edit", BlogController, :edit)
patch("/blogs/:id", BlogController, :update)
delete("/blogs/:id", BlogController, :delete)
BTW /blogs/delete is not a REST path.
Where do you add your plug? I tried to reproduce this behavior (access protected /resource/some-non-int) but I get proper response (redirect to login in my case).
These errors might provide a clue as to what is wrong with the auth check.
As for logged in users seeing the error on wrong URL, you can cast exception by defining an implementation of Plug.Exception. In case of providing a wrong type (string instead of integer) that would be Ecto.Query.CastError which you can handle like this
defimpl Plug.Exception, for: Ecto.Query.CastError do
def status(_), do: 404
end
I do agree that being able to set constraints on the routes themselves would be sometimes handy, but not really sure it warrants being made, simply because usually you’ll want it to group certain types of requests, scope some others, and that all fits well with the plug and pipelines ideas. This particular case seems fit to @yurko’s suggestion, (if you would want to have other specific errors redirected you could also have a generic error controller & page and then just weed the requests on each controller redirecting to that one in a case valid? do pattern, e.g:
def my_action(conn, %{"id" => id}) do
case is_valid_according_to_my_rules_for_this_controller_function(id) do
true -> # do regular stuff
false -> redirect(to: "/my_error_path")
end
end
On the case you mention, although it seems like a “simple” case, it is not as simple, because as far as the protocol cares, the url is always a string. We map segments of it dynamically, but they’re still strings. So in this case is_integer wouldn’t ever work, so to make it work you would need to cast it to integer, which would raise an error if it was passed some parameter such as delete, so then you would need to rescue that, which seems a bit complex to have in the router.
BTW /blogs/delete is not a REST path.
Exactly. So here is the problem. By default, Phoenix expects either blogs/new or blogs/. But if a user by mistake or on purpose enters anythings besides “new” or an integer, it raises because of this piece of code (generated by phx.gen.html):
def get_blog!(id), do: Repo.get!(Blog, id)
I prefer to use the non ! version of get…
You can combine with with, and fallback controller.
Something like
with %Blog{} = blog <- get_blog(id) do
...
# You don't need to manage errors here, delegate to fallback controller
...
end
def get_blog(id), do: Repo.get(Blog, id) # returns blog or nil
And the fallback controller will take care of nil when blog could not be found.
I really like that fallback controller idea, pretty neat.
If you want to have something customisable that executes prior to the controller action you can also write a plug and then use it in whatever controller & actions you need. For e.g.
defmodule YourApp.Plugs.Test do
import Plug.Conn
def init(opts) do
opts
end
def call(conn, [%{param: param, match: match} = param_spec | t]) do
IO.puts("Plug call\n#{inspect conn}\n\n#{inspect conn.params}\n\n#{inspect param_spec}")
call(conn, match, param, param_spec, t)
end
def call(conn, []), do: conn
def call(conn, :integer, param, param_spec, t) do
required = param_spec[:required]
case conn.params[param] do
nil when not required -> call(conn, t)
nil -> redirect_error(conn, {:required, param})
value ->
try do
case is_integer(value) or (is_binary(value) and String.to_integer(value)) do
true ->
call(conn, t)
false ->
redirect_error(conn, param)
int ->
IO.puts(int)
call(conn, t)
end
rescue
ArgumentError -> redirect_error(conn, param)
end
end
end
def call(conn, :in, param, param_spec, t) do
required = param_spec[:required]
case conn.params[param] do
nil when not required -> call(conn, t)
nil -> redirect_error(conn, {:required, param})
value -> case value in param_spec[:in] do
true -> call(conn, t)
false -> redirect_error(conn, param)
end
end
end
def redirect_error(conn, param) do
conn
|> Plug.Conn.halt()
|> Phoenix.Controller.put_flash(:error, message_for(param))
|> Phoenix.Controller.redirect(to: YourAppWeb.Router.Helpers.main_path(conn, :index))
# instead of `put`ing flash messages and redirecting you could place assigns, or read the path that was trying to be accessed from the conn and decide based on that, etc...
end
def message_for({:required, param}), do: "The parameter #{inspect param} is required"
def message_for(param), do: "The parameter you provided for #{inspect param} is invalid"
end
You could now use it in any controller like this:
plug YourApp.Plugs.Test, [%{param: "id", match: :integer, required: true}, %{param: "something", match: :in, in: ["some", "thing"]}] when action in [:update]
For specific actions, or for all actions, or place it in a particular pipeline, and of course you could then expand it to whatever use cases. But for the case you mentioned, the exception handling on ecto plus the fallback controller seem like better options. But plugs are great, you just plug them wherever you need them!
