SEC-2230: Polish pull request

config/src/main/java/org/springframework/security/config/http/HeadersBeanDefinitionParser.java

@@ -62,10 +62,10 @@ public class HeadersBeanDefinitionParser implements BeanDefinitionParser {
 
     private static final String ALLOW_FROM = "ALLOW-FROM";
 
-    private ManagedList headerFactories;
+    private ManagedList headerWriters;
 
     public BeanDefinition parse(Element element, ParserContext parserContext) {
-        headerFactories = new ManagedList();
+        headerWriters = new ManagedList();
         BeanDefinitionBuilder builder = BeanDefinitionBuilder.rootBeanDefinition(HeadersFilter.class);
 
         parseXssElement(element, parserContext);
@@ -74,7 +74,11 @@ public class HeadersBeanDefinitionParser implements BeanDefinitionParser {
 
         parseHeaderElements(element);
 
-        builder.addConstructorArgValue(headerFactories);
+        if(headerWriters.isEmpty()) {
+            parserContext.getReaderContext().error("At least one type of header must be specified when using <headers>",
+                    element);
+        }
+        builder.addConstructorArgValue(headerWriters);
         return builder.getBeanDefinition();
     }
 
@@ -83,12 +87,12 @@ public class HeadersBeanDefinitionParser implements BeanDefinitionParser {
         for (Element headerElt : headerElts) {
             String headerFactoryRef = headerElt.getAttribute(ATT_REF);
             if (StringUtils.hasText(headerFactoryRef)) {
-                headerFactories.add(new RuntimeBeanReference(headerFactoryRef));
+                headerWriters.add(new RuntimeBeanReference(headerFactoryRef));
             } else {
                 BeanDefinitionBuilder builder = BeanDefinitionBuilder.genericBeanDefinition(StaticHeadersWriter.class);
                 builder.addConstructorArgValue(headerElt.getAttribute(ATT_NAME));
                 builder.addConstructorArgValue(headerElt.getAttribute(ATT_VALUE));
-                headerFactories.add(builder.getBeanDefinition());
+                headerWriters.add(builder.getBeanDefinition());
             }
         }
     }
@@ -99,17 +103,16 @@ public class HeadersBeanDefinitionParser implements BeanDefinitionParser {
             BeanDefinitionBuilder builder = BeanDefinitionBuilder.genericBeanDefinition(StaticHeadersWriter.class);
             builder.addConstructorArgValue(CONTENT_TYPE_OPTIONS_HEADER);
             builder.addConstructorArgValue("nosniff");
-            headerFactories.add(builder.getBeanDefinition());
+            headerWriters.add(builder.getBeanDefinition());
         }
     }
 
     private void parseFrameOptionsElement(Element element, ParserContext parserContext) {
-        BeanDefinitionBuilder builder = BeanDefinitionBuilder.genericBeanDefinition(FrameOptionsHeaderWriter.class);
+        BeanDefinitionBuilder builder = BeanDefinitionBuilder.genericBeanDefinition(XFrameOptionsHeaderWriter.class);
 
         Element frameElt = DomUtils.getChildElementByTagName(element, FRAME_OPTIONS_ELEMENT);
         if (frameElt != null) {
             String header = getAttribute(frameElt, ATT_POLICY, "DENY");
-            builder.addConstructorArgValue(header);
             if (ALLOW_FROM.equals(header) ) {
                 String strategyRef = getAttribute(frameElt, ATT_REF, null);
                 String strategy = getAttribute(frameElt, ATT_STRATEGY, null);
@@ -133,7 +136,7 @@ public class HeadersBeanDefinitionParser implements BeanDefinitionParser {
                                     "'value' attribute doesn't represent a valid URI.", frameElt, e);
                         }
                     } else {
-                        RequestParameterAllowFromStrategy allowFromStrategy = null;
+                        AbstractRequestParameterAllowFromStrategy allowFromStrategy = null;
                         if ("whitelist".equals(strategy)) {
                             allowFromStrategy = new WhiteListedAllowFromStrategy(
                                     StringUtils.commaDelimitedListToSet(value));
@@ -146,15 +149,17 @@ public class HeadersBeanDefinitionParser implements BeanDefinitionParser {
                             }
                         }
                         String fromParameter = getAttribute(frameElt, ATT_FROM_PARAMETER, "from");
-                        allowFromStrategy.setParameterName(fromParameter);
+                        allowFromStrategy.setAllowFromParameterName(fromParameter);
                         builder.addConstructorArgValue(allowFromStrategy);
                     }
                 } else {
                     parserContext.getReaderContext().error("One of 'strategy' and 'strategy-ref' must be set.",
                             frameElt);
                 }
+            } else {
+                builder.addConstructorArgValue(header);
             }
-            headerFactories.add(builder.getBeanDefinition());
+            headerWriters.add(builder.getBeanDefinition());
         }
     }
 
@@ -173,7 +178,7 @@ public class HeadersBeanDefinitionParser implements BeanDefinitionParser {
             BeanDefinitionBuilder builder = BeanDefinitionBuilder.genericBeanDefinition(StaticHeadersWriter.class);
             builder.addConstructorArgValue(XSS_PROTECTION_HEADER);
             builder.addConstructorArgValue(value);
-            headerFactories.add(builder.getBeanDefinition());
+            headerWriters.add(builder.getBeanDefinition());
         }
     }
 

config/src/test/groovy/org/springframework/security/config/http/HttpHeadersConfigTests.groovy

