Closed Bug 294307 Opened 17 years ago Closed 14 years ago

seamonkey mailnews doing too-permissive content policy checks

Categories

(SeaMonkey :: Security, defect)

defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: iannbugzilla)

References

Details

(Keywords: fixed-seamonkey1.1a, fixed1.7.9, fixed1.8.1, Whiteboard: [sg:want] need trunk/1.8 patch)

Attachments

(5 files, 17 obsolete files)

7.48 KB, patch
Bienvenu
: review+
mscott
: superreview+
Details | Diff | Splinter Review
28.00 KB, patch
neil
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
21.95 KB, patch
Details | Diff | Splinter Review
4.34 KB, patch
mscott
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
28.71 KB, patch
Details | Diff | Splinter Review
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.
Flags: blocking1.7.9?
Flags: blocking1.7.9?
Flags: blocking1.7.9+
Flags: blocking-seamonkey1.0a?
Whiteboard: [sg:fix]
Flags: blocking-seamonkey1.0a? → blocking-seamonkey1.0a+
Any update here for 1.7.9?  Do we need to find an owner for this?
Assignee: dveditz → mscott
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?
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).
I don't have a seamonkey tree around to try this out yet so I don't even know
if it compiles.
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.
Comment on attachment 186834 [details] [diff] [review]
(untested/compiled) possible fix [needs check-in]

Per 1.0.5 meeting, patch is ok, needs landing.
Attachment #186834 - Attachment description: (untested/compiled) possible fix → (untested/compiled) possible fix [needs check-in]
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.
Attachment #186834 - Flags: approval1.8b3?
Attachment #186834 - Flags: approval1.7.9+
Whiteboard: [sg:fix] → [sg:fix] needs landing by dveditz
Comment on attachment 186834 [details] [diff] [review]
(untested/compiled) possible fix [needs check-in]

a=chofmann
Attachment #186834 - Flags: approval1.8b3? → approval1.8b3+
This appears to exist only in trunk seamonkey (initial landing Nov 2004),
removing from 1.7.9 blocker list.
Flags: blocking1.7.9+
Summary: 1.7 and trunk mailnews doing too-permissive content policy checks → trunk mailnews doing too-permissive content policy checks
Whiteboard: [sg:fix] needs landing by dveditz → [sg:fix]
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.
Flags: blocking1.7.9?
Summary: trunk mailnews doing too-permissive content policy checks → 1.7 and trunk mailnews doing too-permissive content policy checks
Flags: blocking1.7.9? → blocking1.7.9+
Whiteboard: [sg:fix] → [sg:fix] need branch patch
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.  
Assignee: mscott → dveditz
Jay, the problem is the patch doesn't fix the bug for 1.7.x (it looks like it
should work for trunk, though).
mscott/bienvenu:  Can either of you take another look here and see if you can
whip up a branch patch?
nsMsgContentPolicly for Thunderbird 1.0x looks OK - perhaps it's just 1.7.9 that
has issues?
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?
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...
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?
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...
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.
mvl, any idea what's up there?
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.
Flags: blocking1.7.9-
Flags: blocking1.7.9+
Flags: blocking1.7.10+
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.
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.
Flags: blocking1.7.9-
Flags: blocking1.7.9+
Flags: blocking1.7.10+
OK, I just put some printfs in nsImgManager to test.  Those are hit for remote
images in mails in a 1.7 seamonkey build.
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.
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)
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...
Assignee: dveditz → bienvenu
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?
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
Attached patch first crack (obsolete) — Splinter Review
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?)
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.
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.
Attachment #188456 - Attachment is obsolete: true
Attachment #188470 - Flags: review?(bzbarsky)
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.
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.
Blocks: 300144
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.
Attachment #188470 - Flags: review?(bzbarsky) → review-
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.
Attachment #186834 - Flags: review-
> 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...
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.
bz, is this what you had in mind?
Attachment #188470 - Attachment is obsolete: true
Attachment #188789 - Flags: review?(bzbarsky)
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.
Attachment #188789 - Flags: review?(bzbarsky) → review-
>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?
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).
Attachment #188789 - Attachment is obsolete: true
Attachment #188868 - Flags: review?(bzbarsky)
> 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?
Attachment #188868 - Flags: review?(bzbarsky) → review-
Attached patch another round (obsolete) — Splinter Review
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;
Attachment #188873 - Flags: review?(bzbarsky)
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.
Attachment #188873 - Flags: review?(bzbarsky) → review-
Attached patch address prev comments... (obsolete) — Splinter Review
Attachment #188880 - Flags: review?(bzbarsky)
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!
Attachment #188880 - Flags: review?(bzbarsky) → review+
carrying forward bz's r, requesting sr. indentation fixes and typo fix. Thx for
bearing with me, bz...
Attachment #186834 - Attachment is obsolete: true
Attachment #188868 - Attachment is obsolete: true
Attachment #188873 - Attachment is obsolete: true
Attachment #188880 - Attachment is obsolete: true
Attachment #188890 - Flags: superreview?(mscott)
Attachment #188890 - Flags: review+
Attachment #188890 - Flags: superreview?(mscott) → superreview+
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.
Attachment #188890 - Flags: approval1.7.9?
Comment on attachment 188890 [details] [diff] [review]
address nits, typo and indentation.

