Last Comment Bug 374577 - Revise message CAPS policies; enable JS for non-message content by default; remove javascript.allow.mailnews hack from script security mgr
: Revise message CAPS policies; enable JS for non-message content by default; r...
Status: RESOLVED FIXED
[no l10n impact][target hard freeze]
: fixed1.9.1
Product: MailNews Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 3.0b2
Assigned To: Dan Mosedale (:dmose)
:
Mentors:
Depends on: 479906 479920
Blocks: 309258 309263 309267 309276 453928 458881
  Show dependency treegraph
 
Reported: 2007-03-19 23:29 PDT by Scott MacGregor
Modified: 2011-08-05 14:52 PDT (History)
23 users (show)
dmose: blocking‑thunderbird3+
dmose: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
first cut at a fix (5.21 KB, patch)
2007-03-19 23:47 PDT, Scott MacGregor
no flags Details | Diff | Review
first cut at implementing Neil's idea (25.13 KB, patch)
2007-03-29 15:30 PDT, Scott MacGregor
no flags Details | Diff | Review
additonal change to nsCookiePermission.cpp for seamonkey (1.67 KB, patch)
2007-03-29 15:32 PDT, Scott MacGregor
no flags Details | Diff | Review
updated patch based on feedback from Neil and Mark (25.96 KB, patch)
2007-04-24 11:03 PDT, Scott MacGregor
no flags Details | Diff | Review
forgot to include the editor change (716 bytes, patch)
2007-04-24 14:09 PDT, Scott MacGregor
no flags Details | Diff | Review
comm-central patch, v1 (25.19 KB, patch)
2008-11-20 16:26 PST, Dan Mosedale (:dmose)
no flags Details | Diff | Review
unbitrotted version of the scott's patches, mozilla-central portion (6.89 KB, patch)
2008-11-20 16:31 PST, Dan Mosedale (:dmose)
no flags Details | Diff | Review
option 2: comm-central patch, v1 (4.49 KB, patch)
2008-11-22 20:13 PST, Dan Mosedale (:dmose)
no flags Details | Diff | Review
option 2: mozilla-central patch, v1 (1.38 KB, patch)
2008-11-22 20:15 PST, Dan Mosedale (:dmose)
no flags Details | Diff | Review
option 2: comm-central patch, v2 (1.38 KB, patch)
2008-11-24 13:41 PST, Dan Mosedale (:dmose)
no flags Details | Diff | Review
option 2: comm-central patch, v2 (really!) (6.00 KB, patch)
2008-11-24 13:45 PST, Dan Mosedale (:dmose)
no flags Details | Diff | Review
option 2: comm-central patch, v3 (7.17 KB, patch)
2008-11-24 15:23 PST, Dan Mosedale (:dmose)
no flags Details | Diff | Review
option 2: mozilla-central patch, v2 (2.53 KB, patch)
2008-11-25 08:09 PST, Dan Mosedale (:dmose)
bzbarsky: review-
bzbarsky: superreview-
Details | Diff | Review
option 2: mozilla-central patch, v3 (7.11 KB, patch)
2008-11-25 10:59 PST, Dan Mosedale (:dmose)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Review
option 2: mozilla-central patch, v4 (7.11 KB, patch)
2008-11-26 11:35 PST, Dan Mosedale (:dmose)
dmose: review+
dmose: superreview+
Details | Diff | Review
option 2: comm-central patch, v4 (WIP) (22.01 KB, patch)
2008-11-27 15:50 PST, Dan Mosedale (:dmose)
no flags Details | Diff | Review
option 2: comm-central patch, v5 (WIP) (40.58 KB, patch)
2008-11-29 23:30 PST, Dan Mosedale (:dmose)
no flags Details | Diff | Review
option 2: comm-central patch, v6 (42.73 KB, patch)
2008-11-30 15:56 PST, Dan Mosedale (:dmose)
no flags Details | Diff | Review
option 2: comm-central patch, v7 (42.42 KB, patch)
2008-11-30 19:55 PST, Dan Mosedale (:dmose)
mozilla: review+
bzbarsky: superreview+
Details | Diff | Review
option 2: comm-central patch, v8 (42.50 KB, patch)
2009-01-23 19:46 PST, Dan Mosedale (:dmose)
dmose: review+
dmose: superreview+
Details | Diff | Review
option 2: mozilla-central patch, v5 (7.59 KB, patch)
2009-01-24 00:40 PST, Dan Mosedale (:dmose)
dmose: review+
dmose: superreview+
Details | Diff | Review
option 3: comm-central patch, v9 (33.78 KB, patch)
2009-02-11 18:10 PST, Dan Mosedale (:dmose)
no flags Details | Diff | Review
option 3: comm-central patch, v10 (33.97 KB, patch)
2009-02-11 21:59 PST, Dan Mosedale (:dmose)
no flags Details | Diff | Review
comm-central patch, v11 (29.30 KB, patch)
2009-02-13 14:56 PST, Dan Mosedale (:dmose)
mozilla: review+
Details | Diff | Review
mozilla-central patch, v6, merged forward (checked in on trunk and 1.9.1) (7.59 KB, patch)
2009-02-13 15:40 PST, Dan Mosedale (:dmose)
dmose: review+
dmose: superreview+
mbeltzner: approval1.9.1+
Details | Diff | Review
comm-central patch, v12 (variant of this patch checked in; see comments 67 and 68) (29.30 KB, patch)
2009-02-16 15:40 PST, Dan Mosedale (:dmose)
dmose: review+
bzbarsky: superreview+
Details | Diff | Review

