XWIKI-19549: Disallow template override for login, register and skin

* Also do not put a document without view rights into the context.
xwiki · Mar 24, 2022 · 71a6d0b · 71a6d0b
1 parent 3b50f39
commit 71a6d0b
Show file tree

Hide file tree

Showing 6 changed files with 99 additions and 26 deletions.
diff --git a/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/XWiki.java b/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/XWiki.java
@@ -5917,31 +5917,41 @@ && getRightService().hasAccessLevel("view", XWikiRightService.GUEST_USER_FULLNAM
             }
         }
 
-        context.put("doc", doc);
-        context.put("cdoc", doc);
-        vcontext.put("doc", doc.newDocument(context));
-        vcontext.put("cdoc", vcontext.get("doc"));
-        XWikiDocument tdoc;
-
-        // If the parameter language exists and is empty, it means we want to force loading the regular document
-        // not a translation. This should be handled later by doing a better separation between locale used in the UI
-        // and for loading the documents.
-        if ("".equals(context.getRequest().getParameter("language"))) {
-            tdoc = doc;
+        if (!"skin".equals(context.getAction()) && !this.getRightService().hasAccessLevel("view",
+            user.getFullName(), doc.getFullName(), context)) {
+            // If for some reason (e.g., login action) the user has rights for the action but no view right on the
+            // document, do not load the document into the context.
+            setPhonyDocument(reference, context, vcontext);
+            doc = context.getDoc();
+            context.put("tdoc", doc);
+            context.put("cdoc", doc);
         } else {
-            tdoc = doc.getTranslatedDocument(context);
-        }
+            context.put("doc", doc);
+            context.put("cdoc", doc);
+            vcontext.put("doc", doc.newDocument(context));
+            vcontext.put("cdoc", vcontext.get("doc"));
+            XWikiDocument tdoc;
+
+            // If the parameter language exists and is empty, it means we want to force loading the regular document
+            // not a translation. This should be handled later by doing a better separation between locale used in the UI
+            // and for loading the documents.
+            if ("".equals(context.getRequest().getParameter("language"))) {
+                tdoc = doc;
+            } else {
+                tdoc = doc.getTranslatedDocument(context);
+            }
 
-        try {
-            String rev = (String) context.get("rev");
-            if (StringUtils.isNotEmpty(rev)) {
-                tdoc = getDocument(tdoc, rev, context);
+            try {
+                String rev = (String) context.get("rev");
+                if (StringUtils.isNotEmpty(rev)) {
+                    tdoc = getDocument(tdoc, rev, context);
+                }
+            } catch (Exception ex) {
+                // Invalid version, just use the most recent one
             }
-        } catch (Exception ex) {
-            // Invalid version, just use the most recent one
+            context.put("tdoc", tdoc);
+            vcontext.put("tdoc", tdoc.newDocument(context));
         }
-        context.put("tdoc", tdoc);
-        vcontext.put("tdoc", tdoc.newDocument(context));
 
         return true;
     }

diff --git a/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/LoginAction.java b/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/LoginAction.java
@@ -37,6 +37,8 @@
 @Singleton
 public class LoginAction extends XWikiAction
 {
+    private static final String LOGIN = "login";
+
     /**
      * Default constructor.
      */
@@ -45,6 +47,18 @@ public LoginAction()
         this.waitForXWikiInitialization = false;
     }
 
+    @Override
+    public boolean action(XWikiContext context) throws XWikiException
+    {
+        // Disallow template override with xpage parameter.
+        if (!LOGIN.equals(Utils.getPage(context.getRequest(), LOGIN))) {
+            throw new XWikiException(XWikiException.MODULE_XWIKI, XWikiException.ERROR_XWIKI_ACCESS_DENIED,
+                String.format("Template may not be overriden with 'xpage' in [%s] action.", LOGIN));
+        }
+
+        return super.action(context);
+    }
+
     @Override
     public String render(XWikiContext context) throws XWikiException
     {
@@ -53,6 +67,6 @@ public String render(XWikiContext context) throws XWikiException
         if (!"1".equals(context.getRequest().getParameter("loginLink"))) {
             context.getResponse().setStatus(401);
         }
-        return "login";
+        return LOGIN;
     }
 }
diff --git a/...latform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/LoginErrorAction.java b/...latform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/LoginErrorAction.java
@@ -37,9 +37,23 @@
 @Singleton
 public class LoginErrorAction extends XWikiAction
 {
+    private static final String LOGIN = "login";
+
+    @Override
+    public boolean action(XWikiContext context) throws XWikiException
+    {
+        // Disallow template override with xpage parameter.
+        if (!LOGIN.equals(Utils.getPage(context.getRequest(), LOGIN))) {
+            throw new XWikiException(XWikiException.MODULE_XWIKI, XWikiException.ERROR_XWIKI_ACCESS_DENIED,
+                String.format("Template may not be overriden with 'xpage' in [%s] action.", LOGIN));
+        }
+
+        return super.action(context);
+    }
+
     @Override
     public String render(XWikiContext context) throws XWikiException
     {
-        return "login";
+        return LOGIN;
     }
 }
diff --git a/...atform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/LoginSubmitAction.java b/...atform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/LoginSubmitAction.java
@@ -42,13 +42,27 @@
 @Singleton
 public class LoginSubmitAction extends XWikiAction
 {
+    private static final String LOGIN = "login";
+
+    @Override
+    public boolean action(XWikiContext context) throws XWikiException
+    {
+        // Disallow template override with xpage parameter.
+        if (!LOGIN.equals(Utils.getPage(context.getRequest(), LOGIN))) {
+            throw new XWikiException(XWikiException.MODULE_XWIKI, XWikiException.ERROR_XWIKI_ACCESS_DENIED,
+                String.format("Template may not be overriden with 'xpage' in [%s] action.", LOGIN));
+        }
+
+        return super.action(context);
+    }
+
     @Override
     public String render(XWikiContext context) throws XWikiException
     {
         String msg = (String) context.get("message");
         if (StringUtils.isNotBlank(msg)) {
             context.getResponse().setStatus(HttpServletResponse.SC_FORBIDDEN);
         }
-        return "login";
+        return LOGIN;
     }
 }
diff --git a/...-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/RegisterAction.java b/...-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/RegisterAction.java
@@ -62,6 +62,12 @@ public boolean action(XWikiContext context) throws XWikiException
         XWikiRequest request = context.getRequest();
         XWikiResponse response = context.getResponse();
 
+        // Disallow template override with xpage parameter.
+        if (!REGISTER.equals(Utils.getPage(context.getRequest(), REGISTER))) {
+            throw new XWikiException(XWikiException.MODULE_XWIKI, XWikiException.ERROR_XWIKI_ACCESS_DENIED,
+                String.format("Template may not be overriden with 'xpage' in [%s] action.", REGISTER));
+        }
+
         String register = request.getParameter(REGISTER);
         if (register != null && register.equals("1")) {
             // CSRF prevention

diff --git a/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/SkinAction.java b/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/SkinAction.java
@@ -66,6 +66,7 @@
 @Singleton
 public class SkinAction extends XWikiAction
 {
+
     /** Logging helper. */
     private static final Logger LOGGER = LoggerFactory.getLogger(SkinAction.class);
 
@@ -83,14 +84,28 @@ public class SkinAction extends XWikiAction
      */
     private static final String ENCODING = "UTF-8";
 
+    private static final String DOCDOESNOTEXIST = "docdoesnotexist";
+
+    @Override
+    public boolean action(XWikiContext context) throws XWikiException
+    {
+        // Disallow template override with xpage parameter.
+        if (!DOCDOESNOTEXIST.equals(Utils.getPage(context.getRequest(), DOCDOESNOTEXIST))) {
+            throw new XWikiException(XWikiException.MODULE_XWIKI, XWikiException.ERROR_XWIKI_ACCESS_DENIED,
+                "Template may not be overriden with 'xpage' in [skin] action.");
+        }
+
+        return super.action(context);
+    }
+
     @Override
     public String render(XWikiContext context) throws XWikiException
     {
         try {
             return render(context.getRequest().getPathInfo(), context);
         } catch (IOException e) {
             context.getResponse().setStatus(404);
-            return "docdoesnotexist";
+            return DOCDOESNOTEXIST;
         }
     }
 
@@ -177,7 +192,7 @@ public String render(String path, XWikiContext context) throws XWikiException, I
         }
         if (!found) {
             context.getResponse().setStatus(404);
-            return "docdoesnotexist";
+            return DOCDOESNOTEXIST;
         }
         return null;
     }