Closed Bug 117707 Opened 23 years ago Closed 23 years ago

Disabling "Change status bar text" and/or "Cookie" prefs for JS breaks applications

Categories

(Core :: Security: CAPS, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.0

People

(Reporter: brightice, Assigned: caillon)

References

()

Details

(Keywords: relnote)

Attachments

(2 files, 10 obsolete files)

(deleted), text/html
Details
(deleted), patch
caillon
: review+
caillon
: superreview+
caillon
: approval+
Details | Diff | Splinter Review
From Bugzilla Helper: User-Agent: Mozilla/5.0 (X11; U; HP-UX 9000/785; en-US; rv:0.9.7) Gecko/20011227 BuildID: 2001122723 When having disabled "Change status bar text" for JS, the submit button on http://wwwib.gad.de/gwmpb/ob/hbxg5735.htm will not work. Enabling the feature does help. Reproducible: Always Steps to Reproduce: 1. Disable "Change status bar text" for JS 2. Load page, enter some values in the form fields 3. Try to submit, nothing happens Actual Results: nothing Expected Results: submit form data
> Steps to Reproduce: > 1. Disable "Change status bar text" for JS brightice@brightice.de: how are you disabling this? I can't see this in Edit | Preferences anywhere -
brightice@brightice.de: do you mean View | Show/Hide | Status Bar ? If I do this and hide the status bar, I do not see the bug -
It's under preferences|advanced|scripts&windows you have to get a recent build to get this feature (was checked in on december 30), see bug 75731
Thanks, that's it!!! Must be a build from after December 30, and then: Edit|Preferences|Advanced|Scripts&Windows|disable "Change status bar text" However, I'm not sure I can see the bug. When I bring the page up, I see only two textboxes, for a login: Bitte geben Sie Ihre Kontonummer sowie die zugehörige PIN ein. Kontonummer: ___________ PIN: ___________ I can only enter random characters here, since I don't have an account. When I click the submit button, I get a warning that my login is invalid. This functions the same whether or not I have the status text disabled. Reassigning to Browser-General until we can get further information. This is not likely to be a bug in JS Engine. brightice@brightice.de: 1. Is there a test ID and password we can use at this site? 2. Is the bug on the login page, or after the login page? 3. Do you see any errors in Tasks > Tools > JavaScript Console? (be sure to clear it out of any previous errors first)
Assignee: rogerl → asa
Component: Javascript Engine → Browser-General
QA Contact: pschwartau → doronr
->doron.
Assignee: asa → doronr
BTW, in my comment #3 I meant bug 75371, silly me ! sorry for the spam, but i think clarifications were needed.
Phil, if I do this ( disabling status bar changes for JavaScript, load the page, enter account data, press button "Weiter" ) _nothing_ happens, while everything is fine when the JavaScript is allowed to change the status bar. The browser does not load anything, there is no progress bar, nothing. 1. There is no test ID you can use. 2. It's on the login page. (see my description above) 3. The only error I get is "Error: uncaught exception: Permission denied to set property Window.status" as soon as I move the mouse pointer into the browser window. But pressing the Weiter" button does not generate any message in the JS console. When it works, the status bar text is continously changed until the following page is loaded. Thanks, Matthias
cc: mstoltz Seems that the uncaught exception error is causing problems. Seems to be a more general security capability issue, as this still happens if I just add the line user.js rather than using my panel
Status: UNCONFIRMED → NEW
Ever confirmed: true
The problem is indeed that the site is relying on window.status not throwing an exception. I'm really amazed such coding is possible (relying on being able to change the status text to log in a bank, ugh), but anyway... I think putting a warning like "Some sites relying on this functionality may not work if you disable it" is in order. On the other hand, should we really throw an exception if the security manager blocked the call? Note also bug 112612 which says window.onError doesn't catch those exceptions. Maybe getting rid of them altogether is the good solution.
Is our "Change status bar text" pref done by changing the security policy for window.status to noAccess? If so, that's just BROKEN! That will cause any code that touches window.status to throw an exception, and that's totally not expected, or acceptable, it'll of course cause the script to stop execution right there, if they didn't try/catch around setting window.status. Nobody in their right mind would try/catch around setting window.status, that needs fixing. If we want a pref that makes window.status not settable, lets code that up in the setter of window.status so that it doesn't throw an exception! Who is in charge of the backend for the "Change status bar text" pref?
Keywords: mozilla0.9.8
OS: HP-UX → All
Hardware: HP → All
That's exactly what it does (re: setting window.status to noAccess) Mstoltz did the backend of the noAccess prefs I think
Actually, since I wrote the panel, it is probably my fault fabian. Mstoltz just wrote the capabilty code which I used.
Ok, then add a new preference, check for that preference ("dom.disable_window_status" or somesuch) and control that with the UI we already have, and add a check for that pref in GlobalWindowImpl::SetStatus() and just ignore the call if that pref is set (just like GlobalWindowImpl::Open(nsIDOMWindow **) does. Do we have other preferences that are implemented the same way?
The entire Scripts and Windows panel is a front end to the capability policies.... The fact is, web authors _never_ wrap anything in try/catch (because not all browsers support them, for one thing). As a result, any use of security policies for web content will lead to bugs such as this one... I see no reason to stop script execution just because a call to window.resizeTo() was blocked, for example....
Exactly. As a matter of fact, if you're writing code that's supposed to work in 4x as well as other browsers, you can not even use try/catch, they cause a parse time error in 4x.
Taking. The reason I did it the easy way (security policies) was that we wanted it badley for 0.9.7.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.8
Would it be possible to tweak security policies to disallow access but still not throw an exception? Or would this need changes at all points where DOM objects are being accessed via script?
I donn't think we have what we'd need to disallow access to properties with the security code w/p throwing an exception. I don't see how we could fix this w/o touching the implmentation of the setters/getters/methods in question.
Isn't it as simple as removing the following lines (nsScriptSecurityManager.cpp) 448 JS_SetPendingException(aJSContext, 449 STRING_TO_JSVAL(JS_NewStringCopyZ(aJSContext, errorMsg.get()))); 450 //-- if (aProperty) we were called from somewhere other than xpconnect, so we need to 451 // tell xpconnect that an exception was thrown. 452 if (aProperty) 453 { 454 nsCOMPtr<nsIXPConnect> xpc = do_GetService(nsIXPConnect::GetCID()); 455 if (xpc) 456 { 457 nsCOMPtr<nsIXPCNativeCallContext> xpcCallContext; 458 xpc->GetCurrentNativeCallContext(getter_AddRefs(xpcCallContext)); 459 if (xpcCallContext) 460 xpcCallContext->SetExceptionWasThrown(PR_TRUE); 461 } 462 } and add a few lines to log the error in the console as a message instead? (which is easy)
No. Sure, that would make the exception go away, but script execution would still be terminated, now silently, which is even worse than throwing an exception.
got this fixed, doing it now for the rest of the prefs.
Doron, I assume you're taking out all that crap you were doing because the prefs could not just have preftype/prefname attributes and using such attributes instead?
give me some credit, please :) Though I shall probably need functions since I need !prefValue for the checkboxes.
Nah. Just call the prefs "allow_status_change" and the like. Then default them to true in all.js and default them to true in the C++ (for cases when the pref service returns nothing at all).
Ok, I have this working, but it blocks out chrome as well. I noticed that jst patched the dom.disable_window_open (http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsGlobalWindow.cpp&root=/cvsroot&subdir=mozilla/dom/src/base&command=DIFF_FRAMESET&rev1=1.468&rev2=1.469) to not break in chrome. Jst - should I just replicate your code for checking if the caller is chrome or not for my patch (the c++ part is at http://nexgenmedia.net/mozilla/prefs.txt)
Target Milestone: mozilla0.9.8 → mozilla0.9.9
bz, doron, we don't want to default these prefs in all.js, that doesn't give us much other than unnecessary bloat. Default them in C++, it can always be overridden in all.js if needed. As for the chrome issue, what's interesting here is to know if chrome or content is trying to set the status text, not if the status text is being set on a chrome or content window. To test whether the call comes from chrome or content you need to check if the current context is a chrome one or not, to do this, do something like this: nsCOMPtr<nsIDocShell> doc_shell; JSContext *cx = nsnull; nsCOMPtr<nsIThreadJSContextStack> stack(do_GetService(sJSStackContractID)); if (stack) stack->Peek(&cx); if (cx) { nsCOMPtr<nsIScriptGlobalObject> sgo; nsJSUtils::GetDynamicScriptGlobal(cx, getter_AddRefs(sgo)); if (sgo) { sgo->GetDocShell(getter_AddRefs(doc_shell)); } } } if (doc_shell) PRInt32 caller_type = nsIDocShellTreeItem::typeChrome; get caller_type from doc_shell if (caller_type == content) { check pref return if not allowed to change... } } Given that this is a non-trivial amount of code you probably want to split this into a static helper method.
jst the rule last i read was that *all* prefs be listed in all.js so people would know that they can twiddle them. has the rule changed?
Um, well that kinda blows, I had never heard that rule before. Can we then add the prefs but comment them out to avoid bloating the pref code's hashtables n' all?
I should probably take this, or open a new bug on this. Johnny, you're absolutely right, we should have "content controls" that simply no-op the call they block, rather than throwing an exception. That's the difference between security controls and simply filtering out annoying behavior; it's because we're using the same backend for both that we encounter this problem. We could see about adding the ability to configure caps to skip the call in question without throwing an exception - shouldn't be too hard.
It would be nice if there could be warnings (not errors) in the JS console for blocked calls to window.open, window.resizeTo, etc.
Component: Browser-General → Security: CAPS
I don't think there's a need to add code/hacks to caps/XPConnect that would enable skipping of calls, in fact, I don't think it's doable w/o introducing oddities for these not-too-often-seen special cases. Making "denied" attempts at making some calls in these cases throw an information item on the JS console sounds like a good idea to mee though.
While many sites will degrade more gracefully once this "no-op" fix goes in, broadcast.yahoo.com will probably still be blank when pop-ups or window-raising are disabled (bug 123186).
Jesse - which is why I filed a bug about adding a warning to the panel about that.
See also bug 122866, "Annoying-content controls should not stop script execution".
*** Bug 126381 has been marked as a duplicate of this bug. ***
->1.0, I have it working in one place, still some issues in 2 others
Target Milestone: mozilla0.9.9 → mozilla1.0
I have found a site that is affected by this bug: <http://www.lagercrantz.se/>. If "Change status bar text" is disallowed, the dynamic menus won't display when clicking the menu bar, otherwise they will.
Attached file window.status test (deleted) —
Both Test 1 and Test 2 should show an alert box. If Preference -> Scripts & Window -> Disable Change Status Bar Text, Test 2 will fail. All codes after the window.status statement will be ignored.
*** Bug 128696 has been marked as a duplicate of this bug. ***
the simple cookie script test that should write 1234 but only writes 134: <script> document.writeln("1"); var dc = document.cookie; document.writeln("2"); </script> <script> document.writeln("3"); var dc; try { dc = document.cookie; } catch (err) {}; document.writeln("4"); </script>
Summary: Disabling "Change status bar text" for JS breaks application → Disabling "Change status bar text" and/or "Cookie" prefs for JS breaks applications
I think this bug keeps reloading the page again and again on this page: http://www.freeweb.hu/linuxdoc/hogyanok/hogyan/Konfiguracio-HOGYAN/Config.html It is very annoying with the combined effects of bug 51139 (it takes several seconds and reloads to stop it while holding down the Esc key). (I think this is the right place to write, but I'll file a new bug if it isn't.)
*** Bug 129153 has been marked as a duplicate of this bug. ***
-> 0.9.9 I have this partially working, hoping to get this in for 0.9.9
Target Milestone: mozilla1.0 → mozilla0.9.9
I'm a nut, meant 1.0
Keywords: mozilla0.9.9mozilla1.0
Target Milestone: mozilla0.9.9 → mozilla1.0
Ok, I have this working for the Image.src blocking pref. My question is, what should be done first (for performance reasons) - the check if the caller is chrome://, or to check if the pref is set to true/false?
Blocks: 122866
Pick one, post a patch and we'll have a look.
Attached patch partial patch, done to block image.src setting (obsolete) (deleted) — Splinter Review
The example patch works for HTMLImage.src setting
Comment on attachment 73673 [details] [diff] [review] partial patch, done to block image.src setting >+static nsresult IsCallerChrome() Don't pass PRBool values as nsresult's, make this method return a PRBool in stead. >+{ >+ >+ PRBool rv = PR_FALSE; >+ nsCOMPtr<nsIDocShell> mDocShell; >+ >+ JSContext *cx = nsnull; >+ nsCOMPtr<nsIThreadJSContextStack> stack(do_GetService(sJSStackContractID)); >+ >+ if (stack){ If there's no context stack, treat the caller as chrome (i.e. return PR_TRUE). >+ stack->Peek(&cx); >+ >+ if (cx) { If there's no context in the stack, treat the caller as chrome. >+ >+ PRInt32 caller_type = nsIDocShellTreeItem::typeChrome; >+ if (mDocShell){ If there's no docshell (which is unlikely), treat the caller as chrome. [...] > NS_IMETHODIMP > nsHTMLImageElement::SetSrc(const nsAReadableString& aSrc) > { >+ >+ /* If document.enable_image_src_set is set to false, block image.src setting */ >+ nsCOMPtr<nsIPref> prefs(do_GetService(kPrefServiceCID)); >+ >+ if (prefs) >+ { >+ PRBool allowImageSrcSet = PR_FALSE; >+ prefs->GetBoolPref("document.enable_image_src_set", &allowImageSrcSet); Flip this pref around, we shouldn't require that this pref is set to enable setting image.src, we should let users add a pref that *disable* setting image.src. Other than that, this looks like a reasonable approach, but this patch needs work.
Attachment #73673 - Flags: needs-work+
Attached patch next try (obsolete) (deleted) — Splinter Review
Attached patch next try (obsolete) (deleted) — Splinter Review
*** Bug 131610 has been marked as a duplicate of this bug. ***
sorry about that, those 2 are the wrong patch.
Attached patch correct patch for image.src setting blocking (obsolete) (deleted) — Splinter Review
The correct patch this time
Attachment #73673 - Attachment is obsolete: true
Attachment #74614 - Attachment is obsolete: true
Attachment #74615 - Attachment is obsolete: true
re comment 21: Is the status bar instance fixed now? Which others are? FYI: I'd like Beonex Communicator to ship with most of these capabilities disabled (esp. status bar, resize/move, raise/lower).
Blocks: Beonex
the patch is just an example - once approved, I will repeat it for all prefs in the panel and create a final patch.
bz just noted that we might want to put the IsCallerChrome() function somewhere central (or perhaps make a function that returns the Caller type). Any suggestions where?
What do you think of adding it to nsContentUtils? I am surprised there is no "easy" way to know whether we are in chrome or not. Also, will this have any impact on applications that change the src of images in tight loops or timeouts, or is it lost in the noise? Last comment about the patch: add spaces around the equal sign in the second line of the patch.
I have to do this for dom/src/base/nsGlobalWindow.cpp, and using nsContentUtils::GetDynamicScriptGlobal crashes, so have to use here the originally mentioned nsJSUtils. >Also, will this have any impact on applications that change the src of images in >tight loops or timeouts, or is it lost in the noise? Haven't tested yet, if you can provide a testcase, I can try it >Last comment about the patch: add spaces around the equal sign in the second >line of the patch. Which line? the second line is a include
Er oops... I meant here +static const char *sJSStackContractID="@mozilla.org/js/xpc/ContextStack;1"; The testcase should be easy... var img = document.createElement("img"); img.src = "http://mozilla.org/images/mozilla-banner.gif"; document.body.appendChild(img); var start = new Date(); for (var i = 0; i < 1000; i++) { img.src = "http://mozillazine.org/image/new/title.gif"; } var end = new Date(); alert("Elapsed time: " + (end-start) + "ms");
I better hide. with my code: 4422ms without : 3579ms
Doing the Chrome caller check first oseems to not change anything (this is with he pref turned off)
If I have the images cached, it is 1028 vs 998. Might be my sucky connection that was causing the trouble
If you think that this is too much of a cost, we need to backout the panel for 1.0 btw.
What I currently have, moves the entire panel's backend to c++. Still has some useless code in it for the capability prefs.
Attached patch what I have, 2nd try (obsolete) (deleted) — Splinter Review
Attachment #75864 - Attachment is obsolete: true
Since I already caused enough spam, some more data: Setting of window.status 1000 times: with changes: 3719ms without : 3651ms
http://www.nexgenmedia.net/mozilla/117707.txt has my newest patch, and comments/nits welcome if we want to get this working in 1.0
Blocks: 132031
callion asked me to change the code to use if (!IsCallerChrome()) && prefThatDisablesThisFunction) It seems to be faster, in window.status setting it is now with changes: 3666ms without : 3651ms So it looks like chrome caller checking is indeed cheaper than pref getting. New patch coming up with his nits fixed.
While you're at it, please change the nsGlobalWindow code so that you don't need all that code duplicated in all those methods. How about a CanSetProperty(<pref_name>) helper that just returns true of false? Also, please move the IsCallerChrome() into nsContentUtils in the content library so that you don't need to duplicate that code in the content library. Oh, and *don't* prefix local variable names with 'm', as in 'mDocShell', it makes it look like it's a member of a class.
Attached patch next version (obsolete) (deleted) — Splinter Review
I am still not convinced that checking for chrome first is the correct way. Example, window.status setting, I would hazard a guess that this would be hit more by websites, which means it will slow it down a bit compared to checking for the pref first.
Attachment #75148 - Attachment is obsolete: true
Attachment #75865 - Attachment is obsolete: true
*** Bug 134455 has been marked as a duplicate of this bug. ***
Attached patch fixes some tabs (obsolete) (deleted) — Splinter Review
Fixed some tabs and other minor nits from caillion
Attachment #76895 - Attachment is obsolete: true
Preapproving to work on for 1.0 -- please get r= and sr= and mail drivers for a=. /be
Keywords: mozilla1.0mozilla1.0+
Comment on attachment 77040 [details] [diff] [review] fixes some tabs +static NS_DEFINE_CID(kPrefServiceCID, NS_PREF_CID); Do you really need a CID? Can't you use a Contract ID? +//static const char *sJSStackContractID = "@mozilla.org/js/xpc/ContextStack;1"; Why not remove this line? + if (prefValue) + rv = PR_FALSE; + else + rv = PR_TRUE; rv = !prefValue; ? (This is not an r=)
Comment on attachment 77040 [details] [diff] [review] fixes some tabs +static NS_DEFINE_CID(kPrefServiceCID, NS_PREF_CID); Do you really need a CID? Can't you use a Contract ID? +//static const char *sJSStackContractID = "@mozilla.org/js/xpc/ContextStack;1"; Why not remove this line? + if (prefValue) + rv = PR_FALSE; + else + rv = PR_TRUE; rv = !prefValue; ? (This is not an r=)
Comment on attachment 77040 [details] [diff] [review] fixes some tabs boooring *yawn* address biesi's comments and r=fabian either way.
Attachment #77040 - Flags: review+
I'm at netscape and won't have my own mashine for a few days, so won't have a tree. and can't fix those minor nits.
Doron, thanks for your initial work. Since this is a wanted fix, I'll take over the patch from here and address any other comments and seek sr/a=. Assigning to self so I don't forget to do this :) Good luck with your new position!
Assignee: doronr → caillon
Status: ASSIGNED → NEW
Attached patch Updated patch (obsolete) (deleted) — Splinter Review
Most of the patch is still doron's work. He forgot to do the changes in all.js though and I updated the stuff against current tip, and made some additional cleanup and modifications to pref-scripts.js. Removed the commented out code as per biesi, but I left everything else alone -- CIDs are already used throughout the files so I see no reason to change that. If a super-reviewer wants it changed though, I will.
Attachment #77040 - Attachment is obsolete: true
Comment on attachment 77577 [details] [diff] [review] Updated patch Bringing forth r=fabian. Also please ignore the change to nsDocumentEncoder.cpp -- I didn't realize that was in there still. It's from an unrelated bug and I'll make sure to not check that in.
Attachment #77577 - Flags: review+
you still have this in there: + if (prefValue) + rv = PR_FALSE; + else + rv = PR_TRUE; Why? To me that looks highly unlegant. Why not use rv = !prefValue?
Comment on attachment 77577 [details] [diff] [review] Updated patch r=doron as well
Attachment #77577 - Flags: needs-work+
Comment on attachment 77577 [details] [diff] [review] Updated patch Some nits, some real issues: +nsContentUtils::IsCallerChrome() +{ + Loose the empty line. + PRBool rv = PR_TRUE; + nsCOMPtr<nsIDocShell> docShell; + + JSContext *cx = nsnull; cx isn't used outside the if (stack) statement below, move the declaration into that statement. + nsCOMPtr<nsIThreadJSContextStack> stack(do_GetService(sJSStackContractID)); + + if (stack){ [...] + } + The below code has two issues, callerType should be declared more locally, and the if check should check for if (item) and not for if (docShell), do_QueryInterface() is null safe, so no need to check for null before callin it. + PRInt32 callerType = nsIDocShellTreeItem::typeChrome; + if (docShell){ + nsCOMPtr<nsIDocShellTreeItem> item(do_QueryInterface(docShell)); + item->GetItemType(&callerType); + if (callerType != nsIDocShellTreeItem::typeChrome) + rv = PR_FALSE; + } I.e. the above should be more like: + nsCOMPtr<nsIDocShellTreeItem> item(do_QueryInterface(docShell)); + if (item) { + PRInt32 callerType = nsIDocShellTreeItem::typeChrome; + item->GetItemType(&callerType); + + if (callerType != nsIDocShellTreeItem::typeChrome) { + rv = PR_FALSE; + } + } And in stead of using a local |rv| variable for the return value, why not just return early and not have the variable at all? Cleaner, and doesn't use |rv| which typically is used for an nsresult. - In nsHTMLImageElement::SetSrc() > { >+ /* If caller is not chrome and document.disable_image_src_set is set to true, block image.src setting by exiting*/ Please split the comment up so that it fits in 80 columns. [...] >+ if (!nsContentUtils::IsCallerChrome() && disableImageSrcSet) >+ { Move the '{' to the end of the line above, and swap the order of the checks in the if condition. No need to call IsCallerChrome() if disableImageSrcSet is false. - In IsCallerChrome() in nsGlobalWindow.cpp, same as above. +static PRBool CanSetProperty(const nsAFlatCString &prefName) Since the callers of this method always have a const char * that they can pass in, and you end up extracting that pointer out of the nsAFlatCString, why not just make this function take a const char * and don't bother wrapping them in string classes? +{ + PRBool rv = PR_FALSE; + No need for |rv| here either... + nsCOMPtr<nsIPref> prefs(do_GetService(kPrefServiceCID)); + + if (prefs){ + Loose the empty line here. And make the above line: + if (!prefs) { + return PR_FALSE; + } + PRBool prefValue = PR_TRUE; + //if pref is set to true, we can't set the property + prefs->GetBoolPref(prefName.get(), &prefValue); + + if (prefValue) + rv = PR_FALSE; + else + rv = PR_TRUE; ... and then you simply return !prefValue here, as suggested by others as well. NS_IMETHODIMP GlobalWindowImpl::SetStatus(const nsAString& aStatus) { + /* If caller is not chrome and dom.disable_window_status_change is set to true, block window.status setting by exiting*/ Wrap this so that it fits in 80 columns, same for all other new comments. + + if (!IsCallerChrome() && !CanSetProperty(NS_LITERAL_CSTRING("dom.disable_window_status_change"))){ Add a space between ')' and '{', and loose the NS_LITERAL_CSTRING() wrappers (all instances of both). Index: modules/libpref/src/init/all.js =================================================================== RCS file: /cvsroot/mozilla/modules/libpref/src/init/all.js,v retrieving revision 3.363 diff -u -4 -r3.363 all.js --- modules/libpref/src/init/all.js 1 Apr 2002 05:56:24 -0000 3.363 +++ modules/libpref/src/init/all.js 4 Apr 2002 01:35:25 -0000 @@ -348,18 +348,26 @@ pref("capability.principal.codebase.foo.id", "http://www.netscape.com"); pref("capability.principal.codebase.foo.granted", "UniversalFoo"); ////////////////////////////////////////////////////////// -pref("dom.disable_open_during_load", false); +// Scripts & Windows prefs +pref("browser.block.target_new_window", false); +pref("document.disable_image_src_set", false); +pref("document.disable_cookie_get", false); +pref("document.disable_cookie_set", false); +pref("dom.disable_open_during_load", false); +pref("dom.disable_window_move_resize", false); +pref("dom.disable_window_flip", false); +pref("dom.disable_window_status_change", false); These pref names are not consistent, how about +pref("browser.disable_target_new_window", false); +pref("dom.disable_image_src_set", false); +pref("dom.disable_cookie_get", false); +pref("dom.disable_cookie_set", false); Other than that this looks good. Attach a new patch and I'll have one more look.
Attached patch New patch, incorporating jst's comments (obsolete) (deleted) — Splinter Review
Attachment #77577 - Attachment is obsolete: true
Comment on attachment 77780 [details] [diff] [review] New patch, incorporating jst's comments Oops, forgot to carry forth Fabian's review. Jst, would you have another look, please? :)
Attachment #77780 - Flags: review+
Status: NEW → ASSIGNED
Keywords: review
Priority: -- → P1
Apologies for what may be a dumb question, but doesn't this remove the site-specific nature of the security policy? There may be sites that I want to allow JavaScript cookies for, even though I've disabled them globally. Leaning a bit into bug 122866 territory, should these prefs be pref("capability.policy.{policyname}.dom.disable_{function}",{bool}); If this is close to getting into 1.0, though, all is certainly better than nothing.
This bug is essentially about _not_ using security policies for the existing UI. Since the site-specific stuff has no UI yet, this solution is OK for now. Making a version of security policies that would not halt code execution would be a much larger task (certainly not 1.0-fodder).
Comment on attachment 77780 [details] [diff] [review] New patch, incorporating jst's comments sr=jst
Attachment #77780 - Flags: superreview+
Indeed, if and when we get security policies that don't halt code execution, this patch will get backed out and we return to the original security policy patch I wrote.
Comment on attachment 77780 [details] [diff] [review] New patch, incorporating jst's comments a=asa (on behalf of drivers) for checkin to the 1.0 branch
Attachment #77780 - Flags: approval+
Fixes comment wording to be consistent, and also moves a Truncate() up in nsHTMLDocument::GetCookie(nsAString& aCookie)
Attachment #77780 - Attachment is obsolete: true
Attachment #78879 - Flags: superreview+
Attachment #78879 - Flags: review+
Attachment #78879 - Flags: approval+
Attachment 78879 [details] [diff] has been checked in on both branch and trunk. Marking fixed.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Btw, if you are upgrading to a new build you have to delete some items from your prefs.js in order to get this to work properly. I'm working on a release note for this. Any of these following prefs if set will need to be deleted manually (they are still valid prefs, so we can't blindly delete them, however they throw exceptions and cause most sites to break). Once you delete them, you need edit your Scripts/Windows prefs again. Fortunately, this is a one time only deal :) user_pref("capability.policy.default.Window.defaultStatus", "noAccess"); user_pref("capability.policy.default.Window.focus", "noAccess"); user_pref("capability.policy.default.Window.innerHeight.set", "noAccess"); user_pref("capability.policy.default.Window.innerWidth.set", "noAccess"); user_pref("capability.policy.default.Window.moveBy", "noAccess"); user_pref("capability.policy.default.Window.moveTo", "noAccess"); user_pref("capability.policy.default.Window.outerHeight.set", "noAccess"); user_pref("capability.policy.default.Window.outerWidth.set", "noAccess"); user_pref("capability.policy.default.Window.resizeBy", "noAccess"); user_pref("capability.policy.default.Window.resizeTo", "noAccess"); user_pref("capability.policy.default.Window.screenX.set", "noAccess"); user_pref("capability.policy.default.Window.screenY.set", "noAccess"); user_pref("capability.policy.default.Window.sizeToContent", "noAccess"); user_pref("capability.policy.default.Window.status", "noAccess");
Keywords: relnote
I suggest you create a bug to relnote the removal
*** Bug 137953 has been marked as a duplicate of this bug. ***
Btw, the release note for this is being tracked on bug 137748.
adding fixed1.0.0 keyword (branch resolution). This bug has comments saying it was fixed on the 1.0 branch and a bonsai checkin comment that agrees. To verify the bug has been fixed on the 1.0 branch please replace the fixed1.0.0 keyword with verified1.0.0.
Keywords: fixed1.0.0
*** Bug 138824 has been marked as a duplicate of this bug. ***
No longer blocks: Beonex
*** Bug 122866 has been marked as a duplicate of this bug. ***
*** Bug 123233 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: