seamonkey mailnews doing too-permissive content policy checks

RESOLVED FIXED

Status

defect
--
critical
RESOLVED FIXED
14 years ago
11 years ago

People

(Reporter: bzbarsky, Assigned: iann_bugzilla)

Tracking

({fixed-seamonkey1.1a, fixed1.7.9, fixed1.8.1})

Dependency tree / graph
Bug Flags:
blocking1.7.9 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:want] need trunk/1.8 patch)

Attachments

(5 attachments, 17 obsolete attachments)

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+

Comment 1

14 years ago
Any update here for 1.7.9?  Do we need to find an owner for this?
Assignee: dveditz → mscott

Comment 2

14 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?
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).

Comment 4

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

Comment 5

14 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.

Comment 6

14 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.
Attachment #186834 - Attachment description: (untested/compiled) possible fix → (untested/compiled) possible fix [needs check-in]

Comment 7

14 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.
Attachment #186834 - Flags: approval1.8b3?
Attachment #186834 - Flags: approval1.7.9+

Updated

14 years ago
Whiteboard: [sg:fix] → [sg:fix] needs landing by dveditz

Comment 8

14 years ago
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

Comment 11

14 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.  
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).

Comment 13

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

Comment 14

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

Comment 15

14 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?
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...

Comment 17

14 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?
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...

Comment 19

14 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.
mvl, any idea what's up there?

Comment 21

14 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.
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.

Comment 23

14 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.
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)

Comment 27

14 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...
Assignee: dveditz → bienvenu

Comment 28

14 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?
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

Comment 30

14 years ago
Posted 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.

Comment 32

14 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.
Attachment #188456 - Attachment is obsolete: true
Attachment #188470 - Flags: review?(bzbarsky)

Comment 33

14 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.
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-

Comment 37

14 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...
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.

Comment 39

14 years ago
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-

Comment 41

14 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?

Comment 42

14 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).
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-

Comment 44

14 years ago
Posted 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-

Comment 46

14 years ago
Posted 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+

Comment 48

14 years ago
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+

Updated

14 years ago
Attachment #188890 - Flags: superreview?(mscott) → superreview+

Comment 49

14 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.
Attachment #188890 - Flags: approval1.7.9?

Comment 50

14 years ago
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+

Comment 51

14 years ago
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?

Comment 53

14 years ago
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.

Updated

14 years ago
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
Assignee

Comment 56

14 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.
Assignee

Comment 57

14 years ago
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
Assignee

Comment 58

14 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
Attachment #207920 - Attachment is obsolete: true
Assignee

Updated

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

Updated

14 years ago
Depends on: 322824
Assignee

Comment 60

14 years ago
Posted 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)
Assignee

Updated

14 years ago
Attachment #208035 - Flags: review?(mnyromyr)
Unfortunately it refused to block any content :-(
Assignee

Comment 62

14 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
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?

Comment 64

14 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. 
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.
Assignee

Comment 67

14 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.
Assignee

Comment 68

14 years ago
Posted 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
Assignee

Comment 69

14 years ago
Posted 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+
Assignee

Comment 71

14 years ago
Posted 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)
Assignee

Comment 72

14 years ago
Posted patch -w version of patch v0.4b (obsolete) — Splinter Review
Attachment #208208 - Attachment is obsolete: true
Assignee

Updated

14 years ago
Attachment #208429 - Flags: review?(neil.parkwaycc.co.uk)
Assignee

Comment 73

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

Comment 74

14 years ago
Attachment #208430 - Attachment is obsolete: true

Updated

14 years ago
Attachment #208429 - Attachment description: Less ifdefs patch v0.4b → Fewer ifdefs patch v0.4b

Updated

14 years ago
Attachment #208431 - Flags: review?(neil.parkwaycc.co.uk) → review+
Assignee

Updated

14 years ago
Attachment #208431 - Flags: superreview?(bienvenu)

Comment 75

14 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.

Comment 76

14 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.
Attachment #208431 - Flags: superreview?(bienvenu) → superreview+
Assignee

Comment 77

14 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
Attachment #208431 - Flags: approval1.8.1?
Attachment #208431 - Flags: approval-seamonkey1.1?
Assignee

Comment 78

14 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
Attachment #208431 - Attachment description: Test for docshellTreeItem patch v0.4c → Test for docshellTreeItem patch v0.4c (Checked in trunk)

Comment 79

14 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.

Comment 80

14 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...)
Attachment #208431 - Flags: approval-seamonkey1.1? → approval-seamonkey1.1+

Comment 81

14 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. 
Assignee

Comment 82

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

Comment 83

14 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
Attachment #208431 - Flags: approval1.8.1?
Assignee

Updated

14 years ago
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
Assignee

Updated

14 years ago
Attachment #209907 - Flags: superreview?(mscott)
Attachment #209907 - Flags: superreview?(bienvenu)
Attachment #209907 - Flags: review?(neil)
Attachment #209907 - Flags: review?(mscott)

Updated

14 years ago
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.
Assignee

Comment 86

14 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.
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)
Assignee

Comment 88

13 years ago
Scott, do you want this in trunk? 1.8.1 branch?

Comment 89

13 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.
Attachment #209907 - Flags: review?(mscott)
Attachment #209907 - Flags: review+
Attachment #209907 - Flags: approval-branch-1.8.1+
Assignee

Comment 90

13 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
Attachment #209907 - Flags: approval-seamonkey1.1a?
Assignee

Comment 91

13 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
Attachment #209907 - Attachment description: Followup patch for trunk v0.4d_trunk → Followup patch for trunk v0.4d_trunk (Checked in)

Comment 92

13 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.
Attachment #209907 - Flags: approval1.8.0.2?

Comment 93

13 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.
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+
Assignee

Comment 95

13 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
Assignee

Updated

13 years ago
Flags: blocking-seamonkey2.0a1?
Ian,
Are you still working on this ?
Assignee

Comment 97

11 years ago
I think this is okay to mark as fixed, so doing so.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Flags: blocking-seamonkey2.0a1?
You need to log in before you can comment on or make changes to this bug.