Back to development: 0.5.3

When an attacker uses a <form> to downvote a comment, the browser
*should* add a `Content-Type: ...` header with three possible values:

    * application/x-www-form-urlencoded
    * multipart/form-data
    * text/plain

If the header is not sent or requests `application/json`, the
request is not forged (XHR is restricted by CORS separately).

CHANGES.rst

@@ -1,8 +1,14 @@
 Changelog for Isso
 ==================
 
-0.5 (2013-11-17)
-----------------
+0.5.2 (2013-12-08)
+------------------
+
+- Nothing changed yet.
+
+
+0.5.1 (2013-11-21)
+------------------
 
 Major improvements:
 

isso/ext/notifications.py

@@ -96,10 +96,10 @@ class SMTP(object):
         key = self.isso.sign(comment["id"])
 
         rv.write("---\n")
-        rv.write("Kommentar löschen: %s\n" % (uri + "/delete/" + key))
+        rv.write("Delete comment: %s\n" % (uri + "/delete/" + key))
 
         if comment["mode"] == 2:
-            rv.write("Kommentar freischalten: %s\n" % (uri + "/activate/" + key))
+            rv.write("Activate comment: %s\n" % (uri + "/activate/" + key))
 
         rv.seek(0)
         return rv.read()

isso/js/app/api.js

@@ -31,25 +31,21 @@ define(["q"], function(Q) {
      *      .. code-block:: html
      *
      *          <script data-isso="http://example.tld/path/" src="/.../embed.min.js"></script>
-     *
-     *   2. use require.js (during development). When using require.js, we
-     *      assume that the path to the scripts ends with `/js/`.
      */
 
     var script, endpoint,
         js = document.getElementsByTagName("script");
 
     for (var i = 0; i < js.length; i++) {
-        if (js[i].dataset.isso !== undefined) {
-            endpoint = js[i].dataset.isso;
-        } else if (js[i].src.match("require\\.js$")) {
-            endpoint = js[i].dataset.main.replace(/\/js\/(embed|count)$/, "");
+        if (js[i].hasAttribute("data-isso")) {
+            endpoint = js[i].getAttribute("data-isso");
+            break;
         }
     }
 
     if (! endpoint) {
         for (i = 0; i < js.length; i++) {
-            if (js[i].hasAttribute("async") || js[i].hasAttribute("defer")) {
+            if (js[i].getAttribute("async") || js[i].getAttribute("defer")) {
                 throw "Isso's automatic configuration detection failed, please " +
                       "refer to https://github.com/posativ/isso#client-configuration " +
                       "and add a custom `data-isso` attribute.";
@@ -57,14 +53,10 @@ define(["q"], function(Q) {
         }
 
         script = js[js.length - 1];
-
-        if (script.dataset.prefix) {
-            endpoint = script.dataset.prefix;
-        } else {
-            endpoint = script.src.substring(0, script.src.length - "/js/embed.min.js".length);
-        }
+        endpoint = script.src.substring(0, script.src.length - "/js/embed.min.js".length);
     }
 
+    //  strip trailing slash
     if (endpoint[endpoint.length - 1] === "/") {
         endpoint = endpoint.substring(0, endpoint.length - 1);
     }
@@ -93,6 +85,7 @@ define(["q"], function(Q) {
         try {
             xhr.open(method, url, true);
             xhr.withCredentials = true;
+            xhr.setRequestHeader("Content-Type", "application/json");
 
             if (method === "GET") {
                 xhr.setRequestHeader("X-Origin", window.location.origin);

isso/views/comments.py

@@ -31,6 +31,30 @@ class JSON(Response):
         return super(JSON, self).__init__(*args, content_type='application/json')
 
 
+def xhr(func):
+    """A decorator to check for CSRF on POST/PUT/DELETE using a <form>
+    element and JS to execute automatically (see #40 for a proof-of-concept).
+
+    When an attacker uses a <form> to downvote a comment, the browser *should*
+    add a `Content-Type: ...` header with three possible values:
+
+    * application/x-www-form-urlencoded
+    * multipart/form-data
+    * text/plain
+
+    If the header is not sent or requests `application/json`, the request is
+    not forged (XHR is restricted by CORS separately).
+    """
+
+    def dec(self, env, req, *args, **kwargs):
+
+        if req.content_type and not req.content_type.startswith("application/json"):
+            raise Forbidden("CSRF")
+        return func(self, env, req, *args, **kwargs)
+
+    return dec
+
+
 class API(object):
 
     FIELDS = set(['id', 'parent', 'text', 'author', 'website', 'email',
@@ -47,6 +71,7 @@ class API(object):
         ('edit',    ('PUT', '/id/<int:id>')),
         ('delete',  ('DELETE', '/id/<int:id>')),
         ('delete',  ('GET', '/id/<int:id>/delete/<string:key>')),
+        ('activate',('GET', '/id/<int:id>/activate/<string:key>')),
         ('like',    ('POST', '/id/<int:id>/like')),
         ('dislike', ('POST', '/id/<int:id>/dislike')),
         ('checkip', ('GET', '/check-ip'))
@@ -90,6 +115,7 @@ class API(object):
 
         return True, ""
 
+    @xhr
     @requires(str, 'uri')
     def new(self, environ, request, uri):
 
@@ -173,6 +199,7 @@ class API(object):
 
         return Response(json.dumps(rv), 200, content_type='application/json')
 
+    @xhr
     def edit(self, environ, request, id):
 
         try:
@@ -216,6 +243,7 @@ class API(object):
         resp.headers.add("X-Set-Cookie", cookie("isso-%i" % rv["id"]))
         return resp
 
+    @xhr
     def delete(self, environ, request, id, key=None):
 
         try:
@@ -253,7 +281,7 @@ class API(object):
         resp.headers.add("X-Set-Cookie", cookie("isso-%i" % id))
         return resp
 
-    def activate(self, environ, request, _, key):
+    def activate(self, environ, request, id, key):
 
         try:
             id = self.isso.unsign(key, max_age=2**32)
@@ -293,11 +321,13 @@ class API(object):
 
         return JSON(json.dumps(rv), 200)
 
+    @xhr
     def like(self, environ, request, id):
 
         nv = self.comments.vote(True, id, utils.anonymize(str(request.remote_addr)))
         return Response(json.dumps(nv), 200)
 
+    @xhr
     def dislike(self, environ, request, id):
 
         nv = self.comments.vote(False, id, utils.anonymize(str(request.remote_addr)))

setup.py

@@ -17,7 +17,7 @@ else:
 
 setup(
     name='isso',
-    version='0.5',
+    version='0.5.3.dev0',
     author='Martin Zimmermann',
     author_email='info@posativ.org',
     packages=find_packages(),

specs/fixtures.py

@@ -2,6 +2,7 @@
 
 import json
 
+from werkzeug.test import Client
 
 class FakeIP(object):
 
@@ -14,6 +15,13 @@ class FakeIP(object):
         return self.app(environ, start_response)
 
 
+class JSONClient(Client):
+
+    def open(self, *args, **kwargs):
+        kwargs.setdefault('content_type', 'application/json')
+        return super(JSONClient, self).open(*args, **kwargs)
+
+
 class Dummy:
 
     status = 200

specs/test_comments.py

@@ -12,14 +12,13 @@ try:
 except ImportError:
     from urllib import urlencode
 
-from werkzeug.test import Client
 from werkzeug.wrappers import Response
 
 from isso import Isso, core
 from isso.utils import http
 from isso.views import comments
 
-from fixtures import curl, loads, FakeIP
+from fixtures import curl, loads, FakeIP, JSONClient
 http.curl = curl
 
 
@@ -37,7 +36,7 @@ class TestComments(unittest.TestCase):
         self.app = App(conf)
         self.app.wsgi_app = FakeIP(self.app.wsgi_app, "192.168.1.1")
 
-        self.client = Client(self.app, Response)
+        self.client = JSONClient(self.app, Response)
         self.get = self.client.get
         self.put = self.client.put
         self.post = self.client.post
@@ -133,7 +132,7 @@ class TestComments(unittest.TestCase):
 
     def testDeleteWithReference(self):
 
-        client = Client(self.app, Response)
+        client = JSONClient(self.app, Response)
         client.post('/new?uri=%2Fpath%2F', data=json.dumps({'text': 'First'}))
         client.post('/new?uri=%2Fpath%2F', data=json.dumps({'text': 'First', 'parent': 1}))
 
@@ -160,7 +159,7 @@ class TestComments(unittest.TestCase):
                     --- [ comment 4, ref 2 ]
         [ comment 5 ]
         """
-        client = Client(self.app, Response)
+        client = JSONClient(self.app, Response)
 
         client.post('/new?uri=%2Fpath%2F', data=json.dumps({'text': 'First'}))
         client.post('/new?uri=%2Fpath%2F', data=json.dumps({'text': 'Second', 'parent': 1}))
@@ -193,11 +192,11 @@ class TestComments(unittest.TestCase):
 
     def testDeleteAndCreateByDifferentUsersButSamePostId(self):
 
-        mallory = Client(self.app, Response)
+        mallory = JSONClient(self.app, Response)
         mallory.post('/new?uri=%2Fpath%2F', data=json.dumps({'text': 'Foo'}))
         mallory.delete('/id/1')
 
-        bob = Client(self.app, Response)
+        bob = JSONClient(self.app, Response)
         bob.post('/new?uri=%2Fpath%2F', data=json.dumps({'text': 'Bar'}))
 
         assert mallory.delete('/id/1').status_code == 403
@@ -263,10 +262,27 @@ class TestComments(unittest.TestCase):
 
     def testDeleteCommentRemovesThread(self):
 
-            rv = self.client.post('/new?uri=%2F', data=json.dumps({"text": "..."}))
-            assert '/' in self.app.db.threads
-            self.client.delete('/id/1')
-            assert '/' not in self.app.db.threads
+        rv = self.client.post('/new?uri=%2F', data=json.dumps({"text": "..."}))
+        assert '/' in self.app.db.threads
+        self.client.delete('/id/1')
+        assert '/' not in self.app.db.threads
+
+    def testCSRF(self):
+
+        js = "application/json"
+        form = "application/x-www-form-urlencoded"
+
+        self.post('/new?uri=%2F', data=json.dumps({"text": "..."}))
+
+        # no header is fine (default for XHR)
+        assert self.post('/id/1/dislike', content_type="").status_code == 200
+
+        # x-www-form-urlencoded is definitely not RESTful
+        assert self.post('/id/1/dislike', content_type=form).status_code == 403
+        assert self.post('/new?uri=%2F', data=json.dumps({"text": "..."}),
+                                         content_type=form).status_code == 403
+        # just for the record
+        assert self.post('/id/1/dislike', content_type=js).status_code == 200
 
 
 class TestModeratedComments(unittest.TestCase):
@@ -283,7 +299,7 @@ class TestModeratedComments(unittest.TestCase):
 
         self.app = App(conf)
         self.app.wsgi_app = FakeIP(self.app.wsgi_app, "192.168.1.1")
-        self.client = Client(self.app, Response)
+        self.client = JSONClient(self.app, Response)
 
     def tearDown(self):
         os.unlink(self.path)
@@ -314,7 +330,7 @@ class TestPurgeComments(unittest.TestCase):
 
         self.app = App(conf)
         self.app.wsgi_app = FakeIP(self.app.wsgi_app, "192.168.1.1")
-        self.client = Client(self.app, Response)
+        self.client = JSONClient(self.app, Response)
 
     def testPurgeDoesNoHarm(self):
         self.client.post('/new?uri=test', data=json.dumps({"text": "..."}))

specs/test_vote.py

@@ -6,13 +6,12 @@ import json
 import tempfile
 import unittest
 
-from werkzeug.test import Client
 from werkzeug.wrappers import Response
 
 from isso import Isso, core
 from isso.utils import http
 
-from fixtures import curl, loads, FakeIP
+from fixtures import curl, loads, FakeIP, JSONClient
 http.curl = curl
 
 
@@ -33,7 +32,7 @@ class TestVote(unittest.TestCase):
         app = App(conf)
         app.wsgi_app = FakeIP(app.wsgi_app, ip)
 
-        return Client(app, Response)
+        return JSONClient(app, Response)
 
     def testZeroLikes(self):