Description Scott MacGregor 2007-03-19 23:29:24 PDT
Currently, the script security manager has mail specific code to support the javascript.allow.mailnews pref for the mailnews message pane.

A docshell supports the ability to enable/disable javascript directly (and that value is inherited by its children). We should set the allowJavascript attribute directly on the message pane docshell based on javascript.allow.mailnews.

This moves the logic to the application layer, instead of keeping it in the security manager giving mail applications more control.
Comment 1 Scott MacGregor 2007-03-19 23:47:47 PDT
Created attachment 259069 [details] [diff] [review]
first cut at a fix

I need to test this a bit more, but so far it's working well.

For seamonkey, we might want to use a pref observer since the suite still has UI for enabling / disabling JS in the message pane.
Comment 2 Scott MacGregor 2007-03-20 15:18:41 PDT
Comment on attachment 259069 [details] [diff] [review]
first cut at a fix

Neil/Karsten, are the two of you ok with pushing control of the mailnews javascript logic into the application, setting it directly on the message pane docshell instead of in the script security manager?
Comment 3 neil@parkwaycc.co.uk 2007-03-21 03:55:17 PDT
Comment on attachment 259069 [details] [diff] [review]
first cut at a fix

>+  msgPaneDocShell.allowAuth = false;
What's this change for?
Comment 4 Scott MacGregor 2007-03-22 13:13:05 PDT
(In reply to comment #3)
> (From update of attachment 259069 [details] [diff] [review])
> >+  msgPaneDocShell.allowAuth = false;
> What's this change for?
> 

Hi Neil,

that replaces this line:

messagepane.docShell.allowAuth = false;

since I'm using a local var for the msg pane docshell. 
Comment 5 neil@parkwaycc.co.uk 2007-03-24 09:25:37 PDT
(In reply to comment #1)
>For seamonkey, we might want to use a pref observer since the suite still has
>UI for enabling / disabling JS in the message pane.
That would be tricky because setAllowJavascript isn't recursive.
Comment 6 neil@parkwaycc.co.uk 2007-03-24 10:13:02 PDT
This is related to bug 374578, isn't it? You want to be able to have an arbitrary web page in a separate frame unrelated to the message. For instance, this would allow you to have a sidebar, but the problem is that all frames in a message window are treated as if they were a message, when you only want special behaviour for the message pane. I'm really beginning to like my "app type should be set on the message pane doc shell" idea. You could even turn the app type off for the start page or RSS pages. Who do I need to talk further about this to?
Comment 7 Scott MacGregor 2007-03-26 14:37:32 PDT
Hmm, if we go the app type route, would we change all the callers who get the app type from the root docshell and check to see if it's of type mail to instead, start with the current docshell and start walking up the parent tree looking for a parent docshell that's of type mail?  

i.e. fix up call sites like this one:

http://mxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.cpp#2561

with what the seamonkey cookie's permission manager does:

http://mxr.mozilla.org/seamonkey/source/extensions/cookie/nsCookiePermission.cpp#237
Comment 8 neil@parkwaycc.co.uk 2007-03-26 14:58:03 PDT
(In reply to comment #7)
>Hmm, if we go the app type route, would we change all the callers who get the
>app type from the root docshell and check to see if it's of type mail to
>instead, start with the current docshell and start walking up the parent tree
>looking for a parent docshell that's of type mail?  
That's one approach, or they could switch to GetSameTypeRootTreeItem instead.
Comment 9 Scott MacGregor 2007-03-28 14:36:52 PDT
(In reply to comment #5)
> (In reply to comment #1)
> >For seamonkey, we might want to use a pref observer since the suite still has
> >UI for enabling / disabling JS in the message pane.
> That would be tricky because setAllowJavascript isn't recursive.

Why do you need it to be recursive? If the user turns on JS, you can enable JS on the message pane docshell and then reload the message. Any subframes in the msg should then have JS run.

I'm trying to get a better feel for the advantages of adjusting the mail app type vs. using the javascript attribute on the message pane docshell & making the content policy manager look for the msg pane docshell. One advantage is we could easily allow remote images, JS and cookies for the start page url as you mentioned. Are there other advantages? Of course the downside is we have to make changes to platform call sites that get the app type from a docshell...
Comment 10 Scott MacGregor 2007-03-29 15:30:17 PDT
Created attachment 260063 [details] [diff] [review]
first cut at implementing Neil's idea

I haven't done a ton of testing on this yet, but so far so good.

This sets the app type on the message pane docshell and the editor docshell in mail compose for seamonkey and thunderbird. It fixes all callers of nsIDocShell::GetAppType that look for EDITOR or MAIL to use GetSameTypeRootTreeItem instead of GetRootTreeItem. 

There was some nice logic simplification in the content policy manager as a result of this change.

Furthermore, for thunderbird only (let me know if you want the change for the suite), set the app type to unknown on the docshell when loading the start page so we'll support JS/cookies/and remote images for the start page instead of trying to special case that in the mail content policy manager.
Comment 11 Scott MacGregor 2007-03-29 15:32:23 PDT
Created attachment 260065 [details] [diff] [review]
additonal change to nsCookiePermission.cpp for seamonkey

I forgot to include this change when I made my diff.
Comment 12 Scott MacGregor 2007-04-03 16:34:52 PDT
Comment on attachment 259069 [details] [diff] [review]
first cut at a fix

obsoleting this patch
Comment 13 Scott MacGregor 2007-04-03 16:38:14 PDT
Comment on attachment 260063 [details] [diff] [review]
first cut at implementing Neil's idea

Neil, is this roughly along the lines of what you had in mind?
Comment 14 Mark Banner (:standard8) 2007-04-04 03:31:25 PDT
I've just been looking at bug 376460 - some warnings that this bug will go part way to fixing however:

 #ifndef MOZ_THUNDERBIRD
...
   PRUint32 appType;
-  rv = docshell->GetAppType(&appType);
+  rv = GetAppTypeForContext(aRequestingContext, &appType);
   // We only want to deal with mailnews
   if (NS_FAILED(rv) || appType != nsIDocShell::APP_TYPE_MAIL)
     return NS_OK;
...

-nsresult nsMsgContentPolicy::GetRootDocShellForContext(nsISupports * aRequestingContext, nsIDocShell ** aDocShell)
+nsresult nsMsgContentPolicy::GetAppTypeForContext(nsISupports * aRequestingContext, PRUint32 * aAppType)
 {
   NS_ENSURE_ARG_POINTER(aRequestingContext);


This will still throw a warning for SeaMonkey (rather than two previously) because aRequestingContext may be null according to the nsIContentPolicy::shouldLoad documentation, and it certainly seems to be frequently where the content type is a style sheet.

I'm not sure what happens in the Thunderbird case as GetAppTypeForContext ends up being called a lot later after other things have been resolved.
Comment 15 neil@parkwaycc.co.uk 2007-04-04 03:31:40 PDT
Comment on attachment 260063 [details] [diff] [review]
first cut at implementing Neil's idea

> #ifndef MOZ_THUNDERBIRD
>   // Go find out if we are dealing with mailnews. Anything else
>   // isn't our concern and we accept content.
>-  nsCOMPtr<nsIDocShell> docshell;
>-  rv = GetRootDocShellForContext(aRequestingContext, getter_AddRefs(docshell));
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>   PRUint32 appType;
>-  rv = docshell->GetAppType(&appType);
>+  rv = GetAppTypeForContext(aRequestingContext, &appType);
>   // We only want to deal with mailnews
>   if (NS_FAILED(rv) || appType != nsIDocShell::APP_TYPE_MAIL)
>     return NS_OK;

>+  PRUint32 appType;
>+  rv = GetAppTypeForContext(aRequestingContext, &appType);
>+  NS_ENSURE_SUCCESS(rv, rv);
> 
>+  if (appType == nsIDocShell::APP_TYPE_EDITOR)
>+    ComposeShouldLoad(aRequestingContext, aContentLocation, aDecision);
>+  else if (appType == nsIDocShell::APP_TYPE_MAIL)
>+    MailShouldLoad(aRequestingLocation, aContentLocation, aDecision);
>   else
>+    *aDecision = nsIContentPolicy::ACCEPT;
Are we duplicating something here, or are the intervening checks going to break non-mail (e.g. web page editor) windows in SeaMonkey?

>+  NS_ENSURE_ARG_POINTER(aRequestingContext);
I think this is allowed to be null, maybe we should special case this earlier?

>   if (isRSS)
>     *aResult = ACCESS_DEFAULT;
>   }
>-
>+  else
>+    *aResult = ACCESS_DEFAULT;
This looks wrong...

>+      editorElement.docShell.appType = Components.interfaces.nsIDocShell.APP_TYPE_EDITOR;
I think that this belongs in EditorSharedStartup or possibly <editor> XBL?
Comment 16 neil@parkwaycc.co.uk 2007-04-15 09:01:11 PDT
(In reply to comment #15)
>>   if (isRSS)
>>     *aResult = ACCESS_DEFAULT;
>>   }
>>-
>>+  else
>>+    *aResult = ACCESS_DEFAULT;
>This looks wrong...
...but as Scott pointed out on AIM it isn't really, the illusion is caused by the misindentation inside the previous block.
Comment 17 Scott MacGregor 2007-04-24 11:03:24 PDT
Created attachment 262662 [details] [diff] [review]
updated patch based on feedback from Neil and Mark

I've updated the patch based on the feedback from Neil and Mark so far.

I'm still worried about this check for seamonkey:

#ifndef MOZ_THUNDERBIRD
  if (NS_FAILED(rv) || appType != nsIDocShell::APP_TYPE_MAIL)
    return NS_OK;
#endif

for starters, we want seamonkey and thunderbird to see APP_TYPE_EDITOR and APP_TYPE_MAIL. I'm wondering if we can remove this check all together and fall back to the logic used later on in this method where we look at the app type and call the appropriate handler (mail or compose) otherwise we accept the content.
Comment 18 Scott MacGregor 2007-04-24 14:09:14 PDT
Created attachment 262685 [details] [diff] [review]
forgot to include the editor change
Comment 19 neil@parkwaycc.co.uk 2007-05-01 05:23:00 PDT
(In reply to comment #17)
> I'm still worried about this check for seamonkey:
> 
> #ifndef MOZ_THUNDERBIRD
>   if (NS_FAILED(rv) || appType != nsIDocShell::APP_TYPE_MAIL)
>     return NS_OK;
> #endif
You would have to tweak the plugin blocker, although I'm not sure how yet.
Comment 20 bugzilla 2008-10-08 14:53:18 PDT
Adding to CC
Comment 21 Dan Mosedale (:dmose) 2008-11-20 16:26:24 PST
Created attachment 349308 [details] [diff] [review]
comm-central patch, v1

Unbitted version of the comm-central portion of the patches that Scott wrote.
Comment 22 Dan Mosedale (:dmose) 2008-11-20 16:31:42 PST
Created attachment 349309 [details] [diff] [review]
unbitrotted version of the scott's patches, mozilla-central portion
Comment 23 Dan Mosedale (:dmose) 2008-11-22 20:13:46 PST
Created attachment 349612 [details] [diff] [review]
option 2: comm-central patch, v1

This, I think, is a better way to excise the mailnews specific glop from nsScriptSecurityManager and put the relevant policy code into nsMsgContentPolicy.  Assuming this code can be made bug-free, we get a backstop that should cover all content docshells in APP_TYPE_MAIL docshell trees.  This version of the patch doesn't actually have usable policy code yet; it's just a proof-of-concept that I'd like feedback on from some combination of bz, Neil, and bienvenu.  Does this seem like a reasonable way to go?

The YYY comments are notes to myself about things that need to be addressed before this is actually done.
Comment 24 Dan Mosedale (:dmose) 2008-11-22 20:15:05 PST
Created attachment 349614 [details] [diff] [review]
option 2: mozilla-central patch, v1
Comment 25 Dan Mosedale (:dmose) 2008-11-23 22:13:04 PST
Oddly, the above patch (after a bit of tweaking to compile) doesn't trigger when I would expect: every call to nsMsgContentPolicy::ShouldLoad with a TYPE_DOCUMENT descends into TweakDocumentPolicySettings, but then the itemType returned by every single docshell is typeChrome (0).  Still investigating...
Comment 26 Dan Mosedale (:dmose) 2008-11-24 13:41:50 PST
Created attachment 349823 [details] [diff] [review]
option 2: comm-central patch, v2

Work in progress patch; still has issues...
Comment 27 Dan Mosedale (:dmose) 2008-11-24 13:45:04 PST
Created attachment 349826 [details] [diff] [review]
option 2: comm-central patch, v2 (really!)
Comment 28 Dan Mosedale (:dmose) 2008-11-24 15:23:59 PST
Created attachment 349853 [details] [diff] [review]
option 2: comm-central patch, v3

Now, with extra "it actually works".
Comment 29 Dan Mosedale (:dmose) 2008-11-25 08:09:26 PST
Created attachment 349971 [details] [diff] [review]
option 2: mozilla-central patch, v2

Here's the mozilla-central portion of this patch; it will want to land at the same time the comm-central portion does.  There are no tests with this piece, because it's just not plausible that this functionality will somehow creep back into gecko while noone's looking.  I do hope to have tests ready with the comm-central piece, however.
Comment 30 Boris Zbarsky [:bz] (Out June 25-July 6) 2008-11-25 09:30:28 PST
Comment on attachment 349971 [details] [diff] [review]
option 2: mozilla-central patch, v2

>+++ b/caps/src/nsScriptSecurityManager.cpp

Is mIsMailJavaScriptEnabled used for anything after this point?  If not, can it just be removed altogether?

>+++ b/content/base/public/nsContentPolicyUtils.h
>+ * @note  As of this writing, aContext generally points to an element, even for
>+ * nsIContentPolicy::TYPE_DOCUMENT, meaning that when (e.g.)
>+ * nsIContentPolicy::ShouldLoad() is called for the content document
>+ * of a XUL <browser>, calling NS_CP_GetDocShellFromContext on that
>+ * aContext will return the docshell containing the XUL element, not the one
>+ * contained by it.

No, this is false.  What would be true is that specifically for TYPE_DOCUMENT and TYPE_SUBDOCUMENT aContext either points to the frameElement of the window the load is happening in (and in that case NS_CP_GetDocShellFromContext will return the parent of the docshell the load is happening in), or points to the window the load is happening in (and in that case NS_CP_GetDocShellFromContext will return the docshell the load is happening in).  It's up to callers to QI aContext and handle things accordingly if they want the docshell the load is happening in.

It might also be worth a followup bug to decide whether this is the behavior NS_CP_GetDocShellFromContext should have for DOCUMENT and SUBDOCUMENT.
Comment 31 Dan Mosedale (:dmose) 2008-11-25 10:59:36 PST
Created attachment 350005 [details] [diff] [review]
option 2: mozilla-central patch, v3

Bug 466687 has been filed; the other comments have been addressed, and for bonus points, I got rid of the static string containing the mail preference name too.
Comment 32 Boris Zbarsky [:bz] (Out June 25-July 6) 2008-11-25 11:36:16 PST
Comment on attachment 350005 [details] [diff] [review]
option 2: mozilla-central patch, v3

Lose the stray '(' in the comment, and looks good.
Comment 33 Dan Mosedale (:dmose) 2008-11-26 11:35:46 PST
Created attachment 350191 [details] [diff] [review]
option 2: mozilla-central patch, v4

Excised the spare parens.  Carrying forward r+sr.
Comment 34 Dan Mosedale (:dmose) 2008-11-27 15:50:55 PST
Created attachment 350405 [details] [diff] [review]
option 2: comm-central patch, v4 (WIP)

Work-in-progress patch, not yet ready for review.  This is mostly working, the main things left to do are:

* implement MsgGetOrigin.  I'm currently pondering exactly how best to frame this (i.e. what does it mean when a message of one type gets copied/moved to another server).

* clean up, document, and otherwise prep for review

* finish the tests.

My current thinking is to do the first two things, get it reviewed, land it so that it can start baking, and the do the test stuff.

Comments welcome / appreciated.
Comment 35 Dan Mosedale (:dmose) 2008-11-29 23:30:40 PST
Created attachment 350634 [details] [diff] [review]
option 2: comm-central patch, v5 (WIP)

Almost there!  Cleanups are done, it's working for feed summaries, feed-summarized web pages, and nntp articles.  Just need to whip mail messages into shape in the morning, and then it will be ready for review.
Comment 36 Dan Mosedale (:dmose) 2008-11-30 15:56:11 PST
Created attachment 350695 [details] [diff] [review]
option 2: comm-central patch, v6

OK, this seems to do the trick.  There are a few XXX comments that I'm a bit uncomfortable with, and if either of the reviewers cares to propose what to do about them, that'd be fantastic.  The test isn't done yet, so just ignore that for now.  I propose reviewing and landing the rest so that it can start baking for 3.0b1 immediately, and I'll work on the test over next few days while that baking is happening.
Comment 37 neil@parkwaycc.co.uk 2008-11-30 16:09:18 PST
Comment on attachment 350695 [details] [diff] [review]
option 2: comm-central patch, v6

>+    // remove the content policy listener so it doesn't leak
There are good reasons to remove it but leaking isn't one of them ;-)

>+pref("mailnews.messsage_display.allowjavascript.mail", false);
>+pref("mailnews.messsage_display.allowjavascript.nntp", false);
I would have thought "news" would have been more appropriate.
Comment 38 David :Bienvenu 2008-11-30 16:17:36 PST
Comment on attachment 350695 [details] [diff] [review]
option 2: comm-central patch, v6

You're going to want to add an NS_ENSURE_ARG_POINTER(aMsgHdr), or allow caller to pass in null.

+nsMsgLocalMailFolder::AddMessage(const char *aMessage, nsIMsgDBHdr **aMsgHdr)


Please use get/setStringProperty, not get/setProperty - there's no need for unicode origins, and ascii takes up less room in the mork file, and it's less code anyway :-)
Comment 39 David :Bienvenu 2008-11-30 16:23:07 PST
Did we decide we're not going to put defaults for these prefs in mailnews.js? I think if we might ever turn on the rss one by default, we'd need it in mailnews.js
Comment 40 Dan Mosedale (:dmose) 2008-11-30 16:51:52 PST
(In reply to comment #37)
> (From update of attachment 350695 [details] [diff] [review])
> >+    // remove the content policy listener so it doesn't leak
> There are good reasons to remove it but leaking isn't one of them ;-)

Why doesn't leaking matter -- because it's a service?  And what are the good reasons that you're referring to?

> >+pref("mailnews.messsage_display.allowjavascript.mail", false);
> >+pref("mailnews.messsage_display.allowjavascript.nntp", false);
> I would have thought "news" would have been more appropriate.

I initially chose "news", but decided against it because enough people refer to feeds as "news" that it seemed like it would add extra confusion.

(In reply to comment #38)
> (From update of attachment 350695 [details] [diff] [review])
> You're going to want to add an NS_ENSURE_ARG_POINTER(aMsgHdr), or allow caller
> to pass in null.
> 
> +nsMsgLocalMailFolder::AddMessage(const char *aMessage, nsIMsgDBHdr **aMsgHdr)

Fixed to allow the caller to pass in null. 

> Please use get/setStringProperty, not get/setProperty - there's no need for
> unicode origins, and ascii takes up less room in the mork file, and it's less
> code anyway :-)

Both instances fixed.

(In reply to comment #39)
> Did we decide we're not going to put defaults for these prefs in
> mailnews.js? I think if we might ever turn on the rss one by default, 
> we'd need it in mailnews.js

I don't recall deciding that, and the patch already has all the prefs added to mailnews.js.  Am I missing something?
Comment 41 David :Bienvenu 2008-11-30 16:53:39 PST
> I don't recall deciding that, and the patch already has all the prefs added to
> mailnews.js.  Am I missing something?

ah, yes, missed that.
Comment 42 David :Bienvenu 2008-11-30 17:03:39 PST
>+  // XXX If we error out here instead of returning NS_OK, clicking on the
>+  // "create accounts" link fails, which is why we're returning NS_OK.
>+  // Could returning NS_OK cause any security issues?

This scares me, both for any uknown security issues, but also are there other regressions in url running that we haven't discovered yet? E.g., clicking on the partial pop3 download link...
Comment 43 David :Bienvenu 2008-11-30 17:17:54 PST
I'm building with this patch now.
Comment 44 Dan Mosedale (:dmose) 2008-11-30 19:54:47 PST
(In reply to comment #42)
> >+  // XXX If we error out here instead of returning NS_OK, clicking on the
> >+  // "create accounts" link fails, which is why we're returning NS_OK.
> >+  // Could returning NS_OK cause any security issues?
> 
> This scares me, both for any uknown security issues, but also are there other
> regressions in url running that we haven't discovered yet? E.g., clicking on
> the partial pop3 download link...

It turns out the problem described in the comments was a bug in an earlier iteration of the patch where I was incorrectly QIing to nsIMsgMailNewsUrl rather than nsIMsgMessageUrl.  So I put back the (more conservative from a security point of view) NS_ENSURE_SUCCESS(rv, rv), and things seem to be working just fine for the "create accounts" link, at least.
Comment 45 Dan Mosedale (:dmose) 2008-11-30 19:55:52 PST
Created attachment 350709 [details] [diff] [review]
option 2: comm-central patch, v7
Comment 46 David :Bienvenu 2008-11-30 21:44:13 PST
Comment on attachment 350709 [details] [diff] [review]
option 2: comm-central patch, v7

this looks OK - I've run with it briefly and not seen any problems. I haven't done a lot of testing, though, so I would be OK with it slipping to b2 if we don't want to rush it.
Comment 47 neil@parkwaycc.co.uk 2008-12-01 02:09:01 PST
(In reply to comment #40)
> (In reply to comment #37)
> > (From update of attachment 350695 [details] [diff] [review])
> > >+    // remove the content policy listener so it doesn't leak
> > There are good reasons to remove it but leaking isn't one of them ;-)
> Why doesn't leaking matter -- because it's a service?
Oh, that too, but the real reason was because it's a weak reference.

> And what are the good reasons that you're referring to?
No point having it listen for loads we're uninterested in, for instance.
Comment 48 Boris Zbarsky [:bz] (Out June 25-July 6) 2008-12-01 16:43:52 PST
Comment on attachment 350709 [details] [diff] [review]
option 2: comm-central patch, v7

>+++ b/mailnews/base/src/nsMsgContentPolicy.cpp
>+  case nsIContentPolicy::TYPE_DOCUMENT:

I assume that this means that we'll never want different "js enabled" settings for a content docshell and a subframe in that content docshell?  That seems fine to me; just making sure that's the goal.  Might be worth commenting that here so that people don't wonder why TYPE_SUBDOCUMENT is absent.

It's also worth noting that an <object> pointing to HTML will come through here as TYPE_OBJECT, and through ShouldProcess as TYPE_SUBDOCUMENT.  Since you're doin nothing interesting with that type, that's OK.  I think in general chrome that uses an <object> to untrusted content (and in this case it would even need to be mailnews content) has a serious problem, so the <object> handling here is fine.

>+nsresult nsMsgContentPolicy::DisableJSOnMailNewsUrlDocshells(
>+    if (!aRequestingContext) {
>+    return NS_OK;
>+  }

Fix the indent?

>+  // since NS_CP_GetDocShellFromContext returns the containing docshell rather
>+  // than the contained one we need, we can't use that here, so...
>+  
>+  nsCOMPtr<nsIFrameLoaderOwner> flOwner = do_QueryInterface(aRequestingContext,
>+                                                            &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);

Hmm.  I guess that's the safe thing to do, since no one should be loading mailneews URIS in a root docshell, right?  If that _is_ allowed, then the right thing to do here is to try QIing the context to a window if QI to frame loader owner fails....

>+  nsCOMPtr<nsIFrameLoader> frameLoader;
>+  rv = flOwner->GetFrameLoader(getter_AddRefs(frameLoader));
>+  NS_ENSURE_SUCCESS(rv, rv);

You probably want to null-check frameLoader too, since nothing obviously guarantees that it's non-null.

>+  // we're only worried about policy settings in content docshells
>+  // XXX should we be caring about typeContentWrapper as well?

No.  An actual docshell should never have that type.  Only treeitems that aren't docshells will have it.

>+nsMsgContentPolicy::OnLocationChange(nsIWebProgress *aWebProgress,
>+                                     nsIRequest *aRequest, nsIURI *aLocation)
>+  // Work our way down to the docshell.

Just QI aWebProgress?  That should be the right thing, I would think.  If you care, you could assert equality of that and the thing gotten from the channel, I guess.

>+++ b/mailnews/base/src/nsMsgWindow.cpp
>@@ -137,16 +138,29 @@ NS_IMETHODIMP nsMsgWindow::CloseWindow()
>   nsCOMPtr<nsIDocShell> rootShell(do_QueryReferent(mRootDocShellWeak));
>   if(rootShell)

Would it make sense to SetRootDocShell(nsnull) here and avoid code duplication?

> NS_IMETHODIMP nsMsgWindow::SetRootDocShell(nsIDocShell * aDocShell)
>+    rv = docShellProgress->AddProgressListener(contentPolicyListener,
>+                                              nsIWebProgress::NOTIFY_LOCATION);

Fix the weird indent?

sr=bzbarsky with the above nits picked.
Comment 49 David :Bienvenu 2008-12-02 07:38:44 PST
moving to b2
Comment 50 David :Bienvenu 2009-01-21 21:16:21 PST
updating whiteboard
Comment 51 Dan Mosedale (:dmose) 2009-01-23 19:46:24 PST
Created attachment 358539 [details] [diff] [review]
option 2: comm-central patch, v8

Updated patch to the comm-central trunk; carrying forward review & super-review.
Comment 52 Dan Mosedale (:dmose) 2009-01-24 00:40:16 PST
Created attachment 358567 [details] [diff] [review]
option 2: mozilla-central patch, v5

Patch updated to trunk.
Comment 53 Dan Mosedale (:dmose) 2009-02-11 17:51:32 PST
(In reply to comment #48)
> >+  // since NS_CP_GetDocShellFromContext returns the containing docshell rather
> >+  // than the contained one we need, we can't use that here, so...
> >+  
> >+  nsCOMPtr<nsIFrameLoaderOwner> flOwner = do_QueryInterface(aRequestingContext,
> >+                                                            &rv);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> 
> Hmm.  I guess that's the safe thing to do, since no one should be loading
> mailneews URIS in a root docshell, right? 

Indeed; if someone figures out a use case for that that we haven't thought of, they're welcome to file an enhancement request.

All other comments addressed in the forthcoming patch.
Comment 54 Dan Mosedale (:dmose) 2009-02-11 18:10:29 PST
Created attachment 361909 [details] [diff] [review]
option 3: comm-central patch, v9

Different than its predecessors, because this version allows JS in non-message and content, and disallows JS in message content.  There are no longer prefs to allow JS in message content, as per the reasoning described in bug 453928.  There is one possible belt-and-suspenders issue that I discovered while testing (I suspect it was there all along).  I'll describe that in a subsequent comment and then request review & advice from the reviewers...
Comment 55 Dan Mosedale (:dmose) 2009-02-11 21:59:12 PST
Created attachment 361941 [details] [diff] [review]
option 3: comm-central patch, v10

Turns out the belt-and-suspenders issue I thought I saw was a misunderstanding.  Added comments to make the behavior more clear.
Comment 56 David :Bienvenu 2009-02-12 07:35:07 PST
Comment on attachment 361941 [details] [diff] [review]
option 3: comm-central patch, v10

   // this adds a message to the end of the folder, parsing it as it goes, and
   // applying filters, if applicable.
 NS_IMETHODIMP
-nsMsgLocalMailFolder::AddMessage(const char *aMessage)
+nsMsgLocalMailFolder::AddMessage(const char *aMessage, nsIMsgDBHdr **aMsgHdr)

One thing a filter can do is move the message to an other folder, which would mean that the header returned is bogus. You don't care for your purposes, but this introduces a snake in the grass waiting for some unsuspecting caller ;-)
Comment 57 David :Bienvenu 2009-02-12 10:38:44 PST
I'm not sure how you want to handle the AddMessage part - you could either change your test case to find the newly added message by message-id using the msg db(which would  mean remembering the message id in a local var), or make sure AddMessage does the right thing in the case of message filters, or at the very least, making sure it returns null if the message was filtered, and documenting it in the idl...
Comment 58 David :Bienvenu 2009-02-12 10:51:04 PST
Comment on attachment 361941 [details] [diff] [review]
option 3: comm-central patch, v10

+   */
+  nsIMsgDbHdr addMessage(in string aMessage);
+

this should be nsIMsgDBHdr...
Comment 59 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-02-13 06:49:26 PST
dmose, do you mind summarizing what's changed since the v7 version I reviewed?
Comment 60 Dan Mosedale (:dmose) 2009-02-13 12:28:53 PST
bz: 

* I've entirely ripped out the "firstSeenContext" glop, the policy is now much simpler: messages are not allowed to execute JS at all, other loads are always allowed to (presumably modulo any higher level controls enforced by gecko or other code like somebody turning off JS in preferences entirely and having that enforced by the script security manager).

* I've added and improved some comments.

* I've addressed your previous review comments as per comment 53.

* There are now working tests.
 
I still need to fix the thing bienvenu pointed out, but that should be trivial and shouldn't impact your review in any interesting way, so I think it would be entirely reasonable for you to go forward with reviewing this version.

bienvenu: those sound like good suggestions; I'll make one of them work.  New patch forthcoming in a bit.
Comment 61 Dan Mosedale (:dmose) 2009-02-13 14:56:13 PST
Created attachment 362314 [details] [diff] [review]
comm-central patch, v11

Removed the AddMessage changes; they clearly need a bit more thought and probably want some unit tests.  Now using the database to get the message by id.
Comment 62 Dan Mosedale (:dmose) 2009-02-13 15:07:58 PST
Note for reviewers w.r.t. prioritizing this review: it would be a fine thing to be able to land this patch today on the trunk, and then request approval for branch on Monday (branch will close Tuesday 23:59 for Thunderbird changes).  Sorry for the short notice.
Comment 63 Dan Mosedale (:dmose) 2009-02-13 15:40:12 PST
Created attachment 362328 [details] [diff] [review]
mozilla-central patch, v6, merged forward (checked in on trunk and 1.9.1)
Comment 64 Dan Mosedale (:dmose) 2009-02-13 15:42:45 PST
Comment on attachment 362328 [details] [diff] [review]
mozilla-central patch, v6, merged forward (checked in on trunk and 1.9.1)

Carrying forward r+ and sr+.
Comment 65 David :Bienvenu 2009-02-15 17:10:42 PST
Comment on attachment 362314 [details] [diff] [review]
comm-central patch, v11

no need for the uuidgen temp var:

+  let uuidGen = Components.classes["@mozilla.org/uuid-generator;1"]
+                          .getService(Components.interfaces.nsIUUIDGenerator);
+
+  let msgId = uuidGen.generateUUID() + "@mozillamessaging.com";
Comment 66 Dan Mosedale (:dmose) 2009-02-16 15:40:40 PST
Created attachment 362635 [details] [diff] [review]
comm-central patch, v12 (variant of this patch checked in; see comments 67 and 68)

A fair point.  Addressed; carried for r+.
Comment 67 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-02-17 18:34:20 PST
Comment on attachment 362635 [details] [diff] [review]
comm-central patch, v12 (variant of this patch checked in; see comments 67 and 68)

>+++ b/mailnews/base/src/nsMsgContentPolicy.cpp
>+  // If this is a mailnews url, turn off JavaScript
>+  nsCOMPtr<nsIMsgMessageUrl> messageUrl = do_QueryInterface(aLocation, &rv);
>+  if (NS_SUCCEEDED(rv)) {
>+    return docShell->SetAllowJavascript(PR_FALSE);
>+  }
>+
>+  // any other kind of URL gets JavaScript
>+  return docShell->SetAllowJavascript(PR_TRUE);

Could just:

  return docShell->SetAllowJavascript(NS_FAILED(rv));

after the QI, right?

With that, looks great.  Sorry for the review lag...
Comment 68 Dan Mosedale (:dmose) 2009-02-17 20:41:28 PST
bz: checked in a variant of the patch with your proposed change to comm-central.
Comment 69 Mike Beltzner [:beltzner, not reading bugmail] 2009-02-17 21:17:34 PST
Comment on attachment 362328 [details] [diff] [review]
mozilla-central patch, v6, merged forward (checked in on trunk and 1.9.1)

a191=beltzner
Comment 70 Phil Ringnalda (:philor) 2009-02-17 22:10:40 PST
Comment on attachment 362328 [details] [diff] [review]
mozilla-central patch, v6, merged forward (checked in on trunk and 1.9.1)

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c50edb4ed66c
Comment 71 Dan Mosedale (:dmose) 2009-02-18 15:31:44 PST
Adding in-testsuite?, since the test here will need to be wired into a MozMill-based automated suite, once that exists.
Comment 72 Kent James (:rkent) 2009-02-20 13:52:29 PST
I'm getting a startup crash in a debug build that includes this patch. Tracing, the crash is at this statement in nsMsgContentPolicy:

      NS_QueryNotificationCallbacks(channel, docShell2);

where channel is zero. GOing back through the call stack, channel QIs from aReference, which was set to null in nsDocShell::CreateAboutBlankContentViewer at this call:

SetCurrentURI(blankDoc->GetDocumentURI(), nsnull, PR_TRUE);

This is a potential b2 blocker. I am unfortunately travelling and can only be of limited assistance at the moment.
Comment 73 Dan Mosedale (:dmose) 2009-02-20 14:01:32 PST
Not a b2 blocker because this is a correctness check in #ifdef DEBUG code; I'll spin this off into a new bug.
Comment 74 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-02-23 13:44:08 PST
Hmm.  NS_QueryNotificationCallbacks is not null-safe, so we just need a null-check here.
Comment 75 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2009-06-04 12:54:12 PDT
(In reply to comment #71)
> Adding in-testsuite?, since the test here will need to be wired into a
> MozMill-based automated suite, once that exists.

What is the test?
Comment 77 neil@parkwaycc.co.uk 2009-08-06 02:07:29 PDT
So, what happened to the javascript.allow.mailnews pref?
Comment 78 Dan Mosedale (:dmose) 2009-08-06 16:18:40 PDT
It's gone; there are no plans for it to return.
Comment 79 neil@parkwaycc.co.uk 2009-08-07 01:10:44 PDT
(In reply to comment #78)
> It's gone; there are no plans for it to return.
Where was this announced? Someone's already posted in m.d.seamonkey wondering what happened to it. Also it would be nice if someone had told us so that we could remove the now-useless preference from our UI!
Comment 80 Mark Banner (:standard8) 2009-08-07 01:40:51 PDT
(In reply to comment #79)
> (In reply to comment #78)
> > It's gone; there are no plans for it to return.
> Where was this announced? Someone's already posted in m.d.seamonkey wondering
> what happened to it. Also it would be nice if someone had told us so that we
> could remove the now-useless preference from our UI!

mda.thunderbird; message thread titled "JavaScript in messages for Tb3" starting on 30th Jan.
Comment 81 Serge Gautherie (:sgautherie) 2009-10-09 08:07:14 PDT
Comment on attachment 362635 [details] [diff] [review]
comm-central patch, v12 (variant of this patch checked in; see comments 67 and 68)


>diff --git a/mail/test/mozmill/test-msg-content-policy.js b/mail/test/mozmill/test-msg-content-policy.js
>@@ -0,0 +1,216 @@
>+'<script language="javascript"/>\n'+

I assume this should be |type="application/javascript"|.
Can you fix it? Thanks.
Comment 82 Dan Mosedale (:dmose) 2009-10-09 10:02:59 PDT
Please file a new bug; this is already resolved.

Note You need to log in before you can comment on or make changes to this bug.