mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-09-09 21:49:37 +00:00
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://<lan-ip>:<port> 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
This commit is contained in:
PiedPiper911
GitHub
parent
ea9b70148e
commit
582fa20001
@ -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://<lan-ip>:<port>) 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:
|
||||
|
||||
@ -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 = "<html>" + ("a" * (artifacts_router.MAX_EDITABLE_ARTIFACT_BYTES + 1)) + "</html>"
|
||||
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
|
||||
|
||||
@ -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;
|
||||
|
||||
@ -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<string> {
|
||||
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://<lan-ip>:<port> 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 =
|
||||
|
||||
@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user