Boris Zbarsky [:bzbarsky] Reporter
	Description • 20 years ago

The content policy check done by suite mailnews (1.7 and trunk) is too
permissive -- it allows anything that's not linked to via ftp:, http:, https:
urls.  In particular, as Jesse pointed out, a data: URI to an XBL binding will
happily load, and then script in that binding will run.  Random schemes can also
be used to "phone home" from mail.

The code in nsMailnewsContentBlocker.cpp needs some changes copied over from
nsMsgContentPolicy, basically.

	Jay Patel [:jay]
	Comment 1 • 19 years ago

Any update here for 1.7.9?  Do we need to find an owner for this?

	Scott MacGregor
	Comment 2 • 19 years ago

Dan, is there a specific security test case that this is a 1.7.9 blocker for
that one can use to know what to fix?

saying seamonkey needs some content policy changes copied over from thunderbird
is too vague for me :) 

What's the specific vulnerability I (or a seamonkey module owner) needs to fix?

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 3 • 19 years ago

Comment 0 describes the issues that would need to be testing.  data: is one
example; other things that can be used are whatever scheme it is that the
Microsoft media player uses, rtsp: URIs, etc (any URI we'd pass off to the
operating system).  We'd make server connections for those, and we really
shouldn't be.  Same for gopher: urls, as far as that goes.

"some changes" means "everything that landed on Aviary branch in the Thunderbird
content policy impl" (except the content policy API change, of course).

	Scott MacGregor
	Comment 4 • 19 years ago

I don't have a seamonkey tree around to try this out yet so I don't even know
if it compiles.

	Chase Phillips
	Comment 5 • 19 years ago

Comment on attachment 186834 [details] [diff] [review]
(untested/compiled) possible fix [needs check-in]

dveditz will build the patch and if it looks ok he will request conditional
approval to check the patch into the trunk and 1.7 branch.

	Chase Phillips
	Comment 6 • 19 years ago

Comment on attachment 186834 [details] [diff] [review]
(untested/compiled) possible fix [needs check-in]

Per 1.0.5 meeting, patch is ok, needs landing.

	Jay Patel [:jay]
	Comment 7 • 19 years ago

Comment on attachment 186834 [details] [diff] [review]
(untested/compiled) possible fix [needs check-in]

Dveditz: Let's get this landed on the 1.7.9 branch as soon as you have the
patch ready. a=jay

Requesting approval for the Trunk as well.

	chris hofmann
	Comment 8 • 19 years ago

Comment on attachment 186834 [details] [diff] [review]
(untested/compiled) possible fix [needs check-in]

a=chofmann

	Daniel Veditz [:dveditz]
	Comment 9 • 19 years ago

This appears to exist only in trunk seamonkey (initial landing Nov 2004),
removing from 1.7.9 blocker list.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 10 • 19 years ago

This is most certainly a problem on the 1.7 branch.  The code there is in either
nsMsgContentPolicy or extensions/cookie/nsImgManager.cpp (both have
mail-specific content policy code that's too permissive, and I'm not sure which
of the two Mozilla 1.7 uses).  The code did move to a different file on the
trunk, as noted.

Restoring blocking request (since I can't restore the blocking+ flag this used
to have) and summary.

	Jay Patel [:jay]
	Comment 11 • 19 years ago

Reassigning to dveditz, since he was ready to check in this patch before taking
the bug off the radar.  Since it's back on, and we have a patch, let's just get
this landed asap.  

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 12 • 19 years ago

Jay, the problem is the patch doesn't fix the bug for 1.7.x (it looks like it
should work for trunk, though).

	Jay Patel [:jay]
	Comment 13 • 19 years ago

mscott/bienvenu:  Can either of you take another look here and see if you can
whip up a branch patch?

	David :Bienvenu
	Comment 14 • 19 years ago

nsMsgContentPolicly for Thunderbird 1.0x looks OK - perhaps it's just 1.7.9 that
has issues?

	David :Bienvenu
	Comment 15 • 19 years ago

the method signature for nsMsgContentPolicy::ShouldDownload in 1.7.9 is
sufficiently different from 1.0x or the trunk that I'm not sure how to proceed.
1.7.9 doesn't get the requestingLocation, and instead gets an nsIDOMWindow. Can
I get the requesting location from the nsIDOMWindow?

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 16 • 19 years ago

Yes, this is not an issue on the aviary branch.

Depending on what the window is, you should just be able to get the .document
from the window and then QI to nsIDocument and get the documentURI...

	David :Bienvenu
	Comment 17 • 19 years ago

From what I can tell, Seamonkey 1.7.9 uses
nsWebBrowserContentPolicy::ShouldLoad() when loading mail messages. It doesn't
use nsMsgContentPolicy at all, and I don't think it uses
extensions/cookie/nsImgManager.cpp  either. Boris, is that possible?

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 18 • 19 years ago

That sounds pretty unlikely -- nsWebBrowserContentPolicy is used by
nsIWebBrowser and such, which seamonkey doesn't use.  On the other hand, I'm
pretty sure it used nsImgManager...

	David :Bienvenu
	Comment 19 • 19 years ago

I set breakpoints and stepped through the code.
mozilla/embedding/browser/webBrowser/nsWebBrowserContentPolicy::ShouldLoad gets
called when I click on a message, and when I click on a message with images in
html, it gets called over and over again. Whereas the debugger won't even let me
set a breakpoint in nsImgManager. This was 1.7.x Seamonkey. I'm sure it's
Seamonkey, not Thunderbird.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 20 • 19 years ago

mvl, any idea what's up there?

	Jay Patel [:jay]
	Comment 21 • 19 years ago

David says it is too late to get a handle on this for 1.7.9, moving out to the
next release.  Let's see if we can figure this otu for the 1.7.10.

We should at least get this checked in on the Trunk if we think the patch is good.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 22 • 19 years ago

Note that this bug allows script execution in mailnews...  Given the other
issues that raises, I'd really like us to get this fixed for 1.7.9 if at all
possible.  If we're not shipping it within the next 2-3 days, I'm not quite sure
why this isn't fixable by the time we do ship it.

	Jay Patel [:jay]
	Comment 23 • 19 years ago

Ok, returning this to 1.7.9 blocker status to see if we can get a fix in the
next couple of days.  Since this is a 1.7.9 only bug, we can probably take this
fix while we verify the Firefox/Thunderbird 1.0.5 candidates.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 24 • 19 years ago

OK, I just put some printfs in nsImgManager to test.  Those are hit for remote
images in mails in a 1.7 seamonkey build.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 25 • 19 years ago

And for what it's worth, since the mailnews.message_display.disable_remote_image
pref works in seamonkey, it had to be either nsImgManager or
mailnews/base/src/nsMsgContentPolicy.cpp being called -- no one else looks at
that pref.

	Michiel van Leeuwen (email: mvl+moz@)
	Comment 26 • 19 years ago

