Closed Bug 374577 Opened 17 years ago Closed 15 years ago

Revise message CAPS policies; enable JS for non-message content by default; remove javascript.allow.mailnews hack from script security mgr

Categories

(MailNews Core :: Security, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b2

People

(Reporter: mscott, Assigned: dmosedale)

References

Details

(Keywords: fixed1.9.1, Whiteboard: [no l10n impact][target hard freeze])

Attachments

(2 files, 24 obsolete files)

7.59 KB, patch
dmosedale
: review+
dmosedale
: superreview+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
29.30 KB, patch
dmosedale
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
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.
Attached patch first cut at a fix (obsolete) — Splinter Review
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.
OS: Windows XP → All
Hardware: PC → All
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?
Attachment #259069 - Flags: review?(neil)
Comment on attachment 259069 [details] [diff] [review]
first cut at a fix

>+  msgPaneDocShell.allowAuth = false;
What's this change for?
(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. 
(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.
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?
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
(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.
(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...
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.
I forgot to include this change when I made my diff.
Comment on attachment 259069 [details] [diff] [review]
first cut at a fix

obsoleting this patch
Attachment #259069 - Attachment is obsolete: true
Attachment #259069 - Flags: review?(neil)
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?
Attachment #260063 - Flags: review?(neil)
Blocks: 376460
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 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?
(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.
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.
Attachment #260063 - Attachment is obsolete: true
Attachment #260065 - Attachment is obsolete: true
Attachment #262662 - Flags: review?(neil)
Attachment #260063 - Flags: review?(neil)
(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.
Assignee: mscott → nobody
Product: Core → MailNews Core
Adding to CC
Assignee: nobody → dmose
Flags: blocking-thunderbird3+
Target Milestone: --- → Thunderbird 3.0b1
Attachment #262662 - Attachment is obsolete: true
Attachment #262662 - Flags: review?(neil)
Attachment #262685 - Attachment is obsolete: true
Attached patch comm-central patch, v1 (obsolete) — Splinter Review
Unbitted version of the comm-central portion of the patches that Scott wrote.
Attached patch option 2: comm-central patch, v1 (obsolete) — Splinter Review
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.
Whiteboard: [has prototype patch; needs feedback, more work]
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...
Attached patch option 2: comm-central patch, v2 (obsolete) — Splinter Review
Work in progress patch; still has issues...
Attachment #349612 - Attachment is obsolete: true
Attachment #349823 - Attachment is obsolete: true
Attached patch option 2: comm-central patch, v3 (obsolete) — Splinter Review
Now, with extra "it actually works".
Attachment #349826 - Attachment is obsolete: true
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.
Attachment #349308 - Attachment is obsolete: true
Attachment #349309 - Attachment is obsolete: true
Attachment #349614 - Attachment is obsolete: true
Attachment #349971 - Flags: superreview?(bzbarsky)
Attachment #349971 - Flags: review?(bzbarsky)
Whiteboard: [has prototype patch; needs feedback, more work] → [mc patch awaiting review; cc patch in progress]
Attachment #349971 - Flags: superreview?(bzbarsky)
Attachment #349971 - Flags: superreview-
Attachment #349971 - Flags: review?(bzbarsky)
Attachment #349971 - Flags: review-
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.
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.
Attachment #349971 - Attachment is obsolete: true
Attachment #350005 - Flags: superreview?(bzbarsky)
Attachment #350005 - Flags: review?(bzbarsky)
Comment on attachment 350005 [details] [diff] [review]
option 2: mozilla-central patch, v3

Lose the stray '(' in the comment, and looks good.
Attachment #350005 - Flags: superreview?(bzbarsky)
Attachment #350005 - Flags: superreview+
Attachment #350005 - Flags: review?(bzbarsky)
Attachment #350005 - Flags: review+
Excised the spare parens.  Carrying forward r+sr.
Attachment #350005 - Attachment is obsolete: true
Attachment #350191 - Flags: superreview+
Attachment #350191 - Flags: review+
Whiteboard: [mc patch awaiting review; cc patch in progress] → [mc patch awaiting review; will post cc patch iteration thurs PM]
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.
Attachment #349853 - Attachment is obsolete: true
Whiteboard: [mc patch awaiting review; will post cc patch iteration thurs PM] → [mc patch awaiting review; has cc patch iteration; next (hopefully review-ready) iteration Thurs PM or Friday]
Whiteboard: [mc patch awaiting review; has cc patch iteration; next (hopefully review-ready) iteration Thurs PM or Friday] → [mc patch ready to land; has cc patch iteration; next (hopefully review-ready) iteration Thurs PM or Friday]
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.
Attachment #350405 - Attachment is obsolete: true
Whiteboard: [mc patch ready to land; has cc patch iteration; next (hopefully review-ready) iteration Thurs PM or Friday] → [mc patch ready to land; has cc patch iteration; review-ready iteration ETA Sun aft]
Attached patch option 2: comm-central patch, v6 (obsolete) — Splinter Review
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.
Attachment #350634 - Attachment is obsolete: true
Attachment #350695 - Flags: superreview?(bzbarsky)
Attachment #350695 - Flags: review?(bienvenu)
Whiteboard: [mc patch ready to land; has cc patch iteration; review-ready iteration ETA Sun aft] → [mc patch ready to land; cc patch needs r bienvenu, sr bz]
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 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 :-)
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
(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?
> 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.
>+  // 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...
I'm building with this patch now.
(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.
Attached patch option 2: comm-central patch, v7 (obsolete) — Splinter Review
Attachment #350695 - Attachment is obsolete: true
Attachment #350709 - Flags: superreview?(bzbarsky)
Attachment #350709 - Flags: review?(bienvenu)
Attachment #350695 - Flags: superreview?(bzbarsky)
Attachment #350695 - Flags: review?(bienvenu)
Attachment #350709 - Flags: review?(bienvenu) → review+
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.
(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.
Attachment #350709 - Flags: superreview?(bzbarsky) → superreview+
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.
moving to b2
Target Milestone: Thunderbird 3.0b1 → Thunderbird 3.0b2
updating whiteboard
Whiteboard: [mc patch ready to land; cc patch needs r bienvenu, sr bz] → [mc patch ready to land; ]
Blocks: 453928
Summary: Remove javascript.allow.mailnews from the security manager → Revise message CAPS policies; enable JS for non-message content by default; remove javascript.allow.mailnews hack from script security mgr
Component: Backend → Security
QA Contact: backend → security
Attached patch option 2: comm-central patch, v8 (obsolete) — Splinter Review
Updated patch to the comm-central trunk; carrying forward review & super-review.
Attachment #350709 - Attachment is obsolete: true
Attachment #358539 - Flags: superreview+
Attachment #358539 - Flags: review+
Whiteboard: [mc patch ready to land; ] → [needs updates dmose]
Patch updated to trunk.
Attachment #358567 - Flags: superreview+
Attachment #358567 - Flags: review+
Status: NEW → ASSIGNED
Whiteboard: [needs updates dmose] → [needs updates dmose][no l10n impact]
Whiteboard: [needs updates dmose][no l10n impact] → [needs updates dmose][no l10n impact][target slushy]
(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.
Attached patch option 3: comm-central patch, v9 (obsolete) — Splinter Review
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...
Attachment #358539 - Attachment is obsolete: true
Attachment #361909 - Attachment description: comm-central patch, v8 → option 3: comm-central patch, v9
Turns out the belt-and-suspenders issue I thought I saw was a misunderstanding.  Added comments to make the behavior more clear.
Attachment #361909 - Attachment is obsolete: true
Attachment #361941 - Flags: superreview?(bzbarsky)
Attachment #361941 - Flags: review?(bienvenu)
Whiteboard: [needs updates dmose][no l10n impact][target slushy] → [needs reviews bienvenu, bz][no l10n impact][target slushy]
Attachment #350191 - Attachment is obsolete: true
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 ;-)
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 on attachment 361941 [details] [diff] [review]
option 3: comm-central patch, v10

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

this should be nsIMsgDBHdr...
dmose, do you mind summarizing what's changed since the v7 version I reviewed?
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.
Attached patch comm-central patch, v11 (obsolete) — Splinter Review
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.
Attachment #361941 - Attachment is obsolete: true
Attachment #362314 - Flags: superreview?(bzbarsky)
Attachment #362314 - Flags: review?(bienvenu)
Attachment #361941 - Flags: superreview?(bzbarsky)
Attachment #361941 - Flags: review?(bienvenu)
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 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+.
Attachment #362328 - Flags: superreview+
Attachment #362328 - Flags: review+
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";
Attachment #362314 - Flags: review?(bienvenu) → review+
Whiteboard: [needs reviews bienvenu, bz][no l10n impact][target slushy] → [needs reviews bz][no l10n impact][target slushy]
A fair point.  Addressed; carried for r+.
Attachment #362314 - Attachment is obsolete: true
Attachment #362635 - Flags: superreview?(bzbarsky)
Attachment #362635 - Flags: review+
Attachment #362314 - Flags: superreview?(bzbarsky)
Whiteboard: [needs reviews bz][no l10n impact][target slushy] → [needs review bz][needs driver approval for branch][no l10n impact][target hard freeze]
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...
Attachment #362635 - Flags: superreview?(bzbarsky) → superreview+
Attachment #362328 - Attachment description: mozilla-central patch, v6, merged forward → mozilla-central patch, v6, merged forward (checked in on trunk)
bz: checked in a variant of the patch with your proposed change to comm-central.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [needs review bz][needs driver approval for branch][no l10n impact][target hard freeze] → [landed on trunk][mc-patch needs driver approval for branch][no l10n impact][target hard freeze]
Comment on attachment 362328 [details] [diff] [review]
mozilla-central patch, v6, merged forward (checked in on trunk and 1.9.1)

a191=beltzner
Attachment #362328 - Flags: approval1.9.1+
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
Attachment #362328 - Attachment description: mozilla-central patch, v6, merged forward (checked in on trunk) → mozilla-central patch, v6, merged forward (checked in on trunk and 1.9.1)
Keywords: fixed1.9.1
Whiteboard: [landed on trunk][mc-patch needs driver approval for branch][no l10n impact][target hard freeze] → [no l10n impact][target hard freeze]
No longer blocks: 376460
Attachment #362635 - Attachment description: comm-central patch, v12 → comm-central patch, v12 (variant of this patch checked in; see comments 67 and 68)
Adding in-testsuite?, since the test here will need to be wired into a MozMill-based automated suite, once that exists.
Flags: in-testsuite?
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.
Not a b2 blocker because this is a correctness check in #ifdef DEBUG code; I'll spin this off into a new bug.
Hmm.  NS_QueryNotificationCallbacks is not null-safe, so we just need a null-check here.
Depends on: 479906
Depends on: 479920
(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?
So, what happened to the javascript.allow.mailnews pref?
It's gone; there are no plans for it to return.
(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!
(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.
Blocks: 458881
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.
Please file a new bug; this is already resolved.
Blocks: 309267
Blocks: 309263
Blocks: 309276
Blocks: 309258
You need to log in before you can comment on or make changes to this bug.