a=jay, let's get this checked in asap.
Attachment #188890 - Flags: approval1.7.9? → approval1.7.9+
fixed on 1.7.9 branch - leaving open for trunk seamonkey issues, which, as I
understand it, is an issue.
Yeah, trunk needs a separate patch.  Neil, mvl, do you know anyone who can do that?
mvl, do you want to deal with the trunk seamonkey issue that bz brought up?
I think the checkin that was made in the context of this bug to the 1.7 branch
has caused regression bug 300749.
Blocks: 303523
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.
Group: security
Keywords: fixed1.7.9
Summary: 1.7 and trunk mailnews doing too-permissive content policy checks → seamonkey mailnews doing too-permissive content policy checks
Whiteboard: [sg:fix] need branch patch → [sg:want] need trunk/1.8 patch
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.
Problem I now have is that build order is incorrect. The new files depends on stuff that's not yet been built.
Attachment #207897 - Attachment is obsolete: true
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
Attachment #207920 - Attachment is obsolete: true
Attachment #207949 - Flags: review?(bzbarsky)
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.
Attachment #207949 - Flags: review?(bzbarsky)
Depends on: 322824
Attached patch Fixed typo patch v0.2b (obsolete) — Splinter Review
Changes since v0.2a:
* Fixed typo in nsISupportsWeakReference (missing I)
* Removed moving of build order - that's bug 322824
Attachment #207949 - Attachment is obsolete: true
Attachment #208035 - Flags: review?(mnyromyr)
Attachment #208035 - Flags: review?(mnyromyr)
Unfortunately it refused to block any content :-(
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
Assignee: bienvenu → iann_bugzilla
Attachment #208035 - Attachment is obsolete: true
Attachment #208070 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #208078 - Flags: review?(neil.parkwaycc.co.uk)
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?
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. 
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.
Attachment #208078 - Flags: review?(neil.parkwaycc.co.uk) → review+
In fact, if you prefer, we can remove the cookie #ifdefs, as they depend on module registration anyway.
(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.
Attached patch Inlined reduced patch v0.4a (obsolete) — Splinter Review
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.
Attachment #208078 - Attachment is obsolete: true
Attached patch -w version of patch v0.4a (obsolete) — Splinter Review
For easier reading (hopefully)
Clearing obsolete blocking flag.  biesi: do you want to reconsider blocking flags?
Flags: blocking-seamonkey1.0a+
Attached patch Fewer ifdefs patch v0.4b (obsolete) — Splinter Review
Changes since v0.4b:
* Removed ifdefs for cookie stuff as suggested by Neil
* Stopped leaking for docshell
Attachment #208207 - Attachment is obsolete: true
Attachment #208429 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch -w version of patch v0.4b (obsolete) — Splinter Review
Attachment #208208 - Attachment is obsolete: true
Attachment #208429 - Flags: review?(neil.parkwaycc.co.uk)
Changes since v0.4b:
* Corrected test to docshellTreeItem instead of shell.
Attachment #208429 - Attachment is obsolete: true
Attachment #208431 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #208430 - Attachment is obsolete: true
Attachment #208429 - Attachment description: Less ifdefs patch v0.4b → Fewer ifdefs patch v0.4b
Attachment #208431 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #208431 - Flags: superreview?(bienvenu)
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.
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.
Attachment #208431 - Flags: superreview?(bienvenu) → superreview+
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
Attachment #208431 - Flags: approval1.8.1?
Attachment #208431 - Flags: approval-seamonkey1.1?
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
Attachment #208431 - Attachment description: Test for docshellTreeItem patch v0.4c → Test for docshellTreeItem patch v0.4c (Checked in trunk)
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.
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...)
Attachment #208431 - Flags: approval-seamonkey1.1? → approval-seamonkey1.1+
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. 
This patch:
* Changes default for *aDecision for NS_ENSURE_SUCCESS start
* Makes necessary tweaks further down the function after above default change
Attachment #209907 - Flags: superreview?(mscott)
Attachment #209907 - Flags: review?(mscott)
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
Attachment #208431 - Flags: approval1.8.1?
Blocks: 324992
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
Attachment #209907 - Flags: superreview?(mscott)
Attachment #209907 - Flags: superreview?(bienvenu)
Attachment #209907 - Flags: review?(neil)
Attachment #209907 - Flags: review?(mscott)
Attachment #209907 - Flags: superreview?(bienvenu) → superreview+
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.
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.
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.
Attachment #209907 - Flags: review?(neil) → review?(mscott)
Scott, do you want this in trunk? 1.8.1 branch?
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.
Attachment #209907 - Flags: review?(mscott)
Attachment #209907 - Flags: review+
Attachment #209907 - Flags: approval-branch-1.8.1+
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
Attachment #209907 - Flags: approval-seamonkey1.1a?
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
Attachment #209907 - Attachment description: Followup patch for trunk v0.4d_trunk → Followup patch for trunk v0.4d_trunk (Checked in)
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.
Attachment #209907 - Flags: approval1.8.0.2?
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.
Attachment #209907 - Flags: approval1.8.0.2?
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.
Attachment #209907 - Flags: approval-seamonkey1.1a? → approval-seamonkey1.1a+
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
Flags: blocking-seamonkey2.0a1?
Ian,
Are you still working on this ?
I think this is okay to mark as fixed, so doing so.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Flags: blocking-seamonkey2.0a1?
You need to log in before you can comment on or make changes to this bug.