Improve input validation

app/src/main/java/org/apache/roller/weblogger/business/jpa/JPAWeblogManagerImpl.java

@@ -362,11 +362,12 @@ public Weblog getWeblogByHandle(String handle) throws WebloggerException {
      * Return weblog specified by handle.
      */
     @Override
-    public Weblog getWeblogByHandle(String handle, Boolean visible)
-    throws WebloggerException {
+    public Weblog getWeblogByHandle(String handle, Boolean visible) throws WebloggerException {
 
-        if (handle==null) {
+        if (handle == null) {
             throw new WebloggerException("Handle cannot be null");
+        } else if (!isAlphanumeric(handle)) {
+            throw new WebloggerException("Invalid handle: '"+handle+"'");
         }
 
         // check cache first
@@ -704,4 +705,19 @@ public long getWeblogCount() throws WebloggerException {
         return results.get(0);
     }
 
+    /**
+     * Returns true if alphanumeric or '_'.
+     */
+    private boolean isAlphanumeric(String str) {
+        if (str == null) {
+            return false;
+        }
+        for (int i = 0; i < str.length(); i++) {
+            if (!Character.isLetterOrDigit(str.charAt(i)) && str.charAt(i) != '_') {
+                return false;
+            }
+        }
+        return true;
+    }
+
 }

app/src/main/java/org/apache/roller/weblogger/ui/rendering/plugins/comments/CommentAuthenticatorUtils.java

@@ -28,16 +28,18 @@
 import java.util.Locale;
 
 class CommentAuthenticatorUtils {
-    private static Log log = LogFactory.getLog(CommentAuthenticatorUtils.class);
+    private static final Log log = LogFactory.getLog(CommentAuthenticatorUtils.class);
 
     public static Locale getLocale(HttpServletRequest request) {
         String handle = request.getParameter("weblog");
         try {
             Weblog weblog = WebloggerFactory.getWeblogger().getWeblogManager().getWeblogByHandle(handle);
-            return weblog.getLocaleInstance();
+            if(weblog != null) {
+                return weblog.getLocaleInstance();
+            }
         } catch (WebloggerException e) {
             log.debug("Failed to determine weblog's locale. fallback to the locale of the request", e);
-            return request.getLocale();
         }
+        return request.getLocale();
     }
 }

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

@@ -73,7 +73,7 @@
  */
 public class CommentServlet extends HttpServlet {
 
-    private static Log log = LogFactory.getLog(CommentServlet.class);
+    private static final Log log = LogFactory.getLog(CommentServlet.class);
 
     private CommentAuthenticator authenticator = null;
     private CommentValidationManager commentValidationManager = null;
@@ -202,9 +202,7 @@ public void doPost(HttpServletRequest request, HttpServletResponse response)
         try {
             commentRequest = new WeblogCommentRequest(request);
 
-            // lookup weblog specified by comment request
-            weblog = WebloggerFactory.getWeblogger().getWeblogManager()
-                    .getWeblogByHandle(commentRequest.getWeblogHandle());
+            weblog = commentRequest.getWeblog();
 
             if (weblog == null) {
                 throw new WebloggerException("unable to lookup weblog: "

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

@@ -55,7 +55,7 @@
  */
 public class FeedServlet extends HttpServlet {
 
-    private static Log log = LogFactory.getLog(FeedServlet.class);
+    private static final Log log = LogFactory.getLog(FeedServlet.class);
 
     private WeblogFeedCache weblogFeedCache = null;
     private SiteWideCache siteWideCache = null;

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

@@ -59,7 +59,7 @@ public class SearchServlet extends HttpServlet {
 
     private static final long serialVersionUID = 6246730804167411636L;
 
-    private static Log log = LogFactory.getLog(SearchServlet.class);
+    private static final Log log = LogFactory.getLog(SearchServlet.class);
 
     // Development theme reloading
     Boolean themeReload = false;
@@ -87,20 +87,19 @@ public void doGet(HttpServletRequest request, HttpServletResponse response)
 
         log.debug("Entering");
 
-        Weblog weblog = null;
-        WeblogSearchRequest searchRequest = null;
+        Weblog weblog;
+        WeblogSearchRequest searchRequest;
 
         // first off lets parse the incoming request and validate it
         try {
             searchRequest = new WeblogSearchRequest(request);
 
             // now make sure the specified weblog really exists
-            weblog = WebloggerFactory
-                    .getWeblogger()
-                    .getWeblogManager()
-                    .getWeblogByHandle(searchRequest.getWeblogHandle(),
-                            Boolean.TRUE);
-
+            weblog = searchRequest.getWeblog();
+            if (weblog == null) {
+                response.sendError(HttpServletResponse.SC_BAD_REQUEST, "Weblog not found");
+                return;
+            }
         } catch (Exception e) {
             // invalid search request format or weblog doesn't exist
             log.debug("error creating weblog search request", e);
@@ -229,7 +228,7 @@ public void doGet(HttpServletRequest request, HttpServletResponse response)
         }
 
         // lookup Renderer we are going to use
-        Renderer renderer = null;
+        Renderer renderer;
         try {
             log.debug("Looking up renderer");
             renderer = RendererManager.getRenderer(page, deviceType);

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

@@ -22,6 +22,7 @@
 
 import javax.servlet.http.HttpServletRequest;
 
+import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.apache.roller.weblogger.WebloggerException;
@@ -43,7 +44,7 @@
  */
 public class WeblogFeedRequest extends WeblogRequest {
 
-    private static Log log = LogFactory.getLog(WeblogFeedRequest.class);
+    private static final Log log = LogFactory.getLog(WeblogFeedRequest.class);
 
     private static final String FEED_SERVLET = "/roller-ui/rendering/feed";
 
@@ -97,7 +98,9 @@ public WeblogFeedRequest(HttpServletRequest request)
         if(pathInfo != null && pathInfo.trim().length() > 1) {
 
             String[] pathElements = pathInfo.split("/");
-            if(pathElements.length == 2) {
+            if(pathElements.length == 2
+                    && StringUtils.isAlphanumeric(pathElements[0])
+                    && StringUtils.isAlphanumeric(pathElements[1])) {
                 this.type = pathElements[0];
                 this.format = pathElements[1];
             } else {

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

@@ -20,6 +20,7 @@
 
 import java.util.Locale;
 import javax.servlet.http.HttpServletRequest;
+import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.apache.roller.weblogger.WebloggerException;
@@ -48,7 +49,7 @@
  */
 public class WeblogRequest extends ParsedRequest {
 
-    private static Log log = LogFactory.getLog(WeblogRequest.class);
+    private static final Log log = LogFactory.getLog(WeblogRequest.class);
 
     // lightweight attributes
     private String weblogHandle = null;
@@ -85,12 +86,11 @@ public WeblogRequest(HttpServletRequest request)
             }
 
             String[] pathElements = path.split("/", 2);
-            if(!pathElements[0].isBlank()) {
+            if(StringUtils.isAlphanumeric(pathElements[0])) {
                 this.weblogHandle = pathElements[0];
             } else {
-                // no weblogHandle in path info
-                throw new InvalidRequestException("not a weblog request, "+
-                        request.getRequestURL());
+                // no or invalid weblogHandle in path info
+                throw new InvalidRequestException("not a valid weblog request: "+request.getRequestURL());
             }
 
             // if there is more left of the path info then hold onto it

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

@@ -46,7 +46,7 @@
  */
 public class RollerResourceLoader extends ResourceLoader {
 
-	private static Log logger = LogFactory.getLog(RollerResourceLoader.class);
+	private static final Log logger = LogFactory.getLog(RollerResourceLoader.class);
 
     @Override
 	public void init(ExtProperties configuration) {
@@ -105,15 +105,14 @@ public Reader getResourceReader(String name, String encoding) {
 		} catch (UnsupportedEncodingException uex) {
 			// This should never actually happen. We expect UTF-8 in all JRE
 			// installation.
-			// This rethrows as a Runtime exception after logging.
-			logger.error(uex);
+//			logger.error(uex);
 			throw new RuntimeException(uex);
 
 		} catch (WebloggerException | ResourceNotFoundException re) {
 			String msg = "RollerResourceLoader Error: "
 					+ "database problem trying to load resource " + name;
-			logger.error(msg, re);
-			throw new ResourceNotFoundException(msg);
+//			logger.error(msg, re);
+			throw new ResourceNotFoundException(msg, re);
 		}
 	}
 

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

@@ -32,8 +32,6 @@
 import org.apache.roller.weblogger.ui.rendering.model.UtilitiesModel;
 import org.apache.velocity.VelocityContext;
 import org.apache.velocity.context.Context;
-import org.apache.velocity.exception.MethodInvocationException;
-import org.apache.velocity.exception.ParseErrorException;
 import org.apache.velocity.exception.ResourceNotFoundException;
 import org.apache.velocity.exception.VelocityException;
 
@@ -42,11 +40,11 @@
  */
 public class VelocityRenderer implements Renderer {
 
-    private static Log log = LogFactory.getLog(VelocityRenderer.class);
+    private static final Log log = LogFactory.getLog(VelocityRenderer.class);
 
     // the original template we are supposed to render
-    private Template renderTemplate = null;
-    private MobileDeviceRepository.DeviceType deviceType = null;
+    private final Template renderTemplate;
+    private final MobileDeviceRepository.DeviceType deviceType;
 
     // the velocity templates
     private org.apache.velocity.Template velocityTemplate = null;
@@ -77,34 +75,13 @@ public VelocityRenderer(Template template,
             // failed
             throw ex;
 
-        } catch (ParseErrorException ex) {
-            // in the case of a parsing error we want to render an
-            // error page instead so the user knows what was wrong
-            velocityException = ex;
-
-            // need to lookup error page template
-            velocityTemplate = RollerVelocity.getTemplate("error-page.vm",
-                    deviceType);
-
-        } catch (MethodInvocationException ex) {
-
-            // in the case of a invocation error we want to render an
-            // error page instead so the user knows what was wrong
-            velocityException = ex;
-
-            // need to lookup error page template
-            velocityTemplate = RollerVelocity.getTemplate("error-page.vm",
-                    deviceType);
-
         } catch (VelocityException ex) {
-
-            // in the case of a parsing error including a macro we want to
-            // render an error page instead so the user knows what was wrong
+            // in the case of a velocity error we want to render an
+            // error page instead so the user knows what was wrong
             velocityException = ex;
 
             // need to lookup error page template
-            velocityTemplate = RollerVelocity.getTemplate("error-page.vm",
-                    deviceType);
+            velocityTemplate = RollerVelocity.getTemplate("error-page.vm", deviceType);
 
         } catch (Exception ex) {
             // some kind of generic/unknown exception, dump it to the logs
@@ -172,27 +149,9 @@ public void render(Map<String, Object> model, Writer out)
             log.debug("Rendered [" + renderTemplate.getId() + "] in "
                     + renderTime + " secs");
 
-        } catch (ParseErrorException ex) {
-
-            // in the case of a parsing error including a page we want to render
-            // an error on the page instead so the user knows what was wrong
-            velocityException = ex;
-
-            // need to lookup parse error template
-            renderException(model, out, "error-parse.vm");
-
-        } catch (MethodInvocationException ex) {
-
-            // in the case of a parsing error including a page we want to render
-            // an error on the page instead so the user knows what was wrong
-            velocityException = ex;
-
-            // need to lookup parse error template
-            renderException(model, out, "error-parse.vm");
-
         } catch (VelocityException ex) {
 
-            // in the case of a parsing error including a macro we want to
+            // in the case of a velocity error including a page we want to
             // render an error page instead so the user knows what was wrong
             velocityException = ex;
 

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

@@ -28,44 +28,45 @@
 import org.apache.roller.weblogger.ui.rendering.Renderer;
 import org.apache.roller.weblogger.ui.rendering.RendererFactory;
 import org.apache.roller.weblogger.ui.rendering.mobile.MobileDeviceRepository;
+import org.apache.velocity.exception.ResourceNotFoundException;
 
 
 /**
  * RendererFactory for Velocity, creates VelocityRenderers.
  */
 public class VelocityRendererFactory implements RendererFactory {
-    private static Log log = LogFactory.getLog(VelocityRendererFactory.class);
+    private static final Log log = LogFactory.getLog(VelocityRendererFactory.class);
 
     @Override
     public Renderer getRenderer(Template template, 
 			MobileDeviceRepository.DeviceType deviceType) {
-        Renderer renderer = null;
-        TemplateRendition tr;
 
+        // nothing we can do with null values
         if (template == null || template.getId() == null) {
             return null;
         }
 
-        // nothing we can do with null values
+        TemplateRendition tr;
         try {
             tr = template.getTemplateRendition(RenditionType.STANDARD);
+            if (tr == null) {
+                return null;
+            }
         } catch (WebloggerException e) {
             return null;
         }
 
-        if (tr == null) {
-            return null;
-        }
+        Renderer renderer = null;
 
         if (TemplateLanguage.VELOCITY.equals(tr.getTemplateLanguage())) {
             // standard velocity template
             try {
                renderer = new VelocityRenderer(template, deviceType);
+            } catch (ResourceNotFoundException ex) {
+                // allready logged in VelocityRenderer
             } catch(Exception ex) {
-				log.error("ERROR creating VelocityRenderer", ex);
                 // some kind of exception so we don't have a renderer
-                // we do catching/logging in VelocityRenderer constructor
-                return null;
+				log.error("ERROR creating VelocityRenderer", ex);
             }            
         }
         return renderer;