Open Bug 1430750 Opened 7 years ago Updated 2 years ago

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

Categories

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

Product:
Core
Component:
DOM: Security
Type:
enhancement

Tracking

()

People

(Reporter: ckerschb, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-backlog1])

Attachments

(1 file)

bug_1430750_convert_about_blocked.patch
29.65 KB, patch
Gijs
: feedback+
Details | Diff | Splinter Review
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

Creator:
Created:
Updated:
Size: