Skip to content

Commit

Permalink
Remove directory from disposition header (#3515)
Browse files Browse the repository at this point in the history
PBENCH-1240

Now that the dashboard supports download of tarball inventory, I observed that
the generated filename included a dataset-name prefix. This is because the
content disposition header allows only a name, not a path, and the browser is
trying to be "helpful" by converting the `<dataset-name>/<filename>` string,
replacing the disallowed `/` with `_`. The result is ugly and unexpected.

NOTE: although the Mozilla guidelines try to suggest that the filename must be
enclosed in quotes, this conflicts with the RFC, and Werkzeug's `send_file` is
"reluctant" to include quotes (plus it's ugly to construct). Therefore, this
doesn't attempt to insert the (presumably unnecessary) quotes. (And note also
that, despite the ugly name, the current TOC download works without a quoted
filename in Chrome.)

Instead, report only the name of the file. Add unit and functional test
validation.
  • Loading branch information
dbutenhof authored Aug 7, 2023
1 parent 647e9f2 commit 9b071de
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 3 deletions.
6 changes: 3 additions & 3 deletions lib/pbench/server/api/resources/datasets_inventory.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from http import HTTPStatus
from pathlib import Path
from urllib.request import Request

from flask import current_app, send_file
Expand Down Expand Up @@ -84,10 +85,9 @@ def _get(
# We link a callback to close the file stream and TarFile object on
# completion.
stream = file_info["stream"]
name = Path(file_info["name"]).name
try:
resp = send_file(
stream, as_attachment=target is None, download_name=file_info["name"]
)
resp = send_file(stream, as_attachment=target is None, download_name=name)
except Exception as e:
if stream:
stream.close()
Expand Down
9 changes: 9 additions & 0 deletions lib/pbench/test/functional/server/test_datasets.py
Original file line number Diff line number Diff line change
Expand Up @@ -751,6 +751,15 @@ def read_metadata(response: Response) -> JSONOBJECT:
API.DATASETS_INVENTORY,
{"dataset": dataset.resource_id, "target": "metadata.log"},
)

# Note that Werkzeug doesn't understand anything special about the
# ".log" filetype, but will apply a better content-type header for
# file types it understands like ".json".
assert response.headers["content-type"] == "application/octet-stream"
assert (
response.headers["content-disposition"]
== "inline; filename=metadata.log"
)
assert (
response.ok
), f"INVENTORY {dataset.name} failed {response.status_code}:{response.json()['message']}"
Expand Down
17 changes: 17 additions & 0 deletions lib/pbench/test/unit/server/test_datasets_inventory.py
Original file line number Diff line number Diff line change
Expand Up @@ -156,3 +156,20 @@ def mock_send_file(path_or_file, *args, **kwargs):

query_get_as("fio_2", "f1.json", HTTPStatus.INTERNAL_SERVER_ERROR)
assert mock_close

def test_get_inventory(self, query_get_as, monkeypatch):
exp_stream = io.BytesIO(b"file_as_a_byte_stream")

def mock_get_inventory(_s, _d: str, _t: str):
return {
"name": "f1.json",
"type": CacheType.FILE,
"stream": Inventory(exp_stream, None),
}

monkeypatch.setattr(CacheManager, "get_inventory", mock_get_inventory)
response = query_get_as("fio_2", "f1.json", HTTPStatus.OK)
assert response.status_code == HTTPStatus.OK
assert response.text == "file_as_a_byte_stream"
assert response.headers["content-type"] == "application/json"
assert response.headers["content-disposition"] == "inline; filename=f1.json"

0 comments on commit 9b071de

Please sign in to comment.