ContinuingHttpServletRequest: Make cookies immutable

[GoogleCodeExporter]

Description of the issue:

Cookies are lost in Ccontinued background thread in ontinuingHttpServletRequest.

The whole purpose of request scope propagation is to make a
snapshot of the request.  To the snapshot belong all attributes
and cookies. Cookies are mutable and were erroneously not frozen.

Note this TODO comment in the corresponding code:

// TODO(dhanji): Cookies themselves are mutable. Is this a problem?

This breaks web session management in Gerrit Code review project.

Steps to reproduce: [1]

[1] https://gerrit-review.googlesource.com/57284

Original issue reported on code.google.com by David.Os...@gmail.com on 20 May 2014 at 5:39

Attachments:

• 0001-Guice-Servlet-Fix-request-propagation-problem.patch

[GoogleCodeExporter]

I'll take a look.  FYI, if possible you should prefer using 'transferRequest' 
instead of 'continueRequest'.  It's usually better.

Original comment by sberlin on 20 May 2014 at 1:09

[GoogleCodeExporter]

Thanks. The provided patch is missing null pointer check in constructor:

  if (super.getCookies() != null) {
    cookies = super.getCookies().clone();
  }

In our use case the cookies exist only when ContinuingHttpServletRequest
object is created. Later in background thread no cookies available any more on 
the underlying wrapper HTTP request.

Cloning and freezing the cookies in constructor of ContinuingHttpServletRequest 
solves that.

Original comment by David.Os...@gmail.com on 20 May 2014 at 1:38

[GoogleCodeExporter]

Hi Sam I've got a better patch for this in the works if you can wait a day or 
two?

Original comment by dha...@gmail.com on 20 May 2014 at 5:02

[GoogleCodeExporter]

Hi this patch doesn't address the problem with isolating mutable cookies. I 
have a better patch I. The works, plz hold off for a little I'll have it in 
soon.

Original comment by dha...@gmail.com on 20 May 2014 at 5:03

[GoogleCodeExporter]

Sure -- it'd be great to have you actively working on this again, Dhanji!

Original comment by sa...@google.com on 20 May 2014 at 5:05

[GoogleCodeExporter]

@all

Please review here:
https://code.google.com/p/google-guice/source/detail?r=65f9d43bba9c447f01a3dcd1e
2ebb5435b061324&name=continue_request_immutable_cookies

This is on a branch.

Original comment by dha...@gmail.com on 21 May 2014 at 3:30

• Changed state: Accepted

[GoogleCodeExporter]

Actually, I lie, this is the correct commit: 
https://code.google.com/p/google-guice/source/detail?r=3b02622e856e845f9bc5353a4
94b5ccc75dec40c&name=continue_request_immutable_cookies

Please disregard the earlier one.

Original comment by dha...@gmail.com on 21 May 2014 at 3:44

[GoogleCodeExporter]

Thanks for the quick fix! I can confirm that this fix the original problem. 
Until a new version of Guice is release (new Beta?) i have uploaded a change to 
build Gerrit Code Review against Guice snapshot 4.0 [1].

[1] https://gerrit-review.googlesource.com/#/c/57306

Original comment by David.Os...@gmail.com on 21 May 2014 at 6:54

[GoogleCodeExporter]

Great, good to know. I'll merge as soon as it goes through code review.

Sam is your man for release questions.

Original comment by dha...@gmail.com on 21 May 2014 at 7:12

• Changed state: Started

[GoogleCodeExporter]

Any chance to release a new beta version of Guice with this patch included? We 
have to maintain our own fork of Guice for Gerrit Code Review to work on master 
[1]. Thanks.

[1] https://groups.google.com/forum/#!topic/repo-discuss/-ncxUgnxnrs

Original comment by David.Os...@gmail.com on 27 May 2014 at 12:13

[GoogleCodeExporter]

Any ETA for new beta containing this fix?

Original comment by David.Os...@gmail.com on 13 Jun 2014 at 6:00

[GoogleCodeExporter]

Seems that the commit that fixes this issue (3b02622e856e) is contained in 
4.0-beta5:

git tag --contains 3b02622e856e
4.0-beta5

Original comment by adrian.g...@sap.com on 1 Oct 2014 at 8:52

[GoogleCodeExporter]

Yes. I've uploaded the update change to switch to this released version.

Original comment by David.Os...@gmail.com on 1 Oct 2014 at 12:09

[GoogleCodeExporter]

The code.google.com guice project has migrated to GitHub.  This issue site is 
no longer being used.  Please use https://github.com/google/guice/issues/806 
instead.

Original comment by sberlin on 1 Oct 2014 at 1:41

• Changed state: MigratedToGitHub