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
=== 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:38:17 +0000
@@ -31,5 +31,7 @@
3131
32 Header unset Cache-Control32 Header unset Cache-Control
33 Header set Cache-Control "max-age=0, public, must-revalidate"33 Header set Cache-Control "max-age=0, public, must-revalidate"
34 # Avoid user-interface redressing (e.g. clickjacking).
35 Header always append X-Frame-Options SAMEORIGIN
3436
35</VirtualHost>37</VirtualHost>
3638
=== modified file 'revision'
--- revision 2014-04-14 17:00:09 +0000
+++ revision 2014-04-17 09:38:17 +0000
@@ -1,1 +1,1 @@
11111112
22
=== 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:38:17 +0000
@@ -226,6 +226,11 @@
226 """See tornado.web.StaticFileHandler.get_absolute_path."""226 """See tornado.web.StaticFileHandler.get_absolute_path."""
227 return os.path.join(root, 'index.html')227 return os.path.join(root, 'index.html')
228228
229 def set_default_headers(self):
230 """Set custom HTTP headers at the beginning of the request."""
231 # Avoid user-interface redressing (e.g. clickjacking).
232 self.set_header('X-Frame-Options', 'SAMEORIGIN')
233
229234
230class ProxyHandler(web.RequestHandler):235class ProxyHandler(web.RequestHandler):
231 """An HTTP(S) proxy from the server to the given target URL."""236 """An HTTP(S) proxy from the server to the given target URL."""
232237
=== 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:38:17 +0000
@@ -501,6 +501,21 @@
501 # Requests including flags and queries are served by the index file.501 # Requests including flags and queries are served by the index file.
502 self.ensure_index('/:flag:/activated/?my=query')502 self.ensure_index('/:flag:/activated/?my=query')
503503
504 def test_headers(self):
505 # The expected Content-Type, ETag and clickjacking protection headers
506 # are correctly sent by the server.
507 response = self.fetch('/')
508 headers = response.headers
509 # Check response content type.
510 self.assertIn('Content-Type', headers)
511 self.assertEqual('text/html', headers['Content-Type'])
512 # Check cache headers.
513 self.assertIn('ETag', headers)
514 self.assertIn('Last-Modified', headers)
515 # Check X-Frame headers.
516 self.assertIn('X-Frame-Options', headers)
517 self.assertEqual('SAMEORIGIN', headers['X-Frame-Options'])
518
504519
505class TestProxyHandler(LogTrapTestCase, AsyncHTTPTestCase):520class TestProxyHandler(LogTrapTestCase, AsyncHTTPTestCase):
506521

Subscribers

People subscribed via source and target branches