994911 - [FxAccounts] COPPA warning message shows raw html

Status: RESOLVED FIXED

(Firefox OS Graveyard :: FxA, defect)

Description

[No-Jun Park [:njpark]]

Attachment: 2014-04-10-15-55-42.png

STR:

- Go to settings, firefox accounts
- enter a new email address to create a new fxaccount
- Pick 2003 as birthyear

Expected:
- Proper COPPA error message shows up

Actual:
- HTML tag is showing on the dialog

Comment 1

[No-Jun Park [:njpark]]

Version Info:
Gaia      5827cb13b1a033141ce539e8fbd44cd0bf16593d
Gecko     https://hg.mozilla.org/mozilla-central/rev/690c810c8e3e
BuildID   20140410040201
Version   31.0a1

Comment 2

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

I dunno what I am doing wrong, I got a blank dialog here instead of a COPPA message. Maybe merging the system copy updates patch (bug 987418) broke something--I'll bisect in the morning.

Comment 3

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Yeah, looks like 987418 is getting reverted for unrelated problems, so I'll be taking another swing at that first. Hopefully this really is a dupe.

Comment 4

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

The COPPA error message contains HTML, but it's currently being rendered as text.

One way to render this correctly would be to use mozL10n.localize() instead of mozL10n.get(). However, right now, the error strings are looked up inside the fxam_module.js file, then passed to the fxam_error_overlay.js file, which holds pointers to the error overlay's DOM nodes. localize() requires an element as its first argument, so we'd have to move a lot of code from fxam_module into fxam_error_overlay for this to work.

The approach I've used here is much more direct: we just render the error message as innerHTML instead of textContent. Since we control the error message contents, there's no security risk here.

I've used the FTC COPPA website as the target. I found this link inside the fxa-content-server repo, so I assumed that link was approved by product.

Note also that, rather than use a WebActivity, I've simply got an anchor tag. This will open a web browser with no chrome; by hitting the home button, the user will dismiss the COPPA info website as well as the FxA system app. This seems like what we want to happen anyway (show the user the COPPA notice, then bail).

If you are inside the FTE flow, start to sign up for an FxA account, and hit the COPPA error, then click through to the FTC site, and tap the home button to dismiss it, what happens? You are stuck and have to restart your phone. This stinks. I will try to find a better way (with some kind of dismiss button) before I submit for review.

Comment 5

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: Github PR 18460

Comment 6

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Ah, this reminds me: there is a bug with password reset flow, where we don't want to open a WebActivity from within FTE, because there's no way to dismiss the browser there, either.

I think the solution is going to be different for COPPA errors encountered within FTE. I'm going to add a note to fix that as part of the Reset Password FTE bug, bug 980638.

But for COPPA errors encountered outside FTE, this seems like a fine approach.

Comment 7

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8409111 [details] [review]
Github PR 18460

Hi Fernando,

Do you have time to review this?

One thing I'm not sure about. Rather than just open a link directly from the overlay, we could grab the click and send it to fxam_server_request.js, asking that file to open a Web Activity to view the page. I think that's a lot of complexity for little gain (for instance, the error overlay will have to look for that anchor tag, and I'm not sure if it currently delegates clicks to the top level, or if we'd have to check for the element each time a new error message was displayed). However, if that's the preferred approach, I'm happy to change it.

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8409111 [details] [review]
Github PR 18460

Don't you need to change the string id too?

Comment 9

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #8)
> Comment on attachment 8409111 [details] [review]
> Github PR 18460
> 
> Don't you need to change the string id too?

Doh, yes, of course. Thanks :-)

Comment 10

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Master: https://github.com/mozilla-b2g/gaia/commit/6eb70ac0f891f7d7614ee71460e9925ad89ec0f8