Closed Bug 512717 Opened 15 years ago Closed 14 years ago

Dropping a file into a contenteditable area discloses the file's full path to the page

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla2.0b8

People

(Reporter: jruderman, Assigned: ehsan.akhgari)

References

Details

(4 keywords)

Attachments

(2 files)

simple contenteditable testcase
42 bytes, text/html
Details
Patch (v1)
3.94 KB, patch
peterv
: review+
sicking
: feedback+
benjamin
: approval2.0+
Details | Diff | Splinter Review
Dropping a file into a contenteditable area gives the site the file's full path, as both href and anchor text. Web pages being able to find out paths is considered a security hole.
Whiteboard: [sg:low]
While I agree that this is a problem, I'm not sure what the correct fix would look like. Webkit (well, Chrome at least) injects a link with the file name as link text and the full path as href. There is this other side of the story when user drags an image to the editable area, in which case we try to inject an <img> element, which needs the full path name. So whatever solution we come up with should cover that as well. Maybe we can just disable accepting files in the editable area? The only other safe solution that comes to my mind is just displaying the file name, but I'm not sure if that's going to be much useful.
OS: Mac OS X → All
Hardware: x86 → All
I would say that what should happen when someone drops a file (image or otherwise) in a contentEditable area is the same as when someone drops a file in a non-contentEditable area. I.e. we should just fire the normal drag-n-drop events. No modifications to the DOM should happen at all IMHO. If the page wants to display an image, it can manually create an <img> element and set its .src to the .url of the dropped file. Let me know what you think.
(In reply to comment #2) > I would say that what should happen when someone drops a file (image or > otherwise) in a contentEditable area is the same as when someone drops a file > in a non-contentEditable area. > > I.e. we should just fire the normal drag-n-drop events. No modifications to the > DOM should happen at all IMHO. > > If the page wants to display an image, it can manually create an <img> element > and set its .src to the .url of the dropped file. Makes sense to me. Especially that file drag-drop APIs don't actually provide the path to the file, they just provide its contents as a data URI.
Another thing we *could* do is to fire the appropriate drag-n-drop events, and if they aren't canceled insert an <img> with the .src pointing to a File.url url. However that likely doesn't help the page a whole lot unless we expose the corresponding File object somewhere as well, so that the page can upload the file to the server or something similar. I don't have a good suggestion for how to expose the File object though. Should likely be a property on the <img>.
(In reply to comment #4) > Another thing we *could* do is to fire the appropriate drag-n-drop events, and > if they aren't canceled insert an <img> with the .src pointing to a File.url > url. However that likely doesn't help the page a whole lot unless we expose the > corresponding File object somewhere as well, so that the page can upload the > file to the server or something similar. Wouldn't that enable web pages to access the file path by examining the src attribute of the <img> element?
Attached patch Patch (v1) — — Splinter Review
So, this patch disables handling file drops in contenteditable areas. It just removes the code responsible for making that happen. I tested this patch with <http://demos.hacks.mozilla.org/openweb/DnD/> (by manually setting contenteditable=true on the <body> element), and the file drag/drop APIs work as normal.
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Comment on attachment 433187 [details] [diff] [review] Patch (v1) I'll write some tests if you give me the "go" on this approach.
Attachment #433187 - Flags: feedback?(jonas)
No, set the .src to the .url value of File object of the dropped file. That url will look something like: moz-filedata:c920d6af-3232-4eec-a3cc-2c8f33540d67 See http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDOMFile.cpp#164
Comment on attachment 433187 [details] [diff] [review] Patch (v1) Yup, I think this is fine approach, though I haven't looked at the actual patch yet.
(In reply to comment #8) > No, set the .src to the .url value of File object of the dropped file. That url > will look something like: > > moz-filedata:c920d6af-3232-4eec-a3cc-2c8f33540d67 > > See > http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDOMFile.cpp#164 This seems like a very bad idea, because that scheme is not interoperable, so saving the generated HTML for later displays will break.
Yup. There is no way to make an image to be interoperable across browsers and users, other than by using data-uris. What we currently do (file-url) doesn't work if the user sends the HTML to a friend. The url in comment 8 doesn't even work if you leave the page and open it again in the same instance of firefox since the moz-filedata uris have a limited lifetime. Given that, I think your patch is the way to go.
Attachment #433187 - Flags: review?(peterv)
Attachment #433187 - Flags: superreview?(roc)
Dealing with multi megabyte data urls is something I think we should avoid in general. We're sure to end up with multiple copies, likely some expanded to wide chars, in memory. The default event provides information for the page to insert an <img src="moz-filedata:..."> url and then handle the image out-of-band when the page is saved, right?
Actually, the drag and drop API provide the page with a data URI if it wants them. I don't think we need to copy that functionality to HTML editors by default.
Why shouldn't the HTML editor provide this functionality by default?
It creates a similar problem to bug 490879. Not every consumer using the HTML editor wants data: URIs (consider Thunderbird as an example.) That said, I still think that those consumers should deal with this on their own (see bug 490879 comment 12) but others don't seem to agree.
I guess the underlying question is whether 'contenteditable' should provide a minimal set of functionality and we require editor authors to opt-in to extensions or implement those extensions themselves, or instead 'contenteditable' should provide as much functionality as possible and we require editor authors to opt-out. The latter approach sounds better to me, but maybe I'm wrong.
(In reply to comment #17) > I guess the underlying question is whether 'contenteditable' should provide a > minimal set of functionality and we require editor authors to opt-in to > extensions or implement those extensions themselves, or instead > 'contenteditable' should provide as much functionality as possible and we > require editor authors to opt-out. The latter approach sounds better to me, but > maybe I'm wrong. My feeling on this is that the latter approach is better, indeed. I think the sane thing to do here is to inject images with a data: URI scheme, and inject file names (without links) for other file types. I also think that we should land the patch in bug 490879. However, I don't want to do this without the consent of Thunderbird folks, but they don't seem to be responsive (my comment on bug 490879 about what they would want us to do has been unanswered for 11 months now). That's why I suggested not using data: URIs, but I'm not sure how to proceed from this point.
I still think data: uris are not the right solution. It seems to me that that is rarely what someone wants. In pretty much all instances I'd imagine that you'd want to extract the images and send them out-of-band. Though maybe that is not true.
(In reply to comment #19) > I still think data: uris are not the right solution. It seems to me that that > is rarely what someone wants. In pretty much all instances I'd imagine that > you'd want to extract the images and send them out-of-band. > > Though maybe that is not true. I think that's almost what you always want in an email application, but almost what you never want in a web app.
I don't think you want that in an email app either. You don't want your inline images showing up as attachments.
You can send them as inline attachments (maybe I'm using an incorrect terminology -- I mean, like a separate MIME section which is not handled by the mail application as an attachment.) The problem with the email world is that there are few clients which can handle data: URIs.
OK, but it seems to me that it's just as easy for a mail composer to convert data: URIs to external attachments, if it wants to, as to convert a mystery URL or a direct file reference. In fact it's probably easier because you don't have to worry about security considerations and you already have a MIME type and base64 data.
With a direct File reference you also have easy access to the mime type (.type) and access to the raw data (using FileReader.readAsX).
Yes, but data: URLs also "just work" if the app doesn't mind their presence.
Does the approach here affect to Thunderbird or Seamonkey? IMO, contenteditable should be able to handle dropping images.
I'm not sure, I'm CCing Standard8 and Kairo on this for feedback.
(In reply to comment #26) > IMO, contenteditable should be able to handle dropping images. Please note that we already provide the necessary APIs for web pages to handle file drops on any element, and that includes contenteditable elements.
Neil knows more on the SeaMonkey side than me in such cases :)
This sounds like it will affect Thunderbird's compose window. Though I could be completely wrong. I haven't got time to dig into what would happen here, as we're heads-down on 3.1b2. As Robert said, Neil's probably best to look at this. If we need to resolve sooner, then ping again.
Well, this is a security bug, so I'd appreciate a confirmation of how Thunderbird/SM would like to handle this.
I think Thunderbird has it easy, since it only has to be able to read the image data which it will then embed within a multipart/related MIME wrapper. Unfortunately SeaMonkey Composer really needs to be able to access the original file, particularly when it's editing a local file (I don't know Composer well enough to know what happens if you edit a page over FTP). Is it possible for the chrome to take over the drop of an image?
(In reply to comment #32) > I think Thunderbird has it easy, since it only has to be able to read the image > data which it will then embed within a multipart/related MIME wrapper. > > Unfortunately SeaMonkey Composer really needs to be able to access the original > file, particularly when it's editing a local file (I don't know Composer well > enough to know what happens if you edit a page over FTP). Is it possible for > the chrome to take over the drop of an image? Yes, it can handle the drop event, get the transferable object, and then do whatever it wants with it.
Comment on attachment 433187 [details] [diff] [review] Patch (v1) We no longer require sr on security bugs.
Attachment #433187 - Flags: superreview?(roc)
peterv: ping? This has been sitting in your review queue for quite some time...
Peter, this patch has been sitting in your queue for over 4 months now, and this is a security bug (albeit sg:low). Can you please provide an ETA on when you expect to get to review this? roc: do you think you can review this instead of Peter?
Whiteboard: [sg:low] → [sg:low][has patch][needs review peterv]
Comment on attachment 433187 [details] [diff] [review] Patch (v1) It's not clear from the comments in this bug, will this break Thunderbird or not?
Attachment #433187 - Flags: review?(peterv) → review+
(In reply to comment #37) > It's not clear from the comments in this bug, will this break Thunderbird or > not? According to comment 32, I think Thunderbird is safe. But anyways, the Thunderbird developers are aware of this change.
Attachment #433187 - Flags: approval2.0?
Whiteboard: [sg:low][has patch][needs review peterv] → [sg:low][has patch][needs approval]
Is there no way to automatically test this?
Attachment #433187 - Flags: approval2.0? → approval2.0+
(In reply to comment #39) > Is there no way to automatically test this? Unfortunately the editor uses the old drag and drop API, which is not accessible to javascript...
Whiteboard: [sg:low][has patch][needs approval] → [sg:low][needs landing]
Flags: in-litmus?
http://hg.mozilla.org/mozilla-central/rev/794555fec7aa Is this wanted on branches?
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [sg:low][needs landing] → [sg:low]
Target Milestone: --- → mozilla2.0b8
Depends on: 609632
Can this be opened up?
Flags: in-litmus?
Flags: needinfo?(dveditz)
Group: core-security
Flags: needinfo?(dveditz)
Keywords: csec-disclosure, sec-low
Whiteboard: [sg:low]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: