960566 - Move inline scripts and styles into separate files for SeaMonkey privileged about: pages

Reporter

Description

•

10 years ago

> With the current plan to harden the security of Firefox, we want to
> disallow internal privileged pages to use inline JavaScript.

Edmund Wong (:ewong)

Comment 1

•

10 years ago

At the moment (and as I understand the bug), the following needs 
moving:

about:,
about:blocked,
about:certError,
about:life,
about:rights

Am I missing any other?

Frederik Braun [:freddy]

Comment 2

•

10 years ago

I'm not a SeaMonkey user, but I think about:about should list them all.
Feel free to create individual blockers to this tracking bug and mark them as good first bugs just as we did with bugs like 948882. It would be great if you could volunteer as mentors for those bugs too! :)

Keywords: meta

Philip Chee

Reporter

Comment 3

•

10 years ago

> but I think about:about should list them all.
I think Bug 923920 doesn't apply to non-privileged pages. There a few about: pages we can demote since they don't require chrome privileges (about:life)

rsx11m

Comment 4

•

10 years ago

(In reply to Edmund Wong (:ewong) from comment #1)
> Am I missing any other?

about:privatebrowsing has a single onLoad() function, I suppose this would need to be moved as well?

rsx11m

Comment 5

•

10 years ago

From nsAbout.js:

  blockedFlags: SCRIPT | UNTRUSTED | HIDE,
  blockedURI: "chrome://communicator/content/blockedSite.xhtml",
  certerrorFlags: SCRIPT | UNTRUSTED | HIDE,
  certerrorURI: "chrome://communicator/content/certError.xhtml",
  dataFlags: SCRIPT,
  dataURI: "chrome://communicator/content/dataman/dataman.xul",
  feedsFlags: SCRIPT | UNTRUSTED | HIDE,
  feedsURI: "chrome://communicator/content/feeds/subscribe.xhtml",
  lifeFlags: SCRIPT | HIDE,
  lifeURI: "chrome://communicator/content/aboutLife.xhtml",
  privatebrowsingFlags: SCRIPT,
  privatebrowsingURI: "chrome://communicator/content/aboutPrivateBrowsing.xul",
  rightsFlags: SCRIPT | UNTRUSTED,
  rightsURI: "chrome://branding/content/aboutRights.xhtml",
  sessionrestoreFlags: SCRIPT | HIDE,
  sessionrestoreURI: "chrome://communicator/content/aboutSessionRestore.xhtml",
  synctabsFlags: SCRIPT,
  synctabsURI: "chrome://communicator/content/aboutSyncTabs.xul",

It is my understanding that anything saying UNTRUSTED won't be affected by bug 923902 introducing the content-policy restriction. As for my comment #4, does this affect XHTML pages only or XUL pages (like that one) as well?

Ameya Bapat

Comment 6

•

10 years ago

(In reply to Edmund Wong (:ewong) from comment #1)
> At the moment (and as I understand the bug), the following needs 
> moving:
> 
> about:,
> about:blocked,
> about:certError,
> about:life,
> about:rights
> 
> Am I missing any other?

I think we can create dependency bug for all.
http://mxr.mozilla.org/mozilla-central/ident?i=NS_ABOUT_MODULE_CONTRACTID_PREFIX

rsx11m

Comment 7

•

10 years ago

For suite/ this would be http://mxr.mozilla.org/comm-central/ident?i=NS_ABOUT_MODULE_CONTRACTID_PREFIX&tree=comm-central&filter=suite which doesn't get me any match, though.

Frederik Braun [:freddy]

Comment 8

•

10 years ago

(In reply to rsx11m from comment #5)
> It is my understanding that anything saying UNTRUSTED won't be affected by
> bug 923902 introducing the content-policy restriction. As for my comment #4,
> does this affect XHTML pages only or XUL pages (like that one) as well?

Yeah, the ones being "UNTRUSTED" don't run with full privileges, so we're less worried about those. It *could* make things easier if all were being rewritten though.

Also, feel free to file bugs blocking this for each file or about: URI and make them block a SeaMonkey tracking bug. I have made good experience with filing them as good first bugs (for volunteers with JS/HTML knowledge) with a defined mentor for review/feedback on patches.

rsx11m

Comment 9

•

10 years ago

Maybe let's start with an inventory first which pages of those identified in comment #5 actually need work, some of them may be trivial and can be grouped. While pages should be independent, thus would invite solving one per bug, they would bitrot each other for the jar.mn changes and may be less convenient for review (though such conflicts should be trivial to resolve).

The highest priority obviously should be on the "trusted" about: URIs, which are just five pages, given that they would break once bug 923902 goes into effect.

rsx11m

Comment 10

•

10 years ago

Following the list in comment #5:

>    blockedFlags: SCRIPT | UNTRUSTED | HIDE,
>    blockedURI: "chrome://communicator/content/blockedSite.xhtml",
2nd-tier HTML, inline JavaScript and CSS style code.

>    certerrorFlags: SCRIPT | UNTRUSTED | HIDE,
>    certerrorURI: "chrome://communicator/content/certError.xhtml",
2nd-tier HTML, inline script code only, no inline style code.

>    dataFlags: SCRIPT,
>    dataURI: "chrome://communicator/content/dataman/dataman.xul",
1st-tier XUL, no inline script and style code, but "onxxx=" definitions which contain an if() clause. Is that too much already?

>    feedsFlags: SCRIPT | UNTRUSTED | HIDE,
>    feedsURI: "chrome://communicator/content/feeds/subscribe.xhtml",
2nd-tier HTML, no script, no inline style code = OK.

>    lifeFlags: SCRIPT | HIDE,
>    lifeURI: "chrome://communicator/content/aboutLife.xhtml",
2nd-tier HTML though trusted atm, both inline script and style code.

>    privatebrowsingFlags: SCRIPT,
>    privatebrowsingURI: "chrome://communicator/content/aboutPrivateBrowsing.xul",
1st-tier XUL, inline script code only, no inline style code.

>    rightsFlags: SCRIPT | UNTRUSTED,
>    rightsURI: "chrome://branding/content/aboutRights.xhtml",
2nd-tier HTML, inline script code only, no inline style code.

>    sessionrestoreFlags: SCRIPT | HIDE,
>    sessionrestoreURI: "chrome://communicator/content/aboutSessionRestore.xhtml",
1st-tier HTML, no inline script, only one style="display: none;" inline.

>    synctabsFlags: SCRIPT,
>    synctabsURI: "chrome://communicator/content/aboutSyncTabs.xul",
1st-tier XUL, neither inline script nor inline style code = OK.

This assumes that any onxxx="yyy.zzz(abc);" attributes are acceptable.

rsx11m

Comment 11

•

10 years ago

> This assumes that any onxxx="yyy.zzz(abc);" attributes are acceptable.

Looking at https://hg.mozilla.org/mozilla-central/rev/12b62c93d740 which just checked in, along with bug 948883 comment #4 ("I see a lot of onclick and style attributes that should be merged into aboutSupport.js and aboutSupport.css respectively"), it seems that they are not.

Following the actions taking in that patch, any "onxxx" attribute will need to be converted into an EventListener and tied to the XUL/HTML element, apparently no exceptions here.  :-\

rsx11m

Updated

•

10 years ago

Depends on: 989776

rsx11m

Updated

•

10 years ago

Depends on: 989777

rsx11m

Updated

•

10 years ago

Depends on: 989780

rsx11m

Comment 12

•

10 years ago

I've filed bug 989776 and bug 989777 on the "about:" and "about:privatebrowsing" pages given that I've worked on those before. I've also filed bug 989780 on "about:life" where some design decisions have to be made to which extent splitting up that page is useful or if it just should be marked as untrusted.

Given that all events need to be covered and not just the "true" inline scripts, going page by page may be the better approach after all. Unbitrotting the jar.mn files should be straight-forward.

rsx11m

Comment 13

•

10 years ago

(In reply to rsx11m from comment #5)
>   blockedFlags: SCRIPT | UNTRUSTED | HIDE,
>   blockedURI: "chrome://communicator/content/blockedSite.xhtml",

>   certerrorFlags: SCRIPT | UNTRUSTED | HIDE,
>   certerrorURI: "chrome://communicator/content/certError.xhtml",

These two are potentially tricky; at the end of the files they both read:

    <!--
    - Note: It is important to run the script this way, instead of using
    - an onload handler. This is because error pages are loaded as
    - LOAD_BACKGROUND, which means that onload handlers will not be executed.
    -->
    <script type="application/javascript">initPage();</script>

Thus, it may not be trivial to just use window.onload here. Firefox has that problem too, hence it may be worthwhile to wait for them and port any changes made in their versions (no privileged content).

>   dataFlags: SCRIPT,
>   dataURI: "chrome://communicator/content/dataman/dataman.xul",

This page has a rather extensive use of event attributes that need to be translated into event listeners similar to bug 989777 for the private-browsing page. It also contains <commandset> and <keyset> where the latter may be subject to bug 371900, thus depending on it getting fixed.

rsx11m

Updated

•

10 years ago

Depends on: 844098

rsx11m

Updated

•

10 years ago

Depends on: 993845

rsx11m

Updated

•

10 years ago

Depends on: 993847

rsx11m

Updated

•

10 years ago

Depends on: 993850

rsx11m

Updated

•

10 years ago

Depends on: 994849

rsx11m

Comment 14

•

10 years ago

A quick status update:

All pages listed in comment #5 are now covered in one of the dependent bugs and taken care of, with the following exceptions:

>   feedsFlags: SCRIPT | UNTRUSTED | HIDE,
>   feedsURI: "chrome://communicator/content/feeds/subscribe.xhtml",

Nothing to do for this file (no scripting, separate style sheets already).

>   rightsFlags: SCRIPT | UNTRUSTED,
>   rightsURI: "chrome://branding/content/aboutRights.xhtml",

I'll work on this in bug 844098 which wants to add a couple of items to match the Firefox page. No rush as it's not a privileged page, moving the inline script and event handlers into a separate file will be part of that redesign.
 
>   dataFlags: SCRIPT,
>   dataURI: "chrome://communicator/content/dataman/dataman.xul",

This one looks just intimidating to me, be my guest if you want to give it a try. ;-)

Thus, with the (admittedly substantial) exception of the Data Manager, all privileged SeaMonkey pages should now be ready for bug 923902 to land.

Philip Chee

Reporter

Comment 15

•

10 years ago

Possibilities:
Make Data Manager open in a (chrome) window, instead of a in-content tab.
Open in a tab but in a <browser type="chrome">