405924 - ensure places uri schemes are not accessible via content

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P4)

Description

[Dietrich Ayala (:dietrich)]

schemes: place, moz-anno

** TODO: write mochitest test for this
** TODO: confirm blacklisted, LXR link is rotten: http://mxr.mozilla.org/seamonkey/search?string=moz-anno

Comment 1

[(not reading, please use [e-mail] instead)]

1)

About moz-anno, see http://mxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsAnnoProtocolHandler.cpp#90

  90   *aProtocolFlags = (URI_NORELATIVE | URI_NOAUTH | URI_DANGEROUS_TO_LOAD);

I think the last time I debugged, it was URI_DANGEROUS_TO_LOAD that was the key.  But yes, we should confirm and write tests.

2)

about place:, from the url bar, you will get an alert that place is not a registered protocol.

TODO:  will anything unexpected happen if another application registers as a place: protocol handler?

Comment 2

[Brandon Sterne (:bsterne)]

Since this is a dependency of bug 375898, marking P2 so it stays on our radar for Fx3 triage/tracking.

Comment 3

[Daniel Veditz [:dveditz]]

investigation for moz-anno seems complete (and safe). Where is the place: scheme implemented and used?

Comment 4

[cmtalbert]

Setting in test-suite to help track this, since we need some mochikit tests for this.  I'm happy to help write them if needed, once I get bug 384226 out of the way.

Comment 5

[Dietrich Ayala (:dietrich)]

(In reply to comment #3)
> Where is the place:
> scheme implemented and used?
> 

http://mxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsNavHistoryQuery.cpp#233

the place: URI is tokenized and the parameters used to populate a Places query (which in turn gets turned into a SQL query and executed).

these queries are used to create the "smart folders" in the bookmarks UI. for example, the "Most Visited" folder in the Smart Bookmarks folder is basically just a regular bookmark with this URI: place:queryType=0&sort=8&maxResults=10.

Comment 6

[Dietrich Ayala (:dietrich)]

(In reply to comment #4)
> Setting in test-suite to help track this, since we need some mochikit tests for
> this.  I'm happy to help write them if needed, once I get bug 384226 out of the
> way.
> 

thanks Clint, that would be terrific.

Comment 7

[Dietrich Ayala (:dietrich)]

(In reply to comment #1)
> 
> TODO:  will anything unexpected happen if another application registers as a
> place: protocol handler?
> 

It will handle place: URIs in content situations, or when opening links. It will not affect our internal handling of places, as we convert place: bookmarks into queries (http://mxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsNavHistory.cpp#5310).

An example of this in action is to open the context menu of the "Smart Bookmarks" folder, and note that "Open All in Tabs" is disabled.

Here's a contrived scenario:

If a user has an application or extension installed which registers as a handler for the place: protocol, then an extension could open a place: URI as a normal link, which would pass the place: URI to the registered handler.

This is not a realistic scenario, as that same extension/application could get whatever they wanted, or do damage by far easier means at that point.

Comment 8

[Dietrich Ayala (:dietrich)]

Per comment #7, I'm decreasing priority of this bug, and turning it over to Clint for a mochitest (comment #4).

Comment 9

[cmtalbert]

Attachment: Mochitest for this

Here is a mochitest for the behavior.  Thanks to Gavin, I made use of prompt_common.js [1] from the passwordmgr tests.  I'm not sure if/how/ we'd want to go about sharing that code, so I added it to the places/ tree (and I removed the testnum variable, since it didn't make sense for us, but otherwise, it's the same file).  I also had to add support for simple mochitests to places as we didn't have support for those yet.

Let me know if you think of another test case I missed in loading the places URI.  I did the ones I thought of, which are probably the most obvious.

[1]: http://lxr.mozilla.org/seamonkey/source/toolkit/components/passwordmgr/test/prompt_common.js

Comment 10

[Dietrich Ayala (:dietrich)]

Comment on attachment 311722 [details] [diff] [review]
Mochitest for this

a couple of changes:

- indent 2 spaces, per style guidelines
- remove the queries line in the makefile
- move these into a sub dir, instead of cluttering up the test directory

Comment 11

[cmtalbert]

Attachment: Spaces, makefile, directory structure cleanup

w.r.t. the two spaces thing, wasn't sure if you meant the html file (generated by the gen_template script, so it had wonky spaces too) or the .js file (which seems to have used both 4 and 2 space indenting).  I cleaned up both.

* Put the tests into a places/tests/mochitest directory

* removed the query line from the makefile.  Sorry about that oversight.

Comment 12

[Dietrich Ayala (:dietrich)]

Comment on attachment 312053 [details] [diff] [review]
Spaces, makefile, directory structure cleanup

thanks very much, r=me.

Comment 13

[Dietrich Ayala (:dietrich)]

weird, i get this when trying to apply this patch:

patch: **** malformed patch at line 46: Index: toolkit/components/places/tests/mochitest/prompt_common.js

Comment 14

[cmtalbert]

Attachment: A rebuilt patch -- same code

This is the same code Dietrich reviewed, I just rebuilt the patch so that it properly applies now.  When can I check this in?

Comment 15

[Mike Connor [:mconnor]]

Comment on attachment 315108 [details] [diff] [review]
A rebuilt patch -- same code

yay tests

Comment 16

[cmtalbert]

Mconnor, thanks for the a+

Checking in on Trunk

Checking in toolkit/components/places/tests/Makefile.in;
/cvsroot/mozilla/toolkit/components/places/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.8; previous revision: 1.7
done
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/mochitest/prompt_common.js,v
done
Checking in toolkit/components/places/tests/mochitest/prompt_common.js;
/cvsroot/mozilla/toolkit/components/places/tests/mochitest/prompt_common.js,v  <--  prompt_common.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/mochitest/test_bug_405924.html,v
done
Checking in toolkit/components/places/tests/mochitest/test_bug_405924.html;
/cvsroot/mozilla/toolkit/components/places/tests/mochitest/test_bug_405924.html,v  <--  test_bug_405924.html
initial revision: 1.1
done

--> FIXED

Comment 17

[Reed Loden [:reed]]

(In reply to comment #16)
> Mconnor, thanks for the a+

Tests never need approval, fyi, so don't let that be a barrier to you landing tests in the future. :)

Comment 18

[cmtalbert]

Attachment: New Version of the test that won't turn boxes orange

This is pretty much the same test and the same patch.  The major difference is that the "error" strings are removed from the "ok" texts (these were tripping up the tinderbox log parser) and the "finish" logic was placed in the handleDialog event handler rather than the onload because the handleDialog code gets called before the onload event.

Comment 19

[cmtalbert]

The original patch caused the boxes to go orange, so it was backed out and I have a new patch --> REOPENING.

Comment 20

[cmtalbert]

mconnor, can I carry your approval forward?

Comment 21

[Reed Loden [:reed]]

(In reply to comment #20)
> mconnor, can I carry your approval forward?

See comment #17. You never need approval for tests. http://wiki.mozilla.org/TreeStatus also says that tests can be checked-in without approval.

Comment 22

[cmtalbert]

(In reply to comment #21)
> (In reply to comment #20)
> > mconnor, can I carry your approval forward?
> 
> See comment #17. You never need approval for tests.
> http://wiki.mozilla.org/TreeStatus also says that tests can be checked-in
> without approval.
> 
Missed your earlier comment.  Thanks Reed.

Comment 23

[Dietrich Ayala (:dietrich)]

Comment on attachment 315161 [details] [diff] [review]
New Version of the test that won't turn boxes orange

>Index: toolkit/components/places/tests/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/tests/Makefile.in,v
>retrieving revision 1.7
>diff -u -8 -p -r1.7 Makefile.in
>--- toolkit/components/places/tests/Makefile.in	9 Apr 2008 00:54:25 -0000	1.7
>+++ toolkit/components/places/tests/Makefile.in	11 Apr 2008 19:35:20 -0000
>@@ -35,27 +35,38 @@
> # the terms of any one of the MPL, the GPL or the LGPL.
> #
> # ***** END LICENSE BLOCK *****
> 
> DEPTH		= ../../../..
> topsrcdir	= @top_srcdir@
> srcdir		= @srcdir@
> VPATH		= @srcdir@
>+relativesrcdir		= toolkit/components/places/tests

nit: line up values

>+     // Third test, use the iFrame and try loading directly
>+     var iframe = document.getElementById("iframe");
>+     iframe.onload = onloadHandler;
>+     startCallbackTimer();
>+     try {
>+       // This one probably won't throw but it will show the Error Dialog
>+       iframe.src = "place:sort=14&type=6&maxResults=10";

should probably fail here (ie: it doesn't throw)

probably don't need any of the ok(true, "") tests, just a comment.

Comment 24

[cmtalbert]

Attachment: New Patch addressing comments

Addresses Dietrich's comments and adds in a todo in order to fix bug 428585.  Carrying review forward.

Comment 25

[cmtalbert]

Checked in on trunk (again).

Comment 26

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv