ensure places uri schemes are not accessible via content

RESOLVED FIXED in Firefox 3

Status

()

Firefox
Bookmarks & History
P4
normal
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: dietrich, Assigned: cmtalbert)

Tracking

Trunk
Firefox 3
Points:
---
Bug Flags:
blocking-firefox3 -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:investigate])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

10 years ago
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
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?
(Reporter)

Updated

10 years ago
Blocks: 375898
Since this is a dependency of bug 375898, marking P2 so it stays on our radar for Fx3 triage/tracking.
Flags: blocking-firefox3?
Priority: -- → P2
Target Milestone: --- → Firefox 3 beta4
investigation for moz-anno seems complete (and safe). Where is the place: scheme implemented and used?
Whiteboard: [sg:investigate]
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: Firefox 3 beta4 → Firefox 3

Updated

10 years ago
Assignee: nobody → dietrich
(Assignee)

Comment 4

10 years ago
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.
Flags: in-testsuite?
(Reporter)

Updated

10 years ago
Whiteboard: [sg:investigate] → [sg:investigate][swag: 1d]
(Reporter)

Comment 5

10 years ago
(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.
Status: NEW → ASSIGNED
(Reporter)

Comment 6

10 years ago
(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.
(Reporter)

Comment 7

10 years ago
(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.
(Reporter)

Comment 8

10 years ago
Per comment #7, I'm decreasing priority of this bug, and turning it over to Clint for a mochitest (comment #4).
Assignee: dietrich → ctalbert
Status: ASSIGNED → NEW
Priority: P2 → P4
Whiteboard: [sg:investigate][swag: 1d] → [sg:investigate]

Updated

10 years ago
Flags: blocking-firefox3+ → blocking-firefox3-
(Assignee)

Comment 9

10 years ago
Created attachment 311722 [details] [diff] [review]
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
Attachment #311722 - Flags: review?(dietrich)
(Reporter)

Comment 10

10 years ago
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
Attachment #311722 - Flags: review?(dietrich)
(Assignee)

Comment 11

10 years ago
Created attachment 312053 [details] [diff] [review]
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.
Attachment #311722 - Attachment is obsolete: true
Attachment #312053 - Flags: review?(dietrich)
(Reporter)

Comment 12

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

thanks very much, r=me.
Attachment #312053 - Flags: review?(dietrich) → review+
(Reporter)

Comment 13

10 years ago
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
(Assignee)

Comment 14

10 years ago
Created attachment 315108 [details] [diff] [review]
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?
Attachment #312053 - Attachment is obsolete: true
Attachment #315108 - Flags: review+
Comment on attachment 315108 [details] [diff] [review]
A rebuilt patch -- same code

yay tests
Attachment #315108 - Flags: approval1.9+
(Assignee)

Comment 16

10 years ago
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
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
(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. :)
(Assignee)

Comment 18

10 years ago
Created attachment 315161 [details] [diff] [review]
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.
Attachment #315108 - Attachment is obsolete: true
Attachment #315161 - Flags: review?(dietrich)
(Assignee)

Comment 19

10 years ago
The original patch caused the boxes to go orange, so it was backed out and I have a new patch --> REOPENING.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 20

10 years ago
mconnor, can I carry your approval forward?
(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.
(Assignee)

Comment 22

10 years ago
(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.
(Reporter)

Comment 23

10 years ago
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.
Attachment #315161 - Flags: review?(dietrich) → review+
(Assignee)

Comment 24

10 years ago
Created attachment 315195 [details] [diff] [review]
New Patch addressing comments

Addresses Dietrich's comments and adds in a todo in order to fix bug 428585.  Carrying review forward.
Attachment #315161 - Attachment is obsolete: true
Attachment #315195 - Flags: review+
(Assignee)

Comment 25

10 years ago
Checked in on trunk (again).
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.