fix: a dropped file's name reaches a toast as text - #939
Merged
Merged
Conversation
toasts.min.js renders every message with innerHTML, deliberately, since messages carry <br>
separators — so whatever is concatenated into one has to be escaped first. The file upload's
two rejection messages concatenated the dropped File's name and type raw, and a file named
<img src=x onerror=...> that tripped either check ran its handler.
It is the user's own local file, so on its own this is self-XSS, reachable only by
persuading somebody to upload a crafted name. It is fixed because the sink is real and the
fix is one expression: $("<div/>").text(value).html() at both call sites.
The wider issue is left for a separate decision: app-main.min.js routes a server's
description and messages into the same innerHTML, and app-requests.min.js puts a failed
request's entire responseText there, while ActionResponse strings are not escaped the way
templates are.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
toasts.min.jsrenders every message withinnerHTML— deliberately, since messages carry<br>separators. So anything concatenated into a toast has to be escaped first, and the file upload's two
rejection messages concatenated the dropped
File'snameandtyperaw:A file named
<img src=x onerror=…>that trips either check ran its handler in the page.The grade, stated plainly
It is the user's own local file, so on its own this is self-XSS — reachable only by persuading
somebody to upload a crafted name. It is fixed anyway because the sink is real, the fix is one
expression, and the next message to put a value into a toast would inherit the same mistake.
The change
$("<div/>").text(value).html()— set the value as text, read it back escaped — at both call sites.jQuery is already on every page. The toasts keep rendering their
<br>s; only the interpolatedvalues change.
Test
FileNamesReachToastsAsTextTestscansapp-util.min.jsfor every toast message that interpolatesthe chosen file's
nameortype, and asserts each is wrapped in the escape. It checks the textimmediately around each occurrence rather than matching the whole call, because the escape itself
contains parentheses: the first version stopped at the
)inside.text(c.name)and failed withthe fix in place — worth recording, since a guard that fails both ways proves nothing.
Mutation-verified: reverting the JS fails it, naming the byte offset.
Deliberately not in this change
The toast sink is the wider issue.
app-main.min.js's response handler routes a server'sdescriptionandmessagesinto the sameinnerHTML, andapp-requests.min.jsputs a failedrequest's entire
responseTextthere. Those are strings fromActionResponse, which the PHPside does not pass through
$_e()the way it does templates — so they are safe only while noserver message ever carries user text. Switching toasts to text would break every
<br>; makingthe server's messages safe by construction is a separate, larger decision.