diff --git a/raven/rpc/transports/deliverables.py b/raven/rpc/transports/deliverables.py index 576bce709..298d44524 100644 --- a/raven/rpc/transports/deliverables.py +++ b/raven/rpc/transports/deliverables.py @@ -118,6 +118,22 @@ async def download(request: web.Request) -> web.StreamResponse: headers={ "Content-Type": record.media_type, "Content-Disposition": _attachment(record.name), + # `register` reuses the token when a conversation delivers the + # same path again, so a rewritten file keeps its URL -- and the + # delivery card points an straight at this route. Without + # a directive the browser invents a freshness lifetime from + # Last-Modified (about a tenth of the file's age) and answers + # from its own copy without asking, so an image the agent + # replaced went on rendering as the one it replaced. + # `no-cache` would also close that window: `web.FileResponse` + # derives its validator from the file now at this path, so a + # conditional request after a rewrite does get the new bytes. + # `no-store` is chosen for the two reasons that do separate + # them: it matches `/file` and `/knowledge/file`, which serve + # the same kind of file, and it is the only directive that + # keeps an agent's output out of the reader's disk cache. This + # route has no size ceiling, unlike those two. + "Cache-Control": "no-store", }, ) @@ -134,6 +150,10 @@ async def download_archive(request: web.Request) -> web.StreamResponse: headers={ "Content-Type": "application/zip", "Content-Disposition": _attachment("deliverables.zip"), + # Same reason as the single-file route: a token names a + # delivery, not a fixed set of files, and the members behind it + # can be rewritten between two downloads of the same URL. + "Cache-Control": "no-store", } ) if request.method == "HEAD": diff --git a/tests/test_rpc_transports_deliverables.py b/tests/test_rpc_transports_deliverables.py index 63e9c25a1..a61e6a710 100644 --- a/tests/test_rpc_transports_deliverables.py +++ b/tests/test_rpc_transports_deliverables.py @@ -139,8 +139,9 @@ async def test_control_characters_in_a_filename_are_stripped(client) -> None: async def test_archive_head_agrees_with_get(client) -> None: - """The frontend pre-checks "Download all" with HEAD too, so the branch that - skips the zip build must answer with the same status.""" + """The route is registered with ``allow_head=True``, so a HEAD has to answer + what a GET would. The archive builds its zip only on the GET path, so HEAD + is a second path that could drift from it.""" one = _register(client.store, client.tmp_path, "a.txt", b"AAA") ok = await client.head("/files/download-archive", params={"token": one.token}) @@ -205,6 +206,51 @@ async def test_an_unregistered_route_still_answers_404(client) -> None: assert res.status == 404 +async def test_download_forbids_storing_so_a_rewritten_file_is_fetched_again(client) -> None: + """A deliverable that was rewritten in place must not come back from the + browser cache. + + ``register`` reuses the token when a conversation delivers the same path + again, so a rewritten file keeps the URL it had -- and the delivery card + points an ```` straight at this route. With no ``Cache-Control`` the + browser invents a freshness lifetime from ``Last-Modified`` (about 10% of + the file's age) and answers from its own copy without asking. An image the + agent replaced then went on rendering as the one it replaced, which is + indistinguishable from a server that did not pick the change up. + """ + record = _register(client.store, client.tmp_path) + + res = await client.get("/files/download", params={"token": record.token}) + + assert res.headers["Cache-Control"] == "no-store" + + +async def test_archive_download_forbids_storing_too(client) -> None: + """The same route for a set of files: a token whose member changed must not + be answered from a stored copy either.""" + one = _register(client.store, client.tmp_path, "a.txt", b"AAA") + + res = await client.get("/files/download-archive", params={"token": one.token}) + + assert res.headers["Cache-Control"] == "no-store" + + +async def test_head_carries_the_directive_as_well(client) -> None: + """The directive has to be on the response HEAD is answered with. + + HEAD never reaches the archive's zip build: it returns through the early + exit before that work starts, so the directive has to sit on the response + the constructor builds rather than be set on the GET path afterwards.""" + record = _register(client.store, client.tmp_path) + one = _register(client.store, client.tmp_path, "a.txt", b"AAA") + + file_head = await client.head("/files/download", params={"token": record.token}) + archive_head = await client.head("/files/download-archive", params={"token": one.token}) + + assert file_head.headers["Cache-Control"] == "no-store" + assert archive_head.headers["Cache-Control"] == "no-store" + + async def test_resolve_download_drops_stale_entry(tmp_path) -> None: store = DeliverableStore(tmp_path / "deliverables.json") record = _register(store, tmp_path)