mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
fix(web): make Content-Disposition filenames safe on the wire
Attachment `/content`, preview, and workstream-export downloads built the Content-Disposition `filename="..."` value straight from a user-supplied name, stripping only quotes and CR/LF. Three input classes still broke the header: - Non-latin-1 names (CJK, em dash): Starlette encodes header values as latin-1 and raised, 500-ing the serving route. (The original get_content bug.) - ASCII control bytes (NUL, form-feed, VT, DEL): latin-1-encodable, so they passed Starlette, but the HTTP server layer rejects control characters in a header value and 500s one layer later. - Backslash: the RFC 6266 quoted-pair escape. A trailing backslash escaped the closing quote and corrupted the download filename (not a 500, but wrong output; Windows-origin uploads carry it legitimately). Extract one `latin1_safe_filename()` helper in web_helpers that drops every non-printable character plus the double-quote and backslash quoted-string metacharacters, folds any surviving non-latin-1 codepoint to '?', and falls back to a non-empty name so the header never emits an empty filename. Route get_content, preview_response_headers, and the export handler through it, replacing three near-duplicate inline strips. Adds unit tests for the helper (non-latin-1 fold, control-char and backslash stripping, per-site fallback) and an endpoint regression test.
This commit is contained in:
@@ -296,6 +296,24 @@ class TestGetContent:
|
||||
assert "default-src 'none'" in resp.headers.get("content-security-policy", "")
|
||||
assert resp.headers.get("content-disposition", "").startswith("inline;")
|
||||
|
||||
def test_get_content_non_latin1_filename_does_not_500(self, app_client):
|
||||
# Starlette encodes header values as latin-1 and raises on anything
|
||||
# else; an uploaded filename with CJK / em dashes must fold to an
|
||||
# ASCII-safe Content-Disposition rather than 500 the serving route.
|
||||
# Mirrors preview_response_headers' latin-1 fold.
|
||||
client, _ = app_client
|
||||
aid = _upload(client, "ws-A", "userA", "文書 — v1.md", b"x", "text/markdown")
|
||||
resp = client.get(
|
||||
f"/v1/api/workstreams/ws-A/attachments/{aid}/content",
|
||||
headers=_auth("userA"),
|
||||
)
|
||||
assert resp.status_code == 200
|
||||
assert resp.content == b"x"
|
||||
# Non-ASCII folded to '?', ASCII kept — pinning the value proves the
|
||||
# fold actually ran and the header is latin-1 clean (all codepoints
|
||||
# < 0x80), not merely that the route didn't crash.
|
||||
assert resp.headers["content-disposition"] == 'inline; filename="?? ? v1.md"'
|
||||
|
||||
def test_get_content_forces_text_plain_for_text_kinds(self, app_client):
|
||||
# Uploading an HTML-ish file as text/html must NOT be served back
|
||||
# with Content-Type: text/html from our origin (XSS vector).
|
||||
|
||||
@@ -114,3 +114,73 @@ class TestVersionHtml:
|
||||
html = '<script src="/static/app.js?foo=bar"></script>'
|
||||
result = version_html(html)
|
||||
assert result == html # unchanged — already has query string
|
||||
|
||||
|
||||
class TestLatin1SafeFilename:
|
||||
"""Content-Disposition filename sanitizer — must yield a value that is
|
||||
both latin-1 encodable (Starlette) and control-char free (h11)."""
|
||||
|
||||
def _assert_wire_safe(self, out: str) -> None:
|
||||
# Independent oracle — deliberately does NOT reuse the impl's
|
||||
# isprintable() gate (that would pass by construction). Every char
|
||||
# must be printable ASCII (0x20..0x7e) and neither quoted-string
|
||||
# metacharacter, so the value is latin-1 clean, control-free, and
|
||||
# safely quotable.
|
||||
assert all(0x20 <= ord(c) <= 0x7E and c not in '"\\' for c in out)
|
||||
|
||||
def test_plain_ascii_unchanged(self):
|
||||
from turnstone.core.web_helpers import latin1_safe_filename
|
||||
|
||||
assert latin1_safe_filename("report_2026.md") == "report_2026.md"
|
||||
|
||||
def test_non_latin1_folds_to_question_marks(self):
|
||||
from turnstone.core.web_helpers import latin1_safe_filename
|
||||
|
||||
# CJK + em dash (U+2014) are printable but non-latin-1 → fold to '?'.
|
||||
out = latin1_safe_filename("文書 — v1.md")
|
||||
assert out == "?? ? v1.md"
|
||||
self._assert_wire_safe(out)
|
||||
|
||||
def test_latin1_but_control_chars_are_stripped(self):
|
||||
from turnstone.core.web_helpers import latin1_safe_filename
|
||||
|
||||
# All latin-1 encodable, so the old strip/fold left them in the header
|
||||
# and the HTTP server layer then 500'd (h11 rejects NUL/CR/LF/FF/VT;
|
||||
# httptools is stricter). NUL / form-feed / DEL / TAB / VT / C1-NEL
|
||||
# (0x85) must all be dropped, not merely folded.
|
||||
out = latin1_safe_filename("a\x00b\x0cc\x7fd\te\x0bf\x85g.md")
|
||||
assert out == "abcdefg.md"
|
||||
self._assert_wire_safe(out)
|
||||
|
||||
def test_crlf_and_quote_stripped(self):
|
||||
from turnstone.core.web_helpers import latin1_safe_filename
|
||||
|
||||
out = latin1_safe_filename('a"\r\nX-Evil: 1.md')
|
||||
assert "\r" not in out and "\n" not in out and '"' not in out
|
||||
self._assert_wire_safe(out)
|
||||
|
||||
def test_backslash_stripped(self):
|
||||
from turnstone.core.web_helpers import latin1_safe_filename
|
||||
|
||||
# Backslash is the RFC 6266 quoted-pair escape inside filename="..." —
|
||||
# a trailing '\' would escape the closing quote, and '\x' mid-name
|
||||
# becomes a spurious escape. Both must be dropped (Windows-origin
|
||||
# uploads legitimately carry '\').
|
||||
assert latin1_safe_filename("dir\\file.md") == "dirfile.md"
|
||||
assert latin1_safe_filename("trailing\\") == "trailing"
|
||||
self._assert_wire_safe(latin1_safe_filename("a\\b\\c"))
|
||||
|
||||
def test_empty_after_sanitizing_uses_fallback(self):
|
||||
from turnstone.core.web_helpers import latin1_safe_filename
|
||||
|
||||
# A name of only quotes / controls sanitizes to empty → fallback,
|
||||
# never ``filename=""``.
|
||||
assert latin1_safe_filename('"""') == "attachment"
|
||||
assert latin1_safe_filename("\x00\x0c\x7f") == "attachment"
|
||||
assert latin1_safe_filename("", fallback="preview") == "preview"
|
||||
|
||||
def test_all_non_latin1_stays_non_empty_no_fallback(self):
|
||||
from turnstone.core.web_helpers import latin1_safe_filename
|
||||
|
||||
# An all-CJK name folds to '???' (truthy) — must NOT hit the fallback.
|
||||
assert latin1_safe_filename("日本語", fallback="preview") == "???"
|
||||
|
||||
@@ -31,6 +31,7 @@ from turnstone.core.attachments import (
|
||||
sniff_image_mime,
|
||||
sniff_pdf_mime,
|
||||
)
|
||||
from turnstone.core.web_helpers import latin1_safe_filename
|
||||
|
||||
# Rendered-content kinds the pane knows how to display. ``web`` is a fetched
|
||||
# HTML document (sandboxed iframe); ``table`` is CSV/TSV/JSON parsed and
|
||||
@@ -339,11 +340,10 @@ def preview_response_headers(
|
||||
by browser chrome, not an active document. Everything else keeps the
|
||||
attachment endpoints' full ``default-src 'none'; sandbox`` posture.
|
||||
"""
|
||||
# Header values must be latin-1 encodable (Starlette raises on anything
|
||||
# else), and page-title-derived filenames routinely carry em dashes / CJK
|
||||
# — fold to ASCII rather than 500 the route.
|
||||
safe_name = filename.replace('"', "").replace("\r", "").replace("\n", "")
|
||||
safe_name = safe_name.encode("ascii", errors="replace").decode("ascii") or "preview"
|
||||
# Page-title-derived filenames routinely carry em dashes / CJK (non-latin-1)
|
||||
# and can carry control bytes — either would 500 the serving route, so run
|
||||
# the shared header sanitizer rather than emit them verbatim.
|
||||
safe_name = latin1_safe_filename(filename, fallback="preview")
|
||||
headers = {
|
||||
"X-Content-Type-Options": "nosniff",
|
||||
"Content-Disposition": f'inline; filename="{safe_name}"',
|
||||
|
||||
@@ -3670,6 +3670,7 @@ def make_export_handler(cfg: SessionEndpointConfig) -> Handler:
|
||||
from starlette.responses import Response as _Response
|
||||
|
||||
from turnstone.core.export import WorkstreamNotFoundError, export_workstream
|
||||
from turnstone.core.web_helpers import latin1_safe_filename
|
||||
|
||||
# Conversation-only: never bundle children, always JSON. A live
|
||||
# session whose storage row was deleted skips the fallback
|
||||
@@ -3679,10 +3680,10 @@ def make_export_handler(cfg: SessionEndpointConfig) -> Handler:
|
||||
result = await asyncio.to_thread(export_workstream, storage, ws_id)
|
||||
except WorkstreamNotFoundError:
|
||||
return JSONResponse({"error": cfg.not_found_label}, status_code=404)
|
||||
# ws_ids are hex so the filename is already safe, but mirror the
|
||||
# attachment download handler's defensive strip of quotes/CR/LF
|
||||
# so a future non-hex id can't break the Content-Disposition.
|
||||
safe_name = result.filename.replace('"', "").replace("\r", "").replace("\n", "")
|
||||
# ws_ids are hex so the filename is already safe, but run the shared
|
||||
# sanitizer anyway so a future non-hex id can't break the
|
||||
# Content-Disposition (latin-1 fold + control-char strip).
|
||||
safe_name = latin1_safe_filename(result.filename)
|
||||
return _Response(
|
||||
result.data,
|
||||
media_type=result.content_type,
|
||||
@@ -4388,6 +4389,8 @@ def make_attachment_handlers(cfg: SessionEndpointConfig) -> AttachmentHandlers:
|
||||
async def get_content(request: Request) -> Response:
|
||||
from starlette.responses import Response as _Response
|
||||
|
||||
from turnstone.core.web_helpers import latin1_safe_filename
|
||||
|
||||
resolved = await _resolve_served_blob(request)
|
||||
if not isinstance(resolved, tuple):
|
||||
return resolved
|
||||
@@ -4396,7 +4399,10 @@ def make_attachment_handlers(cfg: SessionEndpointConfig) -> AttachmentHandlers:
|
||||
# rendering if a user uploaded an HTML-ish text file. Images keep their
|
||||
# sniffed MIME (allowlist is strict: png/jpeg/gif/webp).
|
||||
response_mime = "text/plain; charset=utf-8" if kind == "text" else stored_mime
|
||||
safe_name = filename.replace('"', "").replace("\r", "").replace("\n", "")
|
||||
# Uploaded filenames routinely carry CJK / em dashes (non-latin-1) and
|
||||
# can carry control bytes — either would 500 the serving route, so run
|
||||
# the shared header sanitizer rather than emit them verbatim.
|
||||
safe_name = latin1_safe_filename(filename)
|
||||
headers = {
|
||||
"X-Content-Type-Options": "nosniff",
|
||||
"Content-Security-Policy": "default-src 'none'; sandbox",
|
||||
|
||||
@@ -14,6 +14,33 @@ if TYPE_CHECKING:
|
||||
from starlette.responses import JSONResponse
|
||||
|
||||
|
||||
def latin1_safe_filename(name: str, *, fallback: str = "attachment") -> str:
|
||||
"""A ``Content-Disposition`` ``filename`` value that is safe on the wire.
|
||||
|
||||
The value is emitted as ``filename="<this>"``, and three independent
|
||||
hazards bite filenames derived from user uploads or fetched-page titles:
|
||||
|
||||
* Starlette encodes header values as latin-1 and raises on anything
|
||||
outside it, so a CJK / em-dash name would 500 the serving route.
|
||||
* The HTTP server layer rejects control characters in a header value even
|
||||
when they are latin-1 encodable (h11 rejects NUL / CR / LF / FF / VT;
|
||||
httptools is stricter still), so a stray control byte 500s there.
|
||||
* ``"`` and ``\\`` are the quoted-string metacharacters — ``\\`` is the
|
||||
RFC 6266 quoted-pair escape — so either would break out of or corrupt
|
||||
the value (a trailing ``\\`` escapes the closing quote).
|
||||
|
||||
Drop every non-printable character (covers CR / LF / TAB / NUL / DEL, the
|
||||
C0 / C1 control ranges, and zero-width / bidi format chars) and both
|
||||
quoted-string metacharacters, then fold any surviving non-ASCII codepoint
|
||||
to ``?``. The result is pure printable ASCII with no ``"`` or ``\\`` —
|
||||
latin-1 clean, control-free, and safely quotable. Falls back to
|
||||
``fallback`` when nothing survives, so the header never emits
|
||||
``filename=""``.
|
||||
"""
|
||||
kept = "".join(c for c in name if c.isprintable() and c not in '"\\')
|
||||
return kept.encode("ascii", errors="replace").decode("ascii") or fallback
|
||||
|
||||
|
||||
def skill_summary_rows(storage: Any) -> list[dict[str, Any]]:
|
||||
"""Build the public picker payload for ``/v1/api/skills``.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user