Hm, that would be better, but there is a problem with some of the actions (DeleteAction,AbstractPropChangeAction,ResetVersionsAction). The CSRF check should only be done before the actual modification, and those actions conditionally do other things, like redirecting to confirmation page if confirm=0 etc. It might be a good idea to do it the way you suggested for most of the actions and treat the rest as exceptions though. I need to take a closer look. Thanks, Alex On 09/23/2010 08:36 AM, Vincent Massol wrote:

Hi Alex,

Couldn't we add the check in some Abstract<XXX>XWikiAction where <XXX> qualifies actions that modify data in order to prevent code duplication and in order to prevent forgetting to do the check when people add new actions (at least make it easier)?

Thanks -Vincent

On Sep 22, 2010, at 11:00 PM, abusenius (SVN) wrote:

...

Author: abusenius Date: 2010-09-22 23:00:46 +0200 (Wed, 22 Sep 2010) New Revision: 31271

Modified: platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/AbstractPropChangeAction.java platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/CommentAddAction.java platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/DeleteAction.java platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/DeleteAttachmentAction.java platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/DeleteVersionsAction.java platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/ObjectAddAction.java platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/ObjectRemoveAction.java platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/PropAddAction.java platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/PropUpdateAction.java platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/RegisterAction.java platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/ResetVersionsAction.java platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/RollbackAction.java platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/SaveAction.java platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/SaveAndContinueAction.java platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/UndeleteAction.java platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/UploadAction.java Log: XWIKI-5464: Added CSRF checks to all actions that modify data

Modified: platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/AbstractPropChangeAction.java =================================================================== --- platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/AbstractPropChangeAction.java 2010-09-22 21:00:42 UTC (rev 31270) +++ platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/AbstractPropChangeAction.java 2010-09-22 21:00:46 UTC (rev 31271) @@ -55,6 +55,10 @@ BaseClass xclass = doc.getXClass();

