295994 - (CVE-2008-5505) Can store cookie-like information via xul persist attribute

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[Hish]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.8) Gecko/20050511
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.8) Gecko/20050511

When user turns off cookies, I still can write cookie-like information by using
the xul persist attribute on a remote page.

Reproducible: Always

Steps to Reproduce:
<xul:element id="test" persist="att1,att2" />
Store:
document.getElementById('test').setAttribute('att1','xxx');
Restore:
alert(document.getElementById('test').getAttribute('att1'));


Expected Results:  
I expect a remote XUL page being able to work with the persist attribute but
onla when the user allows cookies from this site.

Comment 1

[Mike Connor [:mconnor]]

They're entirely separate concepts and expecting that a policy about cookies
automatically applies to any form of persistent data is a false assumption.

That said, this seems to be more of a privacy issue than of actual security. 
There's also the aspect that even if you load "foo.xul" in a hidden iframe from
a third party (in order to track in a pseudo-cookie manner) this is of far less
value, since manipulating the content in the hidden iframe would/should fail a
same-origin check.

Comment 2

[Daniel Veditz [:dveditz]]

There are definite privacy implications here. Not so much in the default case
since cookies are on by default, but this easily bypasses restrictions on cookie
size and number, and could not be managed or deleted by users.

Comment 3

[Bob Clary [:bc] (inactive)]

Hish, I tried to create a testcase to illustrate this but failed to reproduce
the described behavior. I'm not much of a XUL coder, so it is probably my
failing. Could you attach a XUL file containing a working testcase illustrating
the problem?

Comment 4

[Hish]

Attachment: Example file

Comment 5

[Daniel Veditz [:dveditz]]

We should require at least UniversalBrowserWrite to persist information, if not
restrict it to chrome entirely. nsXULTreeBuilder already requires chrome privs
to use anything other than an in-memory datasource, so that's a consistent
choice and quite possibly the easiest.
http://lxr.mozilla.org/mozilla/source/content/xul/templates/src/nsXULTreeBuilder.cpp#798

Looks like we'd want checks in nsXULDocument and nsXULContentBuilder

Comment 6

[Benjamin Smedberg]

xul:persist is an important part of remote XUL and we should not remove the
capability arbitrarily. If we want to come up with some solution to restrict it
based on cookie prefs etc. that's ok.

Comment 7

[Hish]

(In reply to comment #6)
> If we want to come up with some solution to restrict it
> based on cookie prefs etc. that's ok.

Exactly. Restricting it to UniversalBrowserWrite would again mean to cut a
feature. At the end of the day this persistency is nothing else but a cookie
which the user might reject. Best would be if could optionally be able to treat
it like a session cookie. 

Comment 8

[Daniel Veditz [:dveditz]]

Treat it like a cookie? You mean let any site store as much as they want (unlike
cookies which have a limit) without the average user knowing (this wouldn't show
up in the cookie viewer) until the whole thing blows up on us like the Flash
"PIE" thing recently did? Except Flash *did* have a built in mechanism for
limiting remote storage and simply chose generous defaults, and we don't even
have that.

I was going to recommend that XUL apps just *use* cookies and take advantage of
all of the existing user controls and security checks, but the document.cookie
mechanism is apparently not supported in XUL. It probably should be, it looks
like a whole lot less work than adding a parallel set of controls and limits.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

IMHO the best solution here would be to extend our various cookie mechanisms
such that we can treat persistent attributes just like cookies. This means
limiting the storage size of each individual sites usage and the total storage
size (though chrome xul should override these limits). It also means making
persisted attributes show up in the cookie manager and make them be cleared
along with cookies in the FF privacy-thingie (don't remember the name).

I do realize that this might be non-trivial though.

But if this is done well enough we could also let pages persist other things,
such as form-values and even parts of the DOM tree.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I should add that doing that would probably be more of a long term solution. For
now maybe we could make xul support .cookies and limit persitent attributes to
chrome?

Comment 11

[Hish]

#8:  Use cookies within xul. Sounds good.
#10: What do you think we need persists here? Window sizes and positions,
selected states etc. within remote apps. When you limit that to chrome then
another important feature of remote apps is broken. 
Like Daniel Veditz I think that storing unlimited data via persist is dangerous. 
But what I'm talking about less than 1k per app. 
So why not treating persist like a cookie. They have their defined limitations,
the user knows (or should know) what they are, and in fact 'xul persist' is
nothing else.

I'm viewing the whole discussion from my well known remote-app perspective and
I'm really curious if we end like most of the time in disabling a feature
instead of fixing the problem.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I defenetly agree that the best solution is not to kill the persist feature, but
to fix it. However it's a matter of what we can do quick enough for a security
release.

If we enable cookies instead of persist you can still do the exact same thing
and persist positions etc, you just have to go through a little bit of a trouble
to copy attribute-values into a cookie when the window is closed and then copy
it back from the cookie when its opened. You could probably even write a utility
function that did this automatically.

But yeah, ideally i'd like to see the RightThing done right away.

Maybe a relativly simple fix is to hack the persist code so that it behaves
similar to the cookie code for now.

Comment 13

[Benjamin Smedberg]

It seems to me that implementing xul:persist in terms of cookies would not be
all that hard for remote content, I can look at that possibility for 1.0.6/1.1b.

Comment 14

[Benjamin Smedberg]

Attachment: Part 1, rev. 1 - Add additional "safe" helper methods to the cookie service

Comment 15

[Benjamin Smedberg]

Attachment: Part 1, rev. 2 - Add additional "safe" helper methods to the cookie service

Comment 16

[Benjamin Smedberg]

Attachment: Part 2, rev.1 - Use cookies to persist/restore in nsXULDocument

I am going to attach two versions of this patch: this one (the smaller one)
keeps the existing "restore" codepath from localstore which restores attributes
even if they don't have a persist attribute any more.

The second version uses a nsContentList enumerator and walks the tree to
restore persist in both the localstore and cookie cases, which will fix the
mostly-duplicate bug 231333, bug 231334, and bug 251273, but has a much higher
chance of causing Ts/TXul regressions.

Comment 17

[Benjamin Smedberg]

Comment on attachment 190749 [details] [diff] [review]
Part 2, rev.1 - Use cookies to persist/restore in nsXULDocument

This is wrong, needs a little more work.

Comment 18

[Benjamin Smedberg]

Attachment: Part 2, rev.2 - Use cookies to persist/restore in nsXULDocument

This combines "option 1" and "option 2" in one patch, with an #ifdef... the
safe-old-way is to not-define LOAD_PERSIST_BY_CONTENTLIST

Comment 19

[Boris Zbarsky [:bzbarsky]]

I'm not sure I'll be able to get to this before I leave (and hence before I get
back on Aug 14th).

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 190844 [details] [diff] [review]
Part 2, rev.2 - Use cookies to persist/restore in nsXULDocument

>+    // The format of a persist cookie is <URIspec>#<ID>#<attname>
>+    nsCAutoString cookiename;
>+    rv = mDocumentURI->GetSpec(cookiename);

So a URI with a '#' in it is going to mess you up, right? I guess it's not too
likely to happen in normal practice, but could anything malicious be done with
it? Are there elements we don't allow to persist that this could be used to get
some persistence for anyway?

We probably want to set the cookie path from the documentURI so this cookie
doesn't pollute higher level pages on the site. If we've got the right path
would it be enough to use just the filename instead of the full URI? That
assumes we can QI to nsIURL and get the extra filename/query/ref etc bits out
of the location, but for http: and ftp: we can.

Is there a better separator that's not legal in either URIs or ID's?

What happens for non-chrome XUL documents that don't have a host? We wouldn't
want a local file:// testpage to accidentally share a jar:http://foo.com/
persist cookie (although maybe we handle jar:http: cookies correctly). Maybe
the full URI in the cookie name is the right thing afterall, though verbose.

>+    // we pick a value for expire that is 100 years in the future.

Seems a little extreme.

Comment 21

[Benjamin Smedberg]

> So a URI with a '#' in it is going to mess you up, right? I guess it's not too

No. We always form the URI the same way, we never "parse" a cookie looking for
URL/ID/attribute.

> We probably want to set the cookie path from the documentURI so this cookie
> doesn't pollute higher level pages on the site. If we've got the right path
> would it be enough to use just the filename instead of the full URI? That
> assumes we can QI to nsIURL and get the extra filename/query/ref etc bits out
> of the location, but for http: and ftp: we can.

Sounds reasonable.

> >+    // we pick a value for expire that is 100 years in the future.
> 
> Seems a little extreme.

I wanted "forever", but we don't really support that. What is extreme about it?
The user's max-age cookie preferences are still obeyed.

Comment 22

[Benjamin Smedberg]

Attachment: Part 2, rev. 2.1 - Use cookies to persist/restore in nsXULDocument

I misread your comment a bit... I think we should still always set the full URL
in the cookie, but I have also set the cookie path/ when available to avoid
polluting the cookiespace more than necessary.

Comment 23

[Darin Fisher]

Hmm... so the new Cookie Service interface exists for performance?  Is the cost
of building up a cookie string really that significant?

Comment 24

[Benjamin Smedberg]

Setting cookies doesn't look especially perf-intensive: *parsing* the cookie
string does look quite complicated and very hard to make perf-efficient.

Comment 25

[Darin Fisher]

Parsing cookie strings is expensive relative to what?  To page load time? 
Surely not.  That said, it's not that I'm against the new API.  It just seems
like extra engineering for very little gain :-/

Comment 26

[Benjamin Smedberg]

Maybe it can be made not part of a critical codepath if we arrange the code
correctly (get the cookies before entering the contentlist loop), but it's still
a lot of parsing code which I frankly don't know how to write (I never learned
to use cookies from the DOM, and there seem to be lots of "extras" that require
special handling), and to do it efficiently I think I'd have to keep a
parsed-cookie hashtable on the stack while I'm resolving persistence. Overall I
tend to think that the current approach reduces the overall amount of code and
performs better.

Comment 27

[[e-mail]]

I'd like to comment on the cookie parts of this patch... (sometime this week).

Comment 28

[Darin Fisher]

bsmedberg: ok, i agree.  making you parse cookie strings is silly.  for some
reason i was only thinking about setting cookies :(

Comment 29

[Daniel Veditz [:dveditz]]

Comment on attachment 190944 [details] [diff] [review]
Part 2, rev. 2.1 - Use cookies to persist/restore in nsXULDocument

r=dveditz

Comment 30

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 190745 [details] [diff] [review]
Part 1, rev. 2 - Add additional "safe" helper methods to the cookie service

moving review request to dwitte, who said he wanted to look at the patch.

Comment 31

[[e-mail]]

Comment on attachment 190745 [details] [diff] [review]
Part 1, rev. 2 - Add additional "safe" helper methods to the cookie service

>Index: netwerk/cookie/public/nsICookieService2.idl
>===================================================================
>+[scriptable, uuid(7cf4b3d9-bdc6-4763-b912-47c526aacc37)]
>+interface nsICookieService2 : nsICookieService

why does this need to be scriptable? if possible, i'd prefer this interface to
be internal - it exposes details of cookies that can easily change in future,
so there should be no illusion of API compat. (i'm not sure if we have a
convention either way, but perhaps nsICookieServiceInternal would be a better
name?)

>+  void setCookieValue(in nsIURI aURI, in nsIPrompt aPrompt,

aPrompt is a kinda historical parameter in cookies nowadays, you can remove it
here. (and in your other patch that fetches a prompter to pass in.)

>+   * @return the resulting cookie value
>+   * @throws NS_ERROR_NOT_AVAILABLE if no cookie with this name has been set.
>+   */
>+  ACString getCookieValue(in nsIURI aURI, in nsIChannel aChannel, in ACString aCookieName);

can you document the fact that it returns the first cookie found if there are
multiple matches for the name?

>Index: netwerk/cookie/src/nsCookieService.cpp
>===================================================================
>+void
>+nsCookieService::getCookieList(nsIURI           *aHostURI,

can you call this GetCookieList() to be consistent with other methods?

>+NS_IMETHODIMP
>+nsCookieService::GetCookieValue(nsIURI *aHostURI, nsIChannel *aChannel,
>+                                const nsACString& aName, nsACString& aResult)

nit, can you put the params each on their own line?

>+void
>+nsCookieService::CheckAndAdd(nsIURI               *aHostURI,
>+                             nsIChannel           *aChannel,
>+                             nsCookieAttributes   &aAttributes,

ubernit, you can |const|ify the nsCookieAttributes here i think.

r=me if we resolve the internal-ness of the added interface.

Comment 32

[[e-mail]]

(on that note, could you add a warning comment to the idl indicating that it is
internal and subject to change?)

Comment 33

[Darin Fisher]

Comment on attachment 190745 [details] [diff] [review]
Part 1, rev. 2 - Add additional "safe" helper methods to the cookie service

sr=darin with dwitte's nits.  I'm okay with the new interface being scriptable,
but yeah... perhaps "Internal" is a better suffix until we are ready for this
to be used outside of Gecko.

Comment 34

[Benjamin Smedberg]

> aPrompt is a kinda historical parameter in cookies nowadays, you can remove it
> here. (and in your other patch that fetches a prompter to pass in.)

Are we sure that the channel prompter is going to be the same as the xul
document prompter?

Comment 35

[Christian :Biesinger (don't email me, ping me on IRC)]

the channel's prompter is generally whatever the docshell hands out in
GetInterface(nsIPrompt).

Comment 36

[Benjamin Smedberg]

Part 1 checked in on trunk with nits fixed. I'll attach the final patch shortly.

Comment 37

[Benjamin Smedberg]

Attachment: Part 1, final patch as checked in

Need a couple days to verify this didn't break anything on the trunk. Also,
approving it is kinda silly unless we're going to get part 2 reviewed and
landed on the branch also.

Comment 38

[Benjamin Smedberg]

Attachment: Part 2, rev. 2.2 - Use cookies to persist/restore in nsXULDocument

This is the same as part 2 rev 2.1 except I've updated it to the
"nsICookieServiceInternal" name change. Carrying over r=dveditz

Comment 39

[Asa Dotzler [:asa]]

Comment on attachment 196153 [details] [diff] [review]
Part 1, final patch as checked in

please re-request review when this is fully ready to go.

Comment 40

[Asa Dotzler [:asa]]

dbaron, can you help us with a super-review here? Time is running out for 1.8b5.

Comment 41

[Asa Dotzler [:asa]]

I think that this is getting too scary for this late in the game. Since this is
a privacy and not security issue, I don't think we can block on this.

Comment 42

[David Baron :dbaron:]

Comment on attachment 196156 [details] [diff] [review]
Part 2, rev. 2.2 - Use cookies to persist/restore in nsXULDocument

The nsXULDocument.h include changes should probably be removing both
halves, or as much as possible of both so things still compile.

In PersistToCookie, at the very least assert right after
mDocumentURI->GetSpec that cookiename does not contain '#' (but it
should be a runtime check if it could be needed; I'm not sure if it is).
(Likewise for ApplyPersistFromCookies, I think.)

In PersistToCookie, you shouldn't depend on the ID attribute not having
a '#' in it.  You should either add code to escape it or bail out if it
contains a '#'.
(Likewise for ApplyPersistFromCookies, I think.)
Then again, if it's *only* the ID attribute that could have a '#' then
at least persistence couldn't get crossed, so this isn't a problem, as
long as ApplyPersistFromCookies builds up the string rather than
enumerating cookies (which would probably be superior, but isn't what
you're doing).

In PersistToCookie, you should probably put the expiry initialization
within |if (rv == NS_CONTENT_ATTR_HAS_VALUE)| for two reasons:	(1) save
the work of initializing expiry (PR_Now() may not be cheap) and (2) the
way the current code is structured, it's easy for somebody to
accidentally add an assignment to rv between the assignment from
GetAttr's result and the check.

I really don't understand why you use the domain and path values for the
cookie that you do, but SetCookieValue doesn't seem all that well
documented.  Is what you're doing the right way to make the cookie
specific to the page?  If so, say so in a comment, since it's not clear
to me.

In ApplyPersistFromCookies, nsContentList is reference counted; you
shouldn't delete it.  It's also got expensive liveness code; do you
really need that?  (I didn't check that the constructor call actually
makes sense; I didn't find good documentation for what those constructor
parameters mean.)

Why does ApplyPersistFromCookies pass PR_FALSE for aNotify to SetAttr
when ApplyPersistFromLocalstore passes PR_TRUE?

I didn't review the new version of ApplyPersistFromLocalstore; this
should be superreviewed before it is enabled.

Is it a problem that you're changing the behavior when an attribute used
to be part of the persist attribute (and has a persisted value) but no
longer is part of that attribute?  I think this change is probably good,
but it should be documented, it could easily break things, and so far
you're changing only for the cookie case, since the ifdef for new
persistence code for localstore isn't enabled.	You're also currently
changing this only for the persistence to cookie case, since the
CONTENTLIST ifdefs aren't enabled, but it would be easy to make it true
for all cases even with the non-nsContentList code by checking the
persist attribute before restoring.  And enumerating and checking the
persist attribute before restoring might generally be a
better-performing solution for both cases (I'm assuming that elements
with IDs are added to a map when they're created so it's easy to find
them).	(Have you tested the performance of this nsContentList
solution?)

Another place where you're breaking compatibility (although without the
CONTENTLIST ifdef only for the cookie case) is what I'd also consider a
bug fix, but it should probably be document:  previously
persist="abcdef" persisted attributes "abcdef", "a", "c", "cde", etc.,
since nsXULDocument::AttributeChanged only did a substring check.  We'll
*still* be doing the work to persist those (but probably shouldn't be),
but we'll never restore them, at least for some of the persistence
codepaths.  That deserves at least an XXX comment, but we probably
shouldn't persist them at all if we're not going to restore them.

Comment 43

[Darin Fisher]

Question about this patch:

1) Is it going to happen for FF 1.5?

2) How will this impact web traffic if a remote XUL application persists some
stuff?  It seems to me that the web traffic would start to include a bunch of
cookies, or am I missing something?

Comment 44

[Benjamin Smedberg]

Since 1.8 is past and this is significantly risky, I'm going to let this sit and wait for supercookies to develop before proceeding.

Comment 45

[Daniel Veditz [:dveditz]]

Without a safe tested patch I don't think this is 1.0.8 material. Moving from blocking back to the nomination list.

Comment 46

[Daniel Veditz [:dveditz]]

bug 319846 demonstrates a permanent startup "hang" using XUL persist attributes.

Comment 47

[Bob Clary [:bc] (inactive)]

(In reply to comment #4)
> Created an attachment (id=188491) [edit]
> Example file
> 

crashed in firefox 1.5.0.7/20060906/winxp tb22993780
ntdll.dll + 0x1010 (0x7c901010)
DOMGCCallback  [mozilla/dom/src/base/nsJSEnvironment.cpp, line 2234]
js_ForceGC  [mozilla/js/src/jsgc.c, line 1515]

no crash bonecho/minefield

Comment 48

[Mike Kaply [:mkaply]]

Is there any reason to put this on the 1.8 branch? I'm working on getting bug 178993 on the branch, and that patch was on top of this change.

I have a version that doesn't require this change, just trying to decide with The Right Thing(TM) is.

Comment 50

[georgi - hopefully not receiving bugspam]

some public bugs mention |persist| and |localstore| so delaying this looks like a privacy scandal

Comment 51

[Benjamin Smedberg]

I think now that we have DOM storage we should layer localstore directly on it. dcamp, if you can get that working, I can write an importer for the existing data in localstore.rdf.

Comment 52

[[e-mail]]

(In reply to comment #51)
> I think now that we have DOM storage we should layer localstore directly on it.

does this mean the patches here are obsolete? if so, is there any justifiable use for the cookie api changes ("part 1" patch) which are currently in the tree, awaiting consumer code ("part 2")?

Comment 53

[Benjamin Smedberg]

Yes, the patches are obsolete, and yes, the cookie APIs would not be needed. Do with them what you want...

Comment 54

[[e-mail]]

(In reply to comment #53)
> Yes, the patches are obsolete, and yes, the cookie APIs would not be needed. Do
> with them what you want...

changes have been removed (bug 384990). should this bug be closed out, or left open for dom storage work?

Comment 55

[Benjamin Smedberg]

This bug is still here.

Comment 56

[Daniel Veditz [:dveditz]]

Why is this not blocking 1.9? Unlimited, unclearable, unblockable, invisible "cookies" are bad. Look at the rumblings as people discover DOM globalStorage, and that actually has a defensible privacy story.

Comment 57

[Mike Connor [:mconnor]]

I suspect its not a blocker because its sg:low at this point.

I'm tempted to just break persist in remote XUL by default, but I think that remote XUL is something we want to kill in the medium to long term in favour of better HTML.  I'm not yet ready to have that battle, I think that's best timed for Moz2.

Right now the globalStorage stuff gets cleared when cookies get cleared, is tying xul persist to this, and the global cookie prefs (like domstorage also already does) sufficient?  I think its unlikely that anyone using this wouldn't also be using cookies, so anyone caring enough to blocklist sites will be covered without having to be aware of alternate forms of persist.  If this is sufficient, can we do this?  Doesn't seem massively bad, especially if we add a global (hidden) pref to disable this for people who really want only visible cookies.

Comment 58

[David Baron :dbaron:]

Is the patch here *necessarily* obsolete?

Comment 60

[Tony Chung [:tchung]]

Hi, can you please update the target milestone for this bug?   Thanks.

Comment 61

[Honza Bambas (:mayhemer)]

Attachment: Draft according to comment 57

Patch is tested with attachment 188491 [details] and attachment 104910 [details].

Comment 62

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Honza: thanks for that. I think instead of calling IsChromeURI you might want to check the document's principal, IsCapabilityEnabled "UniversalBrowserWrite". That would allow any trusted XUL document to use persistence.

After you're happy with the patch you should ask for review, probably from Jonas Sicking ([e-mail]).

Comment 63

[Honza Bambas (:mayhemer)]

Attachment: Draft according to comment 57, ver2

Checking "UniversalBrowserRead" when applying and "UniversalBrowserWrite" when persisting. I am checking principal of the underlaying channel. If there is no channel or no principal then the result of my isCapabilityEnabled is FALSE. I'm not expert for nsWindow and therefor it might be too restrictive in some extreme cases.

Comment 64

[georgi - hopefully not receiving bugspam]

once upon a time m$ tried `lame super cookies' (hint super cookie, wmplayer, uuid)

this bug and its duplicates are quite to close a lame super cookie imho

please don't make Window Snyder make up stories in her blog like `we care about your privacy. this is sg:low. if you need privacy, fight for it'

Comment 65

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 296354 [details] [diff] [review]
Draft according to comment 57, ver2

You should call NodePrincipal() (lives on nsINode) rather than getting the principal from the channel.

Looks good otherwise.

Comment 66

[Honza Bambas (:mayhemer)]

Attachment: ver3

I am using document's NodePrincipal(). It is guarantied to be non-null so I omitted !null check. Other possible way is to check NodePrincipal of each node/element I want to write/read/restore attributes to/of (?).

Comment 67

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 310493 [details] [diff] [review]
ver3

>+nsXULDocument::IsCapabilityEnabled(const char* aCapabilityLabel)
>+{
>+    nsresult rv;
>+
>+    // NodePrincipal is guarantied to be non-null
>+    nsCOMPtr<nsIPrincipal> nodePrincipal = NodePrincipal();
>+
>+    PRBool ubwEnabled = PR_FALSE;
>+    rv = nodePrincipal->IsCapabilityEnabled(aCapabilityLabel, nsnull, &ubwEnabled);

Simply do
NodePrincipal()->IsCapabilityEnabled(...

> nsXULDocument::LoadScript(nsXULPrototypeScript* aScriptProto, PRBool* aBlock)
> {
>     // Load a transcluded script
>     nsresult rv;
> 
>-    PRBool isChromeDoc = IsChromeURI(mDocumentURI);
>-
>-    if (isChromeDoc && aScriptProto->mScriptObject.mObject) {
>+    if (aScriptProto->mScriptObject.mObject) {
>         rv = ExecuteScript(aScriptProto);
> 
>         // Ignore return value from execution, and don't block
>         *aBlock = PR_FALSE;
>         return NS_OK;
>     }
> 
>     // Try the XUL script cache, in case two XUL documents source the same
>     // .js file (e.g., strres.js from navigator.xul and utilityOverlay.xul).
>     // XXXbe the cache relies on aScriptProto's GC root!
>     PRBool useXULCache = nsXULPrototypeCache::GetInstance()->IsEnabled();
> 
>-    if (isChromeDoc && useXULCache) {
>+    if (useXULCache) {

Why these changes? Bad merge?

Comment 68

[Honza Bambas (:mayhemer)]

Attachment: ver3 final

Sorry for bad diff, my compare threes become different. Now using CVS diff.

Comment 69

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 310756 [details] [diff] [review]
ver3 final

Looks good.

Comment 70

[Mike Schroepfer]

Comment on attachment 310756 [details] [diff] [review]
ver3 final

a+ schrep

Comment 71

[Reed Loden [:reed]]

Checking in content/xul/document/src/nsXULDocument.cpp;
/cvsroot/mozilla/content/xul/document/src/nsXULDocument.cpp,v  <--  nsXULDocument.cpp
new revision: 1.822; previous revision: 1.821
done
Checking in content/xul/document/src/nsXULDocument.h;
/cvsroot/mozilla/content/xul/document/src/nsXULDocument.h,v  <--  nsXULDocument.h
new revision: 1.205; previous revision: 1.204
done

Comment 72

[Johnathan Nightingale [:johnath]]

Backed out since this is likely the cause of a reftest failure:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1207304616.1207306050.2695.gz#err0

That test uses a xul document with persistence, so this seems like the one.  :)  Probably the reftest should be fixed, rather than this bug?

Checking in content/xul/document/src/nsXULDocument.cpp;
/cvsroot/mozilla/content/xul/document/src/nsXULDocument.cpp,v  <--  nsXULDocument.cpp
new revision: 1.823; previous revision: 1.822
done
Checking in content/xul/document/src/nsXULDocument.h;
/cvsroot/mozilla/content/xul/document/src/nsXULDocument.h,v  <--  nsXULDocument.h
new revision: 1.206; previous revision: 1.205
done

Comment 73

[Johnathan Nightingale [:johnath]]

Backing this out did indeed clear the mac and windows failures (linux is still cycling as I type).  Given the simplicity of the patch and the clearly deliberate disabling of persist for xul content, I suspect the test should be neutered or changed, but I'll defer to the experts here.

Jonas, you reviewed the patch, bz, you wrote the test, thoughts?  :)

Comment 74

[Boris Zbarsky [:bzbarsky]]

The test is explicitly testing that persistence works for a particular toolkit widget.  It's testing a bug originally reported against chrome (as you can see if you read the bug corresponding to the test name).  So the right thing to do here is probably to change that test to run against chrome, somehow.

Comment 75

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That's what we have the chrome tests for, right?

http://developer-cluster.mozilla.org/en/docs/Chrome_tests

Honza: Think you'd be able to convert the failing test into a chrome test?

Comment 76

[Honza Bambas (:mayhemer)]

The current test is about testing persistence of the selectedIndex in a tab box. So, have a chrome (mochi) test checking its value after reload should be enough. I will create such a test.

Comment 77

[Honza Bambas (:mayhemer)]

Attachment: changed test for bug 398289

Reftest changed to chrome test.

Comment 78

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 313683 [details] [diff] [review]
changed test for bug 398289

Excellent!

Comment 79

[Damon Sicore (:damons)]

Comment on attachment 313683 [details] [diff] [review]
changed test for bug 398289

thanks for updating the tests!

a1.9+=damons

Comment 80

[Boris Zbarsky [:bzbarsky]]

Does that new test fail if the patch for the relevant bug is backed out?  I distinctly recall that my initial test looked like that and didn't actually test the bug because the property was out of sync with the display....

Comment 81

[Reed Loden [:reed]]

RCS file: /cvsroot/mozilla/content/xul/content/test/398289-resource.xul,v
done
Checking in content/xul/content/test/398289-resource.xul;
/cvsroot/mozilla/content/xul/content/test/398289-resource.xul,v  <--  398289-resource.xul
initial revision: 1.1
done
Checking in content/xul/content/test/Makefile.in;
/cvsroot/mozilla/content/xul/content/test/Makefile.in,v  <--  Makefile.in
new revision: 1.4; previous revision: 1.3
done
RCS file: /cvsroot/mozilla/content/xul/content/test/test_bug398289.html,v
done
Checking in content/xul/content/test/test_bug398289.html;
/cvsroot/mozilla/content/xul/content/test/test_bug398289.html,v  <--  test_bug398289.html
initial revision: 1.1
done
Checking in content/xul/document/src/nsXULDocument.cpp;
/cvsroot/mozilla/content/xul/document/src/nsXULDocument.cpp,v  <--  nsXULDocument.cpp
new revision: 1.824; previous revision: 1.823
done
Checking in content/xul/document/src/nsXULDocument.h;
/cvsroot/mozilla/content/xul/document/src/nsXULDocument.h,v  <--  nsXULDocument.h
new revision: 1.207; previous revision: 1.206
done
Removing layout/reftests/bugs/398289-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/398289-1-ref.html,v  <--  398289-1-ref.html
new revision: delete; previous revision: 1.3
done
Removing layout/reftests/bugs/398289-1.html;
/cvsroot/mozilla/layout/reftests/bugs/398289-1.html,v  <--  398289-1.html
new revision: delete; previous revision: 1.2
done
Removing layout/reftests/bugs/398289-resource.xul;
/cvsroot/mozilla/layout/reftests/bugs/398289-resource.xul,v  <--  398289-resource.xul
new revision: delete; previous revision: 1.1
done
Checking in layout/reftests/bugs/reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.433; previous revision: 1.432
done

Comment 82

[Reed Loden [:reed]]

I screwed up the log message on commit, so I filed bug 427819 to fix it.

Comment 83

[Boris Zbarsky [:bzbarsky]]

I'd still like an answer to the question in comment 80.

Comment 84

[Honza Bambas (:mayhemer)]

(In reply to comment #80)
> Does that new test fail if the patch for the relevant bug is backed out?  I
> distinctly recall that my initial test looked like that and didn't actually
> test the bug because the property was out of sync with the display....
> 

When the patch for this bug is backed out the new (fixed) test will pass either. The patch for this bug only disallows for foreign xul and all html pages use of the persistent attributes. As I understand.

The test (original and the new fixed one too) verifies whether the persistent attribute works for selectedIndex of tabboxes.

Comment 85

[Honza Bambas (:mayhemer)]

Opps.. not patch for this bug for THAT bug. Yes, you are right, I didn't test. Will do that and report.

Comment 86

[Daniel Holbert [:dholbert]]

Note that test_bug398289.html (which was modified in this bug's checkin) is currently failing on Linux:

  * Failure Log:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1207682613.1207686773.29781.gz
  * File itself:
http://bonsai.mozilla.org//cvsblame.cgi?file=mozilla/content/xul/content/test/test_bug398289.html

Honza, is that what you're talking about in comment 85?

Comment 87

[:Gavin Sharp [email: [e-mail]]]

I backed this out because the test was failing:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1207682613.1207686773.29781.gz&fulltext=1

(Also, I think comment 80 should have been addressed before it was checked in.)

Comment 88

[:Gavin Sharp [email: [e-mail]]]

My backout accidentally reverted the reftest.list change without re-adding the removed files, so I relanded that part. All that needs to be relanded are the new/changed files when the test failure is addressed.

Comment 89

[Honza Bambas (:mayhemer)]

So, the test when attachment 282959 [details] [diff] [review] is backed out (needed some merge) *DOES NOT FAIL*. The tab is switched but content of the windows is not. Thus, we need a chrome reftest or a different way of checking in the new pure chrome test.

Comment 90

[Boris Zbarsky [:bzbarsky]]

> The tab is switched but content of the windows is not.

Yep.  That's exactly what I recall seeing and why I made it a reftest...

Comment 91

[Honza Bambas (:mayhemer)]

Hmm.. this won't be that easy. I added by hacking of layout/tools/reftest/jar.mn the resource file for the test to be in the chrome of the test framework. I get the following error:

Security Error: Content at file:///D:/mozilla/layout/reftests/bugs/
398289-1.html may not load or link to chrome://reftest/content/bugs/398289-resou
rce.xul. 

The path is correct. File resides in _obj/dist/bin/chrome/reftest/content/bugs/. I am afraid we will need a chromereftest framework.

Comment 92

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'd say create a test that can be run manually to test your patch. File a bug on getting a chrome-ref-test framework (or expand the reftest framework to support chrome files). Stick your manual test into that bug.

That way we can for now know that your patch is correct. And later make sure it doesn't regress.

Comment 93

[Honza Bambas (:mayhemer)]

If I add the test html and ref html to a chrome (I hack the reftest chrome again) and I add to the test list line "== chrome://reftest/content/bugs/398289-1.html chrome://reftest/content/bugs/398289-1-ref.html" it almost works. With one exception. Bug 371075 added checking if the URL could be loaded by reftest framework. URLs are tested against the path to the test list (to the file URL) that doesn't allow load of any chrome.

I changed the test to check against the location of window of the test (chrome://reftest/content/reftest.xul) and then the framework works with chrome URLs in a list. But I am not sure we may change it this way, David, can you give an advise here please?

Comment 94

[Honza Bambas (:mayhemer)]

Comment on attachment 313683 [details] [diff] [review]
changed test for bug 398289

This test is not correct to check presence of bug 398289.

Comment 95

[Honza Bambas (:mayhemer)]

Attachment: changed test for bug 398289, v2

This patch turns reftest for bug 398289 to a chrome-reftest. There is need to change the framework slightly. Not sure that is correct...

Comment 96

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 316791 [details] [diff] [review]
changed test for bug 398289, v2

I think david would be better for review of reftest changes.

Comment 97

[David Baron :dbaron:]

Could you explain the reftest.js changes?

Waldo might have some better ideas on how to this.  Seem like maybe we might have a way in the manifest to load the files with chrome privs rather than having to give them chrome: URLs.

Comment 98

[Jeff Walden [:Waldo]]

Nothing really springs to mind; we really should make reftest create its own profile which we could customize to make the source tree accessible for chrome: tests.  It wouldn't be hard to do (I think there's a script in build/ to create profiles), but it would need to be done.

Comment 99

[Honza Bambas (:mayhemer)]

I created a script that creates new profile, adds privilege preferences to the pref file and runs the test list. Preferences are:
user_pref("capability.principal.codebaseTrusted.p0.granted", "UniversalXPConnect UniversalBrowserRead UniversalBrowserWrite UniversalPreferencesRead UniversalPreferencesWrite UniversalFileRead");
user_pref("capability.principal.codebaseTrusted.p0.id", "file://");
user_pref("capability.principal.codebaseTrusted.p0.subjectName", "");
user_pref("signed.applets.codebase_principal_support", true); 

This still doesn't help - xul iframe in our test has not got access granted. I get: 
"JavaScript error: chrome://global/content/bindings/dialog.xml, line 186: Permission denied to get property XPCComponents.classes"

The test perfectly works with my changes to the test framework. The reftest.js changes modifies the uri against we do the permission test. Before it were testing against URL of the test list file (maybe a real bug?), with my changes it test against the URL of the file test file.

Comment 100

[Adam Guthrie]

(In reply to comment #99)
> This still doesn't help - xul iframe in our test has not got access granted. I
> get: 
> "JavaScript error: chrome://global/content/bindings/dialog.xml, line 186:
> Permission denied to get property XPCComponents.classes"

I googled this and came across the following Mochitest documentation: http://developer.mozilla.org/en/docs/Mochitest#How_can_I_get_around_the_error_.22Permission_denied_to_get_property_XPCComponents.classes.22.3F. So, I guess one would need to include that line of JS in each test file in order for this approach to work.

Interestingly enough, that documentation suggests adding a hack to Mochitest that would essentially do what your aforementioned patch has done for reftest.

Comment 101

[Honza Bambas (:mayhemer)]

Current progress: I modified my script to use the same approach as mochitest to access the source/test files from chrome. There is no need to modify preferences and actually no need to have a profile. However, I still have this feature. It works on windows, but it will take additional time to port it to Mac OS and I don't have a way to test this on linux (don't have a linux machine). This script still needs the change in reftest.js.

If you agree this approach is correct then let me know please. Otherwise my first patch is probably sufficient, at least because we need to get this in ASAP (the patch fixing this real bug hangs only on a failing test for completely different bug).

Comment 102

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So personally I think it's ok to check in this patch without automated tests, as long as we have manual tests that can be used to verify the fix. We can add automated tests once we have chrome-reftests or reftests-with-custom-profile.

Comment 103

[Boris Zbarsky [:bzbarsky]]

The problem is not that _this_ patch can't be tested, but that other existing regression tests no longer work with this patch.  So it's not verifying this fix that's worrisome, but preventing the other bugs from regressing...

Comment 104

[Honza Bambas (:mayhemer)]

Comment on attachment 316791 [details] [diff] [review]
changed test for bug 398289, v2

see bug 430652

Comment 105

[Mike Schroepfer]

Comment on attachment 310756 [details] [diff] [review]
ver3 final

Removing approval since this missed the 1.9 cutoff.

Comment 106

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #103)
> The problem is not that _this_ patch can't be tested, but that other existing
> regression tests no longer work with this patch.  So it's not verifying this
> fix that's worrisome, but preventing the other bugs from regressing...

This is on the branch nomination radar, but it's unclear if this issue has been resolved. Until it has, can we actually check this in? To get it on the branch, please nominate the patch for approval1.9.0.1

Comment 107

[Honza Bambas (:mayhemer)]

Attachment 310756 [details] [diff] is fix for this bug. The patch is stuck just because of test failure. The test is fixed in bug 430652 that waits for review for several weeks.

Comment 108

[Daniel Veditz [:dveditz]]

This is wanted on the 1.8 branch, but not really blocking -- we'll take it when the trunk fix lands. When landed on the 1.9.0.x branch please request 1.8 branch approval as well.

Comment 109

[David Baron :dbaron:]

Can we just rewrite that one existing test to be a mochitest instead of a reftest?

Comment 110

[Boris Zbarsky [:bzbarsky]]

We should be able to, esp. now that I added a mochitest utility file to do drawWindow stuff and compare things...

Comment 111

[Honza Bambas (:mayhemer)]

(In reply to comment #110)
> We should be able to, esp. now that I added a mochitest utility file to do
> drawWindow stuff and compare things...

For info, I recently added a new (mochi)test to bug 398289.

Comment 112

[georgi - hopefully not receiving bugspam]

so what's the problem for stopping the spyware?

Comment 113

[Honza Bambas (:mayhemer)]

Please land 'ver3 final'.

Comment 114

[Honza Bambas (:mayhemer)]

(In reply to comment #113)
> Please land 'ver3 final'.
It is 1.9.1 (mozilla-central) and 1.9 (CVS-trunk) applicable patch. It could not be landed on 1.8.1 branch.

Comment 115

[Reed Loden [:reed]]

http://hg.mozilla.org/mozilla-central/rev/8086757a77e1

So, I'm very confused from the above comments on whether this is in-testsuite+ or not... can somebody clarify things for me, please?

Comment 116

[georgi - hopefully not receiving bugspam]

according to my tests this seems fixed in -central

Comment 117

[Daniel Veditz [:dveditz]]

Comment on attachment 310756 [details] [diff] [review]
ver3 final

Too late for 1.9.0.4, moving request

Comment 118

[Daniel Veditz [:dveditz]]

Comment on attachment 310756 [details] [diff] [review]
ver3 final

Approved for 1.9.0.5, a=dveditz for release-drivers

Comment 119

[Dave Camp (:dcamp)]

Landed on cvs trunk:

Checking in src/nsXULDocument.cpp;
/cvsroot/mozilla/content/xul/document/src/nsXULDocument.cpp,v  <--  nsXULDocument.cpp
new revision: 1.829; previous revision: 1.828
done
Checking in src/nsXULDocument.h;
/cvsroot/mozilla/content/xul/document/src/nsXULDocument.h,v  <--  nsXULDocument.h
new revision: 1.209; previous revision: 1.208
done

Comment 120

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.5 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5pre) Gecko/2008112505 GranParadiso/3.0.5pre.

Comment 121

[[e-mail]]

The privilege check in ApplyPersistentAttributes makes the check in Persist futile; privileged content pages can persist until they're blue in the face but their persisted attribtes will never get applied. Was this intentional?

Comment 122

[Honza Bambas (:mayhemer)]

Daniel, you have set blocking1.8.1.next+ but according to Firefox 2 (1.8.1 branch) download page and other sources security support for it has been stopped. Do you really want me to make a 1.8.1 patch?

Comment 123

[Boris Zbarsky [:bzbarsky]]

Honza, Firefox is not the only thing shipping off of 1.8.1.  Thunderbird, Seamonkey, and other applications are going to continue to use that branch.

So yes, it would be nice to have a 1.8.1 patch if possible.

Comment 124

[Honza Bambas (:mayhemer)]

Attachment: ver3 for 1.8.1 branch

Tested manually on 1.8.1 branch build with the "Example file" test case. I had to change NodePrincipal()->IsCapabilityChanged to nsIScriptSecurityManager->IsCapabilityChanged(). Please check it's correct, other parts of the patch didn't change, was just merged.

Comment 125

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 355838 [details] [diff] [review]
ver3 for 1.8.1 branch

>+PRBool
>+nsXULDocument::IsCapabilityEnabled(const char* aCapabilityLabel)
>+{
>+    nsresult rv;
>+    
>+    nsIScriptSecurityManager* secMan = nsContentUtils::GetSecurityManager();
>+    NS_ENSURE_TRUE(secMan, PR_FALSE);
>+
>+    PRBool enabled = PR_FALSE;
>+    rv = secMan->IsCapabilityEnabled(aCapabilityLabel, &enabled);
>+    if (NS_FAILED(rv))
>+        return PR_FALSE;
>+ 
>+    return enabled;
>+}

This doesn't look right. You need to make sure to use the principal of the document. This looks like it'll use the principal of whatever javascript happen to be running when we end up in this function.

Comment 126

[Honza Bambas (:mayhemer)]

(In reply to comment #125)
> (From update of attachment 355838 [details] [diff] [review])
> This doesn't look right. You need to make sure to use the principal of the
> document. This looks like it'll use the principal of whatever javascript happen
> to be running when we end up in this function.

I thought it was wrong. However, using this code:

    nsIPrincipal* principal = GetPrincipal();
    rv = principal->IsCapabilityEnabled(aCapabilityLabel, nsnull, &enabled);

also doesn't look right to me. When the second parameter (actually a hashtable where values are stored) is null the capability is always false. So, it cannot be set to true by user or any other preference. I don't know where to take the annotation of the document, there is no example in lxr.

However, the patch as would be with the snippet above disallows attribute persistence (fixes the bug).

Comment 127

[Honza Bambas (:mayhemer)]

Attachment: ver3.1, for 1.8.1

See comment #126.

Comment 128

[Boris Zbarsky [:bzbarsky]]

> So, it cannot be set to true by user or any other preference. 

That's correct, so it's actually equivalent to an IsSystemPrincipal check at the moment.  The idea was that if we later create a way to associate expanded privileges with a principal (and not just a JSStackFrame), things here would Just Work.

Comment 129

[Honza Bambas (:mayhemer)]

Please land "ver3.1, for 1.8.1" on 1.8.1 branch.

Comment 130

[Daniel Veditz [:dveditz]]

Comment on attachment 357745 [details] [diff] [review]
ver3.1, for 1.8.1

Approved for 1.8.1.21, a=dveditz for release-drivers. (add the fixed1.8.1.21 keyword when checked in).

Comment 131

[Honza Bambas (:mayhemer)]

Landed on CVS 1.8 branch:

Checking in nsXULDocument.cpp;
/cvsroot/mozilla/content/xul/document/src/nsXULDocument.cpp,v  <--  nsXULDocument.cpp
new revision: 1.664.2.32; previous revision: 1.664.2.31
done
Checking in nsXULDocument.h;
/cvsroot/mozilla/content/xul/document/src/nsXULDocument.h,v  <--  nsXULDocument.h
new revision: 1.171.8.6; previous revision: 1.171.8.5

Comment 132

[Honza Bambas (:mayhemer)]

Landed before branching, http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8086757a77e1