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
>
_______________________________________________
devs mailing list
[email protected]
http://lists.xwiki.org/mailman/listinfo/devs
