fix(frontend): encode artifact URL path segments#4278
Conversation
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
willem-bd
left a comment
There was a problem hiding this comment.
The core fix is correct and well-scoped: segment-wise encodeURIComponent is the right approach, the backend route (/api/threads/{thread_id}/artifacts/{path:path} + Starlette auto-decode) is the correct counterpart, and encoding threadId brings these helpers in line with the rest of the codebase (which already encodes it). pnpm checks pass.
The comments below are non-blocking edge-case observations at the markdown-link / resolveMessageImageURL boundary, where the input is model-authored rather than a backend-provided artifact path — exactly the inputs the PR's stated scope ("generated artifact filenames") doesn't fully cover. They're low-probability but real, and currently untested.
| return staticDemoArtifactURL({ filepath: absolutePath, threadId }); | ||
| } | ||
| return `${getBackendBaseURL()}/api/threads/${threadId}/artifacts${absolutePath}`; | ||
| return `${getBackendBaseURL()}/api/threads/${encodeURIComponent(threadId)}/artifacts${encodeArtifactPath(absolutePath)}`; |
There was a problem hiding this comment.
When this is reached via resolveMessageImageURL's /mnt/ branch (line 51: if (src.startsWith("/mnt/")) return resolveArtifactURL(src, threadId)), absolutePath is the raw markdown image src, which may carry a ?query or #fragment — e.g.  (cache-buster) or …/report.html#section (fragment nav). encodeArtifactPath now encodes those into the path (chart.png%3Fv%3D2), so the backend looks for a file literally named chart.png?v=2 → 404.
The relative-path branch below deliberately splits ?/# off src and re-appends them as a real query/fragment (${resolveArtifactURL(...)}${src.slice(relativePath.length)}), preserving their meaning. The /mnt/ branch doesn't, so after this PR the two branches disagree on what ?/# mean for the same logical input.
Suggestion: split ?/# in the /mnt/ branch the same way (or document that /mnt/ srcs must not carry query/fragment), and add a test for resolveMessageImageURL("/mnt/.../chart.png?v=2", …).
There was a problem hiding this comment.
Addressed in 60bfbd9. The /mnt/ markdown path now splits off ?query/#fragment before resolving the artifact URL, then re-appends that suffix as the real URL query/fragment. I also added coverage for resolveMessageImageURL with a /mnt/ src carrying query and fragment.
| const EMPTY_ARTIFACT_PATHS: readonly string[] = []; | ||
|
|
||
| function encodeArtifactPath(filepath: string) { | ||
| return filepath.split("/").map(encodeURIComponent).join("/"); |
There was a problem hiding this comment.
encodeURIComponent re-encodes %, so this double-encodes already-percent-encoded input: a%23b.txt → a%2523b.txt, which the backend decodes once to a literal a%23b.txt instead of a#b.txt.
This only affects callers passing model-authored paths: markdown-link.tsx (resolveArtifactURL(href, …) on a /mnt/ markdown href) and resolveMessageImageURL (markdown image src). If the model emits a pre-encoded href/src, this regresses vs. the previous pass-through (which left %23 for the browser to decode to #). Backend-provided paths (present_file_tool, workspace-changes, <uploaded_files>) are raw, so they're unaffected.
The write-file: caller in artifact-file-detail.tsx already does decodeURIComponent(url.pathname) before passing the path down — a decode-before-encode precedent. Consider making encodeArtifactPath idempotent (decode first), or confirm markdown hrefs/srcs can never arrive pre-encoded.
There was a problem hiding this comment.
Addressed in 60bfbd9. Artifact path segments are now decoded before being encoded again, with a safe fallback for malformed percent-encoding, so pre-encoded markdown inputs such as a%23b%3F.txt remain idempotent instead of becoming double-encoded.
| ).toBe("/demo/threads/thread-1/user-data/outputs/style.css"); | ||
| }); | ||
|
|
||
| test("encodes reserved characters in artifact URL path segments", async () => { |
There was a problem hiding this comment.
The added tests cover the three URL builders for #/?/Unicode — nice. Two gaps worth pinning:
resolveMessageImageURLwith a/mnt/src carrying a?query/#fragment(the case whose behavior changes in this PR — see the comment onresolveArtifactURL). No test pins the intended result, so the asymmetry with the relative-path branch is unguarded.- The relative-path branch with a reserved character in the artifact path itself — the existing
#detailtest only exercises thesrcsuffix.
Locking these in would guard both branches against future drift.
There was a problem hiding this comment.
Addressed in 60bfbd9. Added tests for the /mnt/ resolveMessageImageURL query/fragment case, idempotent pre-encoded artifact paths, and a relative markdown image path whose artifact filename contains reserved characters.
…UI hardening - bytedance#4117 XSS hardening (isSafeHref, iframe sandbox, safeLocalStorage) - bytedance#4354 streaming chunk batcher for large file tools - bytedance#4294 LLM concurrency cap + burst-rate retry shedding - bytedance#4355 E2B release serialization - bytedance#4267 transient image context cleanup - bytedance#4374 AI disclaimer (bytedance#4373 reasoning-effort default) - bytedance#4337 /goal length validation + counter - bytedance#4278/bytedance#4302/bytedance#4312 URL encoding fixes - bytedance#4375 list_uploaded_files schema fix - bytedance#4288 uploads markdown companion name claim - bytedance#4258 remove frontend debug logs
Fix artifact URL construction so reserved characters in generated artifact filenames stay inside the path instead of being parsed as URL syntax.
What Problem This Solves
urlOfArtifact()andresolveArtifactURL()are the frontend helpers that turn a thread id plus an artifact virtual path into links for artifact previews, markdown images, and downloads. Before this change, those helpers interpolated the rawfilepathdirectly into the URL string:That is fragile because a URL path is not just plain text. Characters such as
#and?have structural meaning:#starts the fragment and?starts the query string. When those characters appear inside a generated artifact filename, the browser parses them before the request reaches the gateway.The backend artifact route expects the full virtual path at
/api/threads/{thread_id}/artifacts/{path}. If the browser truncates the path first, the gateway receives a request for the wrong artifact. The visible effect is that otherwise valid generated filenames can fail in artifact preview, markdown image rendering, or download links.Changes
The production change is intentionally small and limited to frontend URL construction:
/mnt/user-data/...keeps its/path structure while filename segments likea#b?.txtbecomea%23b%3F.txt.threadIdin the same artifact URL helpers so the full route is assembled from URL-safe dynamic segments.download=truequery parameter behavior unchanged; it is still appended after the encoded path.Evidence
Before the fix, a browser URL parser treats filename characters as URL separators, so the route path is shortened before the request can reach FastAPI:
With segment-level encoding, the same filenames remain inside the path and no accidental query or fragment is produced:
The regression tests pin the important cases directly in
frontend/tests/unit/core/artifacts/utils.test.ts:They also cover the static demo path so the same reserved-character behavior is consistent outside the live gateway flow.
Possible call chain / impact
This PR changes only the frontend helper layer in that chain. The intended boundary is to make the URL safe before the browser parser sees it, rather than adding special cases to the backend after the path has already been truncated.
Sibling surfaces checked:
urlOfArtifact()covers artifact detail links, preview sources, file list download links, and the artifact loader.resolveArtifactURL()covers absolute artifact paths and message image rewriting throughresolveMessageImageURL().Testing / Validation
Local validation was run on Windows with Node v24.15.0 and pnpm 10.26.2; GitHub Actions will re-run on the repo's pinned Node 22 environment.
pnpm test tests/unit/core/artifacts/utils.test.ts— 6 passedpnpm test— 646 passedpnpm format— passedpnpm lint— passedpnpm typecheck— passedBETTER_AUTH_SECRET=local-dev-secret pnpm build— passed; emitted the existing Turbopack NFT-list warning from the mock artifact route