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.

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2016 the original author or authors.
* Copyright 2002-2019 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.
Expand Down Expand Up @@ -73,6 +73,7 @@
* </p>
*
* @author Joe Grandja
* @author Ankur Pathak
* @since 4.1
*/
public final class ContentSecurityPolicyHeaderWriter implements HeaderWriter {
Expand Down Expand Up @@ -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.

response.setHeader(headerName, policyDirectives);
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2018 the original author or authors.
* Copyright 2002-2019 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.
Expand Down Expand Up @@ -33,6 +33,7 @@
* responsible for declaring the restrictions for a particular feature type.
*
* @author Vedran Pavic
* @author Ankur Pathak
* @since 5.1
*/
public final class FeaturePolicyHeaderWriter implements HeaderWriter {
Expand All @@ -54,7 +55,9 @@ public FeaturePolicyHeaderWriter(String policyDirectives) {

@Override
public void writeHeaders(HttpServletRequest request, HttpServletResponse response) {
response.setHeader(FEATURE_POLICY_HEADER, this.policyDirectives);
if (!response.containsHeader(FEATURE_POLICY_HEADER)) {
response.setHeader(FEATURE_POLICY_HEADER, this.policyDirectives);
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2016 the original author or authors.
* Copyright 2002-2019 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.
Expand Down Expand Up @@ -100,6 +100,7 @@
* </p>
*
* @author Tim Ysewyn
* @author Ankur Pathak
* @since 4.1
*/
public final class HpkpHeaderWriter implements HeaderWriter {
Expand Down Expand Up @@ -174,7 +175,10 @@ public HpkpHeaderWriter() {
public void writeHeaders(HttpServletRequest request, HttpServletResponse response) {
if (requestMatcher.matches(request)) {
if (!pins.isEmpty()) {
response.setHeader(reportOnly ? HPKP_RO_HEADER_NAME : HPKP_HEADER_NAME, hpkpHeaderValue);
String headerName = reportOnly ? HPKP_RO_HEADER_NAME : HPKP_HEADER_NAME;
if (!response.containsHeader(headerName)) {
response.setHeader(headerName, hpkpHeaderValue);
}
} if (logger.isDebugEnabled()) {
logger.debug("Not injecting HPKP header since there aren't any pins");
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2013 the original author or authors.
* Copyright 2002-2019 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.
Expand Down Expand Up @@ -53,6 +53,7 @@
* </p>
*
* @author Rob Winch
* @author Ankur Pathak
* @since 3.2
*/
public final class HstsHeaderWriter implements HeaderWriter {
Expand Down Expand Up @@ -159,7 +160,9 @@ public HstsHeaderWriter() {
*/
public void writeHeaders(HttpServletRequest request, HttpServletResponse response) {
if (this.requestMatcher.matches(request)) {
response.setHeader(HSTS_HEADER_NAME, this.hstsHeaderValue);
if (!response.containsHeader(HSTS_HEADER_NAME)) {
response.setHeader(HSTS_HEADER_NAME, this.hstsHeaderValue);
}
}
else if (this.logger.isDebugEnabled()) {
this.logger
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2016 the original author or authors.
* Copyright 2002-2019 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.
Expand Down Expand Up @@ -49,6 +49,7 @@
*
* @author Eddú Meléndez
* @author Kazuki Shimizu
* @author Ankur Pathak
* @since 4.2
*/
public class ReferrerPolicyHeaderWriter implements HeaderWriter {
Expand Down Expand Up @@ -89,7 +90,9 @@ public void setPolicy(ReferrerPolicy policy) {
*/
@Override
public void writeHeaders(HttpServletRequest request, HttpServletResponse response) {
response.setHeader(REFERRER_POLICY_HEADER, this.policy.getPolicy());
if (!response.containsHeader(REFERRER_POLICY_HEADER)) {
response.setHeader(REFERRER_POLICY_HEADER, this.policy.getPolicy());
}
}

public enum ReferrerPolicy {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2016 the original author or authors.
* Copyright 2002-2019 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.
Expand Down Expand Up @@ -30,6 +30,7 @@
*
* @author Marten Deinum
* @author Rob Winch
* @author Ankur Pathak
* @since 3.2
*/
public class StaticHeadersWriter implements HeaderWriter {
Expand All @@ -56,8 +57,10 @@ public StaticHeadersWriter(String headerName, String... headerValues) {

public void writeHeaders(HttpServletRequest request, HttpServletResponse response) {
for (Header header : headers) {
for (String value : header.getValues()) {
response.addHeader(header.getName(), value);
if (!response.containsHeader(header.getName())) {
for (String value : header.getValues()) {
response.addHeader(header.getName(), value);
}
}
}
}
Expand All @@ -66,4 +69,4 @@ public void writeHeaders(HttpServletRequest request, HttpServletResponse respons
public String toString() {
return getClass().getName() + " [headers=" + headers + "]";
}
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2013 the original author or authors.
* Copyright 2002-2019 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.
Expand All @@ -26,6 +26,7 @@
* >X-XSS-Protection header</a>.
*
* @author Rob Winch
* @author Ankur Pathak
* @since 3.2
*/
public final class XXssProtectionHeaderWriter implements HeaderWriter {
Expand All @@ -47,7 +48,9 @@ public XXssProtectionHeaderWriter() {
}

public void writeHeaders(HttpServletRequest request, HttpServletResponse response) {
response.setHeader(XSS_PROTECTION_HEADER, headerValue);
if (!response.containsHeader(XSS_PROTECTION_HEADER)) {
response.setHeader(XSS_PROTECTION_HEADER, headerValue);
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2018 the original author or authors.
* Copyright 2002-2019 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.
Expand Down Expand Up @@ -27,6 +27,7 @@
*
* @author Marten Deinum
* @author Rob Winch
* @author Ankur Pathak
* @since 3.2
*
* @see AllowFromStrategy
Expand Down Expand Up @@ -84,10 +85,14 @@ public void writeHeaders(HttpServletRequest request, HttpServletResponse respons
if (XFrameOptionsMode.ALLOW_FROM.equals(frameOptionsMode)) {
String allowFromValue = this.allowFromStrategy.getAllowFromValue(request);
if (XFrameOptionsMode.DENY.getMode().equals(allowFromValue)) {
response.setHeader(XFRAME_OPTIONS_HEADER, XFrameOptionsMode.DENY.getMode());
if (!response.containsHeader(XFRAME_OPTIONS_HEADER)) {
response.setHeader(XFRAME_OPTIONS_HEADER, XFrameOptionsMode.DENY.getMode());
}
} else if (allowFromValue != null) {
response.setHeader(XFRAME_OPTIONS_HEADER,
XFrameOptionsMode.ALLOW_FROM.getMode() + " " + allowFromValue);
if (!response.containsHeader(XFRAME_OPTIONS_HEADER)) {
response.setHeader(XFRAME_OPTIONS_HEADER,
XFrameOptionsMode.ALLOW_FROM.getMode() + " " + allowFromValue);
}
}
}
else {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2017 the original author or authors.
* Copyright 2002-2019 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.
Expand All @@ -24,12 +24,16 @@

/**
* @author Joe Grandja
* @author Ankur Pathak
*/
public class ContentSecurityPolicyHeaderWriterTests {
private static final String DEFAULT_POLICY_DIRECTIVES = "default-src 'self'";
private MockHttpServletRequest request;
private MockHttpServletResponse response;
private ContentSecurityPolicyHeaderWriter writer;
private static final String CONTENT_SECURITY_POLICY_HEADER = "Content-Security-Policy";
private static final String CONTENT_SECURITY_POLICY_REPORT_ONLY_HEADER = "Content-Security-Policy-Report-Only";


@Before
public void setup() {
Expand Down Expand Up @@ -87,4 +91,21 @@ public void writeHeadersContentSecurityPolicyInvalid() {
writer = new ContentSecurityPolicyHeaderWriter(null);
}

}

@Test
public void writeHeaderOnlyIfNotPresentContentSecurityPolicyHeader(){
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);
}

@Test
public void writeHeaderOnlyIfNotPresentContentSecurityPolicyReportOnlyHeader(){
String value = new String("value");
this.response.setHeader(CONTENT_SECURITY_POLICY_REPORT_ONLY_HEADER, value);
this.writer.setReportOnly(true);
this.writer.writeHeaders(this.request, this.response);
assertThat(this.response.getHeader(CONTENT_SECURITY_POLICY_REPORT_ONLY_HEADER)).isSameAs(value);
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2018 the original author or authors.
* Copyright 2002-2019 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.
Expand Down Expand Up @@ -29,6 +29,7 @@
* Tests for {@link FeaturePolicyHeaderWriter}.
*
* @author Vedran Pavic
* @author Ankur Pathak
*/
public class FeaturePolicyHeaderWriterTests {

Expand All @@ -40,6 +41,8 @@ public class FeaturePolicyHeaderWriterTests {

private FeaturePolicyHeaderWriter writer;

private static final String FEATURE_POLICY_HEADER = "Feature-Policy";

@Before
public void setUp() {
this.request = new MockHttpServletRequest();
Expand Down Expand Up @@ -70,4 +73,12 @@ public void createWriterWithEmptyDirectivesShouldThrowException() {
.hasMessage("policyDirectives must not be null or empty");
}

@Test
public void writeHeaderOnlyIfNotPresent(){
String value = new String("value");
this.response.setHeader(FEATURE_POLICY_HEADER, value);
this.writer.writeHeaders(this.request, this.response);
assertThat(this.response.getHeader(FEATURE_POLICY_HEADER)).isSameAs(value);
}

}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2016 the original author or authors.
* Copyright 2002-2019 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.
Expand Down Expand Up @@ -30,6 +30,7 @@

/**
* @author Tim Ysewyn
* @author Ankur Pathak
*
*/
public class HpkpHeaderWriterTests {
Expand All @@ -47,6 +48,10 @@ public class HpkpHeaderWriterTests {

private HpkpHeaderWriter writer;

private static final String HPKP_HEADER_NAME = "Public-Key-Pins";

private static final String HPKP_RO_HEADER_NAME = "Public-Key-Pins-Report-Only";

@Before
public void setup() {
request = new MockHttpServletRequest();
Expand Down Expand Up @@ -196,4 +201,22 @@ public void addSha256PinsWithNullPin() {
public void setIncorrectReportUri() {
writer.setReportUri("some url here...");
}

@Test
public void writeHeaderOnlyIfNotPresentPublicKeyPins(){
String value = new String("value");
this.response.setHeader(HPKP_HEADER_NAME, value);
this.writer.setReportOnly(false);
this.writer.writeHeaders(this.request, this.response);
assertThat(this.response.getHeader(HPKP_HEADER_NAME)).isSameAs(value);
}

@Test
public void writeHeaderOnlyIfNotPresentPublicKeyPinsReportOnly(){
String value = new String("value");
this.response.setHeader(HPKP_RO_HEADER_NAME, value);
this.writer.setReportOnly(false);
this.writer.writeHeaders(this.request, this.response);
assertThat(this.response.getHeader(HPKP_RO_HEADER_NAME)).isSameAs(value);
}
}
Loading