diff options
author | Marcin Czech <maczech@gmail.com> | 2020-06-15 17:36:18 +0200 |
---|---|---|
committer | Marcin Czech <maczech@gmail.com> | 2020-06-17 11:18:52 +0200 |
commit | 559ea2b49f348d009287bef8f26f6f5e29971c4a (patch) | |
tree | 7b3db796db160c541552ea37108298ad08ea907c | |
parent | 56918394544b6207a626cb0938d0bba4927a9fa4 (diff) |
Set X-Frame-Options header to avoid clickjacking
Add HTTP filter which is applied to all HTTP responses.
Based on gerrit.canLoadInIFrame and gerrit.xframeOption
properties filter adds the X-Frame-Options HTTP
response header. The X-Frame-Options HTTP response header
can be used to indicate whether or not a browser should
be allowed to render a page in a <frame>, <iframe>,
<embed> or <object>. Gerrit can use this to avoid
click-jacking attacks, by ensuring that the content is
not embedded into other sites.
Bug: Issue 12926
Change-Id: If3f6a770332ade9924b3d1a20c092637c9380e0c
4 files changed, 254 insertions, 1 deletions
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 531bb94f7c..006af8607c 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -2086,6 +2086,25 @@ Setting this option to true will prevent this behavior. + By default false. +[[gerrit.xframeOption]]gerrit.xframeOption:: ++ +Add link:https://tools.ietf.org/html/rfc7034[`X-Frame-Options`] header to all HTTP +responses. The `X-Frame-Options` HTTP response header can be used to indicate +whether or not a browser should be allowed to render a page in a +`<frame>`, `<iframe>`, `<embed>` or `<object>`. ++ +Available values: ++ +1. ALLOW - The page can be displayed in a frame. +2. SAMEORIGIN - The page can only be displayed in a frame on the same origin as the page itself. ++ +If link:#gerrit.canLoadInIFrame is set to false this option is ignored and the +`X-Frame-Options` header is always set to `DENY`. +Setting this option to `ALLOW` will cause the `X-Frame-Options` header to be omitted +the the page can be displayed in a frame. ++ +By default SAMEORIGIN. + [[gerrit.cdnPath]]gerrit.cdnPath:: + Path prefix for PolyGerrit's static resources if using a CDN. diff --git a/java/com/google/gerrit/httpd/AllRequestFilter.java b/java/com/google/gerrit/httpd/AllRequestFilter.java index 9d171d5a54..1c3cafea04 100644 --- a/java/com/google/gerrit/httpd/AllRequestFilter.java +++ b/java/com/google/gerrit/httpd/AllRequestFilter.java @@ -18,6 +18,8 @@ import com.google.gerrit.extensions.registration.DynamicSet; import com.google.gerrit.server.plugins.Plugin; import com.google.gerrit.server.plugins.StopPluginListener; import com.google.inject.Inject; +import com.google.inject.Module; +import com.google.inject.Scopes; import com.google.inject.Singleton; import com.google.inject.internal.UniqueAnnotations; import com.google.inject.servlet.ServletModule; @@ -32,11 +34,15 @@ import javax.servlet.ServletResponse; /** Filters all HTTP requests passing through the server. */ public abstract class AllRequestFilter implements Filter { - public static ServletModule module() { + public static Module module() { return new ServletModule() { @Override protected void configureServlets() { DynamicSet.setOf(binder(), AllRequestFilter.class); + DynamicSet.bind(binder(), AllRequestFilter.class) + .to(AllowRenderInFrameFilter.class) + .in(Scopes.SINGLETON); + filter("/*").through(FilterProxy.class); bind(StopPluginListener.class) diff --git a/java/com/google/gerrit/httpd/AllowRenderInFrameFilter.java b/java/com/google/gerrit/httpd/AllowRenderInFrameFilter.java new file mode 100644 index 0000000000..b414aad7c1 --- /dev/null +++ b/java/com/google/gerrit/httpd/AllowRenderInFrameFilter.java @@ -0,0 +1,59 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// 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 com.google.gerrit.httpd; + +import com.google.gerrit.server.config.GerritServerConfig; +import com.google.inject.Inject; +import java.io.IOException; +import javax.servlet.FilterChain; +import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServletResponse; +import org.eclipse.jgit.lib.Config; + +public class AllowRenderInFrameFilter extends AllRequestFilter { + static final String X_FRAME_OPTIONS_HEADER_NAME = "X-Frame-Options"; + + public static enum XFrameOption { + ALLOW, + SAMEORIGIN; + } + + private final String xframeOptionString; + private final boolean skipXFrameOption; + + @Inject + public AllowRenderInFrameFilter(@GerritServerConfig Config cfg) { + XFrameOption xframeOption = + cfg.getEnum("gerrit", null, "xframeOption", XFrameOption.SAMEORIGIN); + boolean canLoadInIFrame = cfg.getBoolean("gerrit", "canLoadInIFrame", false); + xframeOptionString = canLoadInIFrame ? xframeOption.name() : "DENY"; + + skipXFrameOption = xframeOption.equals(XFrameOption.ALLOW) && canLoadInIFrame; + } + + @Override + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + if (skipXFrameOption) { + chain.doFilter(request, response); + } else { + HttpServletResponse httpResponse = (HttpServletResponse) response; + httpResponse.addHeader(X_FRAME_OPTIONS_HEADER_NAME, xframeOptionString); + chain.doFilter(request, httpResponse); + } + } +} diff --git a/javatests/com/google/gerrit/httpd/AllowRenderInFrameFilterTest.java b/javatests/com/google/gerrit/httpd/AllowRenderInFrameFilterTest.java new file mode 100644 index 0000000000..0d6d482032 --- /dev/null +++ b/javatests/com/google/gerrit/httpd/AllowRenderInFrameFilterTest.java @@ -0,0 +1,169 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// 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 com.google.gerrit.httpd; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.gerrit.httpd.AllowRenderInFrameFilter.X_FRAME_OPTIONS_HEADER_NAME; +import static com.google.gerrit.testing.GerritJUnit.assertThrows; +import static org.easymock.EasyMock.expectLastCall; + +import com.google.gerrit.httpd.AllowRenderInFrameFilter.XFrameOption; +import com.google.gerrit.testing.GerritBaseTests; +import java.io.IOException; +import javax.servlet.FilterChain; +import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.http.HttpServletResponse; +import org.easymock.EasyMockSupport; +import org.eclipse.jgit.lib.Config; +import org.junit.Before; +import org.junit.Test; + +public class AllowRenderInFrameFilterTest extends GerritBaseTests { + + Config cfg; + ServletRequest request; + HttpServletResponse response; + FilterChain filterChain; + + EasyMockSupport ems = new EasyMockSupport(); + + @Before + public void setup() throws IOException, ServletException { + cfg = new Config(); + request = ems.createMock(ServletRequest.class); + response = ems.createMock(HttpServletResponse.class); + filterChain = ems.createMock(FilterChain.class); + ems.resetAll(); + // we want to make sure that doFilter is always called + filterChain.doFilter(request, response); + } + + @Test + public void shouldDenyInFrameRenderingWhenCanRenderInFrameIsFalse() + throws IOException, ServletException { + cfg.setBoolean("gerrit", null, "canLoadInIFrame", false); + + response.addHeader(X_FRAME_OPTIONS_HEADER_NAME, "DENY"); + expectLastCall().times(1); + ems.replayAll(); + + AllowRenderInFrameFilter objectUnderTest = new AllowRenderInFrameFilter(cfg); + objectUnderTest.doFilter(request, response, filterChain); + + ems.verifyAll(); + } + + @Test + public void shouldDenyInFrameRenderingWhenCanRenderInFrameIsFalseAndXFormOptionIsSAMEORIGIN() + throws IOException, ServletException { + cfg.setBoolean("gerrit", null, "canLoadInIFrame", false); + cfg.setEnum("gerrit", null, "xframeOption", XFrameOption.SAMEORIGIN); + + response.addHeader(X_FRAME_OPTIONS_HEADER_NAME, "DENY"); + expectLastCall().times(1); + ems.replayAll(); + + AllowRenderInFrameFilter objectUnderTest = new AllowRenderInFrameFilter(cfg); + objectUnderTest.doFilter(request, response, filterChain); + + ems.verifyAll(); + } + + @Test + public void shouldDenyInFrameRenderingWhenCanRenderInFrameIsFalseAndXFormOptionIsALLOW() + throws IOException, ServletException { + cfg.setBoolean("gerrit", null, "canLoadInIFrame", false); + cfg.setEnum("gerrit", null, "xframeOption", XFrameOption.ALLOW); + + response.addHeader(X_FRAME_OPTIONS_HEADER_NAME, "DENY"); + expectLastCall().times(1); + ems.replayAll(); + + AllowRenderInFrameFilter objectUnderTest = new AllowRenderInFrameFilter(cfg); + objectUnderTest.doFilter(request, response, filterChain); + + ems.verifyAll(); + } + + @Test + public void shouldRestrictAccessToSAMEORIGINWhenCanRenderInFrameIsTrue() + throws IOException, ServletException { + cfg.setBoolean("gerrit", null, "canLoadInIFrame", true); + + response.addHeader(X_FRAME_OPTIONS_HEADER_NAME, "SAMEORIGIN"); + expectLastCall().times(1); + ems.replayAll(); + + AllowRenderInFrameFilter objectUnderTest = new AllowRenderInFrameFilter(cfg); + objectUnderTest.doFilter(request, response, filterChain); + + ems.verifyAll(); + } + + @Test + public void shouldSkipHeaderWhenCanRenderInFrameIsTrueAndXFormOptionIsALLOW() + throws IOException, ServletException { + cfg.setBoolean("gerrit", null, "canLoadInIFrame", true); + cfg.setEnum("gerrit", null, "xframeOption", XFrameOption.ALLOW); + ems.replayAll(); + + AllowRenderInFrameFilter objectUnderTest = new AllowRenderInFrameFilter(cfg); + objectUnderTest.doFilter(request, response, filterChain); + + ems.verifyAll(); + } + + @Test + public void shouldRestrictAccessToSAMEORIGINWhenCanRenderInFrameIsTrueAndXFormOptionIsSAMEORIGIN() + throws IOException, ServletException { + cfg.setBoolean("gerrit", null, "canLoadInIFrame", true); + cfg.setEnum("gerrit", null, "xframeOption", XFrameOption.SAMEORIGIN); + + response.addHeader(X_FRAME_OPTIONS_HEADER_NAME, "SAMEORIGIN"); + expectLastCall().times(1); + ems.replayAll(); + + AllowRenderInFrameFilter objectUnderTest = new AllowRenderInFrameFilter(cfg); + objectUnderTest.doFilter(request, response, filterChain); + + ems.verifyAll(); + } + + @Test + public void shouldIgnoreXFrameOriginCaseSensitivity() throws IOException, ServletException { + cfg.setBoolean("gerrit", null, "canLoadInIFrame", true); + cfg.setString("gerrit", null, "xframeOption", "sameOrigin"); + + response.addHeader(X_FRAME_OPTIONS_HEADER_NAME, "SAMEORIGIN"); + expectLastCall().times(1); + ems.replayAll(); + + AllowRenderInFrameFilter objectUnderTest = new AllowRenderInFrameFilter(cfg); + objectUnderTest.doFilter(request, response, filterChain); + + ems.verifyAll(); + } + + @Test + public void shouldThrowExceptionWhenUnknownXFormOptionValue() { + cfg.setBoolean("gerrit", null, "canLoadInIFrame", true); + cfg.setString("gerrit", null, "xframeOption", "unsupported value"); + + IllegalArgumentException e = + assertThrows(IllegalArgumentException.class, () -> new AllowRenderInFrameFilter(cfg)); + assertThat(e).hasMessageThat().contains("gerrit.xframeOption=unsupported value"); + } +} |