SEC-2098, SEC-2099: Fix build

  - hf.doFilter is missing FilterChain argument
  - response.headers does not contain the exact values for the headers so
    should not be used for comparison (note it is a private member so this
    is acceptable)
  - hf does not need non-null check when hf.doFilter is invoked
  - some of the configurations are no longer valid (i.e. ALLOW-FROM
    requires strategy)
  - Some error messages needed updated (some could still use improvement)
  - No validation for missing header name or value
  - rebased off master / merged
  - nsa=frame-options-strategy id should use - not =
  - FramewOptionsHeaderFactory did not produce "ALLOW-FROM " prefix of origin
  - remove @Override on interface overrides to work with JDK5

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

@@ -17,6 +17,7 @@ import org.springframework.security.util.FieldUtils
 import javax.servlet.Filter
 import javax.servlet.http.HttpServletRequest
 
+import org.springframework.beans.factory.BeanCreationException;
 import org.springframework.beans.factory.parsing.BeanDefinitionParsingException
 import org.springframework.beans.factory.xml.XmlBeanDefinitionStoreException;
 import org.springframework.mock.web.MockFilterChain
@@ -56,11 +57,10 @@ class HttpHeadersConfigTests extends AbstractHttpConfigTests {
         createAppContext()
 
         def hf = getFilter(HeadersFilter)
-        MockHttpServletResponse response = new MockHttpServletResponse();
-        hf.doFilter(new MockHttpServletRequest(), response);
+        MockHttpServletResponse response = new MockHttpServletResponse()
+        hf.doFilter(new MockHttpServletRequest(), response, new MockFilterChain())
 
         expect:
-        hf
         response.headers.isEmpty()
     }
 
