Juju Charms Collection
juju-gui package

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

  1. Precise Pangolin (12.04)
  2. clickjacking
  3. Merge into trunk
Proposed by Francesco Banconi
Status: Merged
Merged at revision: 182
Proposed branch: lp:~frankban/charms/precise/juju-gui/clickjacking
Merge into: lp:~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:~frankban/charms/precise/juju-gui/clickjacking
Reviewer Review Type Date Requested Status
charmers Pending
Review via email: mp+216280@code.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/

To post a comment you must log in.
Revision history for this message
Francesco Banconi (frankban) wrote :
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...

Read more...

Revision history for this message
Jeff Pihach (hatch) wrote :

LGTM Looks good thanks for this!

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

Revision history for this message
Francesco Banconi (frankban) wrote :

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

Hi Jeff,

thanks for the review!

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'config/apache-site.template'
2--- config/apache-site.template 2014-01-21 19:48:07 +0000
3+++ config/apache-site.template 2014-04-17 09:38:17 +0000
4@@ -31,5 +31,7 @@
5
6 Header unset Cache-Control
7 Header set Cache-Control "max-age=0, public, must-revalidate"
8+ # Avoid user-interface redressing (e.g. clickjacking).
9+ Header always append X-Frame-Options SAMEORIGIN
10
11 </VirtualHost>
12
13=== modified file 'revision'
14--- revision 2014-04-14 17:00:09 +0000
15+++ revision 2014-04-17 09:38:17 +0000
16@@ -1,1 +1,1 @@
17-111
18+112
19
20=== modified file 'server/guiserver/handlers.py'
21--- server/guiserver/handlers.py 2014-04-09 16:48:14 +0000
22+++ server/guiserver/handlers.py 2014-04-17 09:38:17 +0000
23@@ -226,6 +226,11 @@
24 """See tornado.web.StaticFileHandler.get_absolute_path."""
25 return os.path.join(root, 'index.html')
26
27+ def set_default_headers(self):
28+ """Set custom HTTP headers at the beginning of the request."""
29+ # Avoid user-interface redressing (e.g. clickjacking).
30+ self.set_header('X-Frame-Options', 'SAMEORIGIN')
31+
32
33 class ProxyHandler(web.RequestHandler):
34 """An HTTP(S) proxy from the server to the given target URL."""
35
36=== modified file 'server/guiserver/tests/test_handlers.py'
37--- server/guiserver/tests/test_handlers.py 2014-04-09 13:26:40 +0000
38+++ server/guiserver/tests/test_handlers.py 2014-04-17 09:38:17 +0000
39@@ -501,6 +501,21 @@
40 # Requests including flags and queries are served by the index file.
41 self.ensure_index('/:flag:/activated/?my=query')
42
43+ def test_headers(self):
44+ # The expected Content-Type, ETag and clickjacking protection headers
45+ # are correctly sent by the server.
46+ response = self.fetch('/')
47+ headers = response.headers
48+ # Check response content type.
49+ self.assertIn('Content-Type', headers)
50+ self.assertEqual('text/html', headers['Content-Type'])
51+ # Check cache headers.
52+ self.assertIn('ETag', headers)
53+ self.assertIn('Last-Modified', headers)
54+ # Check X-Frame headers.
55+ self.assertIn('X-Frame-Options', headers)
56+ self.assertEqual('SAMEORIGIN', headers['X-Frame-Options'])
57+
58
59 class TestProxyHandler(LogTrapTestCase, AsyncHTTPTestCase):
60

Subscribers

People subscribed via source and target branches