SEC-2098, SEC-2099 #24

mdeinum · 2013-01-06T19:37:32Z

AddHeadersFilter for setting security headers added including a bean definition parser for easy configuration of the headers. Enables easy configuration for the X-Frame-Options, X-XSS-Protection and X-Content-Type-Options headers. Also allows for additional headers to be added.

I have signed and agree to the terms of the SpringSource Individual
Contributor License Agreement.

Issues: SEC-2098, SEC-2099

AddHeadersFilter for setting security headers added including a bean definition parser for easy configuration of the headers. Enables easy configuration for the X-Frame-Options, X-XSS-Protection and X-Content-Type-Options headers. Also allows for additional headers to be added.

Conflicts: config/src/main/java/org/springframework/security/config/http/AddHeadersBeanDefinitionParser.java config/src/main/resources/org/springframework/security/config/spring-security-3.2.xsd web/src/main/java/org/springframework/security/web/headers/AddHeadersFilter.java

rwinch · 2013-01-07T19:42:19Z

config/src/main/java/org/springframework/security/config/SecurityNamespaceHandler.java

@@ -180,7 +180,7 @@ private boolean namespaceMatchesVersion(Element element) {

    private boolean matchesVersionInternal(Element element) {
        String schemaLocation = element.getAttributeNS("http://www.w3.org/2001/XMLSchema-instance", "schemaLocation");
-        return schemaLocation.matches("(?m).*spring-security-3\\.1.*.xsd.*")
+        return schemaLocation.matches("(?m).*spring-security-3\\.[12].*.xsd.*")


Historically Spring Security's namespace support only supports the current version, so this should only match on 3.2. I could probably be convinced to support other versions, but if we want to do this it should be a distinct JIRA / commit.

Fixed this. Should we also make sure that references to the 3.1 xsd in the documentation are updated to 3.2?

Also updated documentation references to 3.2 as well as the other test cases which load a security based xml file.

rwinch · 2013-01-07T20:12:37Z

@mdeinum - Thanks for your contribution! Some feedback on the pull request:

Can you please rebase your commit so that it merges with master?
Squashing the commits after the rebase is highly preferred (i.e. single commit per JIRA).
Typically we try to keep a single JIRA per pull request / commit. If the code really makes sense in a single commit, then it is probably a single JIRA too. We can probably let this slide since issues are so closely related and you have already done it this way, but it is more of an FYI for the future.
I also have provided some comments on the code. Please feel free to respond if you think my comment is incorrect.
Note if you do not have time to do any of the requested items, please let me know and I can apply the changes. Your contribution as it is will be quite valuable!

rwinch · 2013-01-08T15:37:07Z

config/src/main/java/org/springframework/security/config/SecurityNamespaceHandler.java

@@ -180,7 +181,7 @@ private boolean namespaceMatchesVersion(Element element) {

    private boolean matchesVersionInternal(Element element) {
        String schemaLocation = element.getAttributeNS("http://www.w3.org/2001/XMLSchema-instance", "schemaLocation");
-        return schemaLocation.matches("(?m).*spring-security-3\\.1.*.xsd.*")
+        return schemaLocation.matches("(?m).*spring-security-3\\.2.*.xsd.*")


I saw your response in my email to a previous comment I had of "Fixed this. Should we also make sure that references to the 3.1 xsd in the documentation are updated to 3.2?" but cannot find it on github. Yes...this is a good catch if you can fix the references in the documentation that would be good too.

AddHeadersFilter for setting security headers added including a bean definition parser for easy configuration of the headers. Enables easy configuration for the X-Frame-Options, X-XSS-Protection and X-Content-Type-Options headers. Also allows for additional headers to be added. Issues: SEC-2098, SEC-2099 AddHeadersFilter for setting security headers added including a bean definition parser for easy configuration of the headers. Enables easy configuration for the X-Frame-Options, X-XSS-Protection and X-Content-Type-Options headers. Also allows for additional headers to be added. Processing comments.

As of Spring 3.1 spring has its own cache abstraction. This commit adds cache imlpementations based on that abstraction.

"Créances" is not the right translation. "Identifications" is a lot better in this case.

AddHeadersFilter for setting security headers added including a bean definition parser for easy configuration of the headers. Enables easy configuration for the X-Frame-Options, X-XSS-Protection and X-Content-Type-Options headers. Also allows for additional headers to be added.

mdeinum · 2013-01-09T18:25:34Z

I did a rebase and did some squashing, hopefully it is better now... After some documentation changes I have a failing test again. However I don't see what is wrong (maybe you can have a peek again).

rwinch · 2013-01-09T19:56:55Z

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

@@ -29,6 +29,7 @@
    CONCURRENT_SESSION_FILTER,
    /** {@link WebAsyncManagerIntegrationFilter} */
    WEB_ASYNC_MANAGER_FILTER,


Renamed to HEADERS_FILTER and added to the namespace-config. I noticed that also the WEB_ASYNC_MANAGER_FILTER wasn't documented, maybe you can add this.

Thanks for pointing this out. As the async stuff hasn't solidified yet, I have a separate JIRA for documentation (I didn't want to spend a ton of time documenting before I knew what the final implementation was going to be).

rwinch · 2013-01-09T19:58:37Z

Thanks for the cleanup. I am in the middle of a few other things, but will try and take a look at it tomorrow or Friday at the latest.

rwinch · 2013-01-10T23:04:16Z

It appears this still cannot be merged automatically. I have pushed commits out to the remote repo since, but this does not cause any conflicts since it does not touch the same files. To test that it merges without conflicts you should be able to the following in your local repo (assuming origin is SpringSource/spring-security):

NOTE: This removes everything from master so if you don't have a topic branch it will remove all your work!

git fetch origin
git checkout master
git reset --hard origin/master
git merge pull-24

I also noticed that the resulting code in this pull request uses add-headers in the parser rather than headers.

I will spend some time cleaning this up and also figure why the build isn't working. I think I should be able to get this together tomorrow.

mdeinum · 2013-01-11T07:06:09Z

Hmm strange... Will see if I can cleanup the commits and see what is wrong... Locally I see that I use headers so there must be something wrong (or I forgot to push my commits).

rwinch · 2013-01-11T22:47:50Z

I have a branch that has cleaned up most of my concerns. It also fixes the build issue you were having. I took the commits and squashed them into a single commit ensuring that they would merge cleanly with master. I then put a bit of polish on it in the following commit.

One thing that I am still debating about is the frame-options ALLOW-FROM support may be a bit limited as it is. We may want the value to be dynamically available depending on the URL. For example, you can see that the IE Blog suggests that you would dynamically change the uri for the ALLOW-FROM in the event you need to support multiple domains. The blog author holds some weight because the tentative specification also suggests this and references another post by the blog author in the Informative References. We may want to rethink how we represent the frame-options in xml and how we implement HeadersFilter to accommodate this.

mdeinum · 2013-01-12T10:51:42Z

We could change the filter to contain an Map of Objects then check if it a String or some Strategy interface (HeaderValueProducer or something) if string add directly if interface call it to determine value. This would require changing the parser and maybe the rnc/xsd file depending on the configuration changes. We could then implement the strategy as outlined in those posts and one which would allow static configuration.

Op 11 jan. 2013, om 23:47 heeft Rob Winch het volgende geschreven:

I have a branch that has cleaned up most of my concerns. It also fixes the build issue you were having. I took the commits and squashed them into a single commit ensuring that they would merge cleanly with master. I then put a bit of polish on it in the following commit.

One thing that I am still debating about is the frame-options ALLOW-FROM support may be a bit limited as it is. We may want the value to be dynamically available depending on the URL. For example, you can see that the IE Blog suggests that you would dynamically change the uri for the ALLOW-FROM in the event you need to support multiple domains. The blog author holds some weight because the tentative specification also suggests this and references another post by the blog author in the Informative References. We may want to rethink how we represent the frame-options in xml and how we implement HeadersFilter to accommodate this.

—
Reply to this email directly or view it on GitHub.

rwinch · 2013-01-13T00:23:42Z

@mdeinum - I was thinking of something similar to the strategy interface. Since the strategy will likely need to see the HttpServletRequest (to determine which headers to produce) it may be just as well to pass the HttpServletResponse as well. This prevents the need for creating additional objects (i.e. a map of headers) for each request. Whats nice with introducing the strategy is this code can be used to support SEC-2046

- Implemented different ALLOW-FROM strategies as specified in the proposal.

mdeinum · 2013-02-24T09:37:01Z

Created another pull request and submitted against your custom branch. Probably didn't rebase correctly because some other commits got included as well. You should be able to cherry pick the last commit.

I have a problem now with the testcase that tests the configuration parsers, maybe you can have a look.

rwinch · 2013-08-01T18:51:29Z

I have made the suggested updates and merged this into master. There is also support for JavaConfig with the headers

mdeinum added 4 commits December 21, 2012 14:56

AddHeadersFilter for setting security filters added.

1951961

Issues: SEC-2098, SEC-2099

Fixed type and wrongly named attribute.

6bb9863

rwinch reviewed Jan 7, 2013
View reviewed changes

rwinch reviewed Jan 8, 2013
View reviewed changes

mdeinum and others added 7 commits January 8, 2013 18:27

Make rnc transform part of Gradle build

9b5cb4e

Add spring-security-3.2.rnc

8f1ad62

SEC-2114: Provide Spring Cache Abstraction based cache implementations

956b357

As of Spring 3.1 spring has its own cache abstraction. This commit adds cache imlpementations based on that abstraction.

SEC-2114: Polishing Spring Based Cache

8179a2d

SEC-2115: Improve French translation for "credentials"

38629d4

"Créances" is not the right translation. "Identifications" is a lot better in this case.

rwinch reviewed Jan 9, 2013
View reviewed changes

- Created HeaderFactory abstraction

3a55337

- Implemented different ALLOW-FROM strategies as specified in the proposal.

rwinch closed this Aug 1, 2013

This was referenced Feb 5, 2016

SEC-1994: Spring security 3.1.1.RELEASE is not JDK 1.5 compatible #2215

Closed

SEC-2098: X-Frame-Options to defend against clickjacking #2329

Closed

SEC-2099: X-XSS-Protection Header Support #2330

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

SEC-2098, SEC-2099 #24

SEC-2098, SEC-2099 #24

Uh oh!

mdeinum commented Jan 6, 2013

Uh oh!

rwinch Jan 7, 2013

Uh oh!

mdeinum Jan 8, 2013

Uh oh!

mdeinum Jan 9, 2013

Uh oh!

rwinch commented Jan 7, 2013

Uh oh!

rwinch Jan 8, 2013

Uh oh!

mdeinum commented Jan 9, 2013

Uh oh!

rwinch Jan 9, 2013

Uh oh!

rwinch commented Jan 9, 2013

Uh oh!

rwinch commented Jan 10, 2013

Uh oh!

mdeinum commented Jan 11, 2013

Uh oh!

rwinch commented Jan 11, 2013

Uh oh!

mdeinum commented Jan 12, 2013

Uh oh!

rwinch commented Jan 13, 2013

Uh oh!

mdeinum commented Feb 24, 2013

Uh oh!

rwinch commented Aug 1, 2013

Uh oh!

Uh oh!

SEC-2098, SEC-2099 #24

SEC-2098, SEC-2099 #24

Uh oh!

Conversation

mdeinum commented Jan 6, 2013

Uh oh!

rwinch Jan 7, 2013

Choose a reason for hiding this comment

Uh oh!

mdeinum Jan 8, 2013

Choose a reason for hiding this comment

Uh oh!

mdeinum Jan 9, 2013

Choose a reason for hiding this comment

Uh oh!

rwinch commented Jan 7, 2013

Uh oh!

rwinch Jan 8, 2013

Choose a reason for hiding this comment

Uh oh!

mdeinum commented Jan 9, 2013

Uh oh!

rwinch Jan 9, 2013

Choose a reason for hiding this comment

Uh oh!

rwinch commented Jan 9, 2013

Uh oh!

rwinch commented Jan 10, 2013

Uh oh!

mdeinum commented Jan 11, 2013

Uh oh!

rwinch commented Jan 11, 2013

Uh oh!

mdeinum commented Jan 12, 2013

Uh oh!

rwinch commented Jan 13, 2013

Uh oh!

mdeinum commented Feb 24, 2013

Uh oh!

rwinch commented Aug 1, 2013

Uh oh!

Uh oh!