From 3e4607954671dee21107e6083f0695076987cc3e Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Fri, 29 Jan 2016 13:08:42 -0800 Subject: [PATCH] Add Vary: headers for CORS responses From the (non-normative) Implementation Considerations section of https://www.w3.org/TR/cors/#resource-implementation : > Resources that wish to enable themselves to be shared with multiple > Origins but do not respond uniformly with "*" must in practice > generate the Access-Control-Allow-Origin header dynamically in > response to every request they wish to allow. As a consequence, > authors of such resources should send a Vary: Origin HTTP header or > provide other appropriate control directives to prevent caching of > such responses, which may be inaccurate if re-used across-origins. We do the first part (dynamic Access-Control-Allow-Origin: generation based on the incoming Origin: header), but not the second (send a Vary: Origin header). Consider this scenario: 1. Swift user Alice has some static content that should be available from some (but not all) other domains. She creates a new container with an appropriate X-Container-Meta-Access-Control-Allow-Origin like "http://foo.example.com http://bar.example.com". 2. End user Bob pulls up a browser and visits http://foo.example.com, which references a cross-origin resource. Seeing this, the browser issues a preflight request and gets back a response that includes headers like: Access-Control-Allow-Origin: http://foo.example.com Access-Control-Allow-Methods: HEAD, GET, PUT, POST, COPY, OPTIONS, DELETE Since the preflight succeeded, the browser follows through on the cross-origin request and everything loads properly. 3. Now Bob visits http://bar.example.com, which references the same resource. Ordinarily, the exact same thing would happen, but with http://bar.example.com in the headers. However, if the browser cached the preflight response (because it didn't want to make two requests everytime it needed a resource), it would assume the server would only allow resource-sharing with http://foo.example.com and not load the resource. Similar issues arise from the dynamically-generated Access-Control-Allow-Headers header. For more information on the Vary: header, see http://tools.ietf.org/html/rfc7231#section-7.1.4 Change-Id: I9950e593312f654ee596b7f43f7ab9e5b684d8e5 --- swift/proxy/controllers/base.py | 29 +++++++++++++-------- test/unit/proxy/test_server.py | 45 +++++++++++++++++++++++---------- 2 files changed, 50 insertions(+), 24 deletions(-) diff --git a/swift/proxy/controllers/base.py b/swift/proxy/controllers/base.py index ace3832c1d..dba2c17dbc 100644 --- a/swift/proxy/controllers/base.py +++ b/swift/proxy/controllers/base.py @@ -1853,28 +1853,37 @@ class Controller(object): resp.status = HTTP_UNAUTHORIZED return resp - # Allow all headers requested in the request. The CORS - # specification does leave the door open for this, as mentioned in - # http://www.w3.org/TR/cors/#resource-preflight-requests - # Note: Since the list of headers can be unbounded - # simply returning headers can be enough. - allow_headers = set() - if req.headers.get('Access-Control-Request-Headers'): - allow_headers.update( - list_from_csv(req.headers['Access-Control-Request-Headers'])) - # Populate the response with the CORS preflight headers if cors.get('allow_origin') and \ cors.get('allow_origin').strip() == '*': headers['access-control-allow-origin'] = '*' else: headers['access-control-allow-origin'] = req_origin_value + if 'vary' in headers: + headers['vary'] += ', Origin' + else: + headers['vary'] = 'Origin' + if cors.get('max_age') is not None: headers['access-control-max-age'] = cors.get('max_age') + headers['access-control-allow-methods'] = \ ', '.join(self.allowed_methods) + + # Allow all headers requested in the request. The CORS + # specification does leave the door open for this, as mentioned in + # http://www.w3.org/TR/cors/#resource-preflight-requests + # Note: Since the list of headers can be unbounded + # simply returning headers can be enough. + allow_headers = set( + list_from_csv(req.headers.get('Access-Control-Request-Headers'))) if allow_headers: headers['access-control-allow-headers'] = ', '.join(allow_headers) + if 'vary' in headers: + headers['vary'] += ', Access-Control-Request-Headers' + else: + headers['vary'] = 'Access-Control-Request-Headers' + resp.headers = headers return resp diff --git a/test/unit/proxy/test_server.py b/test/unit/proxy/test_server.py index 41181d102c..784bd83525 100644 --- a/test/unit/proxy/test_server.py +++ b/test/unit/proxy/test_server.py @@ -4971,6 +4971,8 @@ class TestObjectController(unittest.TestCase): 'Access-Control-Request-Method': 'GET'}) resp = controller.OPTIONS(req) self.assertEqual(401, resp.status_int) + self.assertNotIn('Access-Control-Allow-Origin', resp.headers) + self.assertNotIn('Vary', resp.headers) def my_empty_origin_container_info(*args): return {'cors': {'allow_origin': None}} @@ -4982,6 +4984,8 @@ class TestObjectController(unittest.TestCase): 'Access-Control-Request-Method': 'GET'}) resp = controller.OPTIONS(req) self.assertEqual(401, resp.status_int) + self.assertNotIn('Access-Control-Allow-Origin', resp.headers) + self.assertNotIn('Vary', resp.headers) def my_container_info(*args): return { @@ -5002,13 +5006,13 @@ class TestObjectController(unittest.TestCase): self.assertEqual( 'https://foo.bar', resp.headers['access-control-allow-origin']) - for verb in 'OPTIONS GET POST PUT DELETE HEAD'.split(): - self.assertIn(verb, - resp.headers['access-control-allow-methods']) + self.assertEqual('Origin', resp.headers.get('vary')) self.assertEqual( - len(resp.headers['access-control-allow-methods'].split(', ')), - 6) + sorted(resp.headers['access-control-allow-methods'] + .split(', ')), + sorted('OPTIONS GET POST PUT DELETE HEAD'.split())) self.assertEqual('999', resp.headers['access-control-max-age']) + req = Request.blank( '/v1/a/c/o.jpg', {'REQUEST_METHOD': 'OPTIONS'}, @@ -5016,19 +5020,28 @@ class TestObjectController(unittest.TestCase): req.content_length = 0 resp = controller.OPTIONS(req) self.assertEqual(401, resp.status_int) + self.assertNotIn('Access-Control-Allow-Origin', resp.headers) + self.assertNotIn('Vary', resp.headers) + req = Request.blank('/v1/a/c/o.jpg', {'REQUEST_METHOD': 'OPTIONS'}) req.content_length = 0 resp = controller.OPTIONS(req) self.assertEqual(200, resp.status_int) - for verb in 'OPTIONS GET POST PUT DELETE HEAD'.split(): - self.assertIn(verb, resp.headers['Allow']) - self.assertEqual(len(resp.headers['Allow'].split(', ')), 6) + self.assertEqual( + sorted(resp.headers['Allow'].split(', ')), + sorted('OPTIONS GET POST PUT DELETE HEAD'.split())) + self.assertNotIn('Access-Control-Allow-Origin', resp.headers) + self.assertNotIn('Vary', resp.headers) + req = Request.blank( '/v1/a/c/o.jpg', {'REQUEST_METHOD': 'OPTIONS'}, headers={'Origin': 'http://foo.com'}) resp = controller.OPTIONS(req) self.assertEqual(401, resp.status_int) + self.assertNotIn('Access-Control-Allow-Origin', resp.headers) + self.assertNotIn('Vary', resp.headers) + req = Request.blank( '/v1/a/c/o.jpg', {'REQUEST_METHOD': 'OPTIONS'}, @@ -5037,6 +5050,7 @@ class TestObjectController(unittest.TestCase): controller.app.cors_allow_origin = ['http://foo.bar', ] resp = controller.OPTIONS(req) self.assertEqual(200, resp.status_int) + self.assertEqual('Origin', resp.headers.get('vary')) def my_container_info_wildcard(*args): return { @@ -5055,12 +5069,11 @@ class TestObjectController(unittest.TestCase): resp = controller.OPTIONS(req) self.assertEqual(200, resp.status_int) self.assertEqual('*', resp.headers['access-control-allow-origin']) - for verb in 'OPTIONS GET POST PUT DELETE HEAD'.split(): - self.assertIn(verb, - resp.headers['access-control-allow-methods']) + self.assertNotIn('Vary', resp.headers) self.assertEqual( - len(resp.headers['access-control-allow-methods'].split(', ')), - 6) + sorted(resp.headers['access-control-allow-methods'] + .split(', ')), + sorted('OPTIONS GET POST PUT DELETE HEAD'.split())) self.assertEqual('999', resp.headers['access-control-max-age']) def _get_CORS_response(self, container_cors, strict_mode, object_get=None): @@ -7114,11 +7127,13 @@ class TestContainerController(unittest.TestCase): '/v1/a/c/o.jpg', {'REQUEST_METHOD': 'OPTIONS'}, headers={'Origin': 'https://bar.baz', + 'Access-Control-Request-Headers': ' , ,,', 'Access-Control-Request-Method': 'GET'}) req.content_length = 0 resp = controller.OPTIONS(req) self.assertEqual(200, resp.status_int) self.assertEqual('*', resp.headers['access-control-allow-origin']) + self.assertNotIn('access-control-allow-headers', resp.headers) for verb in 'OPTIONS GET POST PUT DELETE HEAD'.split(): self.assertIn(verb, resp.headers['access-control-allow-methods']) @@ -7132,7 +7147,7 @@ class TestContainerController(unittest.TestCase): {'REQUEST_METHOD': 'OPTIONS'}, headers={'Origin': 'https://bar.baz', 'Access-Control-Request-Headers': - 'x-foo, x-bar, x-auth-token', + 'x-foo, x-bar, , x-auth-token', 'Access-Control-Request-Method': 'GET'} ) req.content_length = 0 @@ -7141,6 +7156,8 @@ class TestContainerController(unittest.TestCase): self.assertEqual( sortHeaderNames('x-foo, x-bar, x-auth-token'), sortHeaderNames(resp.headers['access-control-allow-headers'])) + self.assertEqual('Access-Control-Request-Headers', + resp.headers.get('vary')) def test_CORS_valid(self): with save_globals():