Juju Charms Collection
juju-gui package

Code review comment for lp:~frankban/charms/precise/juju-gui/clickjacking

  1. Precise Pangolin (12.04)
  2. clickjacking
  3. Merge into trunk
Revision history for this message
Francesco Banconi (frankban) wrote :

Reviewers: mp+216280_code.launchpad.net,

Message:
Please take a look.

Description:
Avoid clickjacking.

Update the builtin and legacy servers to send
the proper X-Frame-Options header so that
iframing is denied from extraneous origins.

The legacy server has been update to ensure
clickjacking is not possible on jujucharms.com.

Tests: `make unittest`.

QA:
- juju bootstrap an environment;
- run `make deploy`;
- wait for the GUI to be ready/started;
- open the GUI with the browser and log in;
- prepare an HTML page like the following, replacing
   <GUI UNIT HOSTNAME> with the address of the GUI in
   your environment:

<!DOCTYPE html>
<html>
<head>
     <title>test clickjacking</title>
</head>
<body>
<iframe src="https://<GUI UNIT HOSTNAME>"
   height="800" width="1000"></iframe>
</body>
</html>

- open the test page above with the browser,
   the iframe should be empty;
- switch to the legacy server:
   `juju set juju-gui builtin-server=false`;
- wait a minute for the config-changed hook
   to complete;
- open the test page above with the browser,
   the iframe should be empty;
- destroy the environment.

https://code.launchpad.net/~frankban/charms/precise/juju-gui/clickjacking/+merge/216280

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/88090048/

Affected files (+25, -1 lines):
   A [revision details]
   M config/apache-site.template
   M revision
   M server/guiserver/handlers.py
   M server/guiserver/tests/test_handlers.py

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision:
<email address hidden>
+New revision:
<email address hidden>

Index: revision
=== modified file 'revision'
--- revision 2014-04-14 17:00:09 +0000
+++ revision 2014-04-17 09:17:07 +0000
@@ -1,1 +1,1 @@
-111
+112

Index: config/apache-site.template
=== modified file 'config/apache-site.template'
--- config/apache-site.template 2014-01-21 19:48:07 +0000
+++ config/apache-site.template 2014-04-17 09:35:32 +0000
@@ -31,5 +31,7 @@

      Header unset Cache-Control
      Header set Cache-Control "max-age=0, public, must-revalidate"
+ # Avoid user-interface redressing (e.g. clickjacking).
+ Header always append X-Frame-Options SAMEORIGIN

  </VirtualHost>

Index: server/guiserver/handlers.py
=== modified file 'server/guiserver/handlers.py'
--- server/guiserver/handlers.py 2014-04-09 16:48:14 +0000
+++ server/guiserver/handlers.py 2014-04-17 09:08:26 +0000
@@ -226,6 +226,11 @@
          """See tornado.web.StaticFileHandler.get_absolute_path."""
          return os.path.join(root, 'index.html')

+ def set_default_headers(self):
+ """Set custom HTTP headers at the beginning of the request."""
+ # Avoid user-interface redressing (e.g. clickjacking).
+ self.set_header('X-Frame-Options', 'SAMEORIGIN')
+

  class ProxyHandler(web.RequestHandler):
      """An HTTP(S) proxy from the server to the given target URL."""

Index: server/guiserver/tests/test_handlers.py
=== modified file 'server/guiserver/tests/test_handlers.py'
--- server/guiserver/tests/test_handlers.py 2014-04-09 13:26:40 +0000
+++ server/guiserver/tests/test_handlers.py 2014-04-17 09:08:26 +0000
@@ -501,6 +501,21 @@
          # Requests including flags and queries are served by the index
file.
          self.ensure_index('/:flag:/activated/?my=query')

+ def test_headers(self):
+ # The expected Content-Type, ETag and clickjacking protection
headers
+ # are correctly sent by the server.
+ response = self.fetch('/')
+ headers = response.headers
+ # Check response content type.
+ self.assertIn('Content-Type', headers)
+ self.assertEqual('text/html', headers['Content-Type'])
+ # Check cache headers.
+ self.assertIn('ETag', headers)
+ self.assertIn('Last-Modified', headers)
+ # Check X-Frame headers.
+ self.assertIn('X-Frame-Options', headers)
+ self.assertEqual('SAMEORIGIN', headers['X-Frame-Options'])
+

  class TestProxyHandler(LogTrapTestCase, AsyncHTTPTestCase):

« Back to merge proposal