Turning off js in help doesn't work properly

RESOLVED FIXED in seamonkey1.0beta

Status

defect
--
major
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: stefanh, Assigned: bzbarsky)

Tracking

Trunk
seamonkey1.0beta

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

14 years ago
Turning off Javascript in Help by adding the line
helpBrowser.docShell.allowJavascript = false; in help.js doesn't seem to work
properly.

It works if Javascript is enabled in Preferences --> Advanced --> Script &
Plug-ins. But turning off Javascript in preferences seem to enable it in Help.

How to repro:

1. Turn off js in Help by adding helpBrowser.docShell.allowJavascript = false;
in help.js.

2. Add a script in the welcome page (welcome.xhtml)

3. Start Mozilla and turn off js in preferences (Preferences --> Advanced -->
Script & Plug-ins)

4. Notice that the script works.
(Reporter)

Comment 1

14 years ago
Posted patch testcase-diffSplinter Review
Here's a patch that illustrates the problem:

Apply the patch, turn off js in preferences and open Help (Help Contents in
Help menu)
CCing people who might have a clue about how this works (or doesn't...)
Looks like if JS is disabled the security manager special-cases non-system
chrome: uris and doesn't check with the docshell (see
nsScriptSecurityManager::CanExecuteScripts).

That's probably wrong, but I'm not sure what the right behavior is, exactly. 
Should docshells be able to _enable_ script if it's disabled globally and they
explicitly set the flag  to true, say?
The comment refers to about: URLs, but they aren't tested for. What the code
actually does is override the docShell setting for chrome: URLs but only when
javascript is globally disabled. Do we actually need that special case? Or do
we still want it, but for about: URLs rather than chrome: URLs?
about: URLs redirect to chrome, so this code used to be testing for them.  But
now that chrome: ends up using originalURI, I think this test could be safely
switched to about: instead of chrome:.
As it stands that will break Editor because it currently uses about:blank for a
new page and that the code block in question overrides the docShell setting.

The only benefit that I can see of allowing about: pages to run scripts is for
about: itself so that it can show the UA when JavaScript is disabled.
At least that, yes.  I guess the other about: pages that run JS have system
principals....  Is about: broken right now, btw, with JS disabled?
(Reporter)

Comment 8

14 years ago
(In reply to comment #7)
> Is about: broken right now, btw, with JS disabled?

Yes

So either we need to special-case just about: (URL, not scheme) here, or we need
to come up with a more flexible setup for this stuff for about: (scheme) urls.
(Assignee)

Updated

14 years ago
Flags: blocking1.8b?

Updated

14 years ago
Flags: blocking1.8b? → blocking1.8b+
maybe for beta2.
Flags: blocking1.8b+ → blocking1.8b2?
This is a really simple bug to fix; we just need to decide what approach to
take.  Note that not only is help broken, so is "about:"....

sicking, peterv, any ideas?

Bumping severity to major, since this is effectively breaking our security model
in some cases.
Severity: minor → major
So to be precise, we want to:

1)  Not mess with whether JS is enabled in chrome
2)  Enable JS in "about:" (only, not other URIs with that scheme, especially not
    about:blank).

If someone edits "about:" that will give the editor conniptions, but fixing that
would require some new api to communicate "this document should ignore JS
disabling".
Attachment #174486 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #174486 - Flags: review?(peterv)
Comment on attachment 174486 [details] [diff] [review]
Proposed patch

We can worry about that when editor learns xhtml.
Attachment #174486 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Actually... biesi, would this break error pages?

If so, perhaps the right answer is to keep the chrome check (in addition to the
"about" check), and just put them both where I put the "about" check, after we
check for the docshell locking out script.

Of course that will cause error pages to not work in docshells that disable
script....
hm, yes, it seems it would :-/

maybe error pages need an own scheme...
Either that, or we need some other way of telling which principals shouldn't
have script disabling apply to them...
(In reply to comment #15)
>Actually... biesi, would this break error pages?
By my limited understanding error pages currently have chrome:// content URLs
and I therefore assume system principal which is already specially cased.
That's the question... do error pages have system principals?
Attachment #174486 - Flags: review?(peterv) → review+
(Assignee)

Updated

14 years ago
Assignee: neil.parkwaycc.co.uk → bzbarsky
Target Milestone: --- → mozilla1.8beta2
(Assignee)

Updated

14 years ago
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Updated

14 years ago
Target Milestone: mozilla1.8beta2 → ---
(Assignee)

Updated

14 years ago
Target Milestone: --- → Seamonkey1.0beta
You need to log in before you can comment on or make changes to this bug.