diff --git a/docs/system-specs/modules/learn-cron-dashboard.md b/docs/system-specs/modules/learn-cron-dashboard.md index e58b991f7b4..b071f1b1329 100644 --- a/docs/system-specs/modules/learn-cron-dashboard.md +++ b/docs/system-specs/modules/learn-cron-dashboard.md @@ -5138,7 +5138,9 @@ always states the limit, a failed config read is reported through an inline `ErrorNotice` saying uploads still work and the server checks each file, and the page then shows no "Max N MB per file" figure (otherwise it interpolates `knowledge_upload_max_mb`). It is separate from `files._MAX_UPLOAD_BYTES` (50 MB), which still bounds -the in-memory read paths (file-raw, file-download, office and sheet preview). +the in-memory read paths (file-raw, office and sheet preview). `GET /api/file-download` has no size +cap: a file up to that size is read whole and scanned, a larger one streams unscanned once the owner +confirms; see [security.md](security.md) § Dashboard File Download. Every stored name receives a UUID prefix, and the handler attributes accepted and rejected requests through SEL. Binary formats with reliable signatures are validated by content before publication under their destination path. diff --git a/docs/system-specs/modules/security.md b/docs/system-specs/modules/security.md index 82f69ef8e95..fff2ff1fe22 100644 --- a/docs/system-specs/modules/security.md +++ b/docs/system-specs/modules/security.md @@ -3683,6 +3683,43 @@ All outbox downloads include: host provides it, so Windows cannot translate embedded LF bytes to CRLF and corrupt archives or media between validation and the restricted write. +#### Dashboard File Download (`GET /api/file-download`, `file_api/transfer.py`) + +The file viewer's and file tree's Download serve a host file as `attachment` +with `X-Content-Type-Options: nosniff`. Every request passes the owner-only gate, +`validate_tool_args`, dashboard path validation, the sensitive-path filter and +the `O_NOFOLLOW` open, at any size: a `.env` or credential path is refused +whatever the file weighs and whatever the query says. + +- **Up to `_DOWNLOAD_SCAN_MAX_BYTES` (50 MB, the upload cap):** the file is read + whole from the open descriptor and scanned (`redact()` on UTF-8 text, + `binary_content_is_flagged` on other bytes, `wide_content_is_flagged` on both). + A hit refuses 400 `content_redacted` unless the owner's `owner_dashboard` + file-delivery grant releases it, with the refusal or consent audited. A file + that grows past the threshold between the fstat and the read is refused 409 + `file_changed`. +- **Over the threshold: delivered UNSCANNED, by the maintainer's decision.** The + scan needs the whole file in memory, so a multi-GB file would exhaust the + gateway; and the route is owner-only, so the reader is the person whose + machine holds the file. Without `confirm_unscanned=1` the route answers 409 + `large_file_unscanned` with `size` and `limit` and sends nothing; the dashboard + shows a "Too large to scan" dialog naming the file and its size, and only + **Download anyway** retries with the flag. With the flag the file streams in + `_STREAM_CHUNK_BYTES` chunks with `Content-Length` set to its fstat size. + Each chunk is read on the bounded transfer pool, never the shared default + executor, within `_DOWNLOAD_CHUNK_STALL_SECS`. A read that stalls (a hung + mount) or a pool that refuses it is audited and drops the connection at once; + the file's close then runs on a thread of its own, because the abandoned read + still holds the file's buffer lock and a close on the pool would wait on it. + SEL records the delivery with `error="unscanned_large_file"`, so the audit + shows it was not scanned. +- **Small or large is decided by the fstat of the one descriptor every byte is + read from**, so a different file cannot be swapped in between the size check + and the send. A file that grows while it streams is cut at the fstat size; one + that shrinks drops the connection, so the client sees a failed download. +- The flag changes nothing for a file at or under the threshold: it is scanned + exactly as without it. + #### Slack Delivery Audience — Strict Caller-Identity Classification (`file_send`) `file_send` may additionally upload the file to Slack. The Slack **audience** diff --git a/src/kiro_crew/dashboard/file_api/pinned_io.py b/src/kiro_crew/dashboard/file_api/pinned_io.py index b5ec3c2e1f7..c900222bd1d 100644 --- a/src/kiro_crew/dashboard/file_api/pinned_io.py +++ b/src/kiro_crew/dashboard/file_api/pinned_io.py @@ -292,6 +292,54 @@ class _OpenedFile(NamedTuple): data: bytes +def _open_refusal(denied: _OpenDenied, *, tool_name: str) -> _OpenRefusal: + """Audit one :func:`_open_checked_file` refusal and build its response. + + The one mapping of the open-and-check codes onto the whole-read envelope's + SEL vocabulary and response bodies, shared by :func:`_open_checked` and by + the streamed ``/api/file-download``, so the two cannot drift apart. + """ + + def _log(outcome: str, res: str, error: str = "") -> None: + kw = {"error": error} if error else {} + _sel().log_tool_invocation( + session_key="dashboard", + tool_name=tool_name, + outcome=outcome, + resources=res, + **kw, + ) + + code, res = denied.code, denied.path + if code == "invalid_path": + _log("denied", res) + return _OpenRefusal(web.json_response({"error": "invalid or forbidden path"}, status=400)) + if code == "sensitive_path": + _log("denied", res, "sensitive_path") + return _OpenRefusal(web.json_response({"error": "sensitive path blocked"}, status=403)) + if code == "not_found": + _log("not_found", res) + return _OpenRefusal(web.json_response({"error": "not found"}, status=404)) + if code == "symlink_refused": + _log("denied", res, "symlink_rejected") + return _OpenRefusal(web.json_response({"error": "symlinks not allowed"}, status=403)) + if code == "file_too_large": + # Reachable only through a caller that passes fstat_cap; mapped so + # a policy refusal can never masquerade as the 500 below. + _log("denied", res, "file_too_large") + return _OpenRefusal( + web.json_response({"error": "file too large", "code": "file_too_large"}, status=413) + ) + # read_failed: the residual code. (The whole-read envelope's own size + # guard is its bounded read, because an fstat pre-check races a + # concurrent writer while reading at most cap+1 bytes bounds memory + # unconditionally -- the same shape as _load_sheet_payload's guard.) + _log("failure", res) + return _OpenRefusal( + web.json_response({"error": "cannot read file", "code": "read_failed"}, status=500) + ) + + def _open_checked( raw_path: str, *, @@ -338,36 +386,7 @@ def _log(outcome: str, res: str, error: str = "") -> None: checked = _open_checked_file(raw_path, tool_name=tool_name) if isinstance(checked, _OpenDenied): - code, res = checked.code, checked.path - if code == "invalid_path": - _log("denied", res) - return _OpenRefusal( - web.json_response({"error": "invalid or forbidden path"}, status=400) - ) - if code == "sensitive_path": - _log("denied", res, "sensitive_path") - return _OpenRefusal(web.json_response({"error": "sensitive path blocked"}, status=403)) - if code == "not_found": - _log("not_found", res) - return _OpenRefusal(web.json_response({"error": "not found"}, status=404)) - if code == "symlink_refused": - _log("denied", res, "symlink_rejected") - return _OpenRefusal(web.json_response({"error": "symlinks not allowed"}, status=403)) - if code == "file_too_large": - # Reachable only through a caller that passes fstat_cap; mapped so - # a policy refusal can never masquerade as the 500 below. - _log("denied", res, "file_too_large") - return _OpenRefusal( - web.json_response({"error": "file too large", "code": "file_too_large"}, status=413) - ) - # read_failed: the residual code. (This envelope's own size guard is - # the bounded read below, because an fstat pre-check races a - # concurrent writer while reading at most cap+1 bytes bounds memory - # unconditionally -- the same shape as _load_sheet_payload's guard.) - _log("failure", res) - return _OpenRefusal( - web.json_response({"error": "cannot read file", "code": "read_failed"}, status=500) - ) + return _open_refusal(checked, tool_name=tool_name) path = checked.path try: diff --git a/src/kiro_crew/dashboard/file_api/transfer.py b/src/kiro_crew/dashboard/file_api/transfer.py index e00c1e1eb66..6c468ec9eaf 100644 --- a/src/kiro_crew/dashboard/file_api/transfer.py +++ b/src/kiro_crew/dashboard/file_api/transfer.py @@ -8,15 +8,17 @@ import json import mimetypes import os +import threading import urllib.parse -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, BinaryIO from aiohttp import web from aiohttp.client_exceptions import ClientConnectionResetError if TYPE_CHECKING: from kiro_crew.dashboard.handlers.files import ( - _MAX_UPLOAD_BYTES, + _DOWNLOAD_CHUNK_STALL_SECS, + _DOWNLOAD_SCAN_MAX_BYTES, _STREAM_CHUNK_BYTES, _STREAM_MAX_BYTES, _STREAM_TEXT_PROBE_BYTES, @@ -24,10 +26,9 @@ ValidationError, _audit_consented_download, _audit_owner_grant_refusal, - _open_checked, _open_checked_file, + _open_refusal, _OpenDenied, - _OpenRefusal, _owner_grant_releases_flagged, _PathProbeBusy, _probe_busy_response, @@ -177,7 +178,7 @@ def _read_file(p: str, cap: int) -> str: return resp -async def api_file_download(request: web.Request) -> web.Response: +async def api_file_download(request: web.Request) -> web.StreamResponse: """GET /api/file-download?path=... — download a file as raw bytes. Sibling of /api/file-read. file-read decodes content as UTF-8 with @@ -189,18 +190,26 @@ async def api_file_download(request: web.Request) -> web.Response: Security: same path-validation as file-read (validate_tool_args, _validate_dashboard_path, sensitive-path filter). Symlinks rejected - via O_NOFOLLOW. Files larger than _MAX_UPLOAD_BYTES are rejected. - Content is scanned for sensitive material (credentials and exfiltration - URLs); a positive hit aborts the download unless the owner holds the + via O_NOFOLLOW. A file up to _DOWNLOAD_SCAN_MAX_BYTES is read whole and + scanned for sensitive material (credentials and exfiltration URLs); a + positive hit aborts the download unless the owner holds the ``owner_dashboard`` file-delivery grant, the same grant and owner-identity test ``GET /api/outbox/{filename}`` applies. Binary files are served as-is without a MIME allowlist, since attachment disposition + nosniff prevents inline rendering on the dashboard origin. + + A larger file is NOT scanned, by the maintainer's decision: the scan holds + the whole file in memory, and this route is owner-only. It answers 409 + ``large_file_unscanned`` (with ``size`` and ``limit``) until the request + carries ``confirm_unscanned=1``, then streams in bounded chunks; see + :func:`_stream_unscanned_download`. Small or large is decided by the fstat + of the ONE descriptor every byte is read from, so no other file can be + swapped in between the size check and the send. """ owner_denied = await require_owner_dashboard_request(request, "file_download") if owner_denied is not None: return owner_denied - # Path validation now happens inside ``_open_checked``, which keeps the + # Path validation happens inside ``_open_checked_file``, which keeps the # late-binding ``handlers`` alias so tests can still monkey-patch # ``_validate_dashboard_path`` (legitimate circular-import workaround, # listed as an exception in the top-level-imports rule). @@ -238,23 +247,72 @@ async def api_file_download(request: web.Request) -> web.Response: ) return web.json_response({"error": "invalid input"}, status=400) - # Envelope shared with api_file_raw. No header sniff: this endpoint - # serves attachment + nosniff rather than choosing a content type. Offloaded - # to a worker thread: the envelope is synchronous file I/O (realpath, open, - # fstat, full read up to the cap) and must not block the event loop. + # The checked prefix shared with file-raw, file-stream and file-sheet: + # validate, sensitive-path filter, nofollow open, fstat. No header sniff: + # this endpoint serves attachment + nosniff rather than choosing a content + # type. Synchronous file I/O, so it runs on the bounded transfer pool. try: - opened = await _run_path_probe( - functools.partial( - _open_checked, raw_path, tool_name="file_download", max_bytes=_MAX_UPLOAD_BYTES - ), + checked = await _run_path_probe( + functools.partial(_open_checked_file, raw_path, tool_name="file_download"), transfer=True, stall_path=raw_path, ) except _PathProbeBusy: return _probe_busy_response(resource=raw_path, tool_name="file_download") - if isinstance(opened, _OpenRefusal): - return opened.response - path, data = opened.path, opened.data + if isinstance(checked, _OpenDenied): + return _open_refusal(checked, tool_name="file_download").response + f, path = checked.file, checked.path + # Set while a chunk read of the unscanned stream may still be running on a + # pool thread: after a stall (or a cancelled request) that thread holds the + # file's buffer lock, and a close would wait on it. + read_in_flight = [False] + try: + if checked.size > _DOWNLOAD_SCAN_MAX_BYTES: + return await _stream_unscanned_download(request, f, path, checked.size, read_in_flight) + # Bounded read on the same fd: one byte past the threshold says the + # file grew after the fstat, and a file over it is never scanned whole. + try: + data = await _run_path_probe( + f.read, _DOWNLOAD_SCAN_MAX_BYTES + 1, transfer=True, stall_path=path + ) + except _PathProbeBusy: + return _probe_busy_response(resource=path, tool_name="file_download") + except OSError: + logger.exception("file_download read failed for %s", path) + _sel().log_tool_invocation( + session_key="dashboard", + tool_name="file_download", + outcome="failure", + resources=path, + ) + return web.json_response( + {"error": "cannot read file", "code": "read_failed"}, status=500 + ) + finally: + # A close that may wait on a read still running -- the pool is full, or + # a chunk read stalled or was abandoned -- goes to a thread of its own: + # it needs no pool admission, never blocks the loop or this response, + # and only ever waits on this one file. + deferred = read_in_flight[0] + if not deferred: + try: + await _run_path_probe(f.close, transfer=True, stall_path=path) + except _PathProbeBusy: + deferred = True + if deferred: + threading.Thread(target=f.close, name="file-download-close", daemon=True).start() + if len(data) > _DOWNLOAD_SCAN_MAX_BYTES: + _sel().log_tool_invocation( + session_key="dashboard", + tool_name="file_download", + outcome="denied", + resources=path, + error="file_grew_during_read", + ) + return web.json_response( + {"error": "file changed while it was read; retry", "code": "file_changed"}, + status=409, + ) # Defense in depth: scan content for credentials / exfil URLs via the # context-aware redact() shim, which runs BOTH the exfil-URL and credential @@ -341,6 +399,114 @@ async def api_file_download(request: web.Request) -> web.Response: ) +def _download_read_chunk(f: BinaryIO, size: int) -> bytes: + """One chunk of an unscanned download: a plain read on the transfer pool.""" + return f.read(size) + + +async def _stream_unscanned_download( + request: web.Request, f: BinaryIO, path: str, size: int, read_in_flight: list[bool] +) -> web.StreamResponse: + """Serve a download over the scan threshold: confirm first, then stream unscanned. + + Without ``confirm_unscanned=1`` the answer is 409 ``large_file_unscanned`` + with the file's ``size`` and the ``limit``, and no byte is sent, so the + dashboard can tell the owner the file will not be checked and ask first. + With it, the file streams from *f* -- the descriptor whose fstat gave + *size* -- in chunks of ``_STREAM_CHUNK_BYTES``, and never past *size*: a + file that grows while it streams is cut at what the fstat said. A file that + shrinks ends the response short and drops the connection, so the client + sees a failed download rather than a short file it takes for whole. + """ + name = os.path.basename(path) + if request.query.get("confirm_unscanned") != "1": + _sel().log_tool_invocation( + session_key="dashboard", + tool_name="file_download", + outcome="denied", + resources=path, + error="large_file_unconfirmed", + ) + return web.json_response( + { + "error": "file is over the scan limit; confirm to download it unscanned", + "code": "large_file_unscanned", + "size": size, + "limit": _DOWNLOAD_SCAN_MAX_BYTES, + }, + status=409, + ) + content_type, _ = mimetypes.guess_type(path) + resp = web.StreamResponse( + headers={ + "Content-Disposition": f"attachment; filename*=UTF-8''{urllib.parse.quote(name, safe='')}", + "Content-Type": content_type or "application/octet-stream", + "X-Content-Type-Options": "nosniff", + }, + ) + resp.content_length = size + # SEL: the ALLOW decision is recorded before any byte moves, tagged so the + # audit shows this file was delivered without the content scan. + _sel().log_tool_invocation( + session_key="dashboard", + tool_name="file_download", + outcome="success", + resources=path, + error="unscanned_large_file", + ) + await resp.prepare(request) + remaining = size + while remaining > 0: + # Each chunk is one hop on the bounded transfer pool, never the shared + # default executor, and is given _DOWNLOAD_CHUNK_STALL_SECS: a read on + # a hung mount then costs one transfer worker and ends this response, + # instead of parking default-executor threads every file route needs. + error = "" + # Cleared only once the read has returned: a stall or a cancellation + # leaves it set, so the caller does not wait on the abandoned read. + read_in_flight[0] = True + try: + chunk = await asyncio.wait_for( + _run_path_probe( + _download_read_chunk, + f, + min(_STREAM_CHUNK_BYTES, remaining), + transfer=True, + stall_path=path, + ), + timeout=_DOWNLOAD_CHUNK_STALL_SECS, + ) + except _PathProbeBusy: + chunk, error = b"", "unscanned_large_file_pool_busy" + except asyncio.TimeoutError: + chunk, error = b"", "unscanned_large_file_read_stalled" + except OSError: + chunk = b"" + if not error: + read_in_flight[0] = False + if not chunk: + # Shrunk, unreadable, stalled or refused mid-stream: the announced + # length cannot be met, so drop the connection rather than end it + # cleanly, and the client sees a failed download. + _sel().log_tool_invocation( + session_key="dashboard", + tool_name="file_download", + outcome="failure", + resources=path, + error=error or "unscanned_large_file_short_read", + ) + resp.force_close() + return resp + remaining -= len(chunk) + try: + await resp.write(chunk) + except (ConnectionResetError, ConnectionError, ClientConnectionResetError): + return resp # client hung up + with contextlib.suppress(Exception): + await resp.write_eof() + return resp + + def _parse_range_header(value: str, size: int) -> tuple[int, int] | None: """Parse a single-range ``bytes=`` header against ``size``. diff --git a/src/kiro_crew/dashboard/handlers/files.py b/src/kiro_crew/dashboard/handlers/files.py index 152cca08a7e..6c4ab7d2dda 100644 --- a/src/kiro_crew/dashboard/handlers/files.py +++ b/src/kiro_crew/dashboard/handlers/files.py @@ -213,6 +213,7 @@ _open_checked, _open_checked_file, _open_rb_nofollow, + _open_refusal, _OpenDenied, _OpenedFile, _OpenRefusal, @@ -271,7 +272,9 @@ api_file_sheet, ) from kiro_crew.dashboard.file_api.transfer import ( # noqa: F401 + _download_read_chunk, _parse_range_header, + _stream_unscanned_download, api_file_download, api_file_stream, api_file_watch, @@ -544,6 +547,16 @@ def _body_err_code(body_err: web.Response) -> str: _MAX_UPLOAD_BYTES = 50 * 1024 * 1024 # 50 MB per file +#: Largest file ``/api/file-download`` reads whole and scans for credentials. +#: A larger one is streamed UNSCANNED once the owner confirms (409 +#: ``large_file_unscanned`` until then): the scan holds the whole file in +#: memory, and the route is owner-only. The maintainer's decision; see the +#: security spec. +_DOWNLOAD_SCAN_MAX_BYTES = _MAX_UPLOAD_BYTES +#: How long one chunk read of an unscanned download may take before the +#: response is dropped. A healthy disk answers a chunk in milliseconds; a hung +#: network mount never does, and the read must not hold the response open. +_DOWNLOAD_CHUNK_STALL_SECS = 60.0 #: Video gets its own, larger ceiling: a 30-second retina screen recording is #: routinely 60-150 MB, so the 50 MB document cap would reject the dominant #: case and make the feature read as broken. Safe to raise only because video diff --git a/test/test_dashboard_files_composition_contract.py b/test/test_dashboard_files_composition_contract.py index c4bd4936076..e1a3dcbf329 100644 --- a/test/test_dashboard_files_composition_contract.py +++ b/test/test_dashboard_files_composition_contract.py @@ -256,7 +256,8 @@ #: Recaptured with the ``path_probe``, ``outbox``, ``channel_upload``, ``desktop`` and #: ``editor`` owners, from the base file, where each of their names had the same shape. #: Recaptured when ``_run_path_probe`` gained its keyword-only ``stall_path``. -_BASE_SHAPE_DIGEST = "00b01a4e5eb247df7c73163e04a56f9bf00495f64187cd93a253fc958c3a9d8e" +#: Recaptured when ``api_file_download`` began answering a ``web.StreamResponse``. +_BASE_SHAPE_DIGEST = "b462dcc17ed8ba47197333e00c15ef7b63c8817837a43d28d4f582a720c3b788" #: Definitions that stay in the facade file: the seams the owners share -- the SEL #: accessor, the file-send audit, the sub-agent session mapping and the request-body #: error code -- the search walk's clock, the file viewer's owner-view verdict, whose diff --git a/test/test_file_download.py b/test/test_file_download.py index 103f5552597..aa26add84fb 100644 --- a/test/test_file_download.py +++ b/test/test_file_download.py @@ -203,22 +203,322 @@ async def test_symlink_rejected(tmp_path, mock_sel): assert "symlinks" in payload["error"] +# --- Over the scan threshold: confirm, then stream unscanned --- +# +# The threshold is shrunk instead of writing 50 MB to disk; the file is +# genuinely over the (patched) threshold, so the real branch runs. + +_FILES = "kiro_crew.dashboard.handlers.files" + + +@pytest.fixture +def small_threshold(): + with patch(f"{_FILES}._DOWNLOAD_SCAN_MAX_BYTES", 512), \ + patch(f"{_FILES}._STREAM_CHUNK_BYTES", 100): + yield 512 + + +async def _get(path, query=""): + with patch("kiro_crew.dashboard.handlers._validate_dashboard_path", return_value=str(path)): + async with TestClient(TestServer(_make_app())) as client: + resp = await client.get(f"/api/file-download?path={path}{query}") + return resp.status, await resp.read(), resp.headers + + +def _sel_errors(mock_sel) -> list[str]: + return [c.kwargs.get("error", "") for c in mock_sel.log_tool_invocation.call_args_list] + + @pytest.mark.asyncio -async def test_oversize_file_rejected(tmp_path, mock_sel): - """Files larger than _MAX_UPLOAD_BYTES must be rejected with 413. +async def test_a_large_file_asks_before_it_sends_a_byte(tmp_path, mock_sel, small_threshold): + f = tmp_path / "huge.bin" + f.write_bytes(b"\x00" * 1024) + status, body, _ = await _get(f) + assert status == 409 + assert json.loads(body) | {"error": ""} == { + "error": "", + "code": "large_file_unscanned", + "size": 1024, + "limit": 512, + } + assert "large_file_unconfirmed" in _sel_errors(mock_sel) + - The guard is the BOUNDED READ (read cap+1, refuse when over) rather than an - fstat pre-check, so a file growing between check and read cannot outrun the - cap. We shrink the cap instead of writing 50 MB to disk in a unit test; the - file is genuinely over the (patched) cap, exercising the real guard.""" +@pytest.mark.asyncio +async def test_a_confirmed_large_file_streams_whole_and_unscanned( + tmp_path, mock_sel, small_threshold +): + """The bytes arrive whole, no content scan runs, and SEL tags the delivery.""" f = tmp_path / "huge.docx" - f.write_bytes(b"\x00" * 1024) # 1 KiB on disk, over the patched 512-byte cap + raw = bytes(random.Random(5).randrange(256) for _ in range(2048)) + f.write_bytes(raw) + with patch(f"{_FILES}.binary_content_is_flagged") as binary, \ + patch(f"{_FILES}.wide_content_is_flagged") as wide, \ + patch(f"{_FILES}.redact") as redact: + status, body, headers = await _get(f, "&confirm_unscanned=1") + assert status == 200 + assert body == raw + assert headers["Content-Length"] == "2048" + assert headers["Content-Disposition"].startswith("attachment;") + assert headers["X-Content-Type-Options"] == "nosniff" + binary.assert_not_called() + wide.assert_not_called() + redact.assert_not_called() + assert "unscanned_large_file" in _sel_errors(mock_sel) - with patch("kiro_crew.dashboard.handlers._validate_dashboard_path", return_value=str(f)), \ - patch("kiro_crew.dashboard.handlers.files._MAX_UPLOAD_BYTES", 512): + +@pytest.mark.asyncio +async def test_a_large_file_is_never_read_whole(tmp_path, mock_sel, small_threshold): + """Every read is one bounded chunk, never the file.""" + f = tmp_path / "huge.bin" + f.write_bytes(b"x" * 2048) + sizes: list[int] = [] + real_open = os.fdopen + + class _Spy(io.BufferedReader): + def read(self, n=-1): + sizes.append(n) + return super().read(n) + + def spy_fdopen(fd, mode="r", *a, **k): + if mode == "rb": + return _Spy(io.FileIO(fd, "r")) + return real_open(fd, mode, *a, **k) + + with patch("os.fdopen", spy_fdopen): + status, body, _ = await _get(f, "&confirm_unscanned=1") + assert status == 200 + assert body == b"x" * 2048 + assert sizes and all(0 < n <= 100 for n in sizes) + + +@pytest.mark.asyncio +async def test_a_growing_large_file_stops_at_its_fstat_size( + tmp_path, mock_sel, small_threshold +): + """Bytes written after the fstat are not sent: the length is the fstat's.""" + from kiro_crew.dashboard.handlers import files + + f = tmp_path / "huge.log" + f.write_bytes(b"a" * 1024) + real = files._stream_unscanned_download + + async def grow_then_stream(request, fobj, path, size, *rest): + with open(f, "ab") as out: + out.write(b"b" * 1024) + return await real(request, fobj, path, size, *rest) + + with patch(f"{_FILES}._stream_unscanned_download", grow_then_stream): + status, body, _ = await _get(f, "&confirm_unscanned=1") + assert status == 200 + assert body == b"a" * 1024 + + +async def _get_streamed(path): + """GET a confirmed large download; returns (status, bytes received, ended short).""" + import aiohttp + + received = bytearray() + short = False + with patch("kiro_crew.dashboard.handlers._validate_dashboard_path", return_value=str(path)): async with TestClient(TestServer(_make_app())) as client: - resp = await client.get(f"/api/file-download?path={f}") - assert resp.status == 413 + resp = await client.get(f"/api/file-download?path={path}&confirm_unscanned=1") + try: + async for chunk in resp.content.iter_any(): + received.extend(chunk) + except aiohttp.ClientPayloadError: + short = True + return resp.status, bytes(received), short + + +@pytest.mark.asyncio +async def test_a_stalled_chunk_read_holds_no_default_worker_and_ends_the_stream( + tmp_path, mock_sel, small_threshold +): + """A read that never returns runs on the transfer pool and is cut off.""" + import threading + + f = tmp_path / "huge.bin" + f.write_bytes(b"x" * 2048) + release = threading.Event() + ran_on: list[str] = [] + + def hung_read(fobj, n): + ran_on.append(threading.current_thread().name) + release.wait() + return b"" + + try: + with patch(f"{_FILES}._download_read_chunk", hung_read), \ + patch(f"{_FILES}._DOWNLOAD_CHUNK_STALL_SECS", 0.2): + status, body, short = await _get_streamed(f) + finally: + release.set() + assert status == 200 + assert short and body == b"" + assert ran_on and ran_on[0].startswith("mc-pathxfer") + assert "unscanned_large_file_read_stalled" in _sel_errors(mock_sel) + + +@pytest.mark.asyncio +async def test_after_a_stall_the_handler_returns_and_the_close_is_deferred( + tmp_path, mock_sel, small_threshold +): + """The abandoned read still holds the file, so its close must not be awaited. + + The fake close blocks until released, as a close does behind a read stuck + holding the buffer lock: a handler that awaited it would never return. + """ + import threading + + from kiro_crew.dashboard.handlers import files + + f = tmp_path / "huge.bin" + f.write_bytes(b"x" * 2048) + release = threading.Event() + closing = threading.Event() + close_threads: list[str] = [] + real_fdopen = os.fdopen + + class _Locked(io.BufferedReader): + def close(self): + close_threads.append(threading.current_thread().name) + closing.set() + release.wait() + super().close() + + def locked_fdopen(fd, mode="r", *a, **k): + if mode == "rb": + return _Locked(io.FileIO(fd, "r")) + return real_fdopen(fd, mode, *a, **k) + + def hung_read(fobj, n): + release.wait() + return b"" + + real_probe = files._run_path_probe + pooled_closes: list[object] = [] + + async def spy_probe(fn, *args, **kwargs): + if getattr(fn, "__name__", "") == "close": + pooled_closes.append(fn) + return await real_probe(fn, *args, **kwargs) + + try: + with patch(f"{_FILES}._download_read_chunk", hung_read), \ + patch(f"{_FILES}._DOWNLOAD_CHUNK_STALL_SECS", 0.2), \ + patch(f"{_FILES}._run_path_probe", spy_probe), \ + patch("os.fdopen", locked_fdopen): + # A handler that awaited the blocked close would never return here; + # the per-test timeout names that hang. + status, body, short = await _get_streamed(f) + assert status == 200 and short and body == b"" + assert pooled_closes == [] + assert closing.wait(5) + assert close_threads == ["file-download-close"] + finally: + release.set() + + +@pytest.mark.asyncio +async def test_a_busy_transfer_pool_ends_the_stream(tmp_path, mock_sel, small_threshold): + from kiro_crew.dashboard.handlers import files + + f = tmp_path / "huge.bin" + f.write_bytes(b"x" * 2048) + real_probe = files._run_path_probe + + async def busy_on_chunk(fn, *args, **kwargs): + if fn is files._download_read_chunk: + raise files._PathProbeBusy() + return await real_probe(fn, *args, **kwargs) + + with patch(f"{_FILES}._run_path_probe", busy_on_chunk): + status, body, short = await _get_streamed(f) + assert status == 200 + assert short and body == b"" + assert "unscanned_large_file_pool_busy" in _sel_errors(mock_sel) + + +@pytest.mark.asyncio +async def test_a_busy_pool_closes_the_file_off_the_event_loop(tmp_path, mock_sel): + """When the transfer pool refuses the close, it still never runs on the loop.""" + import threading + + from kiro_crew.dashboard.handlers import files + + f = tmp_path / "notes.txt" + f.write_text("hello world") + real_probe = files._run_path_probe + real_fdopen = os.fdopen + closed_on: list[threading.Thread] = [] + closed = threading.Event() + + class _Spy(io.BufferedReader): + def close(self): + closed_on.append(threading.current_thread()) + super().close() + closed.set() + + def spy_fdopen(fd, mode="r", *a, **k): + if mode == "rb": + return _Spy(io.FileIO(fd, "r")) + return real_fdopen(fd, mode, *a, **k) + + async def busy_on_close(fn, *args, **kwargs): + if getattr(fn, "__name__", "") == "close": + raise files._PathProbeBusy() + return await real_probe(fn, *args, **kwargs) + + loop_thread = threading.current_thread() + with patch(f"{_FILES}._run_path_probe", busy_on_close), patch("os.fdopen", spy_fdopen): + status, body, _ = await _get(f) + assert status == 200 + assert body == b"hello world" + assert closed.wait(5) + assert closed_on and closed_on[0] is not loop_thread + + +@pytest.mark.asyncio +async def test_a_small_file_ignores_the_flag_and_is_still_scanned( + tmp_path, mock_sel, small_threshold +): + f = tmp_path / "env.txt" + f.write_text(f"aws_secret_access_key = {_key_shaped(9)}\n") + status, body, _ = await _get(f, "&confirm_unscanned=1") + assert status == 400 + assert json.loads(body)["code"] == "content_redacted" + assert "unscanned_large_file" not in _sel_errors(mock_sel) + + +@pytest.mark.asyncio +async def test_a_sensitive_path_over_the_threshold_is_refused_even_confirmed( + tmp_path, small_threshold +): + f = tmp_path / "huge.env" + f.write_bytes(b"x" * 2048) + with patch(f"{_FILES}.is_sensitive_path", return_value=True), \ + patch("kiro_crew.sel.sel") as m: + m.return_value = MagicMock() + status, _, _ = await _get(f, "&confirm_unscanned=1") + assert status == 403 + + +@pytest.mark.asyncio +async def test_a_non_owner_is_refused_even_confirmed(tmp_path, mock_sel, small_threshold): + f = tmp_path / "huge.bin" + f.write_bytes(b"x" * 2048) + req = _owner_request(f, user="slack-user-7") + req = make_mocked_request( + "GET", + f"/api/file-download?path={urllib.parse.quote(str(f))}&confirm_unscanned=1", + app=req.app, + ) + req["user"] = "slack-user-7" + req["app"] = "" + with patch("kiro_crew.dashboard.handlers._validate_dashboard_path", return_value=str(f)): + resp = await api_file_download(req) + assert resp.status == 403 # --- resolve=1 mirrors api_file_read semantics --- diff --git a/website/src/App.tsx b/website/src/App.tsx index 757c02d3a27..02a8bce18db 100644 --- a/website/src/App.tsx +++ b/website/src/App.tsx @@ -72,6 +72,7 @@ import ShortcutsModal from './components/ShortcutsModal' import QuickSearchSurface from './components/QuickSearchSurface' import ReportProblemModal from './components/ReportProblemModal' import KiroAccountModal from './components/KiroAccountModal' +import UnscannedDownloadConfirmHost from './components/UnscannedDownloadConfirmHost' import { StartupVideo } from './shell/boot/startupVideo' import { i18nT } from './i18n/t' @@ -922,6 +923,8 @@ export default function App() { )} {shortcutsOpen && setShortcutsOpen(false)} />} + {/* Answers the "Too large to scan" question a file download asks. */} + `, so copying the link or opening it in a + * new tab still works. A plain click is taken over, because a bare navigation + * cannot read the endpoint's answer: a file over the scan limit answers 409 and + * must ask the owner first, and a refused file must say why instead of saving + * the refusal's JSON as the file. + * + * A failure renders right after the link unless the host passes `onError`: + * a host whose link sits in a row of file actions takes the message and shows + * it in a row of its own, so that row never carries the notice's controls. + */ +export default function FileDownloadLink({ + filePath, + download = true, + className, + ariaLabel, + onError, + children, +}: { + filePath: string + /** The `download` attribute: a file name, or `true` for the URL's own. */ + download?: string | true + className?: string + ariaLabel?: string + /** Take the failure message (and `null` on a new attempt) instead of rendering it here. */ + onError?: (message: string | null) => void + children: ReactNode +}) { + const [error, setError] = useState(null) + const onClick = (e: MouseEvent) => { + // A modified click (new tab, save link) keeps the browser's own behaviour. + if (e.button !== 0 || e.metaKey || e.ctrlKey || e.shiftKey || e.altKey) return + e.preventDefault() + const report = onError ?? setError + report(null) + void downloadFileToDisk(filePath, report) + } + return ( + <> + + {children} + + setError(null)} + askAgent + testId="file-download-link-error" + /> + + ) +} diff --git a/website/src/components/FileRenderers.tsx b/website/src/components/FileRenderers.tsx index 47682f25b11..188b03a8776 100644 --- a/website/src/components/FileRenderers.tsx +++ b/website/src/components/FileRenderers.tsx @@ -6,8 +6,9 @@ import DOMPurify from 'dompurify' import { i18nT } from '../i18n/t' import { ExcalidrawBlock } from './ExcalidrawBlock' import ErrorNotice from './ErrorNotice' +import FileDownloadLink from './FileDownloadLink' import { useCanOpenFile, useCopyAck } from './FilePathMenu' -import { fileDownloadUrl, fileStreamUrl, fileOfficePreviewUrl, fileOfficeSlidesUrl, fileOfficeSlideUrl } from '../utils/fileReadUrl' +import { fileStreamUrl, fileOfficePreviewUrl, fileOfficeSlidesUrl, fileOfficeSlideUrl } from '../utils/fileReadUrl' import { sendErrorToChat } from '../utils/errorReport' import { useLanguageGeneration } from '../i18n/useLanguageGeneration' import { pathBasename } from '../utils/pathBasename' @@ -405,7 +406,6 @@ function OfficeCard({ filePath, showBigDownload, hideHint, compact }: { // `BIN` fallback: the binary card is reached by a byte sniff, not by an // extension list, so `coredump` and `a.out` land here with nothing to show. const ext = extOf(filePath).replace('.', '').toUpperCase() || 'BIN' - const url = fileDownloadUrl(filePath) const sizeCls = showBigDownload ? 'px-3 py-1.5 text-sm' : 'px-2 py-1 text-xs' const iconSize = showBigDownload ? 16 : 14 // The ONE gate for an Open-with-default-app surface, shared with the file @@ -419,6 +419,19 @@ function OfficeCard({ filePath, showBigDownload, hideHint, compact }: { // so the primary button acknowledges that degrade with the same inline swap // instead of reading as a dead click. const { copyStatus, revealOrOpenWithAck, revealError, clearRevealError } = useCopyAck(filePath) + // A failed Download is reported here and shown beside the Open error, in a + // row of its own, so the Open / Download row keeps exactly two controls. + const [downloadError, setDownloadError] = useState(null) + const downloadNotice = ( + setDownloadError(null)} + testId="office-card-download-error" + /> + ) const openLabel = copyStatus === 'copied' ? i18nT('components.filePathMenu.path_copied') : copyStatus === 'failed' @@ -436,19 +449,20 @@ function OfficeCard({ filePath, showBigDownload, hideHint, compact }: { {openLabel} )} - + ) if (compact) { @@ -465,6 +479,7 @@ function OfficeCard({ filePath, showBigDownload, hideHint, compact }: { onDismiss={clearRevealError} testId="office-card-open-error" /> + {downloadNotice}
{actions}
) @@ -489,6 +504,7 @@ function OfficeCard({ filePath, showBigDownload, hideHint, compact }: { onDismiss={clearRevealError} testId="office-card-open-error" /> + {downloadNotice} {showBigDownload && !hideHint && (
{/* The hint is the card's only instruction, so it must name the action @@ -1136,15 +1152,15 @@ export const MediaPlayer = memo(function MediaPlayer({ filePath, kind }: { fileP
{i18nT('components.fileRenderers.media_preview_failed')}
- +
) @@ -1273,9 +1289,9 @@ export const SheetViewer = memo(function SheetViewer({ filePath }: { filePath: s
{i18nT('components.fileRenderers.sheet_formulas_note')} {' · '} - + {i18nT('components.fileRenderers.download')} - +
)}
@@ -1324,9 +1340,9 @@ export const SheetViewer = memo(function SheetViewer({ filePath }: { filePath: s {(sheet.truncated_rows || sheet.truncated_cols || payload.truncated_sheets) && ( <> {' · '} - + {i18nT('components.fileRenderers.download')} - + )}
diff --git a/website/src/components/UnscannedDownloadConfirmHost.tsx b/website/src/components/UnscannedDownloadConfirmHost.tsx new file mode 100644 index 00000000000..c89516f9830 --- /dev/null +++ b/website/src/components/UnscannedDownloadConfirmHost.tsx @@ -0,0 +1,34 @@ +import { useEffect } from 'react' + +import { useConfirm, type ConfirmOptions } from './ConfirmDialog' +import { i18nT } from '../i18n/t' +import { fmtBytes } from '../i18n/format' +import { setUnscannedDownloadAsker, type UnscannedDownloadInfo } from '../utils/unscannedDownload' + +/** The dialog for a download over the server's scan limit. */ +export function unscannedDownloadOptions(info: UnscannedDownloadInfo): ConfirmOptions { + return { + title: i18nT('components.unscannedDownload.title'), + body: i18nT('components.unscannedDownload.body', { + name: info.name, + size: fmtBytes(info.size), + limit: fmtBytes(info.limit), + }), + confirmLabel: i18nT('components.unscannedDownload.download_anyway'), + // A download can start from the full-screen file preview (z-[9999]); the + // question must paint above it, not under it. + layer: 'top', + } +} + +/** Mount once: answers every `confirmUnscannedDownload` with the shared confirm + * dialog (focus trap, Escape and the backdrop cancel). It lives at the app root + * because the menus that start a download close before the answer comes. */ +export default function UnscannedDownloadConfirmHost() { + const { confirm, confirmDialog } = useConfirm() + useEffect( + () => setUnscannedDownloadAsker(info => confirm(unscannedDownloadOptions(info))), + [confirm], + ) + return <>{confirmDialog} +} diff --git a/website/src/i18n/locales/bn.json b/website/src/i18n/locales/bn.json index 8bd85ac82db..3c234e379cf 100644 --- a/website/src/i18n/locales/bn.json +++ b/website/src/i18n/locales/bn.json @@ -10160,6 +10160,11 @@ "tool_search_label": "সরঞ্জাম অনুসন্ধান", "tool_search_description": "চাহিদা অনুযায়ী বিলম্বিত MCP সরঞ্জাম স্পেসিফিকেশন লোড করো।" } + }, + "unscannedDownload": { + "title": "স্ক্যান করার পক্ষে অনেক বড়", + "body": "{{name}} এর আকার {{size}}। {{limit}}-এর বেশি ফাইল ডাউনলোডের আগে গোপন তথ্যের জন্য পরীক্ষা করা হয় না।", + "download_anyway": "তবুও ডাউনলোড করো" } }, "hooks": { diff --git a/website/src/i18n/locales/de.json b/website/src/i18n/locales/de.json index 0eb944c5e1e..915c2d1fe4a 100644 --- a/website/src/i18n/locales/de.json +++ b/website/src/i18n/locales/de.json @@ -10160,6 +10160,11 @@ "tool_search_label": "Werkzeugsuche", "tool_search_description": "Zurückgestellte MCP-Tool-Spezifikationen bei Bedarf laden." } + }, + "unscannedDownload": { + "title": "Zu groß zum Prüfen", + "body": "{{name}} ist {{size}} groß. Dateien über {{limit}} werden vor dem Download nicht auf Geheimnisse geprüft.", + "download_anyway": "Trotzdem herunterladen" } }, "hooks": { diff --git a/website/src/i18n/locales/en-XA.json b/website/src/i18n/locales/en-XA.json index cbfe63ab707..fc59aaf7dd0 100644 --- a/website/src/i18n/locales/en-XA.json +++ b/website/src/i18n/locales/en-XA.json @@ -9919,6 +9919,11 @@ "tool_search_label": "[Ţøøĺ şèàŕçĥ ··········]", "tool_search_description": "[Ĺøàð ðèƒèŕŕèð ṀÇÞ ţøøĺ şþèçş øñ ðèɱàñð. ····················]" } + }, + "unscannedDownload": { + "title": "[Ţøø ĺàŕğè ţø şçàñ ···············]", + "body": "[{{name}} ìş {{size}}. Ƒìĺèş øṽèŕ {{limit}} àŕè ñøţ çĥèçķèð ƒøŕ şèçŕèţş ƀèƒøŕè ðøẁñĺøàð. ······················]", + "download_anyway": "[Ðøẁñĺøàð àñýẁàý ··············]" } }, "hooks": { diff --git a/website/src/i18n/locales/en.manual.json b/website/src/i18n/locales/en.manual.json index 34cb291b9be..320f11d1493 100644 --- a/website/src/i18n/locales/en.manual.json +++ b/website/src/i18n/locales/en.manual.json @@ -4378,6 +4378,11 @@ "tool_search_label": "Tool search", "tool_search_description": "Load deferred MCP tool specs on demand." } + }, + "unscannedDownload": { + "title": "Too large to scan", + "body": "{{name}} is {{size}}. Files over {{limit}} are not checked for secrets before download.", + "download_anyway": "Download anyway" } }, "hooks": { diff --git a/website/src/i18n/locales/es.json b/website/src/i18n/locales/es.json index 11ad90a9452..57d111fb47f 100644 --- a/website/src/i18n/locales/es.json +++ b/website/src/i18n/locales/es.json @@ -10304,6 +10304,11 @@ "tool_search_label": "Búsqueda de herramientas", "tool_search_description": "Cargar especificaciones de herramientas MCP diferidas bajo demanda." } + }, + "unscannedDownload": { + "title": "Demasiado grande para analizar", + "body": "{{name}} ocupa {{size}}. Los archivos de más de {{limit}} no se revisan en busca de secretos antes de descargarlos.", + "download_anyway": "Descargar de todos modos" } }, "hooks": { diff --git a/website/src/i18n/locales/fr.json b/website/src/i18n/locales/fr.json index 8116ca2d3cb..36bd1fca623 100644 --- a/website/src/i18n/locales/fr.json +++ b/website/src/i18n/locales/fr.json @@ -10304,6 +10304,11 @@ "tool_search_label": "Recherche d’outils", "tool_search_description": "Charger à la demande des spécifications d’outils MCP différées." } + }, + "unscannedDownload": { + "title": "Trop volumineux pour être analysé", + "body": "{{name}} fait {{size}}. Les fichiers de plus de {{limit}} ne sont pas vérifiés à la recherche de secrets avant le téléchargement.", + "download_anyway": "Télécharger quand même" } }, "hooks": { diff --git a/website/src/i18n/locales/hi.json b/website/src/i18n/locales/hi.json index f39c1dab01b..fcfa160d7d2 100644 --- a/website/src/i18n/locales/hi.json +++ b/website/src/i18n/locales/hi.json @@ -10160,6 +10160,11 @@ "tool_search_label": "उपकरण खोज", "tool_search_description": "माँग पर स्थगित MCP उपकरण विनिर्देश लोड करें।" } + }, + "unscannedDownload": { + "title": "स्कैन करने के लिए बहुत बड़ी", + "body": "{{name}} का आकार {{size}} है। {{limit}} से बड़ी फ़ाइलों को डाउनलोड से पहले गोपनीय जानकारी के लिए जाँचा नहीं जाता।", + "download_anyway": "फिर भी डाउनलोड करो" } }, "hooks": { diff --git a/website/src/i18n/locales/it.json b/website/src/i18n/locales/it.json index eff8d16eb5f..7755f68f62a 100644 --- a/website/src/i18n/locales/it.json +++ b/website/src/i18n/locales/it.json @@ -10304,6 +10304,11 @@ "tool_search_label": "Ricerca strumenti", "tool_search_description": "Carica su richiesta le specifiche degli strumenti MCP differite." } + }, + "unscannedDownload": { + "title": "Troppo grande per essere analizzato", + "body": "{{name}} pesa {{size}}. I file oltre {{limit}} non vengono controllati alla ricerca di segreti prima del download.", + "download_anyway": "Scarica comunque" } }, "hooks": { diff --git a/website/src/i18n/locales/ja.json b/website/src/i18n/locales/ja.json index 8a6b3f53557..d7cbe95ba38 100644 --- a/website/src/i18n/locales/ja.json +++ b/website/src/i18n/locales/ja.json @@ -10016,6 +10016,11 @@ "tool_search_label": "ツール検索", "tool_search_description": "保留中の MCP ツール仕様を必要に応じて読み込みます。" } + }, + "unscannedDownload": { + "title": "大きすぎてスキャンできません", + "body": "{{name}} のサイズは {{size}} です。{{limit}} を超えるファイルは、ダウンロード前に機密情報のチェックが行われません。", + "download_anyway": "それでもダウンロード" } }, "hooks": { diff --git a/website/src/i18n/locales/ko.json b/website/src/i18n/locales/ko.json index 9f1ced1d497..2f9b41e5cb2 100644 --- a/website/src/i18n/locales/ko.json +++ b/website/src/i18n/locales/ko.json @@ -10016,6 +10016,11 @@ "tool_search_label": "도구 검색", "tool_search_description": "지연된 MCP 도구 사양을 필요할 때 로드합니다." } + }, + "unscannedDownload": { + "title": "너무 커서 검사할 수 없음", + "body": "{{name}}의 크기는 {{size}}입니다. {{limit}}을(를) 넘는 파일은 다운로드 전에 비밀 정보 검사를 하지 않습니다.", + "download_anyway": "그래도 다운로드" } }, "hooks": { diff --git a/website/src/i18n/locales/pt.json b/website/src/i18n/locales/pt.json index 90caea2a5ad..44e7f0fa556 100644 --- a/website/src/i18n/locales/pt.json +++ b/website/src/i18n/locales/pt.json @@ -10304,6 +10304,11 @@ "tool_search_label": "Busca de ferramentas", "tool_search_description": "Carregar sob demanda especificações de ferramentas MCP adiadas." } + }, + "unscannedDownload": { + "title": "Grande demais para verificar", + "body": "{{name}} tem {{size}}. Arquivos acima de {{limit}} não são verificados em busca de segredos antes do download.", + "download_anyway": "Baixar mesmo assim" } }, "hooks": { diff --git a/website/src/i18n/locales/ru.json b/website/src/i18n/locales/ru.json index 70a89d45f6d..d7e9b126d1f 100644 --- a/website/src/i18n/locales/ru.json +++ b/website/src/i18n/locales/ru.json @@ -10448,6 +10448,11 @@ "tool_search_label": "Поиск инструментов", "tool_search_description": "Загружать отложенные спецификации инструментов MCP по требованию." } + }, + "unscannedDownload": { + "title": "Слишком большой для проверки", + "body": "Размер {{name}} — {{size}}. Файлы больше {{limit}} не проверяются на секреты перед загрузкой.", + "download_anyway": "Всё равно скачать" } }, "hooks": { diff --git a/website/src/i18n/locales/zh-CN.json b/website/src/i18n/locales/zh-CN.json index 69bfa7b1ff1..ea53e32251f 100644 --- a/website/src/i18n/locales/zh-CN.json +++ b/website/src/i18n/locales/zh-CN.json @@ -10016,6 +10016,11 @@ "tool_search_label": "工具搜索", "tool_search_description": "按需加载延迟的 MCP 工具规范。" } + }, + "unscannedDownload": { + "title": "文件太大，无法扫描", + "body": "{{name}} 大小为 {{size}}。超过 {{limit}} 的文件在下载前不会检查其中是否含有密钥等机密信息。", + "download_anyway": "仍要下载" } }, "hooks": { diff --git a/website/src/test/BinaryFileCard.test.tsx b/website/src/test/BinaryFileCard.test.tsx index c8813e00343..67e82389271 100644 --- a/website/src/test/BinaryFileCard.test.tsx +++ b/website/src/test/BinaryFileCard.test.tsx @@ -15,7 +15,7 @@ * screenful of U+FFFD that also looked saveable back over the real bytes. */ import { describe, it, expect, vi, afterEach } from 'vitest' -import { render, renderHook, screen, waitFor, act } from '@testing-library/react' +import { render, renderHook, screen, waitFor, act, fireEvent, within } from '@testing-library/react' import { QueryClient, QueryClientProvider } from '@tanstack/react-query' import type { ReactNode } from 'react' @@ -52,6 +52,25 @@ describe('BinaryFileCard', () => { expect(screen.getByText(/download/i, { selector: 'div' })).toBeInTheDocument() }) + it('a failed Download shows its notice in its own row, outside the file actions', async () => { + vi.stubGlobal('fetch', vi.fn(async () => new Response(JSON.stringify({ error: 'not found' }), { status: 404 }))) + try { + renderWithQuery() + const link = screen.getByRole('link', { name: /index\.sqlite/i }) + fireEvent.click(link) + const notice = await screen.findByTestId('office-card-download-error') + expect(notice.textContent).toContain('Download failed') + // The link's own row holds the file actions and nothing else. + const actionRow = link.parentElement as HTMLElement + expect(actionRow.contains(notice)).toBe(false) + expect(within(actionRow).queryByRole('alert')).toBeNull() + expect(within(actionRow).getAllByRole('link').length + within(actionRow).queryAllByRole('button').length).toBeLessThanOrEqual(2) + expect(screen.queryByTestId('file-download-link-error')).toBeNull() + } finally { + vi.unstubAllGlobals() + } + }) + it('badges an extension-less binary BIN instead of an empty pill', () => { // The verdict is a byte sniff, not an extension list, so `coredump` reaches // this card with no extension to derive a badge from. diff --git a/website/src/test/unscannedDownload.test.tsx b/website/src/test/unscannedDownload.test.tsx new file mode 100644 index 00000000000..f829f7d9992 --- /dev/null +++ b/website/src/test/unscannedDownload.test.tsx @@ -0,0 +1,127 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { render, screen, fireEvent, act, waitFor, within } from '@testing-library/react' + +vi.mock('../utils/download', () => ({ downloadBlob: vi.fn(), downloadUrl: vi.fn() })) + +import { downloadBlob, downloadUrl } from '../utils/download' +import { downloadFileToDisk } from '../utils/fileReadUrl' +import UnscannedDownloadConfirmHost, { unscannedDownloadOptions } from '../components/UnscannedDownloadConfirmHost' +import FileDownloadLink from '../components/FileDownloadLink' + +const PATH = '/home/me/project/recording.mov' + +function answer409() { + return new Response( + JSON.stringify({ error: 'over', code: 'large_file_unscanned', size: 3_000_000_000, limit: 52_428_800 }), + { status: 409, headers: { 'Content-Type': 'application/json' } }, + ) +} + +describe('a download over the scan limit asks first', () => { + beforeEach(() => { + vi.mocked(downloadBlob).mockClear() + vi.mocked(downloadUrl).mockClear() + vi.stubGlobal('fetch', vi.fn(async () => answer409())) + }) + afterEach(() => { vi.unstubAllGlobals() }) + + it('shows the dialog with the name and both sizes, and "Download anyway" retries with the flag', async () => { + render() + const onError = vi.fn() + let done: Promise = Promise.resolve() + act(() => { done = downloadFileToDisk(PATH, onError) }) + + const dialog = await screen.findByRole('dialog') + expect(dialog.textContent).toContain('Too large to scan') + expect(dialog.textContent).toMatch(/recording\.mov is 3\s*GB\./) + expect(dialog.textContent).toMatch(/Files over 52\.4\s*MB/) + expect(dialog.textContent).toContain('not checked for secrets') + expect(downloadUrl).not.toHaveBeenCalled() + + fireEvent.click(screen.getByRole('button', { name: 'Download anyway' })) + await act(async () => { await done }) + + expect(downloadUrl).toHaveBeenCalledTimes(1) + const [url, name] = vi.mocked(downloadUrl).mock.calls[0] + expect(url).toBe('/api/file-download?path=' + encodeURIComponent(PATH) + '&confirm_unscanned=1') + expect(name).toBe('recording.mov') + expect(downloadBlob).not.toHaveBeenCalled() + expect(onError).not.toHaveBeenCalled() + }) + + it('Cancel downloads nothing and reports no error', async () => { + render() + const onError = vi.fn() + let done: Promise = Promise.resolve() + act(() => { done = downloadFileToDisk(PATH, onError) }) + await screen.findByRole('dialog') + fireEvent.click(screen.getByRole('button', { name: 'Cancel' })) + await act(async () => { await done }) + expect(downloadUrl).not.toHaveBeenCalled() + expect(onError).not.toHaveBeenCalled() + }) + + it('Escape cancels', async () => { + render() + let done: Promise = Promise.resolve() + act(() => { done = downloadFileToDisk(PATH, vi.fn()) }) + const dialog = await screen.findByRole('dialog') + fireEvent.keyDown(dialog, { key: 'Escape' }) + await act(async () => { await done }) + expect(downloadUrl).not.toHaveBeenCalled() + }) + + it('a plain click on a Download link takes the same path', async () => { + render( + <> + + Download + , + ) + const link = screen.getByRole('link', { name: 'Download' }) + expect(link.getAttribute('href')).toBe('/api/file-download?path=' + encodeURIComponent(PATH)) + fireEvent.click(link) + await screen.findByRole('dialog') + fireEvent.click(screen.getByRole('button', { name: 'Download anyway' })) + await waitFor(() => expect(downloadUrl).toHaveBeenCalledTimes(1)) + expect(vi.mocked(downloadUrl).mock.calls[0][0]).toContain('&confirm_unscanned=1') + }) + + it('the dialog paints above the full-screen preview layer', () => { + expect(unscannedDownloadOptions({ name: 'a', size: 2, limit: 1 }).layer).toBe('top') + }) + + it('a Windows path saves under its file name, not the whole path', async () => { + const win = 'C:\\work\\report.docx' + render() + let done: Promise = Promise.resolve() + act(() => { done = downloadFileToDisk(win, vi.fn()) }) + const dialog = await screen.findByRole('dialog') + expect(dialog.textContent).toMatch(/^Too large to scanreport\.docx is/) + fireEvent.click(screen.getByRole('button', { name: 'Download anyway' })) + await act(async () => { await done }) + expect(vi.mocked(downloadUrl).mock.calls[0][1]).toBe('report.docx') + + vi.stubGlobal('fetch', vi.fn(async () => new Response('hello', { status: 200 }))) + await downloadFileToDisk(win, vi.fn()) + expect(vi.mocked(downloadBlob).mock.calls[0][1]).toBe('report.docx') + }) + + it('a failed link download offers the agent hand-off', async () => { + vi.stubGlobal('fetch', vi.fn(async () => new Response(JSON.stringify({ error: 'not found' }), { status: 404 }))) + render(Download) + fireEvent.click(screen.getByRole('link', { name: 'Download' })) + const alert = await screen.findByTestId('file-download-link-error') + expect(alert.textContent).toContain('Download failed') + expect(within(alert).getByRole('button', { name: /agent/i })).toBeTruthy() + }) + + it('a small file is fetched and saved as before, with no dialog', async () => { + vi.stubGlobal('fetch', vi.fn(async () => new Response('hello', { status: 200 }))) + render() + await downloadFileToDisk(PATH, vi.fn()) + expect(screen.queryByRole('dialog')).toBeNull() + expect(downloadBlob).toHaveBeenCalledTimes(1) + expect(downloadUrl).not.toHaveBeenCalled() + }) +}) diff --git a/website/src/utils/download.ts b/website/src/utils/download.ts index ae806a1068b..2cad7c5766c 100644 --- a/website/src/utils/download.ts +++ b/website/src/utils/download.ts @@ -1,21 +1,36 @@ -/** Object-URL download; revoke deferred a tick so the click can consume it. */ -export function downloadBlob(blob: Blob, filename: string): void { - // In the Electron shell, announce this download so the main process's - // will-download handler auto-saves ONLY downloads the app explicitly asked - // for (short-lived, single-use). Outside Electron the bridge is absent and - // this is a no-op; the browser handles the download itself. +/** In the Electron shell, announce a download so the main process's + * will-download handler auto-saves ONLY downloads the app explicitly asked + * for (short-lived, single-use). Outside Electron the bridge is absent and + * this is a no-op; the browser handles the download itself. */ +function announceDownload(filename: string): void { try { ;(window as unknown as { kirocrew?: { expectDownload?: (name: string) => void } }) .kirocrew?.expectDownload?.(filename) } catch { // A missing/throwing bridge must never block the download itself. } - const url = URL.createObjectURL(blob) +} + +function clickDownload(href: string, filename: string): void { const a = document.createElement('a') - a.href = url + a.href = href a.download = filename document.body.appendChild(a) a.click() a.remove() +} + +/** Object-URL download; revoke deferred a tick so the click can consume it. */ +export function downloadBlob(blob: Blob, filename: string): void { + announceDownload(filename) + const url = URL.createObjectURL(blob) + clickDownload(url, filename) setTimeout(() => URL.revokeObjectURL(url), 1000) } + +/** Download a same-origin URL straight to disk, so the browser streams the + * response instead of holding it in page memory the way `downloadBlob` must. */ +export function downloadUrl(url: string, filename: string): void { + announceDownload(filename) + clickDownload(url, filename) +} diff --git a/website/src/utils/fileReadUrl.ts b/website/src/utils/fileReadUrl.ts index b84e8139424..6e0cfea0ba7 100644 --- a/website/src/utils/fileReadUrl.ts +++ b/website/src/utils/fileReadUrl.ts @@ -1,5 +1,7 @@ import { i18nT } from '../i18n/t' -import { downloadBlob } from './download' +import { downloadBlob, downloadUrl } from './download' +import { confirmUnscannedDownload } from './unscannedDownload' +import { pathBasename } from './pathBasename' /** Append resolve=1 for relative paths. The backend resolves such paths * against KIROCREW_PROJECT_DIR; absolute and ~-paths pass through unchanged. */ @@ -47,9 +49,13 @@ export function fileReadUrl(filePath: string): string { * * Use this instead of fileReadUrl when saving a file to disk. fileReadUrl * decodes content as UTF-8 with errors='replace', which corrupts binary - * files (.docx, .pdf, images) by replacing non-text bytes with U+FFFD. */ -export function fileDownloadUrl(filePath: string): string { - return withResolve('/api/file-download?path=' + encodeURIComponent(filePath), filePath) + * files (.docx, .pdf, images) by replacing non-text bytes with U+FFFD. + * + * `confirmUnscanned` adds `confirm_unscanned=1`: the owner's yes to a file over + * the server's scan limit, which it then sends without the credential scan. */ +export function fileDownloadUrl(filePath: string, opts?: { confirmUnscanned?: boolean }): string { + const url = withResolve('/api/file-download?path=' + encodeURIComponent(filePath), filePath) + return opts?.confirmUnscanned ? url + '&confirm_unscanned=1' : url } /** Fetch a file's raw bytes through /api/file-download and hand them to the @@ -71,6 +77,11 @@ export function fileDownloadUrl(filePath: string): string { * is how the owner releases a flagged file of their own. Either way the * refusal is surfaced through `onError`, never defeated -- the bytes are not * reached another way. + * + * A file over the server's scan limit answers 409 `large_file_unscanned` and + * sends nothing. The owner is asked (`confirmUnscannedDownload`), and only a + * yes downloads it, as a plain browser download of the confirmed URL so a + * multi-GB file streams to disk instead of filling page memory as a blob. */ export async function downloadFileToDisk( filePath: string, @@ -79,6 +90,14 @@ export async function downloadFileToDisk( try { const res = await fetch(fileDownloadUrl(filePath)) if (!res.ok) { + let body: { code?: unknown; size?: unknown; limit?: unknown } = {} + try { body = (await res.clone().json()) ?? {} } catch { /* non-JSON body */ } + if (body.code === 'large_file_unscanned') { + const name = downloadFileName(filePath) + const yes = await confirmUnscannedDownload({ name, size: Number(body.size) || 0, limit: Number(body.limit) || 0 }) + if (yes) downloadUrl(fileDownloadUrl(filePath, { confirmUnscanned: true }), name) + return + } // eslint-disable-next-line no-console -- surface download failures for diagnostics console.error('downloadFileToDisk failed', res.status, res.statusText) // The credential gate aborts a flagged file with `code: content_redacted` @@ -86,8 +105,7 @@ export async function downloadFileToDisk( // that body code, not the status: the endpoint returns 400 for // invalid/out-of-project paths too, and those must not read as a // credential accusation. A body that will not parse falls back to generic. - let code = '' - try { code = (await res.clone().json())?.code ?? '' } catch { /* non-JSON body */ } + const code = typeof body.code === 'string' ? body.code : '' onError(i18nT(code === 'content_redacted' ? 'components.markdownPanel.download_blocked_credentials' : 'components.markdownPanel.download_failed')) @@ -106,7 +124,7 @@ export async function downloadFileToDisk( * the fetch above and by the side panel's last-copy download, which hands the * buffer of a file no longer on disk to `downloadBlob` under the same name. */ export function downloadFileName(filePath: string): string { - return filePath.split('/').pop() || 'download' + return pathBasename(filePath) || 'download' } /** Build the /api/file-stream URL — Range-capable audio/video serving. diff --git a/website/src/utils/unscannedDownload.ts b/website/src/utils/unscannedDownload.ts new file mode 100644 index 00000000000..7122e39b1b5 --- /dev/null +++ b/website/src/utils/unscannedDownload.ts @@ -0,0 +1,33 @@ +/** The question asked before a download the server will not scan. + * + * `/api/file-download` scans a file for credentials only up to a size limit; a + * larger file answers 409 `large_file_unscanned` until the request confirms. + * The download helper asks here, and the one mounted + * `UnscannedDownloadConfirmHost` answers with a dialog. Kept free of React so + * the helper in `fileReadUrl.ts` can ask without owning a component. */ + +export interface UnscannedDownloadInfo { + /** The file's name as it will be saved. */ + name: string + /** The file's size in bytes, as the server measured it. */ + size: number + /** The largest size the server scans, in bytes. */ + limit: number +} + +type Asker = (info: UnscannedDownloadInfo) => Promise + +let asker: Asker | null = null + +/** Install the dialog that answers; returns the uninstall. */ +export function setUnscannedDownloadAsker(next: Asker): () => void { + asker = next + return () => { + if (asker === next) asker = null + } +} + +/** Whether the owner chose to download anyway. `false` when no dialog is mounted. */ +export function confirmUnscannedDownload(info: UnscannedDownloadInfo): Promise { + return asker ? asker(info) : Promise.resolve(false) +}