fix(chat): escape attachment filename and validate file URL scheme to prevent XSS#5028
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryLow Risk Overview Image attachment preview no longer uses File download fallback (when serve/fetch fails) only calls Adds Vitest coverage for Reviewed by Cursor Bugbot for commit bf85dd3. Configure here. |
Greptile SummaryThis PR fixes two DOM XSS vectors in the chat message UI: it removes a
Confidence Score: 4/5The two active XSS vectors are correctly closed; the new code is safe to ship. The core fixes — replacing Both Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant ChatUI as message.tsx
participant Preview as openAttachmentPreview()
participant FD as file-download.tsx
participant Browser
User->>ChatUI: clicks image attachment
ChatUI->>ChatUI: validate dataUrl starts with "data:"
ChatUI->>Preview: openAttachmentPreview(name, dataUrl)
Preview->>Preview: escapeHtml(name) → safeName
Preview->>Preview: escapeHtml(dataUrl) → safeUrl
Preview->>Preview: build HTML blob with safeName + safeUrl
Preview->>Browser: URL.createObjectURL(blob)
Preview->>Browser: window.open(blobUrl, "_blank", "noopener,noreferrer")
Preview->>Browser: setTimeout → revokeObjectURL after 60s
User->>FD: clicks file download button
FD->>FD: triggerDownload(getFileUrl(file), file.name)
alt download fails
FD->>FD: isSafeHttpUrl(file.url)?
FD->>FD: new URL(url, getBrowserOrigin())
alt "protocol === http: or https:"
FD->>Browser: window.open(file.url, "_blank", "noopener,noreferrer")
else javascript: / data: / blob: / vbscript:
FD->>FD: silently reject
end
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export function isSafeHttpUrl(url: string): boolean { | ||
| try { | ||
| const parsed = new URL(url, getBrowserOrigin() ?? undefined) | ||
| return parsed.protocol === 'http:' || parsed.protocol === 'https:' | ||
| } catch { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
isSafeHttpUrl treats any relative-like string as safe
When getBrowserOrigin() returns a value (as it does in the browser), strings like '', 'not a url', or even ' ' resolve against the browser origin and come back with protocol http: or https:, so the function returns true. The calling site already guards against empty strings with if (file.url && ...), but a purely alphabetic or arbitrary non-URL string supplied by agent output (e.g. "download") would pass validation and be handed to window.open() as a relative path, navigating to <origin>/download in a new tab. This is harmless in isolation, but the function's contract advertises rejecting non-http(s) input, while the tests explicitly document the opposite. Consider adding an early if (!url.startsWith('http://') && !url.startsWith('https://') && !url.startsWith('/')) guard, or at minimum narrowing the test description so future maintainers understand the deliberate choice.
1 participant
Summary
openAttachmentPreviewhelper (escapeHtml + blob URL); delete the unescapeddocument.writepath that interpolatedattachment.nameand the data URL into an origin-inheritingabout:blank(DOM XSS in the app origin)isSafeHttpUrl(http/https allowlist, resolved viagetBrowserOrigin) gating the file-download fallbackwindow.open(file.url), blockingjavascript:/data:/vbscript:/blob:schemes from agent-supplied URLs; also passnoopener,noreferrerescapeHtml(incl. markup-breakout payload) andisSafeHttpUrl(scheme allowlist + same-origin relative handling)Type of Change
Testing
Checklist