Bug 9102

To test, use curl

Before the patch

% curl -I http://192.168.2.135                                    
HTTP/1.1 200 OK
Date: Sun, 18 Nov 2012 06:56:49 GMT
Server: Apache/2.2.22 (Ubuntu)
Pragma: no-cache
Cache-control: no-cache
Content-script-type: text/javascript
Content-style-type: text/css
Set-Cookie: CGISESSID=19689f6e7d8ec94c25269fecebf2f009; path=/
Vary: Accept-Encoding
Content-Type: text/html; charset=UTF-8

After patch

 % curl -I http://192.168.2.135                                    
HTTP/1.1 200 OK
Date: Sun, 18 Nov 2012 07:01:04 GMT
Server: Apache/2.2.22 (Ubuntu)
Pragma: no-cache
Cache-control: no-cache
Content-script-type: text/javascript
Content-style-type: text/css
Set-Cookie: CGISESSID=da25baf03c0bc1e2c512a627028e43e6; path=/; HttpOnly
Vary: Accept-Encoding
Content-Type: text/html; charset=UTF-8

The test works as expected, but can we get a resubmission without tab indentation on the affected lines?

Hi Chris,

There are 2 other occurrences of this kind in C4::Auth::check_api_auth:

l.1141 my $cookie = $query->cookie( CGISESSID => $session->id );
l.1185 my $cookie = $query->cookie(CGISESSID => $sessionID);

Don't you think we have to add this flag for them too?

Not sure about that Jonathan, since those ones are only used by the API, not rendered in a page. Possibly users of the API might want to interact with the cookie with javascript? More likely, since they wont be interacting with it with a browser that understands the httponly flag it will be ignored.

We could add the flag just in case a user is tricked into going to a page from the api, that has been compromised and has xss in it.

Maybe send a follow up, It can't really hurt to have it in it I think.

(In reply to comment #8)
> Bug 9102 : Followup Set HttpOnly on the CGISESSID cookie

Is there a way to test the follow-up like their was for Chris's patch?

(In reply to comment #9)
> (In reply to comment #8)
> > Bug 9102 : Followup Set HttpOnly on the CGISESSID cookie
> 
> Is there a way to test the follow-up like their was for Chris's patch?

I don't know...
I think if you don't have any error on some pages the patch works.
But if we have a doubt about this patch, we don't have to integrate it.

I think we should QA the first patch and push it, the second one should be safe and essentially a no-op but is hard to test. So if we could push the first one and then put this back to needs sign off

The first patch has been pushed to master.

I've spent quite a bit of time reviewing the second patch.  I'm about ready to sign off on it, but I've identified some JavaScript related to tags that expects to read the CGISESSID cookie.  Fortunately, it doesn't actually need to in order to work, but I want to remove references to it.

The offending bits of JavaScript are contained in three files:

koha-tmpl/intranet-tmpl/prog/en/modules/tags/review.tt
koha-tmpl/opac-tmpl/prog/en/js/tags.js
koha-tmpl/opac-tmpl/ccsr/en/js/tags.js

Note for QA/RM - this patch stands alone, but I recommend that the patch be tested and pushed at the same time as the patch for bug 9401.

Created attachment 14994 [details] [review]
Bug 9102 : Followup Set HttpOnly on the CGISESSID cookie

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Tested. See the HttpOnly flag in Firebug. Checked cookie processing in IE9 and FF with opac language and session. Also added an item without problems.
Code looks good to me.

Passed QA

Note for RM: Will take a look too at the related report of Galen.

This patch has been pushed to master.