@@ -32,6 +32,8 @@ import org.springframework.security.web.access.ExceptionTranslationFilter
 import org.springframework.security.web.authentication.rememberme.AbstractRememberMeServices
 import org.springframework.security.web.authentication.ui.DefaultLoginPageGeneratingFilter
 import org.springframework.security.web.headers.HeadersFilter
+import org.springframework.security.web.headers.StaticHeadersWriter;
+import org.springframework.security.web.headers.frameoptions.StaticAllowFromStrategy;
 
 /**
  *
@@ -51,17 +53,14 @@ class HttpHeadersConfigTests extends AbstractHttpConfigTests {
     }
 
     def 'http headers with empty headers'() {
-        httpAutoConfig {
-            'headers'()
-        }
-        createAppContext()
-
-        def hf = getFilter(HeadersFilter)
-        MockHttpServletResponse response = new MockHttpServletResponse()
-        hf.doFilter(new MockHttpServletRequest(), response, new MockFilterChain())
-
-        expect:
-        response.headers.isEmpty()
+        when:
+            httpAutoConfig {
+                'headers'()
+            }
+            createAppContext()
+        then:
+            BeanDefinitionParsingException success = thrown()
+            success.message.contains "At least one type of header must be specified when using <headers>"
     }
 
     def 'http headers content-type-options'() {
@@ -212,6 +211,26 @@ class HttpHeadersConfigTests extends AbstractHttpConfigTests {
         assertHeaders(response , ['a':'b', 'c':'d'])
     }
 
+    def 'http headers with ref'() {
+        setup:
+            httpAutoConfig {
+                'headers'() {
+                    'header'(ref:'headerWriter')
+                }
+            }
+            xml.'b:bean'(id: 'headerWriter', 'class': StaticHeadersWriter.name) {
+                'b:constructor-arg'(value:'abc') {}
+                'b:constructor-arg'(value:'def') {}
+            }
+            createAppContext()
+        when:
+            def hf = getFilter(HeadersFilter)
+            MockHttpServletResponse response = new MockHttpServletResponse()
+            hf.doFilter(new MockHttpServletRequest(), response, new MockFilterChain())
+        then:
+             assertHeaders(response, ['abc':'def'])
+    }
+
     def 'http headers header no name produces error'() {
         when:
         httpAutoConfig {

docs/manual/src/docbook/namespace-config.xml

@@ -617,14 +617,19 @@ List&lt;OpenIDAttribute> attributes = token.getAttributes();</programlisting>The
                 <progamlisting language="xml">
                     <![CDATA[
                             <headers>
+                                <!-- Adds X-XSS-Protection with value of 1 -->
                                 <xss-protection/>
+                                <!-- Add X-Frame-Options with a value of DENY -->
                                 <frame-options/>
+                                <!-- Add X-Content-Type-Options with value of nosniff -->
                                 <content-type-options/>
+                                <!-- Add custom headers -->
                                 <header name="foo" value="bar"/>
                             </headers>
                     ]]>
                 </progamlisting>
             </para>
+            <para>For additional information refer to <link xlink:href="nsa-headers">headers</link> section of the Security Namespace appendix.</para>
         </section>
         <section xml:id="ns-custom-filters">
             <title>Adding in Your Own Filters</title>

web/src/main/java/org/springframework/security/web/headers/Header.java

@@ -1,37 +1,56 @@
 package org.springframework.security.web.headers;
 
 import java.util.Arrays;
+import java.util.List;
 
-/**
- * Created with IntelliJ IDEA.
- * User: marten
- * Date: 29-01-13
- * Time: 20:26
- * To change this template use File | Settings | File Templates.
- */
-public final class Header {
+import javax.servlet.http.HttpServletResponse;
 
-    private final String name;
-    private final String[] values;
+import org.springframework.util.Assert;
 
-    public Header(String name, String... values) {
-        this.name = name;
-        this.values = values;
+/**
+ * Represents a Header to be added to the {@link HttpServletResponse}
+ */
+final class Header {
+
+    private final String headerName;
+    private final List<String> headerValues;
+
+    /**
+     * Creates a new instance
+     * @param headerName the name of the header
+     * @param headerValues the values of the header
+     */
+    public Header(String headerName, String... headerValues) {
+        Assert.hasText(headerName, "headerName is required");
+        Assert.notEmpty(headerValues, "headerValues cannot be null or empty");
+        Assert.noNullElements(headerValues, "headerValues cannot contain null values");
+        this.headerName = headerName;
+        this.headerValues = Arrays.asList(headerValues);
     }
 
+    /**
+     * Gets the name of the header. Cannot be <code>null</code>.
+     * @return the name of the header.
+     */
     public String getName() {
-        return this.name;
+        return this.headerName;
     }
 
-    public String[] getValues() {
-        return this.values;
+    /**
+     * Gets the values of the header. Cannot be null, empty, or contain null
+     * values.
+     *
+     * @return the values of the header
+     */
+    public List<String> getValues() {
+        return this.headerValues;
     }
 
     public int hashCode() {
-        return name.hashCode() + Arrays.hashCode(values);
+        return headerName.hashCode() + headerValues.hashCode();
     }
 
     public String toString() {
-        return "Header [name: " + name + ", values: " + Arrays.toString(values)+"]";
+        return "Header [name: " + headerName + ", values: " + headerValues +"]";
     }
 }

web/src/main/java/org/springframework/security/web/headers/HeaderWriter.java

@@ -19,7 +19,7 @@ import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
 /**
- * Contract for a factory that creates {@code Header} instances.
+ * Contract for writing headers to a {@link HttpServletResponse}
  *
  * @see HeadersFilter
  *

web/src/main/java/org/springframework/security/web/headers/HeadersFilter.java

@@ -15,6 +15,7 @@
  */
 package org.springframework.security.web.headers;
 
+import org.springframework.util.Assert;
 import org.springframework.web.filter.OncePerRequestFilter;
 
 import javax.servlet.FilterChain;
@@ -35,17 +36,22 @@ import java.util.*;
 public class HeadersFilter extends OncePerRequestFilter {
 
     /** Collection of {@link HeaderWriter} instances to  write out the headers to the response . */
-    private final List<HeaderWriter> factories;
-
-    public HeadersFilter(List<HeaderWriter> factories) {
-        this.factories = factories;
+    private final List<HeaderWriter> headerWriters;
+
+    /**
+     * Creates a new instance.
+     *
+     * @param headerWriters the {@link HeaderWriter} instances to write out headers to the {@link HttpServletResponse}.
+     */
+    public HeadersFilter(List<HeaderWriter> headerWriters) {
+        Assert.notEmpty(headerWriters, "headerWriters cannot be null");
+        this.headerWriters = headerWriters;
     }
 
-
     @Override
     protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException {
 
-        for (HeaderWriter factory : factories) {
+        for (HeaderWriter factory : headerWriters) {
             factory.writeHeaders(request, response);
         }
         filterChain.doFilter(request, response);

web/src/main/java/org/springframework/security/web/headers/StaticHeadersWriter.java

@@ -3,8 +3,6 @@ package org.springframework.security.web.headers;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
-import org.springframework.util.Assert;
-
 /**
  * {@code HeaderWriter} implementation which writes the same {@code Header} instance.
  *
@@ -15,11 +13,13 @@ public class StaticHeadersWriter implements HeaderWriter {
 
     private final Header header;
 
-    public StaticHeadersWriter(String name, String... values) {
-        Assert.hasText(name, "Header name is required");
-        Assert.notEmpty(values, "Header values cannot be null or empty");
-        Assert.noNullElements(values, "Header values cannot contain null values");
-        header = new Header(name, values);
+    /**
+     * Creates a new instance
+     * @param headerName the name of the header
+     * @param headerValues the values for the header
+     */
+    public StaticHeadersWriter(String headerName, String... headerValues) {
+        header = new Header(headerName, headerValues);
     }
 
     public void writeHeaders(HttpServletRequest request, HttpServletResponse response) {
@@ -27,4 +27,4 @@ public class StaticHeadersWriter implements HeaderWriter {
             response.addHeader(header.getName(), value);
         }
     }
-}
+}

web/src/main/java/org/springframework/security/web/headers/frameoptions/AbstractRequestParameterAllowFromStrategy.java

@@ -0,0 +1,58 @@
+package org.springframework.security.web.headers.frameoptions;
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.springframework.util.Assert;
+import org.springframework.util.StringUtils;
+
+import javax.servlet.http.HttpServletRequest;
+
+/**
+ * Base class for AllowFromStrategy implementations which use a request parameter to retrieve the origin. By default
+ * the parameter named <code>x-frames-allow-from</code> is read from the request.
+ *
+ * @author Marten Deinum
+ * @since 3.2
+ */
+public abstract class AbstractRequestParameterAllowFromStrategy implements AllowFromStrategy {
+
+    private static final String DEFAULT_ORIGIN_REQUEST_PARAMETER = "x-frames-allow-from";
+
+    private String allowFromParameterName = DEFAULT_ORIGIN_REQUEST_PARAMETER;
+
+    /** Logger for use by subclasses */
+    protected final Log log = LogFactory.getLog(getClass());
+
+
+    public String getAllowFromValue(HttpServletRequest request) {
+        String allowFromOrigin = request.getParameter(allowFromParameterName);
+        if (log.isDebugEnabled()) {
+            log.debug("Supplied origin '"+allowFromOrigin+"'");
+        }
+        if (StringUtils.hasText(allowFromOrigin) && allowed(allowFromOrigin)) {
+            return "ALLOW-FROM " + allowFromOrigin;
+        } else {
+            return "DENY";
+        }
+    }
+
+    /**
+     * Sets the HTTP parameter used to retrieve the value for the origin that is
+     * allowed from. The value of the parameter should be a valid URL. The
+     * default parameter name is "x-frames-allow-from".
+     *
+     * @param allowFromParameterName the name of the HTTP parameter to
+     */
+    public void setAllowFromParameterName(String allowFromParameterName) {
+        Assert.notNull(allowFromParameterName, "allowFromParameterName cannot be null");
+        this.allowFromParameterName = allowFromParameterName;
+    }
+
+    /**
+     * Method to be implemented by base classes, used to determine if the supplied origin is allowed.
+     *
+     * @param allowFromOrigin the supplied origin
+     * @return <code>true</code> if the supplied origin is allowed.
+     */
+    protected abstract boolean allowed(String allowFromOrigin);
+}

web/src/main/java/org/springframework/security/web/headers/frameoptions/AllowFromStrategy.java

@@ -11,5 +11,12 @@ import javax.servlet.http.HttpServletRequest;
  */
 public interface AllowFromStrategy {
 
-    String apply(HttpServletRequest request);
+    /**
+     * Gets the value for ALLOW-FROM excluding the ALLOW-FROM. For example, the
+     * result might be "https://example.com/".
+     *
+     * @param request the {@link HttpServletRequest}
+     * @return the value for ALLOW-FROM or null if no header should be added for this request.
+     */
+    String getAllowFromValue(HttpServletRequest request);
 }

web/src/main/java/org/springframework/security/web/headers/frameoptions/FrameOptionsHeaderWriter.java

@@ -1,44 +0,0 @@
-package org.springframework.security.web.headers.frameoptions;
-
-import org.springframework.security.web.headers.HeaderWriter;
-
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
-
-/**
- * {@code HeaderWriter} implementation for the X-Frame-Options headers. When using the ALLOW-FROM directive the actual
- * value is determined by a {@code AllowFromStrategy}.
- *
- * @author Marten Deinum
- * @since 3.2
- *
- * @see AllowFromStrategy
- */
-public class FrameOptionsHeaderWriter implements HeaderWriter {
-
-    public static final String FRAME_OPTIONS_HEADER = "X-Frame-Options";
-
-    private static final String ALLOW_FROM = "ALLOW-FROM";
-
-    private final AllowFromStrategy allowFromStrategy;
-    private final String mode;
-
-    public FrameOptionsHeaderWriter(String mode) {
-        this(mode, new NullAllowFromStrategy());
-    }
-
-    public FrameOptionsHeaderWriter(String mode, AllowFromStrategy allowFromStrategy) {
-        this.mode=mode;
-        this.allowFromStrategy=allowFromStrategy;
-    }
-
-    public void writeHeaders(HttpServletRequest request, HttpServletResponse response) {
-        if (ALLOW_FROM.equals(mode)) {
-            String value = allowFromStrategy.apply(request);
-            response.addHeader(FRAME_OPTIONS_HEADER, ALLOW_FROM + " " + value);
-        } else {
-            response.addHeader(FRAME_OPTIONS_HEADER, mode);
-        }
-    }
-
-}

web/src/main/java/org/springframework/security/web/headers/frameoptions/NullAllowFromStrategy.java

@@ -1,16 +0,0 @@
-package org.springframework.security.web.headers.frameoptions;
-
-import javax.servlet.http.HttpServletRequest;
-
-/**
- * Created with IntelliJ IDEA.
- * User: marten
- * Date: 30-01-13
- * Time: 11:06
- * To change this template use File | Settings | File Templates.
- */
-public class NullAllowFromStrategy implements AllowFromStrategy {
-    public String apply(HttpServletRequest request) {
-        return null;
-    }
-}

web/src/main/java/org/springframework/security/web/headers/frameoptions/RegExpAllowFromStrategy.java

@@ -5,22 +5,31 @@ import org.springframework.util.Assert;
 import java.util.regex.Pattern;
 
 /**
- * Implementation which uses a regular expression to validate the supplied origin.
+ * Implementation which uses a regular expression to validate the supplied
+ * origin. If the value of the HTTP parameter matches the pattern, then the the
+ * result will be ALLOW-FROM <paramter-value>.
  *
  * @author Marten Deinum
  * @since 3.2
  */
-public class RegExpAllowFromStrategy extends RequestParameterAllowFromStrategy {
+public class RegExpAllowFromStrategy extends AbstractRequestParameterAllowFromStrategy {
 
     private final Pattern pattern;
 
+    /**
+     * Creates a new instance
+     *
+     * @param pattern
+     *            the Pattern to compare against the HTTP parameter value. If
+     *            the pattern matches, the domain will be allowed, else denied.
+     */
     public RegExpAllowFromStrategy(String pattern) {
         Assert.hasText(pattern, "Pattern cannot be empty.");
         this.pattern = Pattern.compile(pattern);
     }
 
     @Override
-    protected boolean allowed(String from) {
-        return pattern.matcher(from).matches();
+    protected boolean allowed(String allowFromOrigin) {
+        return pattern.matcher(allowFromOrigin).matches();
     }
 }

web/src/main/java/org/springframework/security/web/headers/frameoptions/RequestParameterAllowFromStrategy.java

@@ -1,50 +0,0 @@
-package org.springframework.security.web.headers.frameoptions;
-
-import org.apache.commons.logging.Log;
-import org.apache.commons.logging.LogFactory;
-import org.springframework.util.StringUtils;
-
-import javax.servlet.http.HttpServletRequest;
-
-/**
- * Base class for AllowFromStrategy implementations which use a request parameter to retrieve the origin. By default
- * the parameter named <code>from</code> is read from the request.
- *
- * @author Marten Deinum
- * @since 3.2
- */
-public abstract class RequestParameterAllowFromStrategy implements AllowFromStrategy {
-
-
-    private static final String DEFAULT_ORIGIN_REQUEST_PARAMETER = "from";
-
-    private String parameter = DEFAULT_ORIGIN_REQUEST_PARAMETER;
-
-    /** Logger for use by subclasses */
-    protected final Log log = LogFactory.getLog(getClass());
-
-
-    public String apply(HttpServletRequest request) {
-        String from = request.getParameter(parameter);
-        if (log.isDebugEnabled()) {
-            log.debug("Supplied origin '"+from+"'");
-        }
-        if (StringUtils.hasText(from) && allowed(from)) {
-            return "ALLOW-FROM " + from;
-        } else {
-            return "DENY";
-        }
-    }
-
-    public void setParameterName(String parameter) {
-        this.parameter=parameter;
-    }
-
-    /**
-     * Method to be implemented by base classes, used to determine if the supplied origin is allowed.
-     *
-     * @param from the supplied origin
-     * @return <code>true</code> if the supplied origin is allowed.
-     */
-    protected abstract boolean allowed(String from);
-}

web/src/main/java/org/springframework/security/web/headers/frameoptions/StaticAllowFromStrategy.java

@@ -14,7 +14,7 @@ public class StaticAllowFromStrategy implements AllowFromStrategy {
         this.uri=uri;
     }
 
-    public String apply(HttpServletRequest request) {
+    public String getAllowFromValue(HttpServletRequest request) {
         return uri.toString();
     }
 }

web/src/main/java/org/springframework/security/web/headers/frameoptions/WhiteListedAllowFromStrategy.java

@@ -1,9 +1,8 @@
 package org.springframework.security.web.headers.frameoptions;
 
-import org.springframework.util.Assert;
-
 import java.util.Collection;
-import java.util.List;
+
+import org.springframework.util.Assert;
 
 /**
  * Implementation which checks the supplied origin against a list of allowed origins.
@@ -11,17 +10,21 @@ import java.util.List;
  * @author Marten Deinum
  * @since 3.2
  */
-public class WhiteListedAllowFromStrategy extends RequestParameterAllowFromStrategy {
+public class WhiteListedAllowFromStrategy extends AbstractRequestParameterAllowFromStrategy {
 
     private final Collection<String> allowed;
 
+    /**
+     * Creates a new instance
+     * @param allowed the origins that are allowed.
+     */
     public WhiteListedAllowFromStrategy(Collection<String> allowed) {
         Assert.notEmpty(allowed, "Allowed origins cannot be empty.");
         this.allowed = allowed;
     }
 
     @Override
-    protected boolean allowed(String from) {
-        return allowed.contains(from);
+    protected boolean allowed(String allowFromOrigin) {
+        return allowed.contains(allowFromOrigin);
     }
 }

web/src/main/java/org/springframework/security/web/headers/frameoptions/XFrameOptionsHeaderWriter.java

@@ -0,0 +1,110 @@
+/*
+ * Copyright 2002-2013 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.springframework.security.web.headers.frameoptions;
+
+import org.springframework.security.web.headers.HeaderWriter;
+import org.springframework.util.Assert;
+
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+/**
+ * {@code HeaderWriter} implementation for the X-Frame-Options headers. When using the ALLOW-FROM directive the actual
+ * value is determined by a {@code AllowFromStrategy}.
+ *
+ * @author Marten Deinum
+ * @author Rob Winch
+ * @since 3.2
+ *
+ * @see AllowFromStrategy
+ */
+public class XFrameOptionsHeaderWriter implements HeaderWriter {
+
+    public static final String XFRAME_OPTIONS_HEADER = "X-Frame-Options";
+
+    private final AllowFromStrategy allowFromStrategy;
+    private final XFrameOptionsMode frameOptionsMode;
+
+    /**
+     * Creates a new instance
+     *
+     * @param frameOptionsMode
+     *            the {@link XFrameOptionsMode} to use. If using
+     *            {@link XFrameOptionsMode#ALLOW_FROM}, use
+     *            {@link #FrameOptionsHeaderWriter(AllowFromStrategy)}
+     *            instead.
+     */
+    public XFrameOptionsHeaderWriter(XFrameOptionsMode frameOptionsMode) {
+        Assert.notNull(frameOptionsMode, "frameOptionsMode cannot be null");
+        if(XFrameOptionsMode.ALLOW_FROM.equals(frameOptionsMode)) {
+            throw new IllegalArgumentException(
+                    "ALLOW_FROM requires an AllowFromStrategy. Please use FrameOptionsHeaderWriter(AllowFromStrategy allowFromStrategy) instead");
+        }
+        this.frameOptionsMode = frameOptionsMode;
+        this.allowFromStrategy = null;
+    }
+
+    /**
+     * Creates a new instance with {@link XFrameOptionsMode#ALLOW_FROM}.
+     *
+     * @param allowFromStrategy
+     *            the strategy for determining what the value for ALLOW_FROM is.
+     */
+    public XFrameOptionsHeaderWriter(AllowFromStrategy allowFromStrategy) {
+        Assert.notNull(allowFromStrategy, "allowFromStrategy cannot be null");
+        this.frameOptionsMode = XFrameOptionsMode.ALLOW_FROM;
+        this.allowFromStrategy = allowFromStrategy;
+    }
+
+    public void writeHeaders(HttpServletRequest request, HttpServletResponse response) {
+        if (XFrameOptionsMode.ALLOW_FROM.equals(frameOptionsMode)) {
+            String allowFromValue = allowFromStrategy.getAllowFromValue(request);
+            if(allowFromValue != null) {
+                response.addHeader(XFRAME_OPTIONS_HEADER, XFrameOptionsMode.ALLOW_FROM.getMode() + " " + allowFromValue);
+            }
+        } else {
+            response.addHeader(XFRAME_OPTIONS_HEADER, frameOptionsMode.getMode());
+        }
+    }
+
+    /**
+     * The possible values for the X-Frame-Options header.
+     *
+     * @author Rob Winch
+     * @since 3.2
+     */
+    public enum XFrameOptionsMode {
+        DENY("DENY"),
+        SAMEORIGIN("SAMEORIGIN"),
+        ALLOW_FROM("ALLOW-FROM");
+
+        private String mode;
+
+        private XFrameOptionsMode(String mode) {
+            this.mode = mode;
+        }
+
+        /**
+         * Gets the mode for the X-Frame-Options header value. For example,
+         * DENY, SAMEORIGIN, ALLOW-FROM. Cannot be null.
+         *
+         * @return the mode for the X-Frame-Options header value.
+         */
+        public String getMode() {
+            return mode;
+        }
+    }
+}

web/src/test/java/org/springframework/security/web/headers/HeadersFilterTest.java → web/src/test/java/org/springframework/security/web/headers/HeadersFilterTests.java

@@ -15,7 +15,7 @@
  */
 package org.springframework.security.web.headers;
 
-import static org.junit.Assert.assertTrue;
+import static org.fest.assertions.Assertions.assertThat;
 import static org.mockito.Mockito.verify;
 
 import java.util.ArrayList;
@@ -33,27 +33,26 @@ import org.springframework.mock.web.MockHttpServletResponse;
  * Tests for the {@code HeadersFilter}
  *
  * @author Marten Deinum
+ * @author Rob Winch
  * @since 3.2
  */
 @RunWith(MockitoJUnitRunner.class)
-public class HeadersFilterTest {
+public class HeadersFilterTests {
     @Mock
     private HeaderWriter writer1;
 
     @Mock
     private HeaderWriter writer2;
 
-    @Test
+    @Test(expected = IllegalArgumentException.class)
     public void noHeadersConfigured() throws Exception {
         List<HeaderWriter> headerWriters = new ArrayList<HeaderWriter>();
-        HeadersFilter filter = new HeadersFilter(headerWriters);
-        MockHttpServletRequest request = new MockHttpServletRequest();
-        MockHttpServletResponse response = new MockHttpServletResponse();
-        MockFilterChain filterChain = new MockFilterChain();
-
-        filter.doFilter(request, response, filterChain);
+        new HeadersFilter(headerWriters);
+    }
 
-        assertTrue(response.getHeaderNames().isEmpty());
+    @Test(expected = IllegalArgumentException.class)
+    public void constructorNullWriters() throws Exception {
+        new HeadersFilter(null);
     }
 
     @Test
@@ -72,5 +71,6 @@ public class HeadersFilterTest {
 
         verify(writer1).writeHeaders(request, response);
         verify(writer2).writeHeaders(request, response);
+        assertThat(filterChain.getRequest()).isEqualTo(request); // verify the filterChain continued
     }
 }

web/src/test/java/org/springframework/security/web/headers/StaticHeaderWriterTests.java

@@ -1,3 +1,18 @@
+/*
+ * Copyright 2002-2013 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
 package org.springframework.security.web.headers;
 
 import static org.fest.assertions.Assertions.assertThat;
@@ -13,6 +28,7 @@ import org.springframework.mock.web.MockHttpServletResponse;
  * Test for the {@code StaticHeadersWriter}
  *
  * @author Marten Deinum
+ * @author Rob Winch
  * @since 3.2
  */
 public class StaticHeaderWriterTests {
@@ -25,6 +41,21 @@ public class StaticHeaderWriterTests {
         response = new MockHttpServletResponse();
     }
 
+    @Test(expected = IllegalArgumentException.class)
+    public void constructorNullHeaderName() {
+        new StaticHeadersWriter(null, "value1");
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void constructorNullHeaderValues() {
+        new StaticHeadersWriter("name", (String[]) null);
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void constructorContainsNullHeaderValue() {
+        new StaticHeadersWriter("name", "value1", null);
+    }
+
     @Test
     public void sameHeaderShouldBeReturned() {
         String headerName = "X-header";

web/src/test/java/org/springframework/security/web/headers/frameoptions/AbstractRequestParameterAllowFromStrategyTests.java

@@ -0,0 +1,101 @@
+/*
+ * Copyright 2002-2013 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.springframework.security.web.headers.frameoptions;
+
+import static org.fest.assertions.Assertions.assertThat;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.springframework.mock.web.MockHttpServletRequest;
+
+/**
+ * @author Rob Winch
+ *
+ */
+public class AbstractRequestParameterAllowFromStrategyTests {
+    private MockHttpServletRequest request;
+
+    @Before
+    public void setup() {
+        request = new MockHttpServletRequest();
+    }
+
+    @Test
+    public void nullAllowFromParameterValue() {
+        RequestParameterAllowFromStrategyStub strategy = new RequestParameterAllowFromStrategyStub(true);
+
+        assertThat(
+                strategy
+                        .getAllowFromValue(request)).isEqualTo("DENY");
+    }
+
+    @Test
+    public void emptyAllowFromParameterValue() {
+        request.setParameter("x-frames-allow-from", "");
+        RequestParameterAllowFromStrategyStub strategy = new RequestParameterAllowFromStrategyStub(true);
+
+        assertThat(
+                strategy
+                        .getAllowFromValue(request)).isEqualTo("DENY");
+    }
+
+    @Test
+    public void emptyAllowFromCustomParameterValue() {
+        String customParam = "custom";
+        request.setParameter(customParam, "");
+        RequestParameterAllowFromStrategyStub strategy = new RequestParameterAllowFromStrategyStub(true);
+        strategy.setAllowFromParameterName(customParam);
+
+        assertThat(
+                strategy
+                        .getAllowFromValue(request)).isEqualTo("DENY");
+    }
+
+    @Test
+    public void allowFromParameterValueAllowed() {
+        String value = "https://example.com";
+        request.setParameter("x-frames-allow-from", value);
+        RequestParameterAllowFromStrategyStub strategy = new RequestParameterAllowFromStrategyStub(true);
+
+        assertThat(
+                strategy
+                        .getAllowFromValue(request)).isEqualTo("ALLOW-FROM "+value);
+    }
+
+    @Test
+    public void allowFromParameterValueDenied() {
+        String value = "https://example.com";
+        request.setParameter("x-frames-allow-from", value);
+        RequestParameterAllowFromStrategyStub strategy = new RequestParameterAllowFromStrategyStub(false);
+
+        assertThat(
+                strategy
+                        .getAllowFromValue(request)).isEqualTo("DENY");
+    }
+
+    private static class RequestParameterAllowFromStrategyStub extends AbstractRequestParameterAllowFromStrategy {
+        private boolean match;
+
+        RequestParameterAllowFromStrategyStub(boolean match) {
+            this.match = match;
+        }
+
+        @Override
+        protected boolean allowed(String allowFromOrigin) {
+            return match;
+        }
+    }
+}

web/src/test/java/org/springframework/security/web/headers/frameoptions/FrameOptionsHeaderWriterTests.java

@@ -0,0 +1,109 @@
+/*
+ * Copyright 2002-2013 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.springframework.security.web.headers.frameoptions;
+
+import static org.fest.assertions.Assertions.assertThat;
+import static org.mockito.Mockito.when;
+
+import java.util.Collections;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.runners.MockitoJUnitRunner;
+import org.springframework.mock.web.MockHttpServletRequest;
+import org.springframework.mock.web.MockHttpServletResponse;
+import org.springframework.security.web.headers.frameoptions.XFrameOptionsHeaderWriter.XFrameOptionsMode;
+
+/**
+ * @author Rob Winch
+ *
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class FrameOptionsHeaderWriterTests {
+    @Mock
+    private AllowFromStrategy strategy;
+
+    private MockHttpServletResponse response;
+
+    private MockHttpServletRequest request;
+
+    private XFrameOptionsHeaderWriter writer;
+
+    @Before
+    public void setup() {
+        request = new MockHttpServletRequest();
+        response = new MockHttpServletResponse();
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void constructorNullMode() {
+        new XFrameOptionsHeaderWriter((XFrameOptionsMode)null);
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void constructorAllowFromNoAllowFromStrategy() {
+        new XFrameOptionsHeaderWriter(XFrameOptionsMode.ALLOW_FROM);
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void constructorNullAllowFromStrategy() {
+        new XFrameOptionsHeaderWriter((AllowFromStrategy)null);
+    }
+
+    @Test
+    public void writeHeadersAllowFromReturnsNull() {
+        writer = new XFrameOptionsHeaderWriter(strategy);
+
+        writer.writeHeaders(request, response);
+
+        assertThat(response.getHeaderNames().isEmpty()).isTrue();
+    }
+
+    @Test
+    public void writeHeadersAllowFrom() {
+        String allowFromValue = "https://example.com/";
+        when(strategy.getAllowFromValue(request)).thenReturn(allowFromValue);
+        writer = new XFrameOptionsHeaderWriter(strategy);
+
+        writer.writeHeaders(request, response);
+
+        assertThat(response.getHeaderNames().size()).isEqualTo(1);
+        assertThat(response.getHeader(XFrameOptionsHeaderWriter.XFRAME_OPTIONS_HEADER)).isEqualTo("ALLOW-FROM " + allowFromValue);
+    }
+
+    @Test
+    public void writeHeadersDeny() {
+        writer = new XFrameOptionsHeaderWriter(XFrameOptionsMode.DENY);
+
+        writer.writeHeaders(request, response);
+
+        assertThat(response.getHeaderNames().size()).isEqualTo(1);
+        assertThat(response.getHeader(XFrameOptionsHeaderWriter.XFRAME_OPTIONS_HEADER)).isEqualTo("DENY");
+    }
+
+
+    @Test
+    public void writeHeadersSameOrigin() {
+        writer = new XFrameOptionsHeaderWriter(XFrameOptionsMode.SAMEORIGIN);
+
+        writer.writeHeaders(request, response);
+
+        assertThat(response.getHeaderNames().size()).isEqualTo(1);
+        assertThat(response.getHeader(XFrameOptionsHeaderWriter.XFRAME_OPTIONS_HEADER)).isEqualTo("SAMEORIGIN");
+    }
+}

web/src/test/java/org/springframework/security/web/headers/frameoptions/RegExpAllowFromStrategyTest.java → web/src/test/java/org/springframework/security/web/headers/frameoptions/RegExpAllowFromStrategyTests.java

@@ -1,23 +1,18 @@
 package org.springframework.security.web.headers.frameoptions;
 
-import org.junit.Test;
-import org.springframework.mock.web.MockHttpServletRequest;
+import static org.hamcrest.CoreMatchers.is;
+import static org.junit.Assert.assertThat;
 
-import java.util.regex.Pattern;
 import java.util.regex.PatternSyntaxException;
 
-import static org.hamcrest.CoreMatchers.is;
-import static org.junit.Assert.assertThat;
-import static org.junit.Assert.assertTrue;
+import org.junit.Test;
+import org.springframework.mock.web.MockHttpServletRequest;
 
 /**
- * Created with IntelliJ IDEA.
- * User: marten
- * Date: 01-02-13
- * Time: 20:25
- * To change this template use File | Settings | File Templates.
+ *
+ * @author Marten Deinum
  */
-public class RegExpAllowFromStrategyTest {
+public class RegExpAllowFromStrategyTests {
 
     @Test(expected = PatternSyntaxException.class)
     public void invalidRegularExpressionShouldLeadToException() {
@@ -32,18 +27,19 @@ public class RegExpAllowFromStrategyTest {
     @Test
     public void subdomainMatchingRegularExpression() {
         RegExpAllowFromStrategy strategy = new RegExpAllowFromStrategy("^http://([a-z0-9]*?\\.)test\\.com");
+        strategy.setAllowFromParameterName("from");
         MockHttpServletRequest request = new MockHttpServletRequest();
 
         request.setParameter("from", "http://abc.test.com");
-        String result1 = strategy.apply(request);
+        String result1 = strategy.getAllowFromValue(request);
         assertThat(result1, is("ALLOW-FROM http://abc.test.com"));
 
         request.setParameter("from", "http://foo.test.com");
-        String result2 = strategy.apply(request);
+        String result2 = strategy.getAllowFromValue(request);
         assertThat(result2, is("ALLOW-FROM http://foo.test.com"));
 
         request.setParameter("from", "http://test.foobar.com");
-        String result3 = strategy.apply(request);
+        String result3 = strategy.getAllowFromValue(request);
         assertThat(result3, is("DENY"));
     }
 
@@ -51,16 +47,7 @@ public class RegExpAllowFromStrategyTest {
     public void noParameterShouldDeny() {
         RegExpAllowFromStrategy strategy = new RegExpAllowFromStrategy("^http://([a-z0-9]*?\\.)test\\.com");
         MockHttpServletRequest request = new MockHttpServletRequest();
-        String result1 = strategy.apply(request);
+        String result1 = strategy.getAllowFromValue(request);
         assertThat(result1, is("DENY"));
     }
-
-    @Test
-    public void test() {
-        String pattern = "^http://([a-z0-9]*?\\.)test\\.com";
-        Pattern p = Pattern.compile(pattern);
-        String url = "http://abc.test.com";
-        assertTrue(p.matcher(url).matches());
-    }
-
 }

web/src/test/java/org/springframework/security/web/headers/frameoptions/StaticAllowFromStrategyTest.java → web/src/test/java/org/springframework/security/web/headers/frameoptions/StaticAllowFromStrategyTests.java

@@ -13,12 +13,12 @@ import static org.junit.Assert.assertEquals;
  * @author Marten Deinum
  * @since 3.2
  */
-public class StaticAllowFromStrategyTest {
+public class StaticAllowFromStrategyTests {
 
     @Test
     public void shouldReturnUri() {
         String uri = "http://www.test.com";
         StaticAllowFromStrategy strategy = new StaticAllowFromStrategy(URI.create(uri));
-        assertEquals(uri, strategy.apply(new MockHttpServletRequest()));
+        assertEquals(uri, strategy.getAllowFromValue(new MockHttpServletRequest()));
     }
 }

web/src/test/java/org/springframework/security/web/headers/frameoptions/WhiteListedAllowFromStrategyTest.java → web/src/test/java/org/springframework/security/web/headers/frameoptions/WhiteListedAllowFromStrategyTests.java

@@ -15,7 +15,7 @@ import static org.springframework.test.util.MatcherAssertionErrors.assertThat;
  * @author Marten Deinum
  * @since 3.2
  */
-public class WhiteListedAllowFromStrategyTest {
+public class WhiteListedAllowFromStrategyTests {
 
     @Test(expected = IllegalArgumentException.class)
     public void emptyListShouldThrowException() {
@@ -32,10 +32,11 @@ public class WhiteListedAllowFromStrategyTest {
         List<String> allowed = new ArrayList<String>();
         allowed.add("http://www.test.com");
         WhiteListedAllowFromStrategy strategy = new WhiteListedAllowFromStrategy(allowed);
+        strategy.setAllowFromParameterName("from");
         MockHttpServletRequest request = new MockHttpServletRequest();
         request.setParameter("from", "http://www.test.com");
 
-        String result = strategy.apply(request);
+        String result = strategy.getAllowFromValue(request);
         assertThat(result, is("ALLOW-FROM http://www.test.com"));
     }
 
@@ -45,10 +46,11 @@ public class WhiteListedAllowFromStrategyTest {
         allowed.add("http://www.test.com");
         allowed.add("http://www.springsource.org");
         WhiteListedAllowFromStrategy strategy = new WhiteListedAllowFromStrategy(allowed);
+        strategy.setAllowFromParameterName("from");
         MockHttpServletRequest request = new MockHttpServletRequest();
         request.setParameter("from", "http://www.test.com");
 
-        String result = strategy.apply(request);
+        String result = strategy.getAllowFromValue(request);
         assertThat(result, is("ALLOW-FROM http://www.test.com"));
     }
 
@@ -57,10 +59,11 @@ public class WhiteListedAllowFromStrategyTest {
         List<String> allowed = new ArrayList<String>();
         allowed.add("http://www.test.com");
         WhiteListedAllowFromStrategy strategy = new WhiteListedAllowFromStrategy(allowed);
+        strategy.setAllowFromParameterName("from");
         MockHttpServletRequest request = new MockHttpServletRequest();
         request.setParameter("from", "http://www.test123.com");
 
-        String result = strategy.apply(request);
+        String result = strategy.getAllowFromValue(request);
         assertThat(result, is("DENY"));
     }
 
@@ -69,9 +72,10 @@ public class WhiteListedAllowFromStrategyTest {
         List<String> allowed = new ArrayList<String>();
         allowed.add("http://www.test.com");
         WhiteListedAllowFromStrategy strategy = new WhiteListedAllowFromStrategy(allowed);
+        strategy.setAllowFromParameterName("from");
         MockHttpServletRequest request = new MockHttpServletRequest();
 
-        String result = strategy.apply(request);
+        String result = strategy.getAllowFromValue(request);
         assertThat(result, is("DENY"));
 
     }