security vulnerability..controllers must 'return'

I got an email the other day from someone who has apparently built a vulnerability
analysis tool for rails apps.

He claims (and I have no idea whether this is true, I present this purely as a
question) that a very old project of mine still hosted on github, allows malicious
execution because controllers do not "return" at the end of each action.

According to him, a redirect_to does not halt processing and can somehow lead to
people "executing" code.

Has anyone else heard of this or received a smiliar message? I'm mainly just curious
because if it's true, it would mean revising how I personally write apps, and also how
99% of tutorials/guides are written I would think.

I got an email the other day from someone who has apparently built a vulnerability
analysis tool for rails apps.

He claims (and I have no idea whether this is true, I present this purely as a
question) that a very old project of mine still hosted on github, allows malicious
execution because controllers do not "return" at the end of each action.

According to him, a redirect_to does not halt processing and can somehow lead to
people "executing" code.

Has anyone else heard of this or received a smiliar message? I'm mainly just curious
because if it's true, it would mean revising how I personally write apps, and also how
99% of tutorials/guides are written I would think.

Well it's certainly true that redirect_to isn't a magic method in that
it doesn't affect flow of what happens afterwards ie

def some_action
  if bad_guy
    redirect_to '/'
  end
  do_something
end

would still end up running do something. I don't think this is a new
discovery though, I consider this a well know fact.
Also a lot of the time people do this sort of checking in a
before_filter, and rendering or redirecting from there will halt the
filter chain.

Fred

Yes you're correct. However from the way he put it, and the specific line numbers he
referenced in the email to files in my old project, even something like this is
dangerous:

def some_action
  if ...
    *do stuff*
    redirect_to '...'
  else
    *more stuff*
    redirect_to '...'
  end

  *somehow something here will be executed even though it doesn't exist and should
  never be reached unless the code is modified*
end

I completely agree that if you don't cover all the possible return paths, you might
get undesired results. I wish I could find the email in question but it looks like I
deleted it, because it really sounded a bit unbelievable.

I just wanted to check that there hadn't been some big development recently and that I
needed to change all my habits.

> > I got an email the other day from someone who has apparently built a vulnerability
> > analysis tool for rails apps.

> > He claims (and I have no idea whether this is true, I present this purely as a
> > question) that a very old project of mine still hosted on github, allows malicious
> > execution because controllers do not "return" at the end of each action.

> > According to him, a redirect_to does not halt processing and can somehow lead to
> > people "executing" code.

> > Has anyone else heard of this or received a smiliar message? I'm mainly just curious
> > because if it's true, it would mean revising how I personally write apps, and also how
> > 99% of tutorials/guides are written I would think.

> Well it's certainly true that redirect_to isn't a magic method in that
> it doesn't affect flow of what happens afterwards ie

> def some_action
> if bad_guy
> redirect_to '/'
> end
> do_something
> end

> would still end up running do something. I don't think this is a new
> discovery though, I consider this a well know fact.
> Also a lot of the time people do this sort of checking in a
> before_filter, and rendering or redirecting from there will halt the
> filter chain.

Yes you're correct. However from the way he put it, and the specific line numbers he
referenced in the email to files in my old project, even something like this is
dangerous:

def some_action
if ...
*do stuff*
redirect_to '...'
else
*more stuff*
redirect_to '...'
end

*somehow something here will be executed even though it doesn't exist and should
never be reached unless the code is modified*
end

I completely agree that if you don't cover all the possible return paths, you might
get undesired results. I wish I could find the email in question but it looks like I
deleted it, because it really sounded a bit unbelievable.

I just wanted to check that there hadn't been some big development recently and that I
needed to change all my habits.

Sounds like rubbish to me. Pretty much the only thing you could say is
that maybe it makes it more likely that someone will add code after
that if-else-end block without thinking but even that sounds pretty
flimsy to me

Fred

Yes you're correct. However from the way he put it, and the specific line numbers he
referenced in the email to files in my old project, even something like this is
dangerous:

How are you defining "dangerous"? As far as I can see, the worst is
that some of the application's code will be run through, which isn't
as bad as a user being able to execute their own code.

def some_action
if ...
*do stuff*
redirect_to '...'
else
*more stuff*
redirect_to '...'
end

*somehow something here will be executed even though it doesn't exist and should
never be reached unless the code is modified*
end

So if there's a situation that this could be a *problem* use an explicit return:

redirect_to '...' and return

Not exactly a huge problem IMO.