Router sends correctly 400 Bad Request but `conn` is not halted?

Hello :wave:

I have a route like the following:

post "/foo/next", FooController, :next_foo

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).

1 Like

If I do a search on :next_foo in the router file, the only line found is:

post "/foo/next", FooController, :next_foo

The request is:

GET /foo/next HTTP/1.1
Host: myapp.local:4000
Connection: keep-alive
Cache-Control: max-age=0
Upgrade-Insecure-Requests: 1
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/78.0.3904.108 Safari/537.36
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,/;q=0.8,application/signed-exchange;v=b3
Accept-Encoding: gzip, deflate
Accept-Language: en-US,en;q=0.9,nl;q=0.8,fr;q=0.7
Cookie: _my_app_key=xxx.xxxxx.xxxx

The response is:

HTTP/1.1 400 Bad Request
cache-control: max-age=0, private, must-revalidate
content-length: 56594
content-type: text/html; charset=utf-8
cross-origin-window-policy: deny
date: Mon, 02 Dec 2019 10:03:27 GMT
server: Cowboy
x-content-type-options: nosniff
x-download-options: noopen
x-frame-options: SAMEORIGIN
x-permitted-cross-domain-policies: none
x-request-id: xxxxxx
x-xss-protection: 1; mode=block

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.

1 Like

I admit those aspects of Phoenix are rusty for me so can’t help there. But I am puzzled as to why POST and GET requests can get confused…

If you have the choice, I’d opt to change your latter request to something with an unambiguous prefix, like "/show-foo/:foo_id/:foo_slug".

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?

It’s as I showed above, in that order:

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.

Well, a GET to /foo/next should not match because the route is for POST only.

You can force it in your controller function though. Try to parse it as a number and if that fails, redirect to a 404 or another error page.

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.

I don’t see it as an all-or-nothing, no.

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.

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. :stuck_out_tongue:

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. :thinking:

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. :wink:

Anything else comes up, never hesitate to ask.

Interestingly, it’s possible to do something like this in Plug.Router (which Phoenix uses to handle parsing paths with placeholders / globs):

# inside a module that does `use Plug.Router`

match "/foo/bar/:baz" when size(baz) <= 3, via: :get do
  send_resp(conn, 200, "hello world")

Weird… I get the error:

== Compilation error in file lib/foo_web/router.ex ==
** (CompileError) lib/foo_web/router.ex:91: undefined function match/3

Even though I use use FooWeb, :router. Tried even to put directly use Phoenix.Router but still got that compilation error.

Any idea?

Plug.Router != Phoenix.Router

1 Like

So I think no guard clauses possible with routes as @al2o3cr showed above. At least I cannot find ANY examples online.

For now I’ll just go for parameter validation in the controller action, or a plug.