Minor refactoring and TODOs #134

aramrami · Aug 24, 2020 · 5abb3c7 · 5abb3c7
1 parent 1b9b89a
commit 5abb3c7
Show file tree

Hide file tree

Showing 11 changed files with 66 additions and 58 deletions.
diff --git a/csrfguard-test/csrfguard-test-jsp/src/main/webapp/tag.jsp b/csrfguard-test/csrfguard-test-jsp/src/main/webapp/tag.jsp
@@ -1,4 +1,5 @@
 <%@ page language="java" contentType="text/html; charset=UTF-8" pageEncoding="UTF-8"%>
+<%-- FIXME the TLD path is not correct, should be WEB-INF/csrfguard.tld or something --%>
 <%@ taglib uri="http://www.owasp.org/index.php/Category:OWASP_CSRFGuard_Project/Owasp.CsrfGuard.tld" prefix="csrf" %>
 <!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
 <html>

diff --git a/csrfguard/src/main/java/org/owasp/csrfguard/action/Empty.java b/csrfguard/src/main/java/org/owasp/csrfguard/action/Empty.java
@@ -35,12 +35,15 @@
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
+/**
+ * TODO document or why it is needed or remove this Action
+ */
 public final class Empty extends AbstractAction {
 
 	private static final long serialVersionUID = 3530383602177340966L;
 
 	@Override
-	public void execute(HttpServletRequest request, HttpServletResponse response, CsrfGuardException csrfe, CsrfGuard csrfGuard) throws CsrfGuardException {
+	public void execute(final HttpServletRequest request, final HttpServletResponse response, final CsrfGuardException csrfe, final CsrfGuard csrfGuard) throws CsrfGuardException {
 		// nothing to do
 	}
 }
diff --git a/csrfguard/src/main/java/org/owasp/csrfguard/action/Error.java b/csrfguard/src/main/java/org/owasp/csrfguard/action/Error.java
@@ -41,7 +41,7 @@ public final class Error extends AbstractAction {
 	private static final long serialVersionUID = 5479074081984904252L;
 
 	@Override
-	public void execute(HttpServletRequest request, HttpServletResponse response, CsrfGuardException csrfe, CsrfGuard csrfGuard) throws CsrfGuardException {
+	public void execute(final HttpServletRequest request, final HttpServletResponse response, final CsrfGuardException csrfe, final CsrfGuard csrfGuard) throws CsrfGuardException {
 		try {
 			final int code = Integer.parseInt(getParameter("Code"));
 			final String message = getParameter("Message");

diff --git a/csrfguard/src/main/java/org/owasp/csrfguard/action/Forward.java b/csrfguard/src/main/java/org/owasp/csrfguard/action/Forward.java
@@ -42,7 +42,7 @@ public final class Forward extends AbstractAction {
 	private static final long serialVersionUID = -3727752206497452347L;
 
 	@Override
-	public void execute(HttpServletRequest request, HttpServletResponse response, CsrfGuardException csrfe, CsrfGuard csrfGuard) throws CsrfGuardException {
+	public void execute(final HttpServletRequest request, final HttpServletResponse response, final CsrfGuardException csrfe, final CsrfGuard csrfGuard) throws CsrfGuardException {
 		final String errorPage = getParameter("Page");
 
 		try {

diff --git a/csrfguard/src/main/java/org/owasp/csrfguard/action/Log.java b/csrfguard/src/main/java/org/owasp/csrfguard/action/Log.java
@@ -42,7 +42,7 @@ public final class Log extends AbstractAction {
 	private static final long serialVersionUID = 8238761463376338707L;
 
 	@Override
-	public void execute(HttpServletRequest request, HttpServletResponse response, CsrfGuardException csrfe, CsrfGuard csrfGuard) throws CsrfGuardException {
+	public void execute(final HttpServletRequest request, final HttpServletResponse response, final CsrfGuardException csrfe, final CsrfGuard csrfGuard) throws CsrfGuardException {
 		String logMessage = getParameter("Message");
 
 		/* Exception Information */

diff --git a/csrfguard/src/main/java/org/owasp/csrfguard/action/Redirect.java b/csrfguard/src/main/java/org/owasp/csrfguard/action/Redirect.java
@@ -41,7 +41,7 @@ public final class Redirect extends AbstractAction {
 	private static final long serialVersionUID = -2265693822259717332L;
 
 	@Override
-	public void execute(HttpServletRequest request, HttpServletResponse response, CsrfGuardException csrfe, CsrfGuard csrfGuard) throws CsrfGuardException {
+	public void execute(final HttpServletRequest request, final HttpServletResponse response, final CsrfGuardException csrfe, final CsrfGuard csrfGuard) throws CsrfGuardException {
 		final String errorPage = getParameter("Page");
 
 		try {

diff --git a/csrfguard/src/main/java/org/owasp/csrfguard/action/RequestAttribute.java b/csrfguard/src/main/java/org/owasp/csrfguard/action/RequestAttribute.java
@@ -40,7 +40,7 @@ public final class RequestAttribute extends AbstractAction {
 	private static final long serialVersionUID = 6714855990116387348L;
 
 	@Override
-	public void execute(HttpServletRequest request, HttpServletResponse response, CsrfGuardException csrfe, CsrfGuard csrfGuard) throws CsrfGuardException {
+	public void execute(final HttpServletRequest request, final HttpServletResponse response, final CsrfGuardException csrfe, final CsrfGuard csrfGuard) throws CsrfGuardException {
 		final String attributeName = getParameter("AttributeName");
 
 		request.setAttribute(attributeName, csrfe);

diff --git a/csrfguard/src/main/java/org/owasp/csrfguard/action/SessionAttribute.java b/csrfguard/src/main/java/org/owasp/csrfguard/action/SessionAttribute.java
@@ -36,12 +36,17 @@
 import javax.servlet.http.HttpServletResponse;
 import javax.servlet.http.HttpSession;
 
+/**
+ * TODO document
+ * TODO What is the exact/intended purpose of this Action?
+ * TODO Would it make sense to implement for the Custom Token Holder (Stateless) implementation as well?
+ */
 public final class SessionAttribute extends AbstractAction {
 
 	private static final long serialVersionUID = 1367492926060283228L;
 
 	@Override
-	public void execute(HttpServletRequest request, HttpServletResponse response, CsrfGuardException csrfe, CsrfGuard csrfGuard) throws CsrfGuardException {
+	public void execute(final HttpServletRequest request, final HttpServletResponse response, final CsrfGuardException csrfe, final CsrfGuard csrfGuard) throws CsrfGuardException {
 		final String attributeName = getParameter("AttributeName");
 		final HttpSession session = request.getSession(false);
 

diff --git a/.../main/java/org/owasp/csrfguard/config/overlay/ConfigurationAutodetectProviderFactory.java b/.../main/java/org/owasp/csrfguard/config/overlay/ConfigurationAutodetectProviderFactory.java
@@ -56,7 +56,7 @@ public ConfigurationAutodetectProviderFactory() {}
 	/**
 	 * configuration provider cached
 	 */
-	private static ExpirableCache<Boolean, ConfigurationProvider> configurationProviderCache = new ExpirableCache<Boolean, ConfigurationProvider>(2);
+	private static ExpirableCache<Boolean, ConfigurationProvider> configurationProviderCache = new ExpirableCache<Boolean, ConfigurationProvider>(2); // TODO does this really reload the configurations in every 2 minutes?!
 
 	/**
 	 * @see org.owasp.csrfguard.config.ConfigurationProviderFactory#retrieveConfiguration(java.util.Properties)

diff --git a/csrfguard/src/main/java/org/owasp/csrfguard/log/JavaLogger.java b/csrfguard/src/main/java/org/owasp/csrfguard/log/JavaLogger.java
@@ -29,8 +29,6 @@
 
 package org.owasp.csrfguard.log;
 
-import org.apache.commons.lang3.StringUtils;
-
 import java.util.logging.Level;
 import java.util.logging.Logger;
 

diff --git a/csrfguard/src/main/java/org/owasp/csrfguard/servlet/JavaScriptServlet.java b/csrfguard/src/main/java/org/owasp/csrfguard/servlet/JavaScriptServlet.java
@@ -81,7 +81,10 @@ public final class JavaScriptServlet extends HttpServlet {
 	private static final String TOKENS_PER_PAGE_IDENTIFIER = "%TOKENS_PER_PAGE%";
 
 	private static final String UNPROTECTED_EXTENSIONS_IDENTIFIER = "%UNPROTECTED_EXTENSIONS%";
-
+
+	private static final String TEXT_PLAIN_MIME_TYPE = "text/plain";
+	private static final String JAVASCRIPT_MIME_TYPE = "text/javascript";
+
 	private static ServletConfig servletConfig = null;
 
 	/**
@@ -103,25 +106,29 @@ public void init(final ServletConfig theServletConfig) {
 
 	@Override
 	public void doGet(final HttpServletRequest request, final HttpServletResponse response) throws IOException {
+		final CsrfGuard csrfGuard = CsrfGuard.getInstance();
 		final String refererHeader = request.getHeader("referer");
+
 		boolean hasError = false;
-		final Pattern javascriptRefererPattern = CsrfGuard.getInstance().getJavascriptRefererPattern();
-		if (refererHeader != null && !javascriptRefererPattern.matcher(refererHeader).matches()) {
-			CsrfGuard.getInstance().getLogger().log(LogLevel.Error, String.format("Referer domain %s does not match regex: %s", refererHeader, javascriptRefererPattern.pattern()));
-			response.sendError(404);
-			hasError = true;
-		}
-
-		if (refererHeader != null && CsrfGuard.getInstance().isJavascriptRefererMatchDomain()) {
-			final boolean isJavascriptRefererMatchProtocol = CsrfGuard.getInstance().isJavascriptRefererMatchProtocol();
-			// this is something like http://something.com/path or https://something.com/path
-			final String url = request.getRequestURL().toString();
-			final String requestProtocolAndDomain = CsrfGuardUtils.httpProtocolAndDomain(url, isJavascriptRefererMatchProtocol);
-			final String refererProtocolAndDomain = CsrfGuardUtils.httpProtocolAndDomain(refererHeader, isJavascriptRefererMatchProtocol);
-			if (!refererProtocolAndDomain.equals(requestProtocolAndDomain)) {
-				CsrfGuard.getInstance().getLogger().log(LogLevel.Error, String.format("Referer domain %s does not match request domain: %s", refererHeader, url));
-				hasError = true;
+		final Pattern javascriptRefererPattern = csrfGuard.getJavascriptRefererPattern();
+		if (refererHeader != null) {
+			if (!javascriptRefererPattern.matcher(refererHeader).matches()) {
+				csrfGuard.getLogger().log(LogLevel.Error, String.format("Referer domain %s does not match regex: %s", refererHeader, javascriptRefererPattern.pattern()));
 				response.sendError(404);
+				hasError = true;
+			}
+
+			if (csrfGuard.isJavascriptRefererMatchDomain()) {
+				final boolean isJavascriptRefererMatchProtocol = csrfGuard.isJavascriptRefererMatchProtocol();
+				// this is something like http://something.com/path or https://something.com/path
+				final String url = request.getRequestURL().toString();
+				final String requestProtocolAndDomain = CsrfGuardUtils.httpProtocolAndDomain(url, isJavascriptRefererMatchProtocol);
+				final String refererProtocolAndDomain = CsrfGuardUtils.httpProtocolAndDomain(refererHeader, isJavascriptRefererMatchProtocol);
+				if (!refererProtocolAndDomain.equals(requestProtocolAndDomain)) {
+					csrfGuard.getLogger().log(LogLevel.Error, String.format("Referer domain %s does not match request domain: %s", refererHeader, url));
+					hasError = true;
+					response.sendError(404);
+				}
 			}
 		}
 
@@ -151,48 +158,39 @@ public static Set<String> getJavascriptUris() {
 	public void doPost(final HttpServletRequest request, final HttpServletResponse response) throws IOException {
 		final CsrfGuard csrfGuard = CsrfGuard.getInstance();
 		final String isFetchCsrfToken = request.getHeader("FETCH-CSRF-TOKEN");
-		
+
 		if (csrfGuard != null && isFetchCsrfToken != null){
-			fetchCsrfToken(request, response);
+			writeMasterToken(request, response);
 		} else {
 			if (csrfGuard != null && csrfGuard.isTokenPerPageEnabled()) {
-				writePageTokens(request, response);
+				final Map<String, String> pageTokens = csrfGuard.getTokenService().getPageTokens(request);
+				writePageTokens(response, pageTokens);
 			} else {
 				response.sendError(404);
 			}
 		}
 	}
 
-	private void fetchCsrfToken(final HttpServletRequest request, final HttpServletResponse response) throws IOException {
-		final HttpSession session = request.getSession(true);
-		final CsrfGuard csrfGuard = CsrfGuard.getInstance();
-		final String token_name = csrfGuard.getTokenName();
-		final String token_value = (String) session.getAttribute(csrfGuard.getSessionKey());
-		final String token_pair = token_name + ":" + token_value;
-
+	private static void writeMasterToken(final HttpServletRequest request, final HttpServletResponse response) throws IOException {
 		/* setup headers */
-		response.setContentType("text/plain");
+		response.setContentType(TEXT_PLAIN_MIME_TYPE);
 
 		/* write dynamic javascript */
-		response.getWriter().write(token_pair);
+		response.getWriter().write(CsrfGuard.getInstance().getTokenService().getMasterTokenHeader(request));
 	}
 
-	private void writePageTokens(final HttpServletRequest request, final HttpServletResponse response) throws IOException {
-		final HttpSession session = request.getSession(true);
-		@SuppressWarnings("unchecked")
-		final Map<String, String> pageTokens = (Map<String, String>) session.getAttribute(CsrfGuard.PAGE_TOKENS_KEY);
-		final String pageTokensString = (pageTokens != null ? parsePageTokens(pageTokens) : StringUtils.EMPTY);
+	private static void writePageTokens(final HttpServletResponse response, final Map<String, String> pageTokens) throws IOException {
+		final String aggregatedPageTokens = pageTokens.entrySet().stream().map(e -> e.getKey() + ':' + e.getValue()).collect(Collectors.joining(","));
 
 		/* setup headers */
-		response.setContentType("text/plain");
-		response.setContentLength(pageTokensString.length());
+		response.setContentType(TEXT_PLAIN_MIME_TYPE);
+		response.setContentLength(aggregatedPageTokens.length());
 
 		/* write dynamic javascript */
-		response.getWriter().write(pageTokensString);
+		response.getWriter().write(aggregatedPageTokens);
 	}
 
-	private void writeJavaScript(final HttpServletRequest request, final HttpServletResponse response) throws IOException {
-		final HttpSession session = request.getSession(true);
+	private static void writeJavaScript(final HttpServletRequest request, final HttpServletResponse response) throws IOException {
 		final CsrfGuard csrfGuard = CsrfGuard.getInstance();
 
 		/* cannot cache if rotate or token-per-page is enabled */
@@ -201,16 +199,23 @@ private void writeJavaScript(final HttpServletRequest request, final HttpServlet
 			response.setHeader("Pragma", "no-cache");
 			response.setHeader("Expires", "0");
 		} else {
-			response.setHeader("Cache-Control", CsrfGuard.getInstance().getJavascriptCacheControl());
+			response.setHeader("Cache-Control", csrfGuard.getJavascriptCacheControl());
 		}
 
-		response.setContentType("text/javascript");
+		response.setContentType(JAVASCRIPT_MIME_TYPE);
 
 		/* build dynamic javascript */
-		String code = CsrfGuard.getInstance().getJavascriptTemplateCode();
+		String code = csrfGuard.getJavascriptTemplateCode();
 
+		/*--------------------------TODO review --------------------------*/
+		final HttpSession session = request.getSession(true);
+		final String masterToken = (String) session.getAttribute(csrfGuard.getSessionKey());
+
+		code = code.replace(TOKEN_VALUE_IDENTIFIER, StringUtils.defaultString(masterToken));
+		/*--------------------------TODO review--------------------------*/
+
+		// FIXME why not use booleans instead of string representation?
 		code = code.replace(TOKEN_NAME_IDENTIFIER, StringUtils.defaultString(csrfGuard.getTokenName()))
-				   .replace(TOKEN_VALUE_IDENTIFIER, StringUtils.defaultString((String) session.getAttribute(csrfGuard.getSessionKey())))
 				   .replace(INJECT_INTO_FORMS_IDENTIFIER, Boolean.toString(csrfGuard.isJavascriptInjectIntoForms()))
 				   .replace(INJECT_GET_FORMS_IDENTIFIER, Boolean.toString(csrfGuard.isJavascriptInjectGetForms()))
 				   .replace(INJECT_FORM_ATTRIBUTES_IDENTIFIER, Boolean.toString(csrfGuard.isJavascriptInjectFormAttributes()))
@@ -228,11 +233,7 @@ private void writeJavaScript(final HttpServletRequest request, final HttpServlet
 		response.getWriter().write(code);
 	}
 
-	private String parsePageTokens(final Map<String, String> pageTokens) {
-		return pageTokens.entrySet().stream().map(e -> e.getKey() + ':' + e.getValue()).collect(Collectors.joining(","));
-	}
-
-	private String parseDomain(final StringBuffer url) {
+	private static String parseDomain(final StringBuffer url) {
 		try {
 			return new URL(url.toString()).getHost();
 		} catch (final MalformedURLException e) {