@@ -73,11 +73,11 @@ class HttpHeadersConfigTests extends AbstractHttpConfigTests {
         createAppContext()
 
         def hf = getFilter(HeadersFilter)
-        MockHttpServletResponse response = new MockHttpServletResponse();
-        hf.doFilter(new MockHttpServletRequest(), response);
+        MockHttpServletResponse response = new MockHttpServletResponse()
+        hf.doFilter(new MockHttpServletRequest(), response, new MockFilterChain())
+
         expect:
-        hf
-        response.headers == ['X-Content-Type-Options':'nosniff']
+        assertHeaders(response, ['X-Content-Type-Options':'nosniff'])
     }
 
     def 'http headers frame-options defaults to DENY'() {
@@ -89,10 +89,11 @@ class HttpHeadersConfigTests extends AbstractHttpConfigTests {
         createAppContext()
 
         def hf = getFilter(HeadersFilter)
+        MockHttpServletResponse response = new MockHttpServletResponse()
+        hf.doFilter(new MockHttpServletRequest(), response, new MockFilterChain())
 
         expect:
-        hf
-        hf.headers == ['X-Frame-Options':'DENY']
+        assertHeaders(response, ['X-Frame-Options':'DENY'])
     }
 
     def 'http headers frame-options DENY'() {
@@ -104,10 +105,11 @@ class HttpHeadersConfigTests extends AbstractHttpConfigTests {
         createAppContext()
 
         def hf = getFilter(HeadersFilter)
+        MockHttpServletResponse response = new MockHttpServletResponse()
+        hf.doFilter(new MockHttpServletRequest(), response, new MockFilterChain())
 
         expect:
-        hf
-        hf.headers == ['X-Frame-Options':'DENY']
+        assertHeaders(response, ['X-Frame-Options':'DENY'])
     }
 
     def 'http headers frame-options SAMEORIGIN'() {
@@ -119,17 +121,18 @@ class HttpHeadersConfigTests extends AbstractHttpConfigTests {
         createAppContext()
 
         def hf = getFilter(HeadersFilter)
+        MockHttpServletResponse response = new MockHttpServletResponse()
+        hf.doFilter(new MockHttpServletRequest(), response, new MockFilterChain())
 
         expect:
-        hf
-        hf.headers == ['X-Frame-Options':'SAMEORIGIN']
+        assertHeaders(response, ['X-Frame-Options':'SAMEORIGIN'])
     }
 
     def 'http headers frame-options ALLOW-FROM no origin reports error'() {
         when:
         httpAutoConfig {
             'headers'() {
-                'frame-options'(policy : 'ALLOW-FROM')
+                'frame-options'(policy : 'ALLOW-FROM', strategy : 'static')
             }
         }
         createAppContext()
@@ -138,14 +141,14 @@ class HttpHeadersConfigTests extends AbstractHttpConfigTests {
 
         then:
         BeanDefinitionParsingException e = thrown()
-        e.message.contains '<frame-options policy="ALLOW-FROM"/> requires a non-empty string value for the origin attribute to be specified.'
+        e.message.contains "Strategy requires a 'value' to be set." // FIME better error message?
     }
 
     def 'http headers frame-options ALLOW-FROM spaces only origin reports error'() {
         when:
         httpAutoConfig {
             'headers'() {
-                'frame-options'(policy : 'ALLOW-FROM', origin : ' ')
+                'frame-options'(policy : 'ALLOW-FROM', strategy: 'static', value : ' ')
             }
         }
         createAppContext()
@@ -154,23 +157,24 @@ class HttpHeadersConfigTests extends AbstractHttpConfigTests {
 
         then:
         BeanDefinitionParsingException e = thrown()
-        e.message.contains '<frame-options policy="ALLOW-FROM"/> requires a non-empty string value for the origin attribute to be specified.'
+        e.message.contains "Strategy requires a 'value' to be set." // FIME better error message?
     }
 
     def 'http headers frame-options ALLOW-FROM'() {
         when:
         httpAutoConfig {
             'headers'() {
-                'frame-options'(policy : 'ALLOW-FROM', origin : 'https://example.com')
+                'frame-options'(policy : 'ALLOW-FROM', strategy: 'static', value : 'https://example.com')
             }
         }
         createAppContext()
 
         def hf = getFilter(HeadersFilter)
+        MockHttpServletResponse response = new MockHttpServletResponse()
+        hf.doFilter(new MockHttpServletRequest(), response, new MockFilterChain())
 
         then:
-        hf
-        hf.headers == ['X-Frame-Options':'ALLOW-FROM https://example.com']
+        assertHeaders(response, ['X-Frame-Options':'ALLOW-FROM https://example.com'])
     }
 
     def 'http headers header a=b'() {
@@ -183,10 +187,11 @@ class HttpHeadersConfigTests extends AbstractHttpConfigTests {
         createAppContext()
 
         def hf = getFilter(HeadersFilter)
+        MockHttpServletResponse response = new MockHttpServletResponse()
+        hf.doFilter(new MockHttpServletRequest(), response, new MockFilterChain())
 
         then:
-        hf
-        hf.headers == ['a':'b']
+        assertHeaders(response, ['a':'b'])
     }
 
     def 'http headers header a=b and c=d'() {
@@ -200,10 +205,11 @@ class HttpHeadersConfigTests extends AbstractHttpConfigTests {
         createAppContext()
 
         def hf = getFilter(HeadersFilter)
+        MockHttpServletResponse response = new MockHttpServletResponse()
+        hf.doFilter(new MockHttpServletRequest(), response, new MockFilterChain())
 
         then:
-        hf
-        hf.headers.sort() == ['a':'b', 'c':'d'].sort()
+        assertHeaders(response , ['a':'b', 'c':'d'])
     }
 
     def 'http headers header no name produces error'() {
@@ -216,7 +222,7 @@ class HttpHeadersConfigTests extends AbstractHttpConfigTests {
         createAppContext()
 
         then:
-        thrown(XmlBeanDefinitionStoreException)
+        thrown(BeanCreationException)
     }
 
     def 'http headers header no value produces error'() {
@@ -229,7 +235,7 @@ class HttpHeadersConfigTests extends AbstractHttpConfigTests {
         createAppContext()
 
         then:
-        thrown(XmlBeanDefinitionStoreException)
+        thrown(BeanCreationException)
     }
 
     def 'http headers xss-protection defaults'() {
@@ -242,10 +248,11 @@ class HttpHeadersConfigTests extends AbstractHttpConfigTests {
         createAppContext()
 
         def hf = getFilter(HeadersFilter)
+        MockHttpServletResponse response = new MockHttpServletResponse()
+        hf.doFilter(new MockHttpServletRequest(), response, new MockFilterChain())
 
         then:
-        hf
-        hf.headers == ['X-XSS-Protection':'1; mode=block']
+        assertHeaders(response, ['X-XSS-Protection':'1; mode=block'])
     }
 
     def 'http headers xss-protection enabled=true'() {
@@ -258,10 +265,11 @@ class HttpHeadersConfigTests extends AbstractHttpConfigTests {
         createAppContext()
 
         def hf = getFilter(HeadersFilter)
+        MockHttpServletResponse response = new MockHttpServletResponse()
+        hf.doFilter(new MockHttpServletRequest(), response, new MockFilterChain())
 
         then:
-        hf
-        hf.headers == ['X-XSS-Protection':'1; mode=block']
+        assertHeaders(response, ['X-XSS-Protection':'1; mode=block'])
     }
 
     def 'http headers xss-protection enabled=false'() {
@@ -274,10 +282,11 @@ class HttpHeadersConfigTests extends AbstractHttpConfigTests {
         createAppContext()
 
         def hf = getFilter(HeadersFilter)
+        MockHttpServletResponse response = new MockHttpServletResponse()
+        hf.doFilter(new MockHttpServletRequest(), response, new MockFilterChain())
 
         then:
-        hf
-        hf.headers == ['X-XSS-Protection':'0']
+        assertHeaders(response, ['X-XSS-Protection':'0'])
     }
 
     def 'http headers xss-protection enabled=false and block=true produces error'() {
@@ -295,4 +304,11 @@ class HttpHeadersConfigTests extends AbstractHttpConfigTests {
         BeanDefinitionParsingException e = thrown()
         e.message.contains '<xss-protection enabled="false"/> does not allow block="true".'
     }
+
+    def assertHeaders(MockHttpServletResponse response, Map<String,String> expected) {
+        assert response.headerNames == expected.keySet()
+        expected.each { headerName, value ->
+            assert response.getHeaderValues(headerName) == [value]
+        }
+    }
 }

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

@@ -319,7 +319,7 @@
                         including it in a frame it is the same as the one serving the page.
                     </para>
                 </section>
-                <section xml:id="nsa=frame-options-strategy">
+                <section xml:id="nsa-frame-options-strategy">
                     <title><literal>frame-options-strategy</literal></title>
                     <para>
                         Select the <classname>AllowFromStrategy</classname> to use when using the ALLOW-FROM policy.

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

@@ -3,6 +3,8 @@ package org.springframework.security.web.headers;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
+import org.springframework.util.Assert;
+
 /**
  * {@code HeaderFactory} implementation which returns the same {@code Header} instance.
  *
@@ -14,6 +16,9 @@ public class StaticHeaderFactory implements HeaderFactory {
     private final Header header;
 
     public StaticHeaderFactory(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);
     }
 

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

@@ -33,11 +33,10 @@ public class FrameOptionsHeaderFactory implements HeaderFactory {
         this.allowFromStrategy=allowFromStrategy;
     }
 
-    @Override
     public Header create(HttpServletRequest request, HttpServletResponse response) {
         if (ALLOW_FROM.equals(mode)) {
             String value = allowFromStrategy.apply(request);
-            return new Header(FRAME_OPTIONS_HEADER, value);
+            return new Header(FRAME_OPTIONS_HEADER, ALLOW_FROM + " " + value);
         } else {
             return new Header(FRAME_OPTIONS_HEADER, mode);
         }

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

@@ -10,7 +10,6 @@ import javax.servlet.http.HttpServletRequest;
  * To change this template use File | Settings | File Templates.
  */
 public class NullAllowFromStrategy implements AllowFromStrategy {
-    @Override
     public String apply(HttpServletRequest request) {
         return null;
     }

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

@@ -24,7 +24,6 @@ public abstract class RequestParameterAllowFromStrategy implements AllowFromStra
     protected final Log log = LogFactory.getLog(getClass());
 
 
-    @Override
     public String apply(HttpServletRequest request) {
         String from = request.getParameter(parameter);
         if (log.isDebugEnabled()) {

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

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

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

@@ -84,7 +84,6 @@ public class HeadersFilterTest {
         private String name;
         private String value;
 
-        @Override
         public Header create(HttpServletRequest request, HttpServletResponse response) {
             return new Header(name, value);
         }