When I access this URL directly from the browser, that will send a GET request, and I will correctly receive a client error code: 400 Bad Request.
However, on the server I see that the request is executed past the router anyway, and somewhere my application will throw an error in a controller because a POST param is missing. The error is found in FooController.next_foo/2.
Why is the request still executed? It should not match with that route.
You should share the whole router and check with your browser’s developer tools what request content is sent and received. Indeed it should not match that route but maybe it matches some other route that goes in the same place (or you’re running a different version of the code than you think).
Put some IO.inspect statements in the controller function to find out where exactly the error occurs.
I suspect something else though; you might have Plugs that activate before your controller function, and one of them is erroring. I’d check my plugs if I were you.
@dimitarvp I found out exactly where the error occurs as you suggested and I realize @Nicd was right, it matches another route.
post "/foo/next", FooController, :next_foo
get "/:foo_id/:foo_slug", FooController, :show_foo
How can I solve this problem? Is it possible to restrict the parameter :foo_id to a number at the router level, so that that a string at :foo_id doesn’t match?
Of course I could prefix the second route with something static, but I do not want to do that (seo). Would be nice if I can have some regex or other validator at the router level for the parameters. Other frameworks allow to do that.
But I do not want to change my URLs because of some technical limitations. Because of SEO. The success of this website is entirely based on SEO performances, I’d like to name and set a strategy for my URLs as I want and not having to choose the URLs because of some technical limitations of a framework.
In PHP I could add custom routes where I could even implement myself the code that checks if it should match or not; so that gave me full control over routes.
It’s been a while since I tackled similar issues. I am reasonably sure it can be done exactly as you want but I simply don’t remember how at the moment. Sorry I can’t be of further help.
EDIT: One thing comes to mind: is the more specific route above the more abstract route?
post "/foo/next", FooController, :next_foo
get "/:foo_id/:foo_slug", FooController, :show_foo
A GET to /foo/next doesn’t match, so I guess then it continues and matches the get "/:foo_id/:foo_slug" route.
I wish I could force :foo_id to be a number only. Seems basic requirement.
Yeah, that means adding error handling. Then it makes me think, should I validate all and every param in every controller action?
Typically your controller action will be called with some parameters; then you’ll use these parameters to fetch stuff from the database. But that means that for invalid parameters you’re gonna make unnecessary DB requests. If you want to consider this, then you validate allll the params in all of the controller actions, heh.
You have a special case, namely a catch-all route and function. It’s fairly normal to make sure that the content that it is supposed to fetch and present are actually available.
def show_foo(conn, %{"foo_id" => id, "foo_slug" => slug} = _params)
{id, rest} = Integer.parse(id, 10)
case rest do
"" ->
# all okay, proceed with what the controller action is supposed to be doing.
_ ->
# the passed ID is not an integer, show an error page.
end
end
Additionally, you should indeed religiously validate all controller params because that’s the only thing that’s coming your way from the outside world and you should validate and sanitize it, heavily.
EDIT: And finally, the way you can do it in PHP will save you writing, yes, but it does nothing to protect you from bad requests, so you are kind of just skimping on work and hoping for the best. It would not pass a code review if I was your leader.
Should anything in particular be done? Because for example I think just by using Ecto, by default it comes with protection for SQL injection and such. Then the templating mechanism also outputs safe HTML.
I see above that you convert the id to an Integer. That is sanitizing. But really your actions are full of conditions checking for the params to be valid?
Btw that is kind of a all-or-nothing as my case of a catch-all route receiving bad params is in the same context as routes receiving bad params and you said all controller parameters should be validated.
It’s a balance. For personal business initiatives I’d reach for very paranoid validation, but Ecto does sane things by default which f.ex. boot you to a 500.html error page when you go to an URI like /orders/show/123ABC which is a non-integer ID; you’d try to do something like this in your controller function:
Repo.get!(Order, params["id"])
…in your controller, which will of course fail. So for most people a default 500.html page is good enough error handling.
As mentioned before, you have to make sure that your catch-all doesn’t catch quite everything, only a big subset of generic requests. It’s quite reasonable to make sure the ID is integer in this case IMO. (Especially having in mind that the ID really cannot be non-integer).
Not sure where your displeasure comes from at this point.
Sorry if it sounded that way. I’m very grateful for your share of experience. Nice trick {id, rest} = Integer.parse(id, 10) then case. I’ll read more about routes, and probably will go that way.
No worries at all. And hey, I am not saying I am very happy with some boilerplate that I have to write here and there. I do it for the peace of mind, not because I love it. But we don’t live in the Star Trek times yet, where you can converse with a computer and just get everything done for you. Until then, we’ll have to pick the slack and do programming.