if (propertyName != null && xclass.get(propertyName) != null) { + // CSRF prevention + if (!csrfTokenCheck(context)) { + return false; + } changePropertyDefinition(xclass, propertyName, context); } else { return true;

Modified: platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/CommentAddAction.java =================================================================== --- platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/CommentAddAction.java 2010-09-22 21:00:42 UTC (rev 31270) +++ platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/CommentAddAction.java 2010-09-22 21:00:46 UTC (rev 31271) @@ -54,6 +54,11 @@ @Override public boolean action(XWikiContext context) throws XWikiException { + // CSRF prevention + if (!csrfTokenCheck(context)) { + return false; + } + XWiki xwiki = context.getWiki(); XWikiResponse response = context.getResponse(); XWikiDocument doc = context.getDoc();

Modified: platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/DeleteAction.java =================================================================== --- platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/DeleteAction.java 2010-09-22 21:00:42 UTC (rev 31270) +++ platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/DeleteAction.java 2010-09-22 21:00:46 UTC (rev 31271) @@ -55,6 +55,10 @@ if (!"1".equals(request.getParameter(CONFIRM_PARAM))) { return true; } + // CSRF prevention + if (!csrfTokenCheck(context)) { + return false; + }

String sindex = request.getParameter("id"); if (sindex != null && xwiki.hasRecycleBin(context)) {

Modified: platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/DeleteAttachmentAction.java =================================================================== --- platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/DeleteAttachmentAction.java 2010-09-22 21:00:42 UTC (rev 31270) +++ platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/DeleteAttachmentAction.java 2010-09-22 21:00:46 UTC (rev 31271) @@ -46,6 +46,11 @@ @Override public boolean action(XWikiContext context) throws XWikiException { + // CSRF prevention + if (!csrfTokenCheck(context)) { + return false; + } + XWikiRequest request = context.getRequest(); XWikiResponse response = context.getResponse(); XWikiDocument doc = context.getDoc();

Modified: platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/DeleteVersionsAction.java =================================================================== --- platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/DeleteVersionsAction.java 2010-09-22 21:00:42 UTC (rev 31270) +++ platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/DeleteVersionsAction.java 2010-09-22 21:00:46 UTC (rev 31271) @@ -40,21 +40,24 @@ @Override public boolean action(XWikiContext context) throws XWikiException { + // CSRF prevention + if (!csrfTokenCheck(context)) { + return false; + } + DeleteVersionsForm form = (DeleteVersionsForm) context.getForm(); - if (!form.isConfirmed()) { return true; }

Version v1; Version v2; - Version rev = form.getRev(); - if (rev == null) { + if (form.getRev() == null) { v1 = form.getRev1(); v2 = form.getRev2(); } else { - v1 = rev; - v2 = rev; + v1 = form.getRev(); + v2 = form.getRev(); }

if (v1 != null && v2 != null) {

Modified: platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/ObjectAddAction.java =================================================================== --- platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/ObjectAddAction.java 2010-09-22 21:00:42 UTC (rev 31270) +++ platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/ObjectAddAction.java 2010-09-22 21:00:46 UTC (rev 31271) @@ -39,6 +39,11 @@ @Override public boolean action(XWikiContext context) throws XWikiException { + // CSRF prevention + if (!csrfTokenCheck(context)) { + return false; + } + XWiki xwiki = context.getWiki(); XWikiResponse response = context.getResponse(); String username = context.getUser();

Modified: platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/ObjectRemoveAction.java =================================================================== --- platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/ObjectRemoveAction.java 2010-09-22 21:00:42 UTC (rev 31270) +++ platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/ObjectRemoveAction.java 2010-09-22 21:00:46 UTC (rev 31271) @@ -66,6 +66,11 @@ @Override public boolean action(XWikiContext context) throws XWikiException { + // CSRF prevention + if (!csrfTokenCheck(context)) { + return false; + } + XWiki xwiki = context.getWiki(); XWikiResponse response = context.getResponse(); String username = context.getUser(); @@ -74,6 +79,7 @@ if (obj == null) { return true; } + doc.removeObject(obj); doc.setAuthor(username); xwiki.saveDocument(doc, context.getMessageTool().get("core.comment.deleteObject"), true, context);

Modified: platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/PropAddAction.java =================================================================== --- platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/PropAddAction.java 2010-09-22 21:00:42 UTC (rev 31270) +++ platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/PropAddAction.java 2010-09-22 21:00:46 UTC (rev 31271) @@ -37,6 +37,11 @@ @Override public boolean action(XWikiContext context) throws XWikiException { + // CSRF prevention + if (!csrfTokenCheck(context)) { + return false; + } + XWiki xwiki = context.getWiki(); XWikiResponse response = context.getResponse(); XWikiDocument doc = context.getDoc();

Modified: platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/PropUpdateAction.java =================================================================== --- platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/PropUpdateAction.java 2010-09-22 21:00:42 UTC (rev 31270) +++ platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/PropUpdateAction.java 2010-09-22 21:00:46 UTC (rev 31271) @@ -105,6 +105,11 @@ @Override public boolean action(XWikiContext context) throws XWikiException { + // CSRF prevention + if (!csrfTokenCheck(context)) { + return false; + } + try { if (propUpdate(context)) { return true;

Modified: platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/RegisterAction.java =================================================================== --- platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/RegisterAction.java 2010-09-22 21:00:42 UTC (rev 31270) +++ platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/RegisterAction.java 2010-09-22 21:00:46 UTC (rev 31271) @@ -50,6 +50,11 @@

String register = request.getParameter(REGISTER); if (register != null && register.equals("1")) { + // CSRF prevention + if (!csrfTokenCheck(context)) { + return false; + } + int useemail = xwiki.getXWikiPreferenceAsInt("use_email_verification", 0, context); int result; if (useemail == 1) {

Modified: platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/ResetVersionsAction.java =================================================================== --- platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/ResetVersionsAction.java 2010-09-22 21:00:42 UTC (rev 31270) +++ platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/ResetVersionsAction.java 2010-09-22 21:00:46 UTC (rev 31271) @@ -17,6 +17,11 @@

String confirm = request.getParameter("confirm"); if ((confirm != null) && (confirm.equals("1"))) { + // CSRF prevention + if (!csrfTokenCheck(context)) { + return false; + } + XWikiDocument tdoc = getTranslatedDocument(doc, language, context); // Do it tdoc.resetArchive(context);

Modified: platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/RollbackAction.java =================================================================== --- platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/RollbackAction.java 2010-09-22 21:00:42 UTC (rev 31270) +++ platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/RollbackAction.java 2010-09-22 21:00:46 UTC (rev 31271) @@ -30,6 +30,11 @@ @Override public boolean action(XWikiContext context) throws XWikiException { + // CSRF prevention + if (!csrfTokenCheck(context)) { + return false; + } + XWiki xwiki = context.getWiki(); XWikiResponse response = context.getResponse(); XWikiDocument doc = context.getDoc();

Modified: platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/SaveAction.java =================================================================== --- platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/SaveAction.java 2010-09-22 21:00:42 UTC (rev 31270) +++ platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/SaveAction.java 2010-09-22 21:00:46 UTC (rev 31271) @@ -176,6 +176,11 @@ @Override public boolean action(XWikiContext context) throws XWikiException { + // CSRF prevention + if (!csrfTokenCheck(context)) { + return false; + } + if (save(context)) { return true; }

Modified: platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/SaveAndContinueAction.java =================================================================== --- platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/SaveAndContinueAction.java 2010-09-22 21:00:42 UTC (rev 31270) +++ platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/SaveAndContinueAction.java 2010-09-22 21:00:46 UTC (rev 31271) @@ -43,6 +43,11 @@ */ @Override public boolean action(XWikiContext context) throws XWikiException { + // CSRF prevention + if (!csrfTokenCheck(context)) { + return false; + } + // Try to find the URL of the edit page which we came from String back = findBackURL(context);

Modified: platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/UndeleteAction.java =================================================================== --- platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/UndeleteAction.java 2010-09-22 21:00:42 UTC (rev 31270) +++ platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/UndeleteAction.java 2010-09-22 21:00:46 UTC (rev 31271) @@ -42,6 +42,11 @@ @Override public boolean action(XWikiContext context) throws XWikiException { + // CSRF prevention + if (!csrfTokenCheck(context)) { + return false; + } + XWiki xwiki = context.getWiki(); XWikiRequest request = context.getRequest(); XWikiResponse response = context.getResponse();

Modified: platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/UploadAction.java =================================================================== --- platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/UploadAction.java 2010-09-22 21:00:42 UTC (rev 31270) +++ platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/UploadAction.java 2010-09-22 21:00:46 UTC (rev 31271) @@ -80,6 +80,11 @@ } }

+ // CSRF prevention + if (!csrfTokenCheck(context)) { + return false; + } + XWikiDocument doc = (XWikiDocument) context.getDoc().clone();

// The document is saved for each attachment in the group.

_______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs