From 03bf984e304278247fa253696a8edda71d785642 Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Fri, 12 Feb 2016 10:15:31 -0800 Subject: [PATCH] Add Expires header for successful GETs using tempurls This allows caches to actually use the expiration time that's already embedded in the URL. From RFC 2616 [1]: > [S]ince some applications have traditionally used GETs and HEADs with > query URLs (those containing a "?" in the rel_path part) to perform > operations with significant side effects, caches MUST NOT treat > responses to such URIs as fresh unless the server provides an explicit > expiration time. However, RFC 7234 notes that this hasn't played out in practice [2]: > Note: Section 13.9 of [RFC2616] prohibited caches from calculating > heuristic freshness for URIs with query components (i.e., those > containing '?'). In practice, this has not been widely implemented. > Therefore, origin servers are encouraged to send explicit directives > (e.g., Cache-Control: no-cache) if they wish to preclude caching. [1] http://tools.ietf.org/html/rfc2616#section-13.9 [2] http://tools.ietf.org/html/rfc7234#section-4.2.2 Change-Id: Ie8f26b97a124ea220a20800e35e040e4463d82bc Closes-Bug: 1502159 --- swift/common/middleware/tempurl.py | 7 ++++- test/unit/common/middleware/test_tempurl.py | 30 ++++++++++++++++----- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/swift/common/middleware/tempurl.py b/swift/common/middleware/tempurl.py index 234820791c..f2863aa9f6 100644 --- a/swift/common/middleware/tempurl.py +++ b/swift/common/middleware/tempurl.py @@ -163,7 +163,7 @@ __all__ = ['TempURL', 'filter_factory', from os.path import basename -from time import time +from time import time, strftime, gmtime from six.moves.urllib.parse import parse_qs from six.moves.urllib.parse import urlencode @@ -425,6 +425,11 @@ class TempURL(object): # newline into existing_disposition value = disposition_value.replace('\n', '%0A') out_headers.append(('Content-Disposition', value)) + + # include Expires header for better cache-control + out_headers.append(('Expires', strftime( + "%a, %d %b %Y %H:%M:%S GMT", + gmtime(temp_url_expires)))) headers = out_headers return start_response(status, headers, exc_info) diff --git a/test/unit/common/middleware/test_tempurl.py b/test/unit/common/middleware/test_tempurl.py index 0fc895f9e2..81096412b0 100644 --- a/test/unit/common/middleware/test_tempurl.py +++ b/test/unit/common/middleware/test_tempurl.py @@ -30,9 +30,10 @@ import hmac import itertools +import mock import unittest from hashlib import sha1 -from time import time +from time import time, strftime, gmtime from swift.common.middleware import tempauth, tempurl from swift.common.header_key_dict import HeaderKeyDict @@ -134,6 +135,9 @@ class TestTempURL(unittest.TestCase): self.assertEqual(resp.status_int, 200) self.assertEqual(resp.headers['content-disposition'], 'attachment; filename="o"; ' + "filename*=UTF-8''o") + self.assertEqual(resp.headers['expires'], + strftime('%a, %d %b %Y %H:%M:%S GMT', + gmtime(expires))) self.assertEqual(req.environ['swift.authorize_override'], True) self.assertEqual(req.environ['REMOTE_USER'], '.wsgi.tempurl') @@ -181,9 +185,10 @@ class TestTempURL(unittest.TestCase): for sig in (sig1, sig2): self.assert_valid_sig(expires, path, account_keys, sig, environ) - def test_get_valid_with_filename(self): + @mock.patch('swift.common.middleware.tempurl.time', return_value=0) + def test_get_valid_with_filename(self, mock_time): method = 'GET' - expires = int(time() + 86400) + expires = (((24 + 1) * 60 + 1) * 60) + 1 path = '/v1/a/c/o' key = 'abc' hmac_body = '%s\n%s\n%s' % (method, expires, path) @@ -197,6 +202,9 @@ class TestTempURL(unittest.TestCase): self.assertEqual(resp.headers['content-disposition'], 'attachment; filename="bob %22killer%22.txt"; ' + "filename*=UTF-8''bob%20%22killer%22.txt") + self.assertIn('expires', resp.headers) + self.assertEqual('Fri, 02 Jan 1970 01:01:01 GMT', + resp.headers['expires']) self.assertEqual(req.environ['swift.authorize_override'], True) self.assertEqual(req.environ['REMOTE_USER'], '.wsgi.tempurl') @@ -240,9 +248,10 @@ class TestTempURL(unittest.TestCase): get_resp = get_req.get_response(self.tempurl) self.assertEqual(resp.headers, get_resp.headers) - def test_get_valid_with_filename_and_inline(self): + @mock.patch('swift.common.middleware.tempurl.time', return_value=0) + def test_get_valid_with_filename_and_inline(self, mock_time): method = 'GET' - expires = int(time() + 86400) + expires = 1 path = '/v1/a/c/o' key = 'abc' hmac_body = '%s\n%s\n%s' % (method, expires, path) @@ -254,6 +263,9 @@ class TestTempURL(unittest.TestCase): resp = req.get_response(self.tempurl) self.assertEqual(resp.status_int, 200) self.assertEqual(resp.headers['content-disposition'], 'inline') + self.assertIn('expires', resp.headers) + self.assertEqual('Thu, 01 Jan 1970 00:00:01 GMT', + resp.headers['expires']) self.assertEqual(req.environ['swift.authorize_override'], True) self.assertEqual(req.environ['REMOTE_USER'], '.wsgi.tempurl') @@ -271,6 +283,7 @@ class TestTempURL(unittest.TestCase): resp = req.get_response(self.tempurl) self.assertEqual(resp.status_int, 200) self.assertEqual(resp.headers['content-disposition'], 'inline') + self.assertIn('expires', resp.headers) self.assertEqual(req.environ['swift.authorize_override'], True) self.assertEqual(req.environ['REMOTE_USER'], '.wsgi.tempurl') @@ -290,6 +303,7 @@ class TestTempURL(unittest.TestCase): self.assertEqual(resp.headers['content-disposition'], 'attachment; filename="a%0D%0Ab"; ' + "filename*=UTF-8''a%0D%0Ab") + self.assertIn('expires', resp.headers) self.assertEqual(req.environ['swift.authorize_override'], True) self.assertEqual(req.environ['REMOTE_USER'], '.wsgi.tempurl') @@ -309,6 +323,7 @@ class TestTempURL(unittest.TestCase): self.assertEqual(resp.status_int, 200) self.assertEqual(resp.headers['content-disposition'], 'attachment; filename="fu%0Abar"') + self.assertIn('expires', resp.headers) self.assertEqual(req.environ['swift.authorize_override'], True) self.assertEqual(req.environ['REMOTE_USER'], '.wsgi.tempurl') @@ -328,6 +343,7 @@ class TestTempURL(unittest.TestCase): self.assertEqual(resp.headers['content-disposition'], 'attachment; filename="o"; ' + "filename*=UTF-8''o") + self.assertIn('expires', resp.headers) self.assertEqual(req.environ['swift.authorize_override'], True) self.assertEqual(req.environ['REMOTE_USER'], '.wsgi.tempurl') @@ -348,6 +364,7 @@ class TestTempURL(unittest.TestCase): resp.headers['content-disposition'], 'attachment; filename="/i/want/this/just/as/it/is/"; ' + "filename*=UTF-8''/i/want/this/just/as/it/is/") + self.assertIn('expires', resp.headers) self.assertEqual(req.environ['swift.authorize_override'], True) self.assertEqual(req.environ['REMOTE_USER'], '.wsgi.tempurl') @@ -364,7 +381,8 @@ class TestTempURL(unittest.TestCase): sig, expires)}) resp = req.get_response(self.tempurl) self.assertEqual(resp.status_int, 404) - self.assertFalse('content-disposition' in resp.headers) + self.assertNotIn('content-disposition', resp.headers) + self.assertNotIn('expires', resp.headers) self.assertEqual(req.environ['swift.authorize_override'], True) self.assertEqual(req.environ['REMOTE_USER'], '.wsgi.tempurl')