[chat] Sanitize image part URLs and share the URL allow-list with markdown - #23058
Conversation
Route part.url through the existing safeUri() guard in SourceUrlPart and FilePart before binding it to href/src, closing a DOM XSS vector where a model/tool could emit javascript: or data: URLs in message parts. Also closes a gap in safeUri that let protocol-relative //host URLs through unchecked.
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14633fbf28
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (trimmed.startsWith('//')) { | ||
| return ''; | ||
| } | ||
|
|
||
| if (trimmed.startsWith('#') || trimmed.startsWith('/')) { |
There was a problem hiding this comment.
For source/file URLs that start with /\, \/, or \\, this check falls through: /\evil.com/x passes the later startsWith('/') branch and \\evil.com/x parses with an https: protocol, but browsers resolve both as https://evil.com/x (the markdown sanitizer rejects these with /^[/\\]{2}/ in renderMarkdown.tsx). This leaves the protocol-relative form this change is trying to neutralize clickable in href/src; reject any two leading slash/backslash characters before accepting same-origin paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — all three resolve to https://evil.com/x and only // was caught. Moot now that the protocol-relative check is dropped, see the thread above.
Still relevant for markdown, where the guard stays: \/host and \\host never reach the sanitizer in that shape because CommonMark unescapes them first, leaving a same-origin path. Only /\host survives intact; covered by a test in f2e6b98216.
| ownerState, | ||
| }); | ||
|
|
||
| const href = safeUri(part.url); |
There was a problem hiding this comment.
When users send attachments via the built-in composer, useChatComposer creates file parts with url: attachment.previewUrl ?? URL.createObjectURL(attachment.file) (packages/x-chat-headless/src/hooks/useChatComposer.ts:191-196), so those outgoing message parts use blob: URLs. Passing every file URL through safeUri here drops blob: because the allow-list only includes http/https/mailto/tel, leaving uploaded image previews without src and file attachments without href; keep these locally-created blob URLs renderable for file parts or use a file-specific sanitizer.
Useful? React with 👍 / 👎.
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
github-actions
Bot
added
the
PR: out-of-date
| // Protocol-relative `//host` resolves to an external origin — reject it | ||
| // (matches the markdown sanitizer in renderMarkdown.tsx). Fragments and | ||
| // single-slash absolute paths remain safe same-origin references. | ||
| if (trimmed.startsWith('//')) { |
There was a problem hiding this comment.
This feels weird, do we accept external origins or not? If yes, then what does it matter it's obtained through a protocol relative URL, if no, then it should be rejected after url parsing and comparing origin with window.location.origin.
I fail to see what rejecting protocol relative urls but not external origin buys us in terms of security.
There was a problem hiding this comment.
You're right — dropped in f2e6b98216. Source URL parts are RAG citations that link out by design, so safeUri('https://evil.com') is accepted anyway; rejecting the // spelling of a destination we already allow isn't a boundary. This also moots the Codex comment below about the /\, \/ and \\ variants — they bypassed a check that guarded nothing.
| const trimmed = uri.trim(); | ||
|
|
||
| // Protocol-relative `//host` resolves to an external origin — reject it | ||
| // (matches the markdown sanitizer in renderMarkdown.tsx). Fragments and |
There was a problem hiding this comment.
If they have to align, would it make sense to have markdown and react rendering share an implementation?
There was a problem hiding this comment.
Done in f2e6b98216. safeUri was already exported from @mui/x-chat-headless/internals and renderMarkdown.tsx already imports from there, so the sanitizer calls it instead of keeping its own protocol list and the "kept in sync" comment.
Markdown still rejects //host and its \ variants. I kept that guard in renderMarkdown rather than moving it into safeUri, so both callers share the allow-list with no behavior change either side. Whether markdown keeps that policy is the same question as your other comment; left alone here.
There was a problem hiding this comment.
Markdown still rejects
//hostand its\variants.
Why?
There was a problem hiding this comment.
No good reason. It came from a review finding in #22488, not from a real decision. I kept it only because I did not want to change behavior in a refactor.
Your point works here too. Markdown already allows https://evil.com, so blocking //evil.com gives us nothing.
The sanitizer is now just safeUri(value) || null.
The href sinks flagged in GHSA-g895-hqpf-c7f2 were fixed on master by mui#23187. Resolution keeps master's safeUri/safeFileUri wiring and retains this branch's two remaining deltas: safeUri rejects protocol-relative //host, and image src sinks are sanitized alongside their href.
github-actions
Bot
removed
the
PR: out-of-date
hasdfa
changed the title
…kdown Drops the protocol-relative rejection per review: external origins are intentionally allowed for source citations, so blocking //host while accepting https://host buys nothing. Markdown keeps its own stricter guard and now reuses safeUri for the scheme allow-list.
hasdfa
changed the title
|
Rebased on master. #23187 landed the No security claim left in here — |
hasdfa
added
the
type: enhancement
Markdown already allows https://evil.com, so rejecting //evil.com added nothing. The sanitizer is now just safeUri(value) || null, so markdown and part rendering use one implementation.
Summary
This branch originally fixed the unsanitized
hrefsinks flagged in the chat message-part renderers. Those were fixed on master in the meantime by #23187, and after review the remaining protocol-relative change was dropped as well (see below), so what's left is small and carries no security claim:srcsinks reuse the guard their siblinghrefalready has. [chat] Normalize message part link URLs #23187 routed every anchorhrefthroughsafeUri()/safeFileUri()but left the two<img>sinks bindingpart.urldirectly. This is consistency, not a live vulnerability —javascript:does not execute in animg src, anddata:image/svg+xmlcannot run script when loaded as an image.renderMarkdown's sanitizer had its own copy of['http:', 'https:', 'mailto:', 'tel:']plus a "Kept in sync with the headlesssafeUri" comment; it now calls the already-exportedsafeUrifrom@mui/x-chat-headless/internals.Dropped: protocol-relative rejection
The earlier version of this PR made
safeUrireject//host. @Janpot pointed out this buys nothing, and that's right:source-urlparts are RAG citations whose purpose is linking out, sosafeUri('https://evil.com')is accepted by design. Blocking the protocol-relative spelling of a destination that's allowed via the front door is not a security boundary. That change and its test are gone.Markdown is a separate case — it rejects
//hostand its\variants today, and that behavior is unchanged here. The guard stays local torenderMarkdownrather than moving intosafeUri, so the two callers share the allow-list without markdown's stricter policy leaking into part rendering. Whether markdown should keep that policy at all is the same question as above and is left alone in this PR.Changes
packages/x-chat-headless/src/message/parts/FilePart.tsx— the<Preview>srcreuses the samesafeFileUri()result as the linkhref(hoisted into a local, since both bindings need it).packages/x-chat-headless/src/message/defaultMessagePartRenderers.tsx— same forrenderDefaultFilePart's<img src>.packages/x-chat/src/ChatMessage/renderMarkdown.tsx— sanitizer delegates the scheme allow-list tosafeUri; keeps its own/^[/\\]{2}/protocol-relative guard. No behavior change.srcon bothFilePartandrenderDefaultFilePart; markdown coverage for the/\hostvariant.safeFileUriis used for the image sinks rather thansafeUriso same-originblob:previews of composer attachments keep working (this was the second Codex comment on the original commit).Testing
pnpm test:unit --project "x-chat" --run— 290 passed, 9 skippedpnpm test:unit --project "x-chat-headless" --run— 651 passed, 24 skippedpnpm --filter "@mui/x-chat*" run typescript— passespnpm eslint— clean