Merge branch 'cancel-fix' (closes #8)

Author: danielrichman

docs/source/index.rst

@@ -8,6 +8,7 @@ Contents:
 
    quickstart
    security
+   misc
    modules
 
 Links

docs/source/misc.rst

@@ -0,0 +1,54 @@
+Misc
+========
+
+.. _cancel_url:
+
+Response URL for "cancels"
+--------------------------
+
+The short story is that when the WLS wants to send a "response" to the WAA, it
+takes the URL you provided in the request, adds a `WLS-Response` query
+parameter, and redirects the client to that URL.
+
+Happily, it guarantees that this will be done by appending
+`(?|&)WLS-Response=...` to the URL (which means that this process is easy to
+undo, which is a necessary part of :ref:`checking-response-values`).
+
+However: while in version 3 it preserves any query parameters that were already
+in the request URL, in version 1 of the protocol it will not: that is, it
+deletes the query component before appending `?WLS-Response...`. Furthermore,
+while the current version of the WLS appears to reply with version 3 upon
+success, if you click "cancel" then it will use version 1, presumably because
+of reasons.
+
+The WLS does include in its response a copy of some of the request parameters,
+in particular, the return URL. It is possible to extract this from the
+response, and after inspecting WLS-Response, perform a redirect to it,
+recovering the deleted query parameters. The `flask_glue` does exactly this,
+and so hopefully you should not suffer problems on account of this behaviour.
+
+Note that if you for some reason had the requirement that requests to a certain
+page need only be Raven authenticated if a certain query parameter is present,
+then something like this would not work correctly::
+
+    def my_before_request():
+        if "special" in request.args:
+            return flask_glue.before_request()
+        else:
+            return None
+
+... since if a user clicks Cancel, the special query parameter would not be
+set, so the `before_request` function would run, and the response from the WLS
+would not be handled. Instead, something like this would be necessary::
+
+    def my_before_request():
+        if "special" in request.args or "WLS-Response" in request.args:
+            return flask_glue.before_request()
+        else:
+            return None
+
+If you are not using the `flask_glue`, I suggest where possible just avoiding
+having significant query parameters on the URL that you use to perform Raven
+authentication, and then simply check that `request.base_url` matches the URL
+in the signed response. Otherwise, have a look at the implementation of
+`flask_glue` for inspiration.

docs/source/security.rst

@@ -36,6 +36,9 @@ response:
   Alternatively, you could sanitise `Host` and `X-Forwarded-Host` in your
   web-server.
 
+  If you might have query parameters in your `url`, you need to take care to
+  handle negative respones from the WLS. See :ref:`cancel_url`.
+
 * check `issue` is within an acceptable range of *now*
 
   ... lest someone replay an old response to log in again

tests/test_auth_decorator.py

@@ -523,6 +523,40 @@ def test_removes_wls_response_from_URL_401(self):
             r = client.get("/decorated")
             assert r.status_code == 401
 
+    # Sadly, the real world raven still uses version 1 for negative responses
+    def test_handles_v1_removing_query_parameters(self):
+        rig = TestRig()
+
+        with rig as (client, wls, views):
+            r = client.get("/decorated?special")
+            assert r.status_code == 303
+            url, query = r.headers["location"].split("?")
+
+        with rig as (client, wls, views):
+            u = "http://localhost/decorated?special"
+            status = ucam_webauth.STATUS_CANCELLED
+            wls.expect({"url": u}, make_response(ver=1, status=status, url=u))
+
+            r = client.get(url, query_string=query)
+            assert r.status_code == 302
+            url, query = r.headers["location"].split("?")
+            assert url == "http://localhost/decorated"
+
+        # with a v1 response, the WLS actually deletes other query parameters
+        assert query.startswith("special&WLS-Response")
+        query = query[len("special&"):]
+
+        with rig as (client, wls, views):
+            r = client.get("/decorated", query_string=query)
+            assert r.status_code == 303
+            # it should restore the query parameter:
+            assert r.headers["location"] == "http://localhost/decorated?special"
+
+        with rig as (client, wls, views):
+            # and finally give us a 401
+            r = client.get("/decorated?special")
+            assert r.status_code == 401
+
     def test_require_default(self):
         self.check_auth(TestRig(), principal="something",
                         ptags=set(["current"]))
@@ -632,10 +666,22 @@ def test_issue_bounds(self):
 
     def test_checks_url(self):
         rig = TestRig()
+        # Modify response.url: it should cause a mis-match
         self.check_auth_abort(rig, 400, url="http://localhost/other")
         self.check_auth_abort(rig, 400, url="http://other/decorated")
         self.check_auth_abort(rig, 400, url="http://localhost/decorated?test")
 
+        rig = TestRig()
+
+        with rig as (client, wls, views):
+            # keep the response url as before, but modify the request url so
+            # that they do not match
+            url = "http://localhost/decorated?special"
+            wls.expect({"url": url}, make_response(principal="djr61"))
+
+            response = client.get("/decorated?special", follow_redirects=True)
+            assert response.status_code == 400
+
     def test_doesnt_nuke_session(self):
         rig = TestRig()
 
@@ -952,3 +998,9 @@ class R(flask.Request):
 
             assert_raises(RuntimeError,
                           client.get, "/decorated", follow_redirects=True)
+
+    # Check that the v1 branch doesn't break anything.
+    # I'm tempted to say that a ver=1 successful response should be rejected, but
+    # I'm not sure.
+    def test_auth_v1(self):
+        self.check_auth(TestRig(), ver=1)

ucam_webauth/flask_glue.py

@@ -398,7 +398,7 @@ def _handle_response(self, response):
 
         """
 
-        url_without_response = self._check_url(response.url)
+        url_without_response = self._check_url(response)
         if url_without_response is None:
             abort(400)
 
@@ -435,7 +435,7 @@ def _handle_response(self, response):
 
         return redirect(url_without_response, code=303)
 
-    def _check_url(self, wls_response_url):
+    def _check_url(self, response):
         """
         Check if the response from the WLS was intended for us
 
@@ -450,14 +450,16 @@ def _check_url(self, wls_response_url):
                                "Host/X-Forwarded-Host headers and pass "
                                "can_trust_request_host = True")
 
+        wls_response_url = response.url
         actual_url = request.url
 
-        # note: mod_ucam_webauth simply strips everything up to a ?
-        # from both urls and compares.
+        # See the docs (misc/Response URLs for Cancels) for comments on how
+        # the WLS constructs the URL to which the user is redirected after a
+        # request.
+        # Essentially, it either appends (?|&)WLS-Response=... to the URL
+        # in the request, or appends ?WLS-Response=... to the URL in the
+        # request with the query part removed.
 
-        # see waa2wls-protocol.txt - the WLS appends (?|&)WLS-Response=
-        # so, removing that from the end of the string should recover
-        # the exact url sent in the request
         start = max(actual_url.rfind("?WLS-Response="),
                     actual_url.rfind("&WLS-Response="))
         if start == -1:
@@ -475,13 +477,20 @@ def _check_url(self, wls_response_url):
                          response_part, request.args["WLS-Response"])
             return None
 
-        # finally check that they agree.
+        # chop off the WLS-Response
         actual_url = actual_url[:start]
-        if wls_response_url != actual_url:
+
+        if response.ver == 1:
+            expected_response_url, _, _ = wls_response_url.partition("?")
+        else:
+            expected_response_url = wls_response_url
+
+        # finally check that they agree.
+        if expected_response_url != actual_url:
             logger.debug("response.url did not match url visited "
                          "(replay of WLS-Response to other website?) "
-                         "response.url=%r request.url=%r",
-                         wls_response_url, actual_url)
+                         "expected_response_url=%r actual_url=%r",
+                         expected_response_url, actual_url)
             return None
 
         return wls_response_url