Fix issue exposed when saving the cookie on every request.

[watsonmw]

Redirect routes were returning the same Result object every time and this
object could be modified by the session or by filters. The fix is to
make a copy of this Result on each request before it can be modified.

Since setting the cookie on every request is prone to race conditions,
dont do it in the default example and update the session docs to
explain the issue.

This pull request originates from this discussion:

#416

[jjlauer]

@watsonmw Can you walk me thru a detailed example of of why "result.copy()" in FilterChainEnd fixes this issue? Something like Results.redirect("/") creates a new instance, so I'm not quite sure I understand where the statement "Redirect routes were returning the same Result object every time" comes from.

[watsonmw]

@jjlauer If I understand correctly if you add this to Routes.java

router.GET().route("/.").with(Results.redirect("/"));

then the result is created only once when the route is being setup, each request that comes in gets the same result object.

I added testRedirectResultNotShared() to test this, test might be a bit nosey of the the internals of FilterChainEnd, but it fails in the old codebase.

[jjlauer]

I haven't had to dig into the filter chain much so please excuse my lack of
knowledge about it. Does results.copy() get called on requests that don't
use a static instance? I'm curious if your PR triggers an unnecessary
copy().

On Fri, Jan 22, 2016 at 5:08 PM, Mark Watson notifications@github.com
wrote:

@jjlauer https://github.com/jjlauer If I understand correctly if you
add this to Routes.java

router.GET().route("/.").with(Results.redirect("/"));

then the result is created only once when the route is being setup, each
request that comes in gets the same result object.

I added testRedirectResultNotShared() to test this, test might be a bit
nosey of the the internals of FilterChainEnd, but it fails in the old
codebase.

—
Reply to this email directly or view it on GitHub
#450 (comment).

[watsonmw]

Good point, I'll check around some more, to see if there's any routes that create instances on the fly.

[watsonmw]

I guess the only way a copy would be unnecessary is if you don't ever add a header or cookie (or change anything else in a filter), which is the typical case to be fair. We could do copy on write semantics for Result, but sounds like something which should fall out of doing optimisation work.

[watsonmw]

meaning the typical case is you dont need a copy, hence why this issue hasn't come up till now.

[jfendler]

Why not have a separate method like redirectWithNewResult(String) to use in this case? The existing codebase wouldn't change and no unnecessary copies would be created unless this new method is explicitly used.

[watsonmw]

The problem is the issue can cause a web user to accidentally steal an others authentication token, if during a redirect cookies are saved. It's fairly easy for a web developer to get into this situation. I think by default ninja should try really hard to avoid this issue. Besides a special method to get around the problem is not needed as you can do it currently by specifying a controller method to do the redirect, like so:

router.GET().route("/.*").with(RedirectController.class, "redirect");

public class RedirectController {
    public Result redirect() {
       return Results.redirect("/");
    }
}

[rrarrarra]

I am a bit afraid using ninja-framework when reading that it could be that easy to "accidentally" run into such a major security issue as of "can cause a web user to accidentally steal an others authentication token".

I did not read the whole previous posts in every detail. So my question is as follows:

Does this described problem ONLY occur, if "Results.redirect("/")" is used in the "Routes.java" like the following line-of-code represents?

router.GET().route("/.").with(Results.redirect("/"));

Or can this problem generally occur, by setting cookie-data within a "ninja.Filter" (and NOT using this one line router.GET().route("/.").with(Results.redirect("/"));)?

@watsonmw by saying that "It's fairly easy for a web developer to get into this situation.", could you please describe and summarize in short form what a web-developer "should-not-do" to not run into this problem.

Thanks a lot.

[watsonmw]

To run into this issue, both of the following must be true:

1. Use a Route.with(Result) that sends the same result object every time. Eg.

   router.GET().route("/.*").with(Results.redirect("/"));

2. Set a cookie on that exact request. Either in a filter or using application.session.send_only_if_changed = false and setting a session expiry time.

Then to reproduce, make a request that hits the redirect once as a logged in user and then in a logged out session.

[raphaelbauer]

Something is strange here... I got the very same question as @jjlauer : "Redirect routes were returning the same Result object every time" does currently not make much sense to me. Redirect.results creates a new result... and the copy also makes no sense to me right now... That new result does not set any cookies... so nothing will change...

Is there a testcase that tests against the bug?

(Should not sound snappy :) I think it is very important to find the root cause of those seemingly strange issues - so good work :) )

[watsonmw]

Like a integration/Selenium test? I added a unit test above, and it does illustrate the issue I think. Can of course add an integration test if that would be useful.

Anyway, here's the manual steps:

• Start with the jpa-blog-integration test, and make the following modifications

  • In Routes.java, remove the catch all route and replace with a redirect:

    -    router.GET().route("/.*").with(ApplicationController.class, "index");
    +    router.GET().route("/").with(ApplicationController.class, "index");
    +    router.GET().route("/.*").with(Results.redirect("/"));
    

  • Make sure the following config is set in application.conf:

     application.session.expire_time_in_seconds=3600
     application.session.send_only_if_changed=false
    

application.session.expire_time_in_seconds is needed to force the cookie to be saved, application.session.send_only_if_changed=false on it's own won't do it.

• Launch the app and open a browser
• Login as bob@gmail.com
• Go to localhost:8080/notaurl, to trigger the redirect (the cookie now gets added to the single redirect result)
• Open a new window incognito to get a fresh set of cookies
• Go to localhost:8080/notaurl, now you are logged in a bob@gmail.com (due to the cookie getting added to the redirect request).

[jjlauer]

Another question is if the .with() method accepting a Result is a bad
idea/practice in general. It seems like its the root cause of eventually
problematic programming/implementations.

On Mon, Jan 25, 2016 at 11:09 AM, Mark Watson notifications@github.com
wrote:

Like a integration/Selenium test? I added a unit test above, and it does
illustrate the issue I think. Can of course add an integration test if that
would be useful.

Anyway, here's the manual steps:

Start with the jpa-blog-integration test, and make the following
modifications

  In Routes.java, remove the catch all route and replace with a
  redirect:

  -     router.GET().route("/.*").with(ApplicationController.class, "index");
  +    router.GET().route("/").with(ApplicationController.class, "index");
  +    router.GET().route("/.*").with(Results.redirect("/"));

  -

  Make sure the following config is set in application.conf:

   application.session.expire_time_in_seconds=3600
   application.session.send_only_if_changed=false

application.session.expire_time_in_seconds is needed to force the cookie
to be saved, application.session.send_only_if_changed=false on it's own
won't do it.

Launch the app and open a browser

Login as bob@gmail.com

Go to localhost:8080/notaurl, to trigger the redirect (the cookie now
gets added to the single redirect result)

Open a new window incognito to get a fresh set of cookies

Go to localhost:8080/notaurl, now you are logged in a bob@gmail.com
(due to the cookie getting added to the redirect request).

—
Reply to this email directly or view it on GitHub
#450 (comment).

[danielsawan]

Is there another way to do redirection in Routes.java file ?
The problem with

router.GET().route("/.*").with(ApplicationController.class, "index");

is that it don't change the url path and you can have a "/wrongurl" that show the exact same result than / which is very bad for SEO etc..

I can understand that :

router.GET().route("/.*").with(Results.redirect("/"));

is a bad practice but Ninja actually allow this.

Another thing that surprise me is that we are talking about Ninja which having, in a certain condition, a bug which grant user session to a random visitor. I mean it can't be more important security breach than that. This bug shouldn't be the most important thing to fix right now ?

And the most important thing : there are peoples right now having Ninja in production and maybe having this use case occurred without even knowing it.

I was expecting more implication in that issue from Ninja's creators / moderator. Fixing this should be the priority and a hot patch with new version release should be already released since i posted a test case to prove that actually there is a bug 7 days ago (cf : #416)

[jjlauer]

Daniel,

A very simple workaround that doesn't leave a weird url and avoid the issue
entirely in the current version...

router.GET().route("/.*").with(ApplicationController.class, "redirector");

In ApplicationController

public Result redirector() {
return Results.redirect("/");
}

This is very much an edge case issue that we're happy to resolve as fast as
possible, but it needs to be solved correctly with a good design. That's
why we're all debating the best steps forward. In the meantime, use my
workaround.

-Joe

On Tue, Jan 26, 2016 at 12:00 AM, Daniel SAWAN notifications@github.com
wrote:

Is there another way to do redirection in Routes.java file ?
The problem with

router.GET().route("/.*").with(ApplicationController.class, "index");

is that it don't change the url path and you can have a "/wrongurl" that
show the exact same result than / which is very bad for SEO etc..

I can understand that :

router.GET().route("/.*").with(Results.redirect("/"));

is a bad practice but Ninja actually allow this.

Another thing that surprise me is that we are talking about Ninja which
having, in a certain condition, a bug which grant user session to a random
visitor. I mean it can't be more important security breach than that. This
bug shouldn't be the most important thing to fix right now ?

And the most important thing : there are peoples right now having Ninja in
production and maybe having this use case occurred without even knowing it.