nsMsgContentPolicy.cpp is only build for thunderbird. nsImgManager is what
seamonkey uses. Or should, reading from previous comments... (I don't have a 1.7
build, so I can't really test)

	David :Bienvenu
	Comment 27 • 19 years ago

I wasn't building my 1.7 tree with the right options in mozconfig. so the cookie
extension wasn't getting built. I'm rebuilding now...

	David :Bienvenu
	Comment 28 • 19 years ago

ok, now imgManager.cpp is getting hit. If I change that code, am I going to be
changing the way the browser checks content as well? It certainly seems to get
hit when browsing. Do I only want to be looking at aContentType ==
nsIContentPolicy::IMAGE?

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 29 • 19 years ago

I think you want all types (eg <object>).  And yes, if you just change the file
it'll change what browser does.  Mailnews-specific code should go inside a check
like http://lxr.mozilla.org/mozilla1.7/source/extensions/cookie/nsImgManager.cpp#220

	David :Bienvenu
	Comment 30 • 19 years ago

OK, I really don't know what I'm doing here, but this attempts to do the same
kind of checking for mail that thunderbird does, w/o affecting what the browser
does (or do you want the browser to change as well?)

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 31 • 19 years ago

We don't want to change the browser behavior.  In fact, it might make sense to
do a check for a mailnews docshell up front and call a separate method to handle
that case; if that doesn't return "allow", then just return, otherwise do the
existing browser checks.

	David :Bienvenu
	Comment 32 • 19 years ago

this attempts to do what bzbarsky suggests - check if it's a mailnews doc type
first; if it is, return whatever the mailnews checks say. Otherwise, fall
through to the old code.

	David :Bienvenu
	Comment 33 • 19 years ago

I haven't tested this against the bug scenario, but I've verified that mail and
browsing seem to basically work, and ftp works from the browser.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 34 • 19 years ago

Comment on attachment 188470 [details] [diff] [review]
check for mailnews doc type first.

I think we should do the "non-mailnews" checks even in mailnews.  That is, even
if remote images are allowed in mailnews, if I'm blocking images from evil.com
they shouldn't show up in the mail window; with this patch they would.	In
other words, no need for the isMailNewsWindow return value.

I haven't had a chance to read the rest of the patch in detail yet; will try to
do that tonight.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 35 • 19 years ago

Comment on attachment 188470 [details] [diff] [review]
check for mailnews doc type first.

>Index: nsImgManager.cpp
>+PRBool nsImgManager::CheckMailNews(nsIURI *baseURI,

>+      // if aContentLoc is a protocol we handle (imap, pop3, mailbox, etc) or is a chrome url, then allowe the load

This is no good for suite.  For tbird it's OK because tbird claims to not
handle http, but suite _can_ handle http, so for http this will always OK the
load.  Similar for ftp:, gopher:, etc.	This code needs to be changed
accordingly...

>+    if (aContentType == nsIContentPolicy::IMAGE && baseURI)
>+      rv =  TestPermission(aContentLoc, baseURI, aShouldLoad);

I may be missing something (the -w diff is very very hard to review; it'd help
to have the normal diff in addition to the -w), but I think this is inside the
|if (aContentType == nsIContentPolicy::IMAGE) {| block, so no need to recheck
aContentType here.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 36 • 19 years ago

Comment on attachment 186834 [details] [diff] [review]
(untested/compiled) possible fix [needs check-in]

This patch is no good for the same reason as the branch one -- it just allows
all loads, since suite supports all sorts of protocols that thunderbird does
not support.

	David :Bienvenu
	Comment 37 • 19 years ago

> This code needs to be changed accordingly...

Should I just check for the protocols mail knows how to do, i.e., pop3, imap,
imap, ldap, and mailbox? Or should I check for http, https, gopher, and ftp, and
in that case, let the code after the mail code decide, i.e., TestPermission? If
I just fall through to the TestPermission code anyway, by ignoring the
isMailNewsWindow return value, that can't be right...

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 38 • 19 years ago

I think for mail windows we want to replace the "external protocol" check by a
check that the protocol is not in the list of mail protocols...  Otherwise
random protocols like rtsp:// can be used in mail, which is not desirable.

	David :Bienvenu
	Comment 39 • 19 years ago

bz, is this what you had in mind?

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 40 • 19 years ago

Comment on attachment 188789 [details] [diff] [review]
try to incorporate last round of comments

>Index: nsImgManager.cpp
>+void nsImgManager::CheckMailNews(nsIURI *baseURI,
>+  if (!baseURI)
>+    return;

This leaves shouldLoad uninitialized.  More on this later.

>+      if (isChrome || contentScheme.Equals("mailto") || 

You've already checked isChrome.  Checking it again is unnecessary (and wrong,
since if that SchemeIs call failed the boolean here would be bogus anyway).

>@@ -158,17 +243,20 @@ NS_IMETHODIMP nsImgManager::ShouldLoad(P

>+  CheckMailNews(baseURI, isFtp, aContentType, aContentLoc, aContext, aWindow, aShouldLoad);

So if this set aShouldLoad to false...

>   if (aContentType == nsIContentPolicy::IMAGE) {
...
>+    if (baseURI)
>+      rv =  TestPermission(aContentLoc, baseURI, aShouldLoad);

And this sets it to true, then we'll load even though we shouldn't.  I think
CheckMailNews should always set its out param (default to true for non-mailnews
windows), and we should bail out of this method immediately if CheckMailNews
returns false.

	David :Bienvenu
	Comment 41 • 19 years ago

>This leaves shouldLoad uninitialized.  More on this later.

It's initialized to true at the very beginning of nsImgManager::ShouldLoad,
right before we call CheckIfMailNews.

>we should bail out of this method immediately if CheckMailNews
>returns false

If I have an html e-mail which has http urls to fetch images, CheckIfMailnews
will return false, and we'll bail out, and the image won't load. So, to do this,
I'll need to also check for http and https urls in CheckMailNews. Does that
sound OK?

	David :Bienvenu
	Comment 42 • 19 years ago

add checks for http and https, check contentLoc for isChrome before checking
isChrome boolean for the second time (this matches what's in
nsMsgContentPolicy.cpp).

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 43 • 19 years ago

> If I have an html e-mail which has http urls to fetch images, CheckIfMailnews
> will return false, and we'll bail out, and the image won't load.

If remote stuff is disabled in mailnews this is the right behavior, no?  Looking
at the patch again, if mBlockInMailNewsPref we'll disallow all sorts of loads we
should be allowing (eg imap:, pop:, etc).  If it's false, of course,
CheckIfMailnews should do pretty much nothing.

So the patch looks wrong...  The protocol checks (and the baseURI scheme checks)
should be before mBlockInMailNewsPref and isFtp are checked, as far as I can
tell.  Unless I'm missing something?

	David :Bienvenu
	Comment 44 • 19 years ago

ok, this probably isn't right...if mBlockInMailNewsPref is false, should I just
set *shouldLoad=PR_TRUE, and not bother with the http/https contentScheme
check?

      if (mBlockInMailNewsPref || isFtp) 
	return;
      // if we're not blocking images, allow http and https urls.
      if (contentScheme.Equals("http") || contentScheme.Equals("https"))
	*shouldLoad = PR_TRUE;

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 45 • 19 years ago

I think so.  I see nothing special here about http/https.

Also, this still has code like:

+      contentLoc->SchemeIs("chrome", &isChrome);
+      rv = contentLoc->GetScheme(contentScheme);
+      if (isChrome 

The SchemeIs call needs to be error-checked.  So does the GetScheme call.

> +      if (isChrome || contentScheme.Equals("mailto") ||
contentScheme.Equals("news") || contentScheme.Equals("snews")
> +        || contentScheme.Equals("nntp") || contentScheme.Equals("imap") ||
contentScheme.Equals("addbook")

80-char lines, please, and have the || at the end of the line, not at the beginning.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 47 • 19 years ago

Comment on attachment 188880 [details] [diff] [review]
address prev comments...

>Index: Makefile.in

>+		  exthandler \

This isn't needed anymore, right?

>Index: nsImgManager.cpp
>+void nsImgManager::CheckMailNews(nsIURI *baseURI,
>+                                   PRBool isFtp,
>+                                   PRInt32 contentType, 
>+                                   nsIURI *contentLoc,
>+                                   nsISupports *context,
>+                                   nsIDOMWindow *window,
>+                                   PRBool *shouldLoad)

Fix indent here, please.

>+      // or is a chrome url, then allowe the load

s/allowe/allow/

>+      if (isChrome || contentScheme.Equals("mailto") || 
>+        contentScheme.Equals("news") || contentScheme.Equals("snews") ||
>+        contentScheme.Equals("nntp") || contentScheme.Equals("imap") || 
>+        contentScheme.Equals("addbook") || contentScheme.Equals("pop") ||
>+        contentScheme.Equals("mailbox") ) {
>+        *shouldLoad = PR_TRUE;

Fix indent here too, please (the if condition continuations need to be indented
two more spaces).  And no need for the trailing space in the if condition.

>+        // never allow ftp for mail messages, 
>+        // because we don't want to send the users email address
>+        // as the anonymous password
>+      if (mBlockInMailNewsPref || isFtp) 

That comment should be outdented two spaces.

>@@ -158,17 +250,23 @@ NS_IMETHODIMP nsImgManager::ShouldLoad(P
>   if (!aContentLoc)
>-    return rv;
>+      return NS_OK;

Fix indent, please.

r=bzbarsky with those nits picked.  Thanks for fixing this, David!

	David :Bienvenu
	Comment 48 • 19 years ago

carrying forward bz's r, requesting sr. indentation fixes and typo fix. Thx for
bearing with me, bz...

	David :Bienvenu
	Comment 49 • 19 years ago

Comment on attachment 188890 [details] [diff] [review]
address nits, typo and indentation.

if we want this for 1.7.9, I can check it in today.

Also, I've fixed the Makefile.in in my tree.

	Jay Patel [:jay]
	Comment 50 • 19 years ago

Comment on attachment 188890 [details] [diff] [review]
address nits, typo and indentation.

a=jay, let's get this checked in asap.

	David :Bienvenu
	Comment 51 • 19 years ago

fixed on 1.7.9 branch - leaving open for trunk seamonkey issues, which, as I
understand it, is an issue.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 52 • 19 years ago

Yeah, trunk needs a separate patch.  Neil, mvl, do you know anyone who can do that?

	David :Bienvenu
	Comment 53 • 19 years ago

mvl, do you want to deal with the trunk seamonkey issue that bz brought up?

	Kai Engert (:KaiE:)
	Comment 54 • 19 years ago

I think the checkin that was made in the context of this bug to the 1.7 branch
has caused regression bug 300749.

	Daniel Veditz [:dveditz]
	Comment 55 • 19 years ago

Adding fixed keyword to note the 1.7.9 fix.

This does not appear fixed on the trunk or 1.8 branches. Clearing confidential bit, the 1.7.9 checkin made this public.

	Ian Neal Assignee
	Comment 56 • 19 years ago

This should do the equivalent for trunk and 1.8 branch (I think) but things have changed alot since 1.7 so feel free to blow it apart.

	Ian Neal Assignee
	Comment 57 • 19 years ago

Problem I now have is that build order is incorrect. The new files depends on stuff that's not yet been built.

	Ian Neal Assignee
	Comment 58 • 19 years ago

Changes since v0.2:
* the build order has been altered so that tier_94 is built after tier_97
* not asking for exposedprotocols as mailnews does not seem to give the same answer as TB

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 59 • 19 years ago

I can't review that (certainly not the build changes or the changes that use mailnews apis, etc).  Please find someone else.  I suspect the build change is wrong, but check with bsmedberg.

	Ian Neal Assignee
	Comment 60 • 19 years ago

Changes since v0.2a:
* Fixed typo in nsISupportsWeakReference (missing I)
* Removed moving of build order - that's bug 322824

	neil@parkwaycc.co.uk
	Comment 61 • 19 years ago

Unfortunately it refused to block any content :-(

	Ian Neal Assignee
	Comment 62 • 19 years ago

This patch:
* Merges nsMailnewsContentBlocker.h/cpp into nsMsgContentPolicy.h/cpp
* Makes relevant changes to Makefile.in and nsModuleFactory.cpp in extensions/permissions to stop using nsMailnewsContentBlocker.h/cpp
* Makes relevant changes to Makefile.in and nsMsgFactory.cpp in mailnews/base/build to start using updated nsMsgContentPolicy.h/cpp
* Makes relevant changes to Makefile.in in mailnews/base/src to build nsMsgContentPolicy.cpp for mailnews as well as Thunderbird
* Adds extra prefs to mailnews.js for nsMsgContentPolicy.cpp

Only thing not in the patch is the removal of nsMailnewsContentBlocker.h/cpp from extensions/permissions which is just a matter of cvs removing them

	neil@parkwaycc.co.uk
	Comment 63 • 19 years ago

Comment on attachment 208078 [details] [diff] [review]
Merged nsMsgContentPolicy patch v0.4

>+  return catman->AddCategoryEntry("content-policy",
I wonder if this should use the #define in nsContentPolicyUtils.h

>+#ifdef MOZ_THUNDERBIRD
> NS_IMPL_ADDREF(nsMsgContentPolicy)
> NS_IMPL_RELEASE(nsMsgContentPolicy)
> 
> NS_INTERFACE_MAP_BEGIN(nsMsgContentPolicy)
>    NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIContentPolicy)
>    NS_INTERFACE_MAP_ENTRY(nsIContentPolicy)
>    NS_INTERFACE_MAP_ENTRY(nsIObserver)
>    NS_INTERFACE_MAP_ENTRY(nsSupportsWeakReference)
> NS_INTERFACE_MAP_END
>+#else
>+NS_IMPL_ISUPPORTS3(nsMsgContentPolicy, 
>+                   nsIContentPolicy,
>+                   nsIObserver,
>+                   nsISupportsWeakReference)
>+#endif
nsSupportsWeakReference is illegal anyway, don't bother with the #ifdef

>+static inline already_AddRefed<nsIDocShell>
>+GetRootDocShell(nsISupports *context)
I think I'd prefer if you manually inlined this inside the caller's #ifdef

>+  rv = aContentLocation->SchemeIs("chrome", &isChrome);
Actually we already checked this.

>+  if (isChrome || contentScheme.Equals("mailto") ||
>+      contentScheme.Equals("news") || contentScheme.Equals("snews") ||
>+      contentScheme.Equals("nntp") || contentScheme.Equals("imap") ||
>+      contentScheme.Equals("addbook") || contentScheme.Equals("pop") ||
>+      contentScheme.Equals("mailbox") || contentScheme.Equals("about"))
LowerCaseEqualsLiteral? (check with biesi)

>+  // Look into http and https more closely to determine if the load should be allowed
>+  // default to blocking remote content
>+  *aDecision = mBlockRemoteImages ? nsIContentPolicy::REJECT_REQUEST :
>+                                    nsIContentPolicy::ACCEPT;
Surely if you're not blocking images you can accept at this point?

	Scott MacGregor
	Comment 64 • 19 years ago

Iann, can you explain in a bit more detail why seamonkey is moving its implementation into the Thunderbird file? From a quick glance at the patch it looks like the behaviors for the two apps are quite different and by putting them in the same class we are adding a bunch of MOZ_THUNDERBIRD ifdefs. Seems like it would be cleaner to leave them alone like they were already. I'm finding it very hard to read through the file with all the ifdefs in it in this patch. But maybe that's just old age on my part :)

A couple other caveats, this patch is changing Thunderbird's behavior in several places (besides just adding the ifdefs), particularly in the ShouldLoad method. I view this file currently as a Thunderbird file where review and checkin policies are dictated by the current Thunderbird checkin rules. You can share this file with me if you'd like (I'm still worried about readability with all the ifdefs though), but I'm still going to treat it with the Thunderbird rules which may not be in synch at any given point in time with seamonkey's checkin rules. Just a caveat and FYI if you decide to move the suite to start using this thunderbird file. 

	neil@parkwaycc.co.uk
	Comment 65 • 19 years ago

Comment on attachment 208078 [details] [diff] [review]
Merged nsMsgContentPolicy patch v0.4

Nice :-) r=me with my previous comments addressed.

(In reply to comment #64)
>From a quick glance at the patch it
>looks like the behaviors for the two apps are quite different and by putting
>them in the same class we are adding a bunch of MOZ_THUNDERBIRD ifdefs.
As I read the patch for the review, here's a quick run through:
1. TB needs cookie headers and Suite needs docShell headers
2. Isn't necessary as the old code has a typo and should be removed altogether
3. Suite only wants to block content in messages
4. As 3, so would save one if 3 was inlined here as suggested
5. TB and Suite differ as to which protocols are supported
6. Suite's cookie permissions are different
So, not that many differences really. We haven't changed any other behaviours because we need them to fix this bug! All the other code "changes" are simply readability improvements to make up for the extra #ifdefs ;-)

>I view this file currently as a Thunderbird file where review and
>checkin policies are dictated by the current Thunderbird checkin rules.
I'm sure he was intending to ask a Thunderbird peer for review too.

	neil@parkwaycc.co.uk
	Comment 66 • 19 years ago

In fact, if you prefer, we can remove the cookie #ifdefs, as they depend on module registration anyway.

	Ian Neal Assignee
	Comment 67 • 19 years ago

(In reply to comment #64)
> Iann, can you explain in a bit more detail why seamonkey is moving its
> implementation into the Thunderbird file? From a quick glance at the patch it
> looks like the behaviors for the two apps are quite different and by putting
> them in the same class we are adding a bunch of MOZ_THUNDERBIRD ifdefs. Seems
> like it would be cleaner to leave them alone like they were already. I'm
> finding it very hard to read through the file with all the ifdefs in it in this
> patch. But maybe that's just old age on my part :)
I will produce a -w version for the next iteration of the patch to help make things clearer and that iteration will have less ifdefs in too.

> A couple other caveats, this patch is changing Thunderbird's behavior in
> several places (besides just adding the ifdefs), particularly in the ShouldLoad
> method. 
There should be no change in the output from ShouldLoad for Thunderbird, the changes are to do with returning with the value earlier instead of putting the rest of the code in an "else" and sometimes doing unnecessary extra checks (for example after checking if we do not block remote content we were still looking at message headers, RSS urls, etc when we already know we will be accepting the content).

I was fully intending to have a TB peer review these code changes too.

	Ian Neal Assignee
	Comment 68 • 19 years ago

Changes since v0.4:
* Just uses NS_IMPL_ISUPPORTS3
* Inlined GetRootDocShell
* Tweaked some of the coding to reduce number of ifdefs
* Accepts at the point of checking we are not blocking remote content
* Removed another else now we accept at above point

isChrome is used for checking both aRqeuestingLocation and aContentLocation so not a repeated check.
From investigation, schemes are all in lowercase so staying with .Equals

Not sure of the advantages of using #define from nsContentPolicyUtils.h, couple of ways to achieve it though (neither of them tested):
* change #include of nsIContentPolicy.h to nsContentPolicyUtils.h in nsMsgContentPolicy.h and remove it from nsMsgContentPolicy.cpp
* add #include of nsContentPolicyUtils.h directly into nsMsgFactory.cpp

Note tested removing idefs from any of the cookie stuff either.

	Ian Neal Assignee
	Comment 69 • 19 years ago

For easier reading (hopefully)

	Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
	Comment 70 • 19 years ago

Clearing obsolete blocking flag.  biesi: do you want to reconsider blocking flags?

	Ian Neal Assignee
	Comment 71 • 19 years ago

Changes since v0.4b:
* Removed ifdefs for cookie stuff as suggested by Neil
* Stopped leaking for docshell

	Ian Neal Assignee
	Comment 73 • 19 years ago

Changes since v0.4b:
* Corrected test to docshellTreeItem instead of shell.

	David :Bienvenu
	Comment 75 • 19 years ago

Ian, with Scott's caveats, the changes look OK to me, but I need to run with them to be sure, which I'll try to do this weekend.

	David :Bienvenu
	Comment 76 • 19 years ago

Comment on attachment 208431 [details] [diff] [review]
Test for docshellTreeItem patch v0.4c (Checked in trunk)

Tb builds and seems to run fine with this patch.

	Ian Neal Assignee
	Comment 77 • 19 years ago

Comment on attachment 208431 [details] [diff] [review]
Test for docshellTreeItem patch v0.4c (Checked in trunk)

Requesting approval to land on MOZILLA_1_8_BRANCH too

	Ian Neal Assignee
	Comment 78 • 19 years ago

Comment on attachment 208431 [details] [diff] [review]
Test for docshellTreeItem patch v0.4c (Checked in trunk)

Checking in (trunk)
extensions/permissions/Makefile.in;
new revision: 1.7; previous revision: 1.6
extensions/permissions/nsModuleFactory.cpp;
new revision: 1.2; previous revision: 1.1
mailnews/mailnews.js;
new revision: 3.259; previous revision: 3.258
mailnews/base/build/Makefile.in;
new revision: 1.76; previous revision: 1.75
mailnews/base/build/nsMsgFactory.cpp;
new revision: 1.117; previous revision: 1.116
mailnews/base/src/Makefile.in;
new revision: 1.125; previous revision: 1.124
mailnews/base/src/nsMsgContentPolicy.h;
new revision: 1.8; previous revision: 1.7
mailnews/base/src/nsMsgContentPolicy.cpp;
new revision: 1.23; previous revision: 1.22
done
Removing (trunk)
extensions/permissions/nsMailnewsContentBlocker.h;
new revision: delete; previous revision: 1.1
extensions/permissions/nsMailnewsContentBlocker.cpp;
new revision: delete; previous revision: 1.4
done

	Scott MacGregor
	Comment 79 • 19 years ago

Comment on attachment 208431 [details] [diff] [review]
Test for docshellTreeItem patch v0.4c (Checked in trunk)

This is going to need to bake on the trunk for a little while before we approve it for the branch.

	Robert Kaiser
	Comment 80 • 19 years ago

Comment on attachment 208431 [details] [diff] [review]
Test for docshellTreeItem patch v0.4c (Checked in trunk)

a=me for SeaMonkey 1.1 (given that this needs drivers' approval and mscott already stated baking on trunk is needed for that one, it sounds good to me to make it depend on their approval only...)

	Scott MacGregor
	Comment 81 • 19 years ago

Hey guys, with this patch, we now initialize *aDecision to nsIContentPolicy::Accept. That means if any of the calls inside this routine generate an error (i.e. NS_ENSURE_SUCCESS(rv, rv)), we are going to end up allowing the content which is not what we want. 

This seems like the opposite of what I had intended. 

	Ian Neal Assignee
	Comment 82 • 19 years ago

This patch:
* Changes default for *aDecision for NS_ENSURE_SUCCESS start
* Makes necessary tweaks further down the function after above default change

	Ian Neal Assignee
	Comment 83 • 19 years ago

Comment on attachment 208431 [details] [diff] [review]
Test for docshellTreeItem patch v0.4c (Checked in trunk)

Cancelling old approval request, new patch for branch to come

	Brendan Eich [:brendan]
	Comment 84 • 19 years ago

Except for QueryInterface, XPCOM callers are supposed to ignore out parameters on failure nsresult return (JS exception thrown).  I don't see a problem looking at http://lxr.mozilla.org/mozilla/search?string=ShouldLoad%28&regexp=on where an "Accept" code would be stored persistently on ShouldLoad failure -- but maybe I'm missing something.

Doesn't hurt to defend with the Reject default, mind you!  I'm just spouting the old XPCOM rules to see whether they still make sense to people.

/be

	neil@parkwaycc.co.uk
	Comment 85 • 19 years ago

Actually you need to search for NS_CheckContentLoadPolicy; I checked all ten callers and they all fail on failed or rejected load, except for PRBool nsContentUtils::CanLoadImage which returns false on failed or rejected load.

	Ian Neal Assignee
	Comment 86 • 19 years ago

Comment on attachment 209907 [details] [diff] [review]
Followup patch for trunk v0.4d_trunk (Checked in)

All though not strictly necessary, I'd still like to get this patch into trunk.

	neil@parkwaycc.co.uk
	Comment 87 • 19 years ago

Comment on attachment 209907 [details] [diff] [review]
Followup patch for trunk v0.4d_trunk (Checked in)

I still don't see the point of this, because all of the callers treat REJECT as an error anyway. As Scott asked for it I think he should review it.

	Ian Neal Assignee
	Comment 88 • 19 years ago

Scott, do you want this in trunk? 1.8.1 branch?

	Scott MacGregor
	Comment 89 • 19 years ago

Comment on attachment 209907 [details] [diff] [review]
Followup patch for trunk v0.4d_trunk (Checked in)

In general I agree with the good XPCOM rules about ignoring out parameters for methods that return nsresult failure codes. 

And I agree that all of the existing callers today are handling this correctly.

I'm worried about future callers and this routine in particular because of the security / privacy issues an abuse of this routine would expose. 

In short, I'd rather be very defensive given the sensitivity of this routine.

Thanks for making the change Iann.

	Ian Neal Assignee
	Comment 90 • 19 years ago

Comment on attachment 209907 [details] [diff] [review]
Followup patch for trunk v0.4d_trunk (Checked in)

Requesting a= for SM1.1a as this is shared code

	Ian Neal Assignee
	Comment 91 • 19 years ago

Comment on attachment 209907 [details] [diff] [review]
Followup patch for trunk v0.4d_trunk (Checked in)

Checking in (trunk)
nsMsgContentPolicy.cpp;
new revision: 1.27; previous revision: 1.26
done

	Scott MacGregor
	Comment 92 • 19 years ago

Comment on attachment 209907 [details] [diff] [review]
Followup patch for trunk v0.4d_trunk (Checked in)

We're actually going to need this for 1.8.0.2 now in order for my fix in Bug #328917 to work.

	Scott MacGregor
	Comment 93 • 19 years ago

Comment on attachment 209907 [details] [diff] [review]
Followup patch for trunk v0.4d_trunk (Checked in)

scratch that, it looks like the seamonkey changes to nsMsgContentPolicy aren't on the 1.8.0.x branch so we don't need this change with 328917. Sorry for the noise.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 94 • 19 years ago

Note that returning error from ShouldLoad will actually make your content policy be ignored completely no matter what you set the out param to....  At least as things stand.

	Ian Neal Assignee
	Comment 95 • 19 years ago

Checking in (1.8 branch)
extensions/permissions/Makefile.in;
new revision: 1.5.8.1; previous revision: 1.5
extensions/permissions/nsModuleFactory.cpp;
new revision: 1.1.20.1; previous revision: 1.1
mailnews/mailnews.js;
new revision: 3.249.2.7; previous revision: 3.249.2.6
mailnews/base/build/Makefile.in;
new revision: 1.74.8.1; previous revision: 1.74
mailnews/base/build/nsMsgFactory.cpp;
new revision: 1.112.20.3; previous revision: 1.112.20.2
mailnews/base/src/Makefile.in;
new revision: 1.122.8.1; previous revision: 1.122
mailnews/base/src/nsMsgContentPolicy.h;
new revision: 1.7.2.1; previous revision: 1.7
mailnews/base/src/nsMsgContentPolicy.cpp;
new revision: 1.21.2.3; previous revision: 1.21.2.2
done

	Serge Gautherie (:sgautherie)
	Comment 96 • 16 years ago

Ian,
Are you still working on this ?

	Ian Neal Assignee
	Comment 97 • 16 years ago

I think this is okay to mark as fixed, so doing so.