CodeQL fixes

.github/codeql/codeql-config.yml

@@ -0,0 +1,14 @@
+name: "Roller CodeQL config"
+
+# paths-ignore only influences interpreted languages according to the doc
+# don't scan JS files three times:
+#   - ignore test folder and source folder
+#   - target is kept to only scan what is deployed
+paths-ignore: 
+    - app/target/test-classes
+    - app/src
+
+# If you wish to specify custom queries, you can do so here or in a config file.
+# By default, queries listed here will override any specified in a config file.
+# Prefix the list here with "+" to use these queries and those in the config file.
+# queries: ./path/to/local/query, your-org/your-repo/queries@main

.github/workflows/codeql-analysis.yml

@@ -4,11 +4,6 @@
 # You may wish to alter this file to override the set of languages analyzed,
 # or to provide custom queries or build logic.
 #
-# ******** NOTE ********
-# We have attempted to detect the languages in your repository. Please check
-# the `language` matrix defined below to confirm you have the correct set of
-# supported CodeQL languages.
-#
 name: "CodeQL"
 
 on:
@@ -45,10 +40,7 @@ jobs:
       uses: github/codeql-action/init@v1
       with:
         languages: ${{ matrix.language }}
-        # If you wish to specify custom queries, you can do so here or in a config file.
-        # By default, queries listed here will override any specified in a config file.
-        # Prefix the list here with "+" to use these queries and those in the config file.
-        # queries: ./path/to/local/query, your-org/your-repo/queries@main
+        config-file: ./.github/codeql/codeql-config.yml
 
     - name: Build with Maven
       run: mvn -DskipTests=true -V -ntp install

app/src/main/java/org/apache/roller/weblogger/business/FileContentManagerImpl.java

@@ -19,11 +19,12 @@
 package org.apache.roller.weblogger.business;
 
 import java.io.File;
-import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.math.BigDecimal;
+import java.nio.file.Files;
+import java.nio.file.Path;
 
 import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.logging.Log;
@@ -42,7 +43,7 @@
  */
 public class FileContentManagerImpl implements FileContentManager {
 
-    private static Log log = LogFactory.getLog(FileContentManagerImpl.class);
+    private static final Log log = LogFactory.getLog(FileContentManagerImpl.class);
 
     private String storageDir = null;
 
@@ -102,34 +103,19 @@ public FileContent getFileContent(Weblog weblog, String fileId)
     public void saveFileContent(Weblog weblog, String fileId, InputStream is)
             throws FileNotFoundException, FilePathException, FileIOException {
 
+        checkFileName(fileId);
+
         // make sure uploads area exists for this weblog
         File dirPath = this.getRealFile(weblog, null);
 
         // create File that we are about to save
-        File saveFile = new File(dirPath.getAbsolutePath() + File.separator
-                + fileId);
+        Path saveFile = Path.of(dirPath.getAbsolutePath(), fileId);
 
-        byte[] buffer = new byte[RollerConstants.EIGHT_KB_IN_BYTES];
-        int bytesRead;
-        OutputStream bos = null;
-        try {
-            bos = new FileOutputStream(saveFile);
-            while ((bytesRead = is.read(buffer, 0,
-                    RollerConstants.EIGHT_KB_IN_BYTES)) != -1) {
-                bos.write(buffer, 0, bytesRead);
-            }
-            log.debug("The file has been written to ["
-                    + saveFile.getAbsolutePath() + "]");
-        } catch (Exception e) {
+        try (OutputStream os = Files.newOutputStream(saveFile)) {
+            is.transferTo(os);
+            log.debug("The file has been written to ["+saveFile+"]");
+        } catch (IOException e) {
             throw new FileIOException("ERROR uploading file", e);
-        } finally {
-            try {
-                if (bos != null) {
-                    bos.flush();
-                    bos.close();
-                }
-            } catch (Exception ignored) {
-            }
         }
 
     }
@@ -400,40 +386,39 @@ private File getRealFile(Weblog weblog, String fileId)
             throws FileNotFoundException, FilePathException {
 
         // make sure uploads area exists for this weblog
-        File weblogDir = new File(this.storageDir + weblog.getHandle());
-        if (!weblogDir.exists()) {
-            weblogDir.mkdirs();
+        Path weblogDir = Path.of(this.storageDir, weblog.getHandle());
+        if (!Files.exists(weblogDir)) {
+            try {
+                Files.createDirectories(weblogDir);
+            } catch (IOException ex) {
+                throw new FilePathException("Can't create storage dir [" + weblogDir + "]", ex);
+            }
         }
 
         // now form the absolute path
-        String filePath = weblogDir.getAbsolutePath();
+        Path filePath = weblogDir.toAbsolutePath();
         if (fileId != null) {
-            filePath += File.separator + fileId;
+            checkFileName(fileId);
+            filePath = filePath.resolve(fileId);
         }
 
         // make sure path exists and is readable
-        File file = new File(filePath);
-        if (!file.exists()) {
+        if (!Files.isReadable(filePath)) {
             throw new FileNotFoundException("Invalid path [" + filePath + "], "
-                    + "file does not exist.");
-        } else if (!file.canRead()) {
-            throw new FilePathException("Invalid path [" + filePath + "], "
-                    + "cannot read from path.");
+                    + "file does not exist or is not readable.");
         }
 
-        try {
-            // make sure someone isn't trying to sneek outside the uploads dir
-            if (!file.getCanonicalPath().startsWith(
-                    weblogDir.getCanonicalPath())) {
-                throw new FilePathException("Invalid path " + filePath + "], "
-                        + "trying to get outside uploads dir.");
-            }
-        } catch (IOException ex) {
-            // rethrow as FilePathException
-            throw new FilePathException(ex);
-        }
+        return filePath.toFile();
+    }
 
-        return file;
+    /**
+     * Make sure someone isn't trying to sneak outside the uploads dir.
+     */
+    private static void checkFileName(String fileId) throws FilePathException {
+        if(fileId.contains("..")) {
+            throw new FilePathException("Invalid file name [" + fileId + "], "
+                    + "trying to get outside uploads dir.");
+        }
     }
 
 }

app/src/main/java/org/apache/roller/weblogger/business/themes/ThemeManagerImpl.java

@@ -65,7 +65,7 @@
 @com.google.inject.Singleton
 public class ThemeManagerImpl implements ThemeManager {
 
-	static FileTypeMap map = null;
+	private static final FileTypeMap map;
 	static {
 		// TODO: figure out why PNG is missing from Java MIME types
 		map = FileTypeMap.getDefaultFileTypeMap();
@@ -77,7 +77,7 @@ public class ThemeManagerImpl implements ThemeManager {
 		}
 	}
 
-	private static Log log = LogFactory.getLog(ThemeManagerImpl.class);
+	private static final Log log = LogFactory.getLog(ThemeManagerImpl.class);
 	private final Weblogger roller;
 	// directory where themes are kept
 	private String themeDir = null;
@@ -354,7 +354,7 @@ public void importTheme(Weblog weblog, SharedTheme theme, boolean skipStylesheet
 				RollerMessages errors = new RollerMessages();
 				fileMgr.createThemeMediaFile(weblog, mf, errors);
 				try {
-					resource.getInputStream().close();
+					is.close();
 				} catch (IOException ex) {
 					errors.addError("error.closingStream");
 					log.debug("ERROR closing inputstream");

app/src/main/java/org/apache/roller/weblogger/ui/core/filters/InitFilter.java

@@ -30,6 +30,7 @@
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
+import org.apache.commons.validator.routines.UrlValidator;
 import org.apache.roller.weblogger.config.WebloggerRuntimeConfig;
 
 /**
@@ -41,7 +42,7 @@
  */
 public class InitFilter implements Filter {
 
-    private static Log log = LogFactory.getLog(InitFilter.class);
+    private static final Log log = LogFactory.getLog(InitFilter.class);
 
     private boolean initialized = false;
 
@@ -53,22 +54,29 @@ public void doFilter(ServletRequest req, ServletResponse res,
 
             // first request, lets do our initialization
             HttpServletRequest request = (HttpServletRequest) req;
-            // HttpServletResponse response = (HttpServletResponse) res;
-
-            // determine absolute and relative url paths to the app
-            String relPath = request.getContextPath();
-            String absPath = this.getAbsoluteUrl(request);
-
-            // set them in our config
-            WebloggerRuntimeConfig.setAbsoluteContextURL(absPath);
-            WebloggerRuntimeConfig.setRelativeContextURL(relPath);
-
-            if (log.isDebugEnabled()) {
-                log.debug("relPath = " + relPath);
-                log.debug("absPath = " + absPath);
+
+            UrlValidator validator = new UrlValidator(
+                            new String[]{"http", "https"},
+                            UrlValidator.ALLOW_LOCAL_URLS); // for integration tests
+
+            if(validator.isValid(request.getRequestURL().toString())) {
+
+                // determine absolute and relative url paths to the app
+                String relPath = request.getContextPath();
+                String absPath = this.getAbsoluteUrl(request);
+
+                // set them in our config
+                WebloggerRuntimeConfig.setAbsoluteContextURL(absPath);
+                WebloggerRuntimeConfig.setRelativeContextURL(relPath);
+
+                if (log.isDebugEnabled()) {
+                    log.debug("relPath = " + relPath);
+                    log.debug("absPath = " + absPath);
+                }
+
+                this.initialized = true;
             }
 
-            this.initialized = true;
         }
 
         chain.doFilter(req, res);
@@ -90,9 +98,9 @@ public void destroy() {
 
     protected static String getAbsoluteUrl(boolean secure, String serverName, String contextPath, String requestURI, String requestURL){
 
-        String url = null;
+        String url;
 
-        String fullUrl = null;
+        String fullUrl;
 
         if (!secure) {
             fullUrl = requestURL;

app/src/main/java/org/apache/roller/weblogger/ui/core/security/RollerRememberMeServices.java

@@ -31,8 +31,8 @@
 
 
 public class RollerRememberMeServices extends TokenBasedRememberMeServices {
-    private static final Log log = LogFactory.getLog(RollerRememberMeServices.class);
 
+    private static final Log log = LogFactory.getLog(RollerRememberMeServices.class);
 
     public RollerRememberMeServices(UserDetailsService userDetailsService) {
 
@@ -51,7 +51,7 @@ public RollerRememberMeServices(UserDetailsService userDetailsService) {
 
     /**
      * Calculates the digital signature to be put in the cookie. Default value is
-     * MD5 ("username:tokenExpiryTime:password:key")
+     * SHA-512 ("username:tokenExpiryTime:password:key")
      *
      * If LDAP is enabled then a configurable dummy password is used in the calculation.
      */
@@ -70,9 +70,9 @@ protected String makeTokenSignature(long tokenExpiryTime, String username, Strin
         String data = username + ":" + tokenExpiryTime + ":" + password + ":" + getKey();
         MessageDigest digest;
         try {
-            digest = MessageDigest.getInstance("MD5");
+            digest = MessageDigest.getInstance("SHA-512");
         } catch (NoSuchAlgorithmException e) {
-            throw new IllegalStateException("No MD5 algorithm available!");
+            throw new IllegalStateException("Required by Spec.", e);
         }
 
         return new String(Hex.encode(digest.digest(data.getBytes())));

app/src/main/java/org/apache/roller/weblogger/ui/rendering/WeblogRequestMapper.java

@@ -46,7 +46,7 @@
  */
 public class WeblogRequestMapper implements RequestMapper {
 
-    private static Log log = LogFactory.getLog(WeblogRequestMapper.class);
+    private static final Log log = LogFactory.getLog(WeblogRequestMapper.class);
 
     private static final String PAGE_SERVLET = "/roller-ui/rendering/page";
     private static final String FEED_SERVLET = "/roller-ui/rendering/feed";
@@ -199,7 +199,7 @@ public boolean handleRequest(HttpServletRequest request, HttpServletResponse res
             // this means someone referred to a weblog index page with the 
             // shortest form of url /<weblog> or /<weblog>/<locale> and we need
             // to do a redirect to /<weblog>/ or /<weblog>/<locale>/
-            String redirectUrl = request.getRequestURI() + "/";
+            String redirectUrl = "/" + weblogHandle + "/";
             if(request.getQueryString() != null) {
                 redirectUrl += "?"+request.getQueryString();
             }

app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/FolderEdit.java

@@ -40,7 +40,7 @@
 // TODO: make this work @AllowedMethods({"execute","save"})
 public class FolderEdit extends UIAction implements ServletResponseAware {
 
-    private static Log log = LogFactory.getLog(FolderEdit.class);
+    private static final Log log = LogFactory.getLog(FolderEdit.class);
 
     // bean for managing form data
     private FolderBean bean = new FolderBean();
@@ -127,7 +127,10 @@ public String save() {
                     addMessage("folderForm.updated");
                 }
 
-                httpServletResponse.addHeader("folderId", folderId );
+                // HTTP response splitting defense
+                String sanetizedFolderID = folderId.replace("\n", "").replace("\r", "");
+
+                httpServletResponse.addHeader("folderId", sanetizedFolderID);
 
                 return SUCCESS;
 

app/src/main/java/org/apache/roller/weblogger/webservices/tagdata/TagDataServlet.java

@@ -186,20 +186,20 @@ public void doGet(
                         0, true);
                 int frequency = stat.getCount();
                 pw.print("<atom:category term=\"" + term + "\" tagdata:frequency=\"" + frequency + "\" ");
-                pw.println("tagdata:href=\"" + viewURI + "\" />");
+                pw.println("tagdata:href=\"" + StringEscapeUtils.escapeXml10(viewURI) + "\" />");
                 if (count++ > MAX) {
                     break;
                 }
             }
             if (tags.size() > MAX) {
                 // get next URI, if site-wide then don't specify weblog
                 String nextURI = urlstrat.getWeblogTagsJsonURL(weblog, true, page + 1);
-                pw.println("<atom:link rel=\"next\" href=\"" + nextURI + "\" />");
+                pw.println("<atom:link rel=\"next\" href=\"" + StringEscapeUtils.escapeXml10(nextURI) + "\" />");
             }
             if (page > 0) {
                 // get prev URI, if site-wide then don't specify weblog
                 String prevURI = urlstrat.getWeblogTagsJsonURL(weblog, true, page - 1);
-                pw.println("<atom:link rel=\"previous\" href=\"" + prevURI + "\" />");
+                pw.println("<atom:link rel=\"previous\" href=\"" + StringEscapeUtils.escapeXml10(prevURI) + "\" />");
             }
             pw.println("</categories>");
             response.flushBuffer();

app/src/main/webapp/theme/scripts/roller.js

@@ -16,11 +16,12 @@
 * directory of this distribution.
 */
 /* This function is used to set cookies */
-function setCookie(name,value,expires,path,domain,secure) {
+function setCookie(name, value, expires, path, domain, secure=true, sameSite=true) {
   document.cookie = name + "=" + escape (value) +
     ((expires) ? "; expires=" + expires.toGMTString() : "") +
     ((path) ? "; path=" + path : "") +
-    ((domain) ? "; domain=" + domain : "") + ((secure) ? "; secure" : "");
+    ((domain) ? "; domain=" + domain : "") + ((secure) ? "; secure" : "") +
+    ((sameSite) ? "; SameSite=Strict" : "");
 }
 
 /* This function is used to get cookies */