I was expecting more implication in that issue from Ninja's creators /
moderator. Fixing this should be the priority and a hot patch with new
version release should be already released since i posted a test case to
prove that actually there is a bug 7 days ago (cf : #416
#416)

—
Reply to this email directly or view it on GitHub
#450 (comment).

[jjlauer]

Mark & Raphael,

I'm leaning more and more towards deprecating .with() in Routes.java.
Mark, while I think your solution fixes it, it seems like accepting an
essentially singleton result is prone to other unforeseen issues / poor
design down the road. In Java 8, I'd say replace it with a closure so a
new Result is returned each time. But until we get to Java 8, I'd say
deprecate it & throw runtime exception with a detailed message of the
workaround I just sent Daniel's way earlier.

What do you guys think?

-Joe

On Tue, Jan 26, 2016 at 12:07 AM, Joe Lauer joe@lauer.bz wrote:

Daniel,

A very simple workaround that doesn't leave a weird url and avoid the
issue entirely in the current version...

router.GET().route("/.*").with(ApplicationController.class, "redirector");

In ApplicationController

public Result redirector() {
return Results.redirect("/");
}

This is very much an edge case issue that we're happy to resolve as fast
as possible, but it needs to be solved correctly with a good design.
That's why we're all debating the best steps forward. In the meantime, use
my workaround.

-Joe

On Tue, Jan 26, 2016 at 12:00 AM, Daniel SAWAN notifications@github.com
wrote:

Is there another way to do redirection in Routes.java file ?
The problem with

router.GET().route("/.*").with(ApplicationController.class, "index");

is that it don't change the url path and you can have a "/wrongurl" that
show the exact same result than / which is very bad for SEO etc..

I can understand that :

router.GET().route("/.*").with(Results.redirect("/"));

is a bad practice but Ninja actually allow this.

Another thing that surprise me is that we are talking about Ninja which
having, in a certain condition, a bug which grant user session to a random
visitor. I mean it can't be more important security breach than that. This
bug shouldn't be the most important thing to fix right now ?

And the most important thing : there are peoples right now having Ninja
in production and maybe having this use case occurred without even knowing
it.

I was expecting more implication in that issue from Ninja's creators /
moderator. Fixing this should be the priority and a hot patch with new
version release should be already released since i posted a test case to
prove that actually there is a bug 7 days ago (cf : #416
#416)

—
Reply to this email directly or view it on GitHub
#450 (comment)
.

[danielsawan]

Thanks for this tips @jjlauer i will try this.
I understand that it is complicated and you are actually talking about the best way to solve this. But maybe for this sort of security issue we should think differently. If the door of my house can’t close anymore I will try to use cardboard as temporary solution and then I get more time to buy and install my new door. But I will never sleep with an opened door. But it is now too late for a quick fix.So let’s go for the final one ;)

This is only my point of view but you get the idea…

I promise that I stop polluting this request anymore and let you work :)

[watsonmw]

I agree the Result copy is definitely a short term solution, and deprecating is a good idea.

I'm 50/50 on throwing an exception when initializating ninja .with(Result) routes, it's a bit unfriendly but could help with issues if a user is doing something funny with the result object.

[jjlauer]

Alright,

Let's take a middle road approach. Here's what I think:

deprecate .with() accepting a Result object
if it's used, output a logger warning about how this feature will be
removed in the future due to potential race conditions with Results
keep the .copy() you implemented and unit tests
add some comments around the .copy() and .with() method about how that code
should actually be deleted in a future version

Won't break any existing apps, but should trigger enough warnings for now
to hopefully get folks to stop using it.

-Joe

On Tue, Jan 26, 2016 at 1:16 AM, Mark Watson notifications@github.com
wrote:

I agree the Result copy is definitely a short term solution, and
deprecating is good idea.

I'm 50/50 on throwing an exception when initializating ninja .with(Result)
routes, it's a bit unfriendly but could help with issues if a user is doing
something funny with the result object.

—
Reply to this email directly or view it on GitHub
#450 (comment).

[raphaelbauer]

Hey... Sorry, I was a bit busy... I'll catch up with the discussion and the problem today fwiw...

[raphaelbauer]

Wow. What a bug. Very good catch! Big compliments to everyone involved. It's very hard to find and isolate those kind of issues.

Looking through the code I also can't see any good way how to make ..with(Result) work in a side-effect-free way. The obvious solution is to use Closures of Java 8 ()-> Result, later when we support Java8 officially...

I think @jjlauer's approach sounds good. But if you want we can even take the shortcut and kill with(Result) immediately and intentionally break projects that use it. Better to break things as soon as possible than too late.

[jjlauer]

I was digging into the code and I'm curious if there is a better way to
solve this.

It seems like the only reason Results cannot be reused is that
Context.finalizeHeaders() first saves sessions and flash to the Result
first in order for them to be written out to the underlying stream. I
think this could be refactored so that instead, they are written out to the
Context and we leave the Result untouched. Sessions and flash scopes were
always originally in Context -- which is definitely a new instance for each
request. I think there's just a small mixing in of writing things to
Result that could be refactored to write to the Context instead.

https://github.com/ninjaframework/ninja/blob/develop/ninja-core/src/main/java/ninja/utils/AbstractContext.java#L306

https://github.com/ninjaframework/ninja/blob/develop/ninja-servlet/src/main/java/ninja/servlet/NinjaServletContext.java#L361

The save() only seems to be there to write out the session and flash scopes
to the headers array to be then written out to the response stream. Seems
like we could tweak this code to write/merge headers to the context or even
a new final array first or back to the Context and then write it to the
response stream.

I think Results could be reused then -- albeit carefully. We could also
think of introducing single-use or reusable Results that get invalidated
once finalizeHeaders() is called.

What do you guys think?

On Thu, Jan 28, 2016 at 9:12 AM, Raphael A. Bauer notifications@github.com
wrote:

Wow. What a bug. Very good catch! Big compliments to everyone involved.
It's very hard to find and isolate those kind of issues.

Looking through the code I also can't see any good way how to make
..with(Result) work in a side-effect-free way. The obvious solution is to
use Closures of Java 8 ()-> Result, later when we support Java8
officially...

I think @jjlauer https://github.com/jjlauer's approach sounds good. But
if you want we can even take the shortcut and kill with(Result) immediately
and intentionally break projects that use it. Better to break things as
soon as possible than too late.

—
Reply to this email directly or view it on GitHub
#450 (comment).

[raphaelbauer]

That sounds good (and also why does the method return null when it should return ResponseStreams...). At the same time the root of the problem is not solved. If we eg at some point decide to establish global filters, then we run into the same issue. I'd favour deleting the with(Result) method and adding a testcase to make sure that issue does not resurface again... But I am open to suggestions...

[watsonmw]

Ok! I removed RouteBuilder.with(Result) style routes. I guess we need to add an explanation to the changelog.md or somewhere else for the breaking change?

(Git really hates me tonight BTW, sorry about the crazy commit log)

[raphaelbauer]

Looks good. There is a file called upgrade_guide.md where those breaking changes are described.

[raphaelbauer]

Another take on the issue. Regarding closures. Well. We can simply define an interface

interface ResultCreator {
    Result creater();
}

and use that like

router.GET().route("/.").with(new ResultCreator { Result create() { return Results.redirect("/")}});

That should work. Looks ugly. But will look less ugly in Java8 because it's a functional interface...

router.GET().route("/.").with(() ->Results.redirect("/")); // Java8 

Just an idea. I think it's more important to kill the with(new Result) first... We can add the closure in another Pr...

[raphaelbauer]

When this PR is finished and @jjlauer agrees I will cut a new release... (maybe even today)

[jjlauer]

@raphaelbauer Based on the unit test I submitted with #460 on -- I believe the root cause is fixed by #459. I'd suggest doing a release as soon as you can and we can circle back on this PR next week.

[raphaelbauer]

Yes. Will do that now.

@ALL Joe fixed the issue by making sure that cookies are never written to the result, but only to the context. That way the session / flash cookies are never written to the single instance of the Result that caused trouble. Joe also added a unit test to make sure the bug is gone.

We can therefore also keep with(Result result).

I'll close this PR then. That does not mean that this PR was useless. Quite the contrary - it lead to a very good solution. Thanks to everyone involved. Great job!

[jjlauer]

Mark -- kudos on digging into this. You rock! Daniel -- appreciate your
persistence working thru it.

On Fri, Jan 29, 2016 at 5:18 PM, Raphael A. Bauer notifications@github.com
wrote:

Yes. Will do that now.

@ALL https://github.com/all Joe fixed the issue by making sure that
cookies are never written to the result, but only to the context. That way
the session / flash cookies are never written to the single instance of the
Result that caused trouble. Joe also added a unit test to make sure the bug
is gone.

We can therefore also keep with(Result result).

I'll close this PR then. That does not mean that this PR was useless.
Quite the contrary - it lead to a very good solution. Thanks to everyone
involved. Great job!

—
Reply to this email directly or view it on GitHub
#450 (comment).

[watsonmw]

No problem, glad to get to the bottom of it. I'll put together another pull request to cover the other changes to the docs and examples.

[danielsawan]

Thanks for the bug fix :) I will test this asap !