Bug 9401 - Javascript used for tags handling wants access to CGISESSID cookie
Summary: Javascript used for tags handling wants access to CGISESSID cookie
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Target Milestone: ---
Assignee: Galen Charlton
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 9102
  Show dependency treegraph
 
Reported: 2013-01-16 05:07 UTC by Galen Charlton
Modified: 2014-05-26 21:04 UTC (History)
1 user (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
bug 9401: remove direct reads of CGISESSID cookie by JavaScript (8.79 KB, patch)
2013-01-16 05:55 UTC, Galen Charlton
Details | Diff | Splinter Review
bug 9401: remove direct reads of CGISESSID cookie by JavaScript (8.94 KB, patch)
2013-02-01 10:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Galen Charlton 2013-01-16 05:07:19 UTC
Some Javascript used for processing bib record tags in the OPAC and staff interface wants to read the value of the CGISESSID cookie.  If it actually needed to, that would be a blocker to making the session cookie be httpOnly (see bug 9102).  Fortunately, as it doesn't actually need to know the session key, it suffices to remove the offending code.
Comment 1 Galen Charlton 2013-01-16 05:55:10 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2013-02-01 10:02:39 UTC
Created attachment 14998 [details] [review]
bug 9401: remove direct reads of CGISESSID cookie by JavaScript

Having embedded JavaScript read the session cookie directly
is unnecessary and prevents the CGISESSID cookie being marked
httpOnly as a security measure.  The only Koha JS attempting
this was the AJAX tags code.

To test:

- In general, verify that there are no regression withs
  adding tags in the OPAC or reviewing them in the staff interface.
- In specific, for the OPAC
  - log into the OPAC
  - retrieve a bib record
  - add a tag
  - refresh the bib details page to verify that the
    tag was added
  - make sure the TagsInputOnList syspref is on
  - perform a search
  - add a tag to more than one record from the search results page
  - repeat the preceding using the CCSR theme
- And in the staff interface
  - Go to the review tags tool
  - Reject a tag
  - Refresh to verify that the tag was rejected

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2013-02-01 10:12:46 UTC
QA Comment:
Welcome addition to the cookies HTTPOnly patch!
Tested adding tags, accepting and rejecting (toggling TagsModeration as well).
Works fine still. Note that the session parameter from the javascript already was a plan-B-measure in the perl code, but it should not be. This patch nicely removes that.
Code looks good to me. 
Just one remark (had to find something :)
Line 279 (and below) still mentions CGISESSID as ajax post parameter in comments.. (opac-tags.pl)

Passed QA
Comment 4 Jared Camins-Esakov 2013-02-01 16:08:19 UTC
This patch has been pushed to Master. Tagging worked for me both before and after applying this patch on top of the patch for bug 9102. I am not entirely sure why. However, since it worked after, I am pushing the patch anyway since it removes unnecessary cruft.
Comment 5 Marcel de Rooy 2013-02-03 17:56:58 UTC
(In reply to comment #4)
> This patch has been pushed to Master. Tagging worked for me both before and
> after applying this patch on top of the patch for bug 9102. I am not
> entirely sure why. However, since it worked after, I am pushing the patch
> anyway since it removes unnecessary cruft.

Hi Jared
Tagging worked since the session id from javascript was only plan-B in the script.