Skip to content

Consider having HeaderWriters check before writing #6456

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Consider having HeaderWriters check before writing #6456

wants to merge 1 commit into from

Conversation

ankurpathak
Copy link
Contributor

All HeadersWriter only write Header if its not already
written.

Fixes: gh-6454

@ankurpathak ankurpathak mentioned this pull request Jan 18, 2019
Closed
@ankurpathak ankurpathak mentioned this pull request Jan 25, 2019
Closed
@ankurpathak ankurpathak force-pushed the gh-6454-fix branch 3 times, most recently from eeacf8b to 5366b42 Compare January 25, 2019 10:16
jzheaux
jzheaux requested changes Feb 1, 2019
View reviewed changes
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, @ankurpathak! I've left some feedback inline.

...src/main/java/org/springframework/security/web/header/writers/CacheControlHeadersWriter.java Outdated
@@ -61,7 +62,7 @@ public void writeHeaders(HttpServletRequest request, HttpServletResponse respons
}

private boolean hasHeader(HttpServletResponse response, String headerName) {
return response.getHeader(headerName) != null;
return response.containsHeader(headerName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the cleanup, but since it isn't necessary for this PR and this is the only change to this class, let's leave this file out for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

.../java/org/springframework/security/web/header/writers/ContentSecurityPolicyHeaderWriter.java
@@ -100,7 +101,10 @@ public ContentSecurityPolicyHeaderWriter(String policyDirectives) {
*/
@Override
public void writeHeaders(HttpServletRequest request, HttpServletResponse response) {
response.setHeader((!reportOnly ? CONTENT_SECURITY_POLICY_HEADER : CONTENT_SECURITY_POLICY_REPORT_ONLY_HEADER), policyDirectives);
String headerName = !reportOnly ? CONTENT_SECURITY_POLICY_HEADER : CONTENT_SECURITY_POLICY_REPORT_ONLY_HEADER;
if (!response.containsHeader(headerName)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to have tests that confirm this behavior now for each header writer. Something that demonstrates if the header is already present, now it doesn't get overridden.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests added.

@jzheaux
Copy link
Contributor

jzheaux commented Feb 1, 2019

Also, @ankurpathak would you please update the commit message to indicate that this fixes #5193 as well?

@ankurpathak
Copy link
Contributor Author

Also, @ankurpathak would you please update the commit message to indicate that this fixes #5193 as well?

I tried to fix it as well. But not sure if I am correct. I listed issues by space separated list.

jzheaux
jzheaux requested changes Feb 5, 2019
View reviewed changes
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left some inline feedback about the tests you added. Thanks, again!

.../org/springframework/security/web/header/writers/ContentSecurityPolicyHeaderWriterTests.java Outdated
public void writeHeaderOnlyIfNotPresentContentSecurityPolicyHeader(){
this.response = spy(this.response);
doReturn(true).when(response).containsHeader(CONTENT_SECURITY_POLICY_HEADER);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests might be a bit brittle since they depend on how the writer is verifying the existence of the header.

If the tests did:

String value = new String("value");
this.response.setHeader(CONTENT_SECURITY_POLICY_HEADER, value);
this.writer.writeHeaders(this.request, this.response);
assertThat(this.response.getHeader(CONTENT_SECURITY_POLICY_HEADER)).isSameAs(value);

Then it would verify regardless of the way the writer implemented the check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

...springframework/security/web/header/writers/frameoptions/XFrameOptionsHeaderWriterTests.java Outdated
@Test
public void writeHeadersWhenWhiteList() {
WhiteListedAllowFromStrategy whitelist = new WhiteListedAllowFromStrategy(Arrays.asList("example.com"));
WhiteListedAllowFromStrategy whitelist = new WhiteListedAllowFromStrategy(Collections.singletonList("example.com"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change necessary for this PR? I see that you are using Collections.singletonList below in your test, but I'd recommend bringing other tests into conformance be in a different PR. (For example, you could also change old tests to refer to member variables via this., but we aren't doing that here since that isn't the purpose of this PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

...springframework/security/web/header/writers/frameoptions/XFrameOptionsHeaderWriterTests.java Outdated
* @since 5.0
*/
public class XFrameOptionsHeaderWriterTests {
private MockHttpServletRequest request = new MockHttpServletRequest();
private MockHttpServletResponse response = new MockHttpServletResponse();

public static final String XFRAME_OPTIONS_HEADER = "X-Frame-Options";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave this private so you don't have to worry about other classes accessing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

...springframework/security/web/header/writers/frameoptions/XFrameOptionsHeaderWriterTests.java
@@ -20,21 +20,32 @@
import org.springframework.mock.web.MockHttpServletRequest;
import org.springframework.mock.web.MockHttpServletResponse;

import java.util.Arrays;
import java.util.Collections;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will you update the copyright year here, please, and any other files you edit where it doesn't yet say 2019?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@ankurpathak
Consider having HeaderWriters check before writing
76fe944 
All HeadersWriter only write Header if its not already
written.

Fixes: spring-projectsgh-6454 spring-projectsgh-5193
@jzheaux
Copy link
Contributor

jzheaux commented Feb 7, 2019

Thanks again, @ankurpathak! This is now merged into master via 93d6a38. Also I did a quick polish commit via 67fb936 for formatting.

@jzheaux jzheaux closed this Feb 7, 2019
@jzheaux jzheaux added this to the 5.2.0.M2 milestone Feb 7, 2019
@jzheaux jzheaux added status: duplicate A duplicate of another issue in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jzheaux jzheaux jzheaux requested changes

Assignees

@jzheaux jzheaux

Labels
in: web An issue in web modules (web, webmvc) status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Milestone
5.2.0.M2
Development

Successfully merging this pull request may close these issues.
2 participants
@ankurpathak @jzheaux