1609635 - Snippets broken in Nightly 74

Status: VERIFIED FIXED

(Firefox :: Messaging System, defect, P1)

Description

[Alin Ilea, Desktop QA]

Affected versions:

Nightly 74.0a1 (2020-01-16)

Affected platforms:

ALL

Preconditions:

Have the "browser.newtabpage.activity-stream.asrouter.devtoolsEnabled" pref set to "true" in the "about:config" page.

Steps:

1. Navigate to the "about:newtab#asrouter" page and select the "snippets_local_testing" from the "Messages" section's dropdown menu.
2. Click the "Show" button from the "SIMPLE_TEST_1" section.

Actual result:

The "SIMPLE_TEST_1" snippet is displayed in the bottom part of the browser but there is no hyperlink.

Expected result:

The body text of the snippet should contain a hyperlink in the right part of the firefox icon.

Please see also the following bug, might be related to this issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1601880

Regression range:

Last good revision: 72bc4f39b6598b06925c9a3372a5076f5e6f78ab
First bad revision: 01fde39940c3d8baa41dc747bf4454cf312be78c
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=72bc4f39b6598b06925c9a3372a5076f5e6f78ab&tochange=01fde39940c3d8baa41dc747bf4454cf312be78c

Comment 1

[Andrei Oprea [:andreio]]

It's not related to bug 1601880

Comment 2

[Johann Hofmann [:johannh]]

This seems to have regressed with bug 1600941. Briefly talking to Andrei, we were a bit confused why the Snippet system (which pipes to Fluent) get their links stripped while other Fluent consumers like about:preferences (which is also sanitized) are working fine.

Our suspicion is that this is because they're not using the data-l10n-name system with pre-embedded elements but rather trying to feed arbitray links directly into Fluent.

I'm not sure what the right solution to this is. We could allow-list about:newtab to insert arbitrary links? Assuming there's a strong product desire to continue showing relatively arbitrary links, I don't think we have a safer viable alternative.

CC'ing some folks to see what they think.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Stas and Pike are the right people to advise you how to use Fluent in a canonical fashion.

I'll point you at the docs:

• https://github.com/projectfluent/fluent.js/wiki/DOM-Overlays#a-example
• https://firefox-source-docs.mozilla.org/l10n/fluent/tutorial.html#markup-localization

It would help us to understand why you can't use the recommended solution, so that we can evaluate what's the best option for your use case. We're also collecting requirements for the future features of Fluent :)

Comment 4

[Andrei Oprea [:andreio]]

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #3)

Some context:

• We created the current solution together with the Fluent team
• The constraint is the snippets delivery service:
  • it serves a JSON payload with the snippet content that needs to be rendered in the page
  • That content has arbitrarily placed links and HTML styling.
  • The links need to have attached event listeners for telemetry purposes but also for navigation (a snippet can have multiple states, actions etc).

Comment 5

[Axel Hecht [:Pike]]

So, this has nothing to do with Fluent, but w/ innerHTML having changed behavior for about pages, right?

The Fluent-related question could be: Is there a way that Fluent can help here? I don't think there is.

Which brings me to the same conclusion that Johann has, maybe this needs an option to disable the security checks.

Comment 6

[:Gijs (he/him)]

Can we file a separate bug for automated tests for this functionality? The fact that we broke it with routine security hardening work and didn't notice is disconcerting.

Where does the actual insertion of the link happen? I found https://searchfox.org/mozilla-central/rev/c52d5f8025b5c9b2b4487159419ac9012762c40c/browser/components/newtab/content-src/asrouter/components/RichText/RichText.jsx#24 but I can't see the trees for the react forest.

(In reply to Axel Hecht [:Pike] from comment #5)

So, this has nothing to do with Fluent, but w/ innerHTML having changed behavior for about pages, right?

The Fluent-related question could be: Is there a way that Fluent can help here? I don't think there is.

There is in the sense that fluent would do DOM reconciliation, and if the href and onclick were already in the DOM then they won't be inserted via innerHTML. That may not be the most straightforward solution though.

Which brings me to the same conclusion that Johann has, maybe this needs an option to disable the security checks.

Or it could use DOM manipulation rather than innerHTML ; the simplest solution would be using data- attributes in whatever gets stuck in innerHTML and then move them to the "real" attributes (href and onclick, I think?) afterwards. Assuming we're not just removing the <a> element here altogether...

Comment 7

[Andrei Oprea [:andreio]]

We have mochitests for snippets but they don't test the content itself.

Comment 8

[Andrei Oprea [:andreio]]

Attachment: Bug 1609635 - Whitelist about:newtab/home from content sanitization

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf5cb55383a5
Whitelist about:newtab/home from content sanitization r=ckerschb

Comment 12

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/cf5cb55383a5

Comment 13

[Alin Ilea, Desktop QA]

Verified - Fixed in latest Nightly 74.0a1 (Build id: 20200204214324) using Windows 10, Ubuntu 18.04 and Mac OS 10.15. The hyperlink is displayed in the body text of the snippet.