I can't get this to work on Firefox 13 on OS X. I can add it to the quick-launch cell on the new tab page but it cannot be opened from there.
Ah, I wasn't paying attention to that line in the bookmarklet. Yeah, this is bad. :-)
(Verified in Firefox 13.0.1)
Hrm, I don't understand why this wouldn't have been fixed by bug 723808. Perhaps because of the way newtab loads URLs...
Ah, so I think the newtab just inserts the links as normal <a href="">s. Which means that we probably have an explicit owner for the load (the current page), and so don't go through the "find an owner to inherit from" code that was changed by bug 723808?
Ah. I was clicking the button in the bookmarks toolbar. Yeah, if you stick the URI in the page there will be no inheritance involved at all, so bug 723808 does not help. What should probably happen is that adding a quick-launch cell should CheckLoadURI at least with DISALLOW_INHERIT_OWNER, and possibly with some sort of not-too-trusted source principal. Do we want to allow dragging file:// URIs in there? I guess we do...
One could argue this is a power-user feature rather than a vulnerability, but I guess we should fix it because power-users have other options (a chrome scratchpad, for example) and this is plausible social-engineering target.
Just adding a checkLoadURI call with DISALLOW_INHERIT_PRINCIPAL before adding pages to newtab sounds reasonable to me. Tim, do you want to take this?
Created attachment 635532 [details] [diff] [review] patch v1 Using checkLoadURIWithPrincipal(DISALLOW_INHERIT_PRINCIPAL) to decide whether to accept external drags.
Comment on attachment 635532 [details] [diff] [review] patch v1 spoke to Tim on IRC - approach looks fine, but we should have this apply to all links, not just drag&drop
Created attachment 636057 [details] [diff] [review] patch v2 Preventing 'bad links' from being dropped onto the grid and from appearing on the grid when included in history query results. I included a test case for drag and drop. I couldn't really test the PlacesProvider filtering as that depends on bug 765235 being fixed.
I just duped that to bug 728313, but yeah, if that's an easy fix I think it'd be a nice solution to these issues!
(In reply to :Gavin Sharp (use email@example.com for email) from comment #16) > I just duped that to bug 728313, but yeah, if that's an easy fix I think > it'd be a nice solution to these issues! Took a quick stab at it today and looks like it's not as easy as I thought. I have no idea how to communicate between the content script and the actual web page. Also, unprivileged XUL files don't load as they're considered remote XUL.
Comment on attachment 636057 [details] [diff] [review] patch v2 Can you add a test for the PlacesProvider path? (I assume you've ensured that the drop test fails without the fix). Duplicating the "extract URL" code from _pinDraggedSite in isValid is sub-optimal, can we factor that out somehow?
(In reply to :Gavin Sharp (use firstname.lastname@example.org for email) from comment #18) > Can you add a test for the PlacesProvider path? (I assume you've ensured > that the drop test fails without the fix). Yes, the drop test fails without the fix. As per conversation on IRC: we can't test the PlacesProvider path as the navigation history will not allow us to add 'bad' links to it. > Duplicating the "extract URL" code from _pinDraggedSite in isValid is > sub-optimal, can we factor that out somehow? Created a gDragDataHelper that contains this part of the code now.
mass remove verifyme requests greater than 4 months old