Merge into trunk : clickjacking : Precise Pangolin (12.04) : Code : juju-gui package : Juju Charms Collection

Status:

Merged

Merged at revision:

182

Proposed branch:

lp://staging/~frankban/charms/precise/juju-gui/clickjacking

Merge into:

lp://staging/~juju-gui/charms/precise/juju-gui/trunk

Diff against target:

59 lines (+23/-1)

4 files modified

config/apache-site.template (+2/-0)
revision (+1/-1)
server/guiserver/handlers.py (+5/-0)
server/guiserver/tests/test_handlers.py (+15/-0)

To merge this branch:

bzr merge lp://staging/~frankban/charms/precise/juju-gui/clickjacking

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
charmers		2014-04-17	Pending
Review via email: mp+216280@code.staging.launchpad.net

Description of the change

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://codereview.appspot.com/88090048/

Revision history for this message

Francesco Banconi (frankban) wrote on 2014-04-17:

#

Download full text (4.2 KiB)

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
=== modi...

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:  
francesco.banconi@canonical.com-20140416125503-n5c4yvmlp1ut7uql
+New revision:  
francesco.banconi@canonical.com-20140417093532-qcrz6xljtvx1e29c

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):

Revision history for this message

Jeff Pihach (hatch) wrote on 2014-04-17:

#

LGTM Looks good thanks for this!

https://codereview.appspot.com/88090048/

Revision history for this message

Francesco Banconi (frankban) wrote on 2014-04-18:

#

*** Submitted:

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.

R=jeff.pihach
CC=
https://codereview.appspot.com/88090048

https://codereview.appspot.com/88090048/

Revision history for this message

Francesco Banconi (frankban) wrote on 2014-04-18:

#

Hi Jeff,

thanks for the review!

https://codereview.appspot.com/88090048/

Juju Charms Collection
juju-gui package

Merge lp://staging/~frankban/charms/precise/juju-gui/clickjacking into lp://staging/~juju-gui/charms/precise/juju-gui/trunk

Commit message

Description of the change

Preview Diff

Subscribers

Juju Charms Collectionjuju-gui package

Merge lp://staging/~frankban/charms/precise/juju-gui/clickjacking into lp://staging/~juju-gui/charms/precise/juju-gui/trunk

Commit message

Description of the change

Preview Diff

Subscribers

Juju Charms Collection
juju-gui package