Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 590 - Implement CSRF protection #594

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ There's a frood who really knows where his towel is.
1.1b2 (unreleased)
^^^^^^^^^^^^^^^^^^

- Implement CSRF protection (closes `#590`_).
[rodfersou]

- Handle `AssertionError` on upgrade step to profile 13 to avoid failures when a cover object has duplicated tiles on it.
Now, an error message will be logged and the object will be skipped;
you must manually remove the duplicated tiles (closes #619).
Expand Down Expand Up @@ -86,4 +89,5 @@ Previous entries can be found in the HISTORY.rst file.
.. _`#578`: https://github.com/collective/collective.cover/issues/578
.. _`#581`: https://github.com/collective/collective.cover/issues/581
.. _`#584`: https://github.com/collective/collective.cover/issues/584
.. _`#590`: https://github.com/collective/collective.cover/issues/590
.. _`#608`: https://github.com/collective/collective.cover/issues/608
3 changes: 3 additions & 0 deletions buildout.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ parts +=
rebuild_i18n-sh
robot

test-eggs +=
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rodfersou this should be added on a version basis also or we will end with the package installed in Plone 5

plone4.csrffixes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hvelarde should I run this just in 4.3 version? If so, I need to figure out when CSRF is enabled into RF test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rodfersou no, we have to test this in all Plone versions supported by the package; figuring out how is the hard part.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, then I'll replicate the pins into all versions 4.x files

[checkversions]
recipe = zc.recipe.egg
eggs = z3c.checkversions [buildout]
Expand Down
25 changes: 25 additions & 0 deletions src/collective/cover/browser/templates/compose.pt
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,31 @@
tal:replace="structure layout/render_compose" />
<div class="content-contentchooser"
tal:replace="structure context/@@select-content-contentchooser" />
<tal:comment tal:replace="nothing">

FIXME
BBB
plone4.csrffixes automatically adds, to all AJAX calls,
a X-CSRF-TOKEN so plone.protect will automatically add
CSRF protection without configuration, because from
plone.protect 3.0.x onwards it checks for this token.

But for Plone 5 (that doesn't use
plone4.csrffixes for obvious reasons) this token isn't
set, so you need to add a form to all your templates
and an _authenticator variable to all your requests in
your javascripts.

If this issue of adding this behavior of setting the
token in

https://github.com/plone/plone.protect/issues/42

is solved, these forms and all _authenticator variables
can be removed from code.

</tal:comment>
<form id="cover-compose-form" method="POST"></form>
</metal:main>

</body>
Expand Down
26 changes: 25 additions & 1 deletion src/collective/cover/browser/templates/layoutedit.pt
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,31 @@
<div id="export-layout" class="modal hide" tal:condition="can_export_layout">
<h1 class="documentFirstHeading">Export Layout</h1>
<p class="discreen">Save a copy of this layout under a new name</p>
<form action="." tal:attributes="action request/URL" method="post"
<tal:comment tal:replace="nothing">

FIXME
BBB
plone4.csrffixes automatically adds, to all AJAX calls,
a X-CSRF-TOKEN so plone.protect will automatically add
CSRF protection without configuration, because from
plone.protect 3.0.x onwards it checks for this token.

But for Plone 5 (that doesn't use
plone4.csrffixes for obvious reasons) this token isn't
set, so you need to add a form to all your templates
and an _authenticator variable to all your requests in
your javascripts.

If this issue of adding this behavior of setting the
token in

https://github.com/plone/plone.protect/issues/42

is solved, these forms and all _authenticator variables
can be removed from code.

</tal:comment>
<form id="cover-layout-form" tal:attributes="action request/URL" method="post"
enctype="multipart/form-data">
<div class="modal-fields">
<label for="layout-name" i18n:translate="">Layout name</label>
Expand Down
4 changes: 4 additions & 0 deletions src/collective/cover/static/js/compose.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,11 @@ function removeObjFromTile() {
tile.find('.loading-mask').addClass('show remove-tile');
var tile_type = tile.attr("data-tile-type");
var tile_id = tile.attr("id");
var authenticator = $('#cover-compose-form input[name="_authenticator"]').val();
$.ajax({
url: "@@removeitemfromlisttile",
data: {
'_authenticator': authenticator,
'tile-type': tile_type,
'tile-id': tile_id,
'uuid': uuid
Expand Down Expand Up @@ -73,9 +75,11 @@ $(document).ready(function() {
var tile = $(this).closest('.tile');
var tile_type = tile.attr("data-tile-type");
var tile_id = tile.attr("id");
var authenticator = $('#cover-compose-form input[name="_authenticator"]').val();
$.ajax({
url: "@@updatelisttilecontent",
data: {
'_authenticator': authenticator,
'tile-type': tile_type,
'tile-id': tile_id,
'uuids': uuids
Expand Down
13 changes: 13 additions & 0 deletions src/collective/cover/static/js/contentchooser.js
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,9 @@ var coveractions = {
var url = $("#contentchooser-content-search-button").attr("data-url");
var queryVal = $("#contentchooser-content-search-input").val();
var nextpage = parseInt($ul.attr('data-nextpage'), 10);
var authenticator = $('#cover-compose-form input[name="_authenticator"]').val();
var data = {
'_authenticator': authenticator,
'q': queryVal,
'page': nextpage
};
Expand Down Expand Up @@ -263,7 +265,9 @@ var coveractions = {
timeoutIDs = [];
$('#ajax-spinner').hide();
var timeoutID = setTimeout(function() {
var authenticator = $('#cover-compose-form input[name="_authenticator"]').val();
var data = {
'_authenticator': authenticator,
'q': queryVal,
'page': page
};
Expand Down Expand Up @@ -329,8 +333,12 @@ var coveractions = {

$(document).on("click", "#recent .contentchooser-clear", function(e) {
$(e.currentTarget).prev().children("input").val("");
var authenticator = $('#cover-compose-form input[name="_authenticator"]').val();
ajaxSearchRequest.push($.ajax({
url: portal_url + "/@@content-search",
data: {
'_authenticator': authenticator
},
success: function(info) {
$("#contentchooser-content-search #recent .item-list").html(info);
$('#recent .filter-count').text("");
Expand Down Expand Up @@ -414,6 +422,7 @@ var coveractions = {
var origin_has_subitem = $origin.attr('data-has-subitem') === 'True';
var draggable_uuid = $draggable.attr('data-content-uuid');
var draggable_type = $draggable.attr('data-content-type');
var authenticator = $('#cover-compose-form input[name="_authenticator"]').val();
if (target_id === draggable_id) {
return false;
}
Expand All @@ -426,6 +435,7 @@ var coveractions = {
$.ajax({
url: "@@removeitemfromlisttile",
data: {
'_authenticator': authenticator,
'tile-type': origin_type,
'tile-id': draggable_id,
'uuid': draggable_uuid
Expand All @@ -452,6 +462,7 @@ var coveractions = {
$.ajax({
url: '@@movetilecontent',
data: {
'_authenticator': authenticator,
'origin-type': origin_type,
'origin-id': draggable_id,
'target-type': target_type,
Expand All @@ -468,6 +479,7 @@ var coveractions = {
$.ajax({
url: '@@updatetile',
data: {
'_authenticator': authenticator,
'tile-type': origin_type,
'tile-id': draggable_id,
},
Expand Down Expand Up @@ -499,6 +511,7 @@ var coveractions = {
$.ajax({
url: '@@updatetilecontent',
data: {
'_authenticator': authenticator,
'tile-type': target_type,
'tile-id': target_id,
'uuid': draggable_uuid
Expand Down
2 changes: 2 additions & 0 deletions src/collective/cover/static/js/galleria.cover_theme.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,11 @@ var rm_button = function(data) {
tile.find('.loading-mask').addClass('show remove-tile');
var tile_type = "collective.cover.carousel";
var tile_id = tile.attr("id");
var authenticator = $('#cover-compose-form input[name="_authenticator"]').val();
$.ajax({
url: "@@removeitemfromlisttile",
data: {
'_authenticator': authenticator,
'tile-type': tile_type,
'tile-id': tile_id,
'uuid': uuid
Expand Down
2 changes: 2 additions & 0 deletions src/collective/cover/static/js/layout_base.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,15 @@ $(document).ready(function() {
event.preventDefault();
json = layout.layoutmanager().html2json(layout);
var $this = $(this);
var authenticator = $('#cover-layout-form input[name="_authenticator"]').val();
$this.removeClass(function(index, css) {
return (css.match(/\bbtn-\S+/g) || []).join(' ');
});
$this.find('span').text('Saving...');
$.ajax({
'url': '@@save_layout',
'data': {
'_authenticator': authenticator,
'cover_layout': JSON.stringify(json)
},
'type': 'POST',
Expand Down
8 changes: 7 additions & 1 deletion src/collective/cover/static/js/layout_edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -306,10 +306,12 @@
//XXX are you sure
tiles_to_delete.each(function() {
var $this = $(this);
var authenticator = $('#cover-compose-form input[name="_authenticator"]').val();

$.ajax({
url: 'deletetile',
data: {
'_authenticator': authenticator,
'tile-type': $this.data('tileType'),
'tile-id': $(this).attr('id')
},
Expand Down Expand Up @@ -502,7 +504,9 @@
e.preventDefault();
var url = $("#configure_tile").attr("action");
var data = $("#configure_tile").serialize();
data = data + '&buttons.save=Save&ajax_load=true';
var authenticator = $('#cover-compose-form input[name="_authenticator"]').val();
data += '&buttons.save=Save&ajax_load=true';
data += '&_authenticator=' + authenticator;
$.ajax({
type: 'POST',
url: url,
Expand All @@ -525,11 +529,13 @@
$(document).on("click", ".config-tile-link", function(e) {
e.preventDefault();
var url = $(this).attr("href");
var authenticator = $('#cover-compose-form input[name="_authenticator"]').val();
$('#tile-configure').modal();
$.ajax({
type: 'GET',
url: url,
data: {
'_authenticator': authenticator,
'ajax_load': true
},
success: function(data) {
Expand Down
3 changes: 3 additions & 0 deletions src/collective/cover/testing.py
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,9 @@ def setUpZope(self, app, configurationContext):
self.loadZCML(package=Products.PloneFormGen)
z2.installProduct(app, 'Products.PloneFormGen')

import plone4.csrffixes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rodfersou we can't install this under Plone 5

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hvelarde did you see the if PLONE_VERSION < '5.0': above?

self.loadZCML(package=plone4.csrffixes)

import collective.cover
self.loadZCML(package=collective.cover)

Expand Down
19 changes: 18 additions & 1 deletion src/collective/cover/tests/cover.robot
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ ${column_drop_area_selector} = div.cover-row
${tile_drop_area_selector} = div.cover-column
${tile_cancel_area_selector} = div.modal-backdrop
${delete_tile_selector} = button.close
${confirm_action_selector} = input.standalone
${CONTENT_CHOOSER_SELECTOR} = div#contentchooser-content-search

*** Keywords ***
Expand All @@ -37,9 +38,25 @@ Start Browser and Log In as Site Owner
Log In As Site Owner
Click Link link=Home

# BBB: This needs to be customized to handle both Plone 4.2 and 4.3.
# In Plone 4.2, we still have the confirm action button that it's gone in 4.3.
# Customized from
# https://github.com/plone/plone.app.robotframework/blob/9ab289c5b93c2aa1acb6078b955694ad458d9c3b/src/plone/app/robotframework/keywords.robot#L53
Go to homepage
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rodfersou add information on what are you doing here and why


# Both.
Go to ${PLONE_URL}

# Plone 4.2.
Run keyword if '${CMFPLONE_VERSION}' < '4.3' Wait Until Page Contains Element css=${confirm_action_selector}
Run keyword if '${CMFPLONE_VERSION}' < '4.3' Click Element css=${confirm_action_selector}
Run keyword if '${CMFPLONE_VERSION}' < '4.3' Page Should Contain Plone site

# Plone >= 4.3.
Run keyword if '${CMFPLONE_VERSION}' >= '4.3' Wait until location is ${PLONE_URL}

Setup Cover Test Case
Start Browser and Log In as Site Owner
Go to Homepage

Click Add Cover
Open Add New Menu
Expand Down
1 change: 1 addition & 0 deletions src/collective/cover/tests/test_banner_tile.robot
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ Test Banner Tile
# XXX: test is randomly failing under Plone 4.2 only
Run keyword if '${CMFPLONE_VERSION}' >= '4.3' Remove Tags Mandelbug

Set Library Search Order cover
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rodfersou document this keyword all over the place

Enable Autologin as Site Administrator
Go to Homepage
Create Cover Title Description
Expand Down
1 change: 1 addition & 0 deletions src/collective/cover/tests/test_basic_tile.robot
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ Test Basic Tile
# XXX: test is randomly failing under Plone 4.2 only
Run keyword if '${CMFPLONE_VERSION}' >= '4.3' Remove Tags Mandelbug

Set Library Search Order cover
Enable Autologin as Site Administrator
Go to Homepage
Create Cover Title Description
Expand Down
1 change: 1 addition & 0 deletions src/collective/cover/tests/test_carousel_tile.robot
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ Get Total Carousel Images
*** Test cases ***

Test Carousel Tile
Set Library Search Order cover
Enable Autologin as Site Administrator
Go to Homepage
Create Cover Title Description
Expand Down
1 change: 1 addition & 0 deletions src/collective/cover/tests/test_collection_tile.robot
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ Test Collection Tile
# XXX: test is randomly failing under Plone 4.2 only
Run keyword if '${CMFPLONE_VERSION}' >= '4.3' Remove Tags Mandelbug

Set Library Search Order cover
Enable Autologin as Site Administrator
Go to Homepage
Create Cover My Cover Description
Expand Down
3 changes: 3 additions & 0 deletions src/collective/cover/tests/test_contentbody_tile.robot
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ ${document_body} body.portaltype-document
*** Test cases ***

Test Content Body Tile
[Tags] issue_580

Set Library Search Order cover
Enable Autologin as Site Administrator
Go to Homepage
Create Cover My Cover Description
Expand Down
7 changes: 6 additions & 1 deletion src/collective/cover/tests/test_contentchooser.robot
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ ${tile_selector} div.tile-container div.tile
*** Test cases ***

Test Content Chooser
Set Library Search Order cover
Enable Autologin as Site Administrator
Go to Homepage
Create Cover Title Description
Expand All @@ -38,13 +39,16 @@ Test Content Chooser

# make a search on Recent items
Click Element link=Recent items
Wait Until Element Is Visible css=#contentchooser-content-search-input
Input Text css=#recent input folder
# FIXME: we have no result counter in here
#Wait Until Page Contains 1 Results
Click Element css=#recent ${contentchooser_search_clear}
Sleep 4s Wait for complete ajax call @@content-search for cleaning

Click Element link=Content tree
Wait Until Page Contains Plone site
Sleep 2s Wait for content tree to load
Wait Until Element Is Visible css=#contentchooser-content-trees

# make a search on Content tree
Input Text css=#content-trees input file
Expand All @@ -53,6 +57,7 @@ Test Content Chooser
# navigate the tree
Click Element css=#content-trees ${contentchooser_search_clear}
Input Text css=#content-trees input file
Sleep 2s Wait for content tree to load
Wait Until Page Contains 1 Results
Page Should Contain Element css=${file_selector}
# TODO: Refactor this test before https://github.com/collective/collective.cover/issues/508
Expand Down
2 changes: 2 additions & 0 deletions src/collective/cover/tests/test_cover.robot
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ Test Teardown Close all browsers
*** Test cases ***

Test CRUD
Set Library Search Order cover
Enable Autologin as Site Administrator
Go to Homepage

Expand All @@ -17,6 +18,7 @@ Test CRUD
Delete

Test renderBase
Set Library Search Order cover
Enable Autologin as Site Administrator
Goto Homepage

Expand Down
1 change: 1 addition & 0 deletions src/collective/cover/tests/test_css_class.robot
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ ${tile_class} div.cover-tile
*** Test cases ***

Test CSS Class
Set Library Search Order cover
Enable Autologin as Site Administrator
Go to Homepage

Expand Down
Loading