Merge lp:~lifeless/js-oopsd/post into lp:js-oopsd

[Robert Collins]

keep browsers happier - implement CORS rules to permit any browser to use
POST or GET, and permit preflight checks.

[Robert Collins (lifeless)]

https://developer.mozilla.org/en-US/docs/HTTP_access_control and http://www.w3.org/TR/cors if you want the train wreck that is the specs.

[Ian Booth (wallyworld)]

Looks ok, but I'm not really sufficiently across the finer detail of the specs to know for sure that nothing unintended is not being introduced. So a conditional approval from me.

review: Approve

[John A Meinel (jameinel)]

I think the headers listed here aren't quite correct. You mention wanting to support GET and POST, but you list PUT and DELETE.

I also think it is good to test the exact headers in the test suite, so that changing the input in 'main.py' won't have the test suite still pass without updating. (It is good to test that the headers make it into the output, but we *also* need to test that the headers contain the fields we need in the real world.)

Beyond that, I think getting something landed, so it can be tested with real browsers is the most important thing. Having an allow '*' is a little concerning for me, I'd be happier with an Allow *.canonical.com *.launchpad.net or something like that, but I haven't dug into the spec to be sure its all ok.

And in the end, I think we're pretty confident that POST/GET from OOPS isn't going to be a cross-site-scripting attack, so we can just allow it.

So approve with some tweaks.

review: Approve (code*)

[lp:~lifeless/js-oopsd/post]

14. By Robert Collins on 2012-09-10

Fix methods, make one non-circular test.

=== modified file 'NEWS'
--- NEWS	2012-09-09 04:08:16 +0000
+++ NEWS	2012-09-10 05:30:25 +0000
@@ -7,3 +7,6 @@
 ----
 
 * GET and POST support added.
+
+* CORS headers added to the responses, and CORS preflight OPTIONS check added.
+  (Robert Collins)

=== modified file 'js_oopsd/main.py'
--- js_oopsd/main.py	2012-09-09 04:08:16 +0000
+++ js_oopsd/main.py	2012-09-10 05:30:25 +0000
@@ -29,6 +29,14 @@
 import oops_wsgi 
 from paste.request import parse_formvars
 
+cors_headers = (
+    ('Access-Control-Allow-Origin', '*'),
+    ('Access-Control-Max-Age', '3628800'),
+    ('Access-Control-Allow-Methods', 'GET, POST'),
+    ('Access-Control-Expose-Headers', 'X-OOPS-ID'),
+    )
+
+
 def make_app(jsoops_config):
     def app(environ, start_response):
         # NB: probably want oops on_create hooks and to create the context, not the
@@ -53,9 +61,12 @@
         elif method == 'GET':
             query = environ.get('QUERY_STRING', '')
             json_content = unquote(query)
+        elif method == 'OPTIONS':
+            start_response('200 OK', list(cors_headers))
+            return
         else:
             # Not supported
-            start_response('405 Method Not Allowed', [('Allow', 'GET, POST')])
+            start_response('405 Method Not Allowed', [('Allow', 'GET, OPTIONS, POST')])
             return
         if not json_content:
             start_response('400 Empty OOPS report', [])
@@ -77,7 +88,8 @@
         # future.
         report.update(report_extra)
         jsoops_config.publish(report)
-        start_response('200 OK', [('X-OOPS-ID', str(report['id']))])
+        start_response('200 OK',
+            list(cors_headers) + [('X-OOPS-ID', str(report['id']))])
         yield ''
     return app
 

=== modified file 'js_oopsd/tests/test_main.py'
--- js_oopsd/tests/test_main.py	2012-09-09 04:08:16 +0000
+++ js_oopsd/tests/test_main.py	2012-09-10 05:30:25 +0000
@@ -20,7 +20,11 @@
 from testtools import TestCase
 from testtools.matchers import Equals
 
-from js_oopsd.main import make_app
+from js_oopsd.main import (
+    cors_headers,
+    make_app,
+    )
+
 
 def capture():
     results = []
@@ -55,8 +59,25 @@
         output = list(app(environ, start_response))
         self.assertThat(output, Equals([]))
         self.assertThat(events,
-            Equals([('405 Method Not Allowed', [('Allow', 'GET, POST')])]))
-    
+            Equals([('405 Method Not Allowed',
+                [('Allow', 'GET, OPTIONS, POST')])]))
+
+    def test_options(self):
+        # CORS requires us to tell browsers they are allowed to use the
+        # service.
+        app, events, start_response = test_app()
+        environ = {
+            'PATH_INFO': '/',
+            'REQUEST_METHOD': 'OPTIONS',
+            }
+        output = list(app(environ, start_response))
+        self.assertThat(output, Equals([]))
+        self.assertThat(events, Equals([('200 OK',
+            [('Access-Control-Allow-Origin', '*'),
+             ('Access-Control-Max-Age', '3628800'),
+             ('Access-Control-Allow-Methods', 'GET, POST'),
+             ('Access-Control-Expose-Headers', 'X-OOPS-ID')])]))
+
     def test_post_empty(self):
         # Empty generates a error response.
         app, events, start_response = test_app()
@@ -97,5 +118,5 @@
         self.assertThat(output, Equals(['']))
         self.assertThat(events,
             Equals([{'id': '1', u'reporter': u'fred'},
-                    ('200 OK', [('X-OOPS-ID', '1')])]))
+                    ('200 OK', list(cors_headers) + [('X-OOPS-ID', '1')])]))