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.
