Closed Bug 280120 Opened 20 years ago Closed 20 years ago

Turning off js in help doesn't work properly

Categories

(SeaMonkey :: Help Documentation, defect)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey1.0beta

People

(Reporter: stefanh, Assigned: bzbarsky)

Details

Attachments

(3 files)

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.
Attached patch testcase-diff (deleted) — Splinter 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?
Attached patch Remove chrome: special case (deleted) — Splinter Review
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?
(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.
Flags: blocking1.8b?
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".
Attached patch Proposed patch (deleted) — Splinter Review
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: neil.parkwaycc.co.uk → bzbarsky
Target Milestone: --- → mozilla1.8beta2
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.8beta2 → ---
Target Milestone: --- → Seamonkey1.0beta
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: