From 582fa20001fb424dcb14c45710495be7f263dacd Mon Sep 17 00:00:00 2001 From: PiedPiper911 <32931126+PiedPiper911@users.noreply.github.com> Date: Mon, 24 Aug 2026 07:45:30 +0800 Subject: [PATCH] fix(artifacts): serve SHA-256 via ETag so preview/edit work on non-secure contexts (#4865) * fix(artifacts): serve SHA-256 via ETag so preview/edit work on non-secure contexts crypto.subtle is only available in secure contexts (HTTPS or localhost). The frontend fell back to it to compute an artifact's SHA-256 when the Gateway did not return one, which threw on http://: and broke both artifact preview and inline editing (issue #4864). - Gateway now returns the real SHA-256 as an ETag header for inline text and active-content artifact responses (and skill-archive members). - Frontend prefers the ETag and only computes a hash as a last resort, falling back gracefully (FNV-1a) instead of throwing when crypto.subtle is missing. * fix(artifacts): address PR review feedback for #4864 - Cache SHA-256 digests by (path, mtime_ns, size) so the many small Range requests a browser issues while scrubbing/paginating a preview do not each re-hash a potentially huge artifact from scratch (performance). - Gate inline editing on a real 64-hex revision: hasRevision requires sha256.length === 64, so the FNV-1a fallback on non-secure origins keeps preview working but no longer 422s on save (contract). - Anchor and lowercase the ETag regex and accept the weak W/ prefix gzip emits, so uppercase hex and longer digests (sha-384/512) can't masquerade as sha-256. - Cover the forced-download ETag on the backend and add frontend tests for weak-ETag parsing and the non-secure-context FNV fallback. Feedback from reviewer willem-bd on PR #4865. * style: fix ruff format and prettier issues - test_artifacts_router.py: collapse two over-split client.get() calls to satisfy ruff format (line-length 240) - loader.ts / artifact-file-detail.tsx / loader.test.ts: apply prettier formatting and restore LF line endings * style: fix ruff format and prettier issues - test_artifacts_router.py: collapse two over-split client.get() calls to satisfy ruff format (line-length 240) - loader.ts / artifact-file-detail.tsx / loader.test.ts: apply prettier formatting and restore LF line endings * style: fix ruff format and prettier issues - test_artifacts_router.py: collapse two over-split client.get() calls to satisfy ruff format (line-length 240) - loader.ts / artifact-file-detail.tsx / loader.test.ts: apply prettier formatting and restore LF line endings * style: fix ruff format and prettier issues - test_artifacts_router.py: collapse two over-split client.get() calls to satisfy ruff format (line-length 240) - loader.ts / artifact-file-detail.tsx / loader.test.ts: apply prettier formatting and restore LF line endings * style: reformat artifact-file-detail.tsx for prettier with tailwind class ordering * fix: invalidate SHA-256 cache after artifact edit Clear the LRU cache after os.replace() so the next preview request computes the new digest. Edits are rare, so clearing the whole 256-entry cache costs nothing (addressing PR review comment #5). * fix(artifacts): skip ETag for oversized files + CRLF->LF + cache invalidation (#4865) * fix(loader): use real empty-content SHA-256 for empty 416 range (#4865) * test(artifacts): assert oversized artifacts carry no SHA-256 ETag (#4865) * style(frontend): format long sha256 constant (prettier) * test(backend): fix oversized-artifact ETag assertions and formatting (ruff) * test(backend): keep oversized-payload line within ruff 240-col config --- backend/app/gateway/routers/artifacts.py | 70 +++++++++++++- backend/tests/test_artifacts_router.py | 93 +++++++++++++++++++ .../artifacts/artifact-file-detail.tsx | 2 +- frontend/src/core/artifacts/loader.ts | 26 +++++- .../tests/unit/core/artifacts/loader.test.ts | 43 +++++++++ 5 files changed, 227 insertions(+), 7 deletions(-) diff --git a/backend/app/gateway/routers/artifacts.py b/backend/app/gateway/routers/artifacts.py index 08f12537f..63ccde9a4 100644 --- a/backend/app/gateway/routers/artifacts.py +++ b/backend/app/gateway/routers/artifacts.py @@ -1,4 +1,5 @@ import asyncio +import functools import hashlib import logging import mimetypes @@ -134,6 +135,10 @@ def _replace_artifact_atomically(actual_path: Path, content: bytes, file_stat: o handle.flush() os.fsync(handle.fileno()) os.replace(temp_path, actual_path) + # Invalidate the SHA-256 cache after a successful edit so the next + # preview request computes the new digest. Edits are rare, so + # clearing the whole 256-entry LRU costs nothing (see PR review). + _sha256_of_file_cached.cache_clear() finally: if temp_fd >= 0: os.close(temp_fd) @@ -308,6 +313,32 @@ def _read_artifact_payload(actual_path: Path, path: str, download: bool) -> tupl return ("inline_file", mime_type) +def _sha256_of_file(path: Path) -> str: + """Return the hex SHA-256 digest of *path* without loading it whole. + + Computing the digest on the Gateway lets the browser skip its own + crypto.subtle-based hashing, which is unavailable in non-secure contexts + (e.g. http://:) and otherwise breaks artifact preview + + inline editing (see issue #4864). + + The digest is cached by (path, mtime_ns, size) so the many small ``Range`` + requests a browser issues while scrubbing/paginating a preview do not each + re-hash a potentially huge artifact from scratch (raised in PR review). + """ + stat = path.stat() + return _sha256_of_file_cached(str(path), stat.st_mtime_ns, stat.st_size) + + +@functools.lru_cache(maxsize=256) +def _sha256_of_file_cached(path: str, mtime_ns: int, size: int) -> str: + """Cached SHA-256 of *path*; the size/mtime args invalidate stale entries.""" + digest = hashlib.sha256() + with open(path, "rb") as handle: + for chunk in iter(lambda: handle.read(1 << 20), b""): + digest.update(chunk) + return digest.hexdigest() + + @router.get( "/threads/{thread_id}/artifacts/{path:path}", summary="Get Artifact File", @@ -382,7 +413,13 @@ async def get_artifact(thread_id: ThreadId, path: str, request: Request, downloa request_headers = request.headers if request is not None else {} range_header = None if request_headers.get("if-range") else request_headers.get("range") ranged_content, status_code, range_headers = _slice_byte_range(content, range_header) - inline_headers = {**cache_headers, **range_headers} + inline_headers = { + **cache_headers, + **range_headers, + # Real SHA-256 so the browser can skip crypto.subtle (unavailable on + # non-secure contexts) when previewing / editing artifacts (#4864). + "ETag": f'"{hashlib.sha256(content).hexdigest()}"', + } if mime_type and mime_type.startswith("text/"): return Response(content=ranged_content, status_code=status_code, media_type=mime_type, headers=inline_headers) @@ -411,15 +448,38 @@ async def get_artifact(thread_id: ThreadId, path: str, request: Request, downloa if kind == "file": # Always force download for active content types to prevent script # execution in the application origin when users open generated artifacts. - return FileResponse(path=actual_path, filename=actual_path.name, media_type=mime_type, headers=_build_attachment_headers(actual_path.name)) + headers = {**_build_attachment_headers(actual_path.name)} + file_size = await asyncio.to_thread(lambda: actual_path.stat().st_size) + if file_size <= MAX_EDITABLE_ARTIFACT_BYTES: + # Real SHA-256 so the browser can skip crypto.subtle (unavailable + # on non-secure contexts) when previewing / editing artifacts (#4864). + # Skipped for oversized artifacts to avoid a full-file read on every + # GET / Range request (raised in review as a performance P1). + content_sha256 = await asyncio.to_thread(_sha256_of_file, actual_path) + headers["ETag"] = f'"{content_sha256}"' + return FileResponse( + path=actual_path, + filename=actual_path.name, + media_type=mime_type, + headers=headers, + ) if kind == "inline_file": # FileResponse honors byte-Range requests for large text previews and # media seeking without buffering the full artifact in the Gateway. + headers = {"Content-Disposition": _build_content_disposition("inline", actual_path.name)} + file_size = await asyncio.to_thread(lambda: actual_path.stat().st_size) + if file_size <= MAX_EDITABLE_ARTIFACT_BYTES: + # Real SHA-256 so the browser can skip crypto.subtle (unavailable + # on non-secure contexts) when previewing / editing artifacts (#4864). + # Skipped for oversized artifacts to avoid a full-file read on every + # GET / Range request (raised in review as a performance P1). + content_sha256 = await asyncio.to_thread(_sha256_of_file, actual_path) + headers["ETag"] = f'"{content_sha256}"' return FileResponse( path=actual_path, media_type=mime_type, - headers={"Content-Disposition": _build_content_disposition("inline", actual_path.name)}, + headers=headers, ) raise AssertionError(f"Unhandled artifact response kind: {kind!r}") @@ -473,6 +533,10 @@ async def update_artifact( if sandbox is not None: await asyncio.to_thread(_sync_artifact_to_sandbox, sandbox, virtual_path, updated) await asyncio.to_thread(_replace_artifact_atomically, actual_path, updated, file_stat) + # Invalidate any cached digest for this path so a subsequent GET + # serves the fresh SHA-256. The (path, mtime_ns, size) LRU key can + # collide on a same-size, sub-nanosecond re-write (review nit). + _sha256_of_file_cached.cache_clear() except Exception: if sandbox is not None: try: diff --git a/backend/tests/test_artifacts_router.py b/backend/tests/test_artifacts_router.py index d3485d76c..9a20482c2 100644 --- a/backend/tests/test_artifacts_router.py +++ b/backend/tests/test_artifacts_router.py @@ -358,6 +358,22 @@ def test_get_artifact_text_preview_supports_bounded_range_requests(tmp_path, mon assert invalid.headers["content-range"] == f"bytes */{len(payload)}" +def test_get_artifact_inline_text_returns_sha256_etag(tmp_path, monkeypatch) -> None: + payload = b"hello artifact world" + artifact_path = tmp_path / "note.txt" + artifact_path.write_bytes(payload) + monkeypatch.setattr(artifacts_router, "resolve_thread_virtual_path", lambda _thread_id, _path, user_id=None: artifact_path) + + app = make_authed_test_app() + app.include_router(artifacts_router.router) + with TestClient(app) as client: + response = client.get("/api/threads/thread-1/artifacts/mnt/user-data/outputs/note.txt") + + assert response.status_code == 200 + expected = hashlib.sha256(payload).hexdigest() + assert response.headers.get("etag") == f'"{expected}"' + + def test_get_skill_archive_preview_supports_bounded_range_requests(tmp_path, monkeypatch) -> None: payload = ("skill preview \u4e2d\u6587\n" * 100_000).encode() skill_path = tmp_path / "sample.skill" @@ -386,6 +402,24 @@ def test_get_skill_archive_preview_supports_bounded_range_requests(tmp_path, mon assert invalid.headers["content-range"] == f"bytes */{len(payload)}" +def test_get_skill_archive_inline_returns_sha256_etag(tmp_path, monkeypatch) -> None: + payload = ("skill preview \u4e2d\u6587\n" * 100).encode() + skill_path = tmp_path / "sample.skill" + with zipfile.ZipFile(skill_path, "w", compression=zipfile.ZIP_DEFLATED) as zip_ref: + zip_ref.writestr("SKILL.md", payload) + + monkeypatch.setattr(artifacts_router, "resolve_thread_virtual_path", lambda _thread_id, _path, user_id=None: skill_path) + + app = make_authed_test_app() + app.include_router(artifacts_router.router) + with TestClient(app) as client: + response = client.get("/api/threads/thread-1/artifacts/mnt/user-data/outputs/sample.skill/SKILL.md") + + assert response.status_code == 200 + expected = hashlib.sha256(payload).hexdigest() + assert response.headers.get("etag") == f'"{expected}"' + + @pytest.mark.parametrize(("filename", "content"), ACTIVE_ARTIFACT_CASES) def test_get_artifact_forces_download_for_active_content(tmp_path, monkeypatch, filename: str, content: str) -> None: artifact_path = tmp_path / filename @@ -397,6 +431,9 @@ def test_get_artifact_forces_download_for_active_content(tmp_path, monkeypatch, assert isinstance(response, FileResponse) assert response.headers.get("content-disposition", "").startswith("attachment;") + # The forced-download branch must carry a real SHA-256 ETag so the + # frontend can enable inline editing (see issue #4864 review feedback). + assert response.headers.get("etag") == f'"{hashlib.sha256(content.encode()).hexdigest()}"' @pytest.mark.parametrize(("filename", "content"), ACTIVE_ARTIFACT_CASES) @@ -591,3 +628,59 @@ def test_skill_archive_preview_rejects_oversized_member_before_decompression(tmp artifacts_router._extract_file_from_skill_archive(skill_path, "SKILL.md") assert exc_info.value.status_code == 413 + + +def test_get_artifact_large_text_skips_etag(tmp_path, monkeypatch) -> None: + # A text artifact larger than MAX_EDITABLE_ARTIFACT_BYTES must not be hashed + # on every GET / Range request (performance P1 from review). The response + # still streams, but carries no full-content SHA-256 ETag; the client falls + # back to its own hashing where crypto.subtle is available. + payload = b"a" * (artifacts_router.MAX_EDITABLE_ARTIFACT_BYTES + 1) + artifact_path = tmp_path / "large.txt" + artifact_path.write_bytes(payload) + monkeypatch.setattr( + artifacts_router, + "resolve_thread_virtual_path", + lambda _thread_id, _path, user_id=None: artifact_path, + ) + + app = make_authed_test_app() + app.include_router(artifacts_router.router) + with TestClient(app) as client: + response = client.get( + "/api/threads/thread-1/artifacts/mnt/user-data/outputs/large.txt", + ) + + assert response.status_code == 200 + # Oversized artifacts skip the full-file SHA-256 pass, so there must be no + # 64-hex content-hash ETag. Starlette's FileResponse may still attach a + # cheap mtime/size-derived ETag, which requires no file read. + etag = response.headers.get("etag") + assert etag is None or len(etag.strip('"')) != 64 + assert response.content == payload + + +def test_get_artifact_large_active_content_skips_etag(tmp_path, monkeypatch) -> None: + # Active content (e.g. .html) is force-downloaded. A large active file must + # still force a download but skip the full-file SHA-256 pass (performance P1). + payload = "" + ("a" * (artifacts_router.MAX_EDITABLE_ARTIFACT_BYTES + 1)) + "" + artifact_path = tmp_path / "large.html" + artifact_path.write_text(payload, encoding="utf-8") + monkeypatch.setattr( + artifacts_router, + "resolve_thread_virtual_path", + lambda _thread_id, _path, user_id=None: artifact_path, + ) + + response = asyncio.run( + call_unwrapped( + artifacts_router.get_artifact, + "thread-1", + "mnt/user-data/outputs/large.html", + _make_request(), + ) + ) + + assert isinstance(response, FileResponse) + assert response.headers.get("content-disposition", "").startswith("attachment;") + assert response.headers.get("etag") is None diff --git a/frontend/src/components/workspace/artifacts/artifact-file-detail.tsx b/frontend/src/components/workspace/artifacts/artifact-file-detail.tsx index 07e6adbde..eec396036 100644 --- a/frontend/src/components/workspace/artifacts/artifact-file-detail.tsx +++ b/frontend/src/components/workspace/artifacts/artifact-file-detail.tsx @@ -231,7 +231,7 @@ export function ArtifactFileDetail({ isWriteFile, isSkillFile, isMock: Boolean(isMock), - hasRevision: typeof sha256 === "string", + hasRevision: typeof sha256 === "string" && sha256.length === 64, isStaticWebsite: env.NEXT_PUBLIC_STATIC_WEBSITE_ONLY === "true", }); const editorContent = isDirty ? activeDraft.draftContent : visibleContent; diff --git a/frontend/src/core/artifacts/loader.ts b/frontend/src/core/artifacts/loader.ts index eb8e8a92f..c9ed257be 100644 --- a/frontend/src/core/artifacts/loader.ts +++ b/frontend/src/core/artifacts/loader.ts @@ -7,8 +7,27 @@ import type { AgentThreadState } from "../threads"; import { buildWriteFileDraftContent } from "./preview"; import { urlOfArtifact } from "./utils"; +function fnv1aHash(value: string): string { + let hash = 0x811c9dc5; + for (let index = 0; index < value.length; index += 1) { + hash ^= value.charCodeAt(index); + hash = Math.imul(hash, 0x01000193); + } + return (hash >>> 0).toString(16).padStart(8, "0"); +} + async function sha256OfText(content: string): Promise { - const digest = await globalThis.crypto.subtle.digest( + const subtle = globalThis.crypto?.subtle; + if (!subtle) { + // crypto.subtle is only exposed in secure contexts (HTTPS or localhost). + // On a non-secure origin such as http://: it is undefined, so + // hashing would throw and break artifact preview + inline editing + // (issue #4864). The Gateway returns the real SHA-256 via the ETag header, + // so this fallback is only a last-resort fingerprint used for draft + // reconciliation when that header is absent. + return fnv1aHash(content); + } + const digest = await subtle.digest( "SHA-256", new TextEncoder().encode(content), ); @@ -58,7 +77,8 @@ export async function loadArtifactContent({ truncated: false, previewBytes: 0, totalBytes: 0, - sha256: await sha256OfText(""), + sha256: + "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", // SHA-256 of empty content (keeps empty artifacts editable on non-secure origins) }; } if (!response.ok) { @@ -76,7 +96,7 @@ export async function loadArtifactContent({ const content = new TextDecoder().decode(bytes, { stream: truncated }); const etag = response.headers.get("etag"); const sha256 = - etag?.match(/^"([0-9a-f]{64})"$/)?.[1] ?? + etag?.match(/^(?:W\/)?"([0-9a-fA-F]{64})"$/)?.[1]?.toLowerCase() ?? (!truncated ? await sha256OfText(content) : undefined); const contentLengthHeader = response.headers.get("Content-Length"); const contentLength = diff --git a/frontend/tests/unit/core/artifacts/loader.test.ts b/frontend/tests/unit/core/artifacts/loader.test.ts index ca2947e1e..353f2cb46 100644 --- a/frontend/tests/unit/core/artifacts/loader.test.ts +++ b/frontend/tests/unit/core/artifacts/loader.test.ts @@ -146,4 +146,47 @@ describe("loadArtifactContent", () => { "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", }); }); + + it("parses a weak (W/) SHA-256 ETag returned by a gzipped response", async () => { + rs.spyOn(globalThis, "fetch").mockResolvedValue( + new Response("content", { + status: 200, + headers: { ETag: `W/"${"b".repeat(64)}"` }, + }), + ); + + const loaded = await loadArtifactContent({ + filepath: "/mnt/user-data/outputs/report.md", + threadId: "thread-1", + }); + + expect(loaded.sha256).toBe("b".repeat(64)); + }); + + it("resolves without throwing when crypto.subtle is unavailable (non-secure context)", async () => { + rs.stubGlobal("crypto", { subtle: undefined } as unknown as Crypto); + + const bytes = new TextEncoder().encode("complete"); + rs.stubGlobal( + "fetch", + rs.fn(async (_url: string, init?: RequestInit) => { + expect(new Headers(init?.headers).has("Range")).toBe(false); + return new Response(bytes, { + status: 200, + headers: { "Content-Length": String(bytes.length) }, + }); + }), + ); + + const loaded = await loadArtifactContent({ + filepath: "/mnt/user-data/outputs/non-secure.html", + threadId: "thread-1", + full: true, + }); + + // FNV-1a fallback keeps preview working and returns a string; because it + // is not a 64-hex digest the UI treats it as non-editable (no 422 on save). + expect(typeof loaded.sha256).toBe("string"); + expect(loaded.sha256).toHaveLength(8); + }); });