Open Bug 1430750 Opened 6 years ago Updated 2 years ago

Apply unified behavior model for about: pages to about:blocked

Categories

(Core :: DOM: Security, enhancement, P3)

enhancement

Tracking

()

People

(Reporter: ckerschb, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-backlog1])

Attachments

(1 file)

      No description provided.
Assignee: nobody → ckerschb
Blocks: 1430748
Status: NEW → ASSIGNED
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
Gijs, instead of relying on custom events fired within blockedSite.xhtml (which are caught in content.js) we can expose a readonly webidl attribute on the document which allows to query the required information from within about:blocked. Using a webidl dictionary we can return a string in JSON format which can be parsed within aboutBlocked.html which then allows to dynamically the blank HTML containers for about:blocked.

Here is a detailed description of the changes including its benefits:
* Provide readonly string (in JSON format) within Document.webidl which allows to query the required information about the blocked site
* Using Func="CallerIsAboutPage" we can enforce that attribute is only exposed to about: pages (I would like to keep that generic for now)
* blockedSite.xhtml is replaced by aboutBlocked.html; I think we should use *.html instead of xhtml.
* Using the new JSON we can detect the error type within nsDocument and already return a localized version of the strings. In turn this change allows to shrink and simplify the *.html quite a bit. For example, no need to remove elements from the *.html anymore and no need for various div containers.
* Remove the custom event listener from content.js
* Allows to define the prefs (e.g. for the learnMore link) within all.js and not within content.js. I think those prefs don't need to be hardcoded within content.js but rather in a unified place like all.js.
* All JS can be loaded from an external file
* Allows to apply CSP so no inline script can be executed
* Remove the query arguments from about:blocked because all that inforamtion is available within DocShell (that change is not in the patch as of now)

Open Issues/Questions/Todos:
* I would like to get even a clearer separation of markup and content, which seems tricky because of localization. Any opinion if we should entirely remove markup from blocked.properties?
* Should we have a separate file for the functionality currently in GetBlockedData? Would it make sense to encapsulate that within an AboutPageHandler.cpp?
* Currently Android uses it's own version of blockedSite.xhtml; Desktop and mobile version seem completely identical; do you think it's possible to unify the two to remove complexity?

Obviously there is way more work to be done for this bug, but I wanted to make sure we are on the right track.
Attachment #8942884 - Flags: feedback?(gijskruitbosch+bugs)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)
> * I would like to get even a clearer separation of markup and content, which
> seems tricky because of localization. Any opinion if we should entirely
> remove markup from blocked.properties?

Yes please.

> * Should we have a separate file for the functionality currently in
> GetBlockedData? Would it make sense to encapsulate that within an
> AboutPageHandler.cpp?

I don't have a strong opinion on this, though I will say that docshell.cpp is very big and making it smaller is going to be helpful. I don't know how hard it is to put this elsewhere though.

> * Currently Android uses it's own version of blockedSite.xhtml; Desktop and
> mobile version seem completely identical; do you think it's possible to
> unify the two to remove complexity?

Probably, but it'd be best to check with a mobile peer like :jchen or :snorp.

> Obviously there is way more work to be done for this bug, but I wanted to
> make sure we are on the right track.

Yep, makes sense!
Comment on attachment 8942884 [details] [diff] [review]
bug_1430750_convert_about_blocked.patch

Review of attachment 8942884 [details] [diff] [review]:
-----------------------------------------------------------------

I think in general this is on the right track.

::: browser/base/content/aboutBlocked.html
@@ +2,5 @@
> +  <head>
> +    <link rel="stylesheet" href="chrome://browser/skin/blockedSite.css" type="text/css" media="all" />
> +    <link rel="icon" type="image/png" id="favicon" href="chrome://global/skin/icons/blacklist_favicon.png"/>
> +    <!-- CSP does not apply to chrome:// URLs, hence we can use script-src 'none' -->
> +    <meta http-equiv="Content-Security-Policy" content="script-src 'none'">

Can/should we add further restrictions to this CSP for things other than scripts?

::: browser/base/content/aboutBlocked.js
@@ +3,5 @@
> +let blockedDataObj = JSON.parse(document.blockedData);
> +
> +// alert(document.blockedData);
> +
> +document.getElementById("errorTitleText").innerHTML = blockedDataObj.errorTitleText;

Please use .textContent instead.

Where the use of DOM things like links is required, please fetch the string, split on replacement items ( /%(?:\$\d)?S/ ) and put markup in there, with content for markup (ie link text, for instance) in a separate string.

@@ +14,5 @@
> +document.getElementById("errorLearnMoreText").innerHTML = blockedDataObj.learnMoreText;
> +
> +// learnMoreLink
> +
> +document.getElementById("seeDetailsButton").onclick = function(event)

Please use addEventListener instead.

::: browser/base/content/browser.js
@@ +3076,5 @@
>  
>      switch (elementId) {
>        case "goBackButton":
> +      // todo: currently the 'go back' button collects telemetry, we have to incorporate
> +      // that change (probably into nsDocument)

I'm not sure I follow this bit. Can't we do this from within content.js ?

::: browser/base/content/moz.build
@@ +104,1 @@
>  with Files("blockedSite.xhtml"):

We should propagate the rename of the file but that should be it.

::: browser/base/jar.mn
@@ +147,5 @@
>  # the following files are browser-specific overrides
>  *       content/browser/license.html                  (/toolkit/content/license.html)
>  % override chrome://global/content/license.html chrome://browser/content/license.html
>          content/browser/report-phishing-overlay.xul     (content/report-phishing-overlay.xul)
> +# probably we should remove blockedSite.xhtml

Yes.

::: docshell/base/nsDocShell.cpp
@@ +8799,5 @@
>      MOZ_ASSERT(failedURI, "We don't have a URI for history APIs.");
>  
>      mFailedChannel = nullptr;
>      mFailedURI = nullptr;
> +    //mFailedChannelErrorType.Truncate();

Can you elaborate on what's going on here? Why can't we truncate things here?

::: dom/base/nsDocument.cpp
@@ +3450,5 @@
>    return true;
>  }
>  
>  bool
> +nsDocument::CallerIsAboutPage(JSContext* aCx, JSObject* /*unused*/)

I'm pretty nervous about applying this to all about: pages. Maybe ping someone who knows more about webidl if there's a way we can restrict this specifically to about:blocked?

If that's not possible, I think I would like to ensure that the about: page isn't world-linkable. You should be able to do this by getting the per-URI flags from the about: protocol handler and checking it's DANGEROUS_TO_LOAD or whatever it is.

::: dom/locales/en-US/chrome/about/blocked.properties
@@ +10,5 @@
> +# %1$S is the brandname
> +SafeBrowsingBlockedMalwareShortDesc = %1$S blocked this page because it might attempt to install malicious software that may steal or delete personal information on your computer.
> +SafeBrowsingGoBackButtonLabel = Go back
> +# %1$S is the blocked URL
> +SafeBrowsingBlockedMalwareErrorDescOverride = %1$S <span id='malware_sitename'/> has been <a id='error_desc_link'>reported as containing malicious software</a>. You can <a id='report_detection'>report a detection problem</a> or <a id='ignore_warning_link'>ignore the risk</a> and go to this unsafe site.

Looks like these links are supposed to have behaviour attached to them that I'm not seeing implemented right now?
Attachment #8942884 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
Assignee: ckerschb → nobody
Status: ASSIGNED → NEW
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: