1244929 - Allow UITour's showFirefoxAccounts command to accept URL params and have then passed to about:accounts such that they are then passed to accounts.firefox.com

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

See bug 1162192 comment 62 - when bedrock arranges via the UITour library to open about:accounts, it doesn't have a way to pass URL params, so campaign tracking information is lost.

It looks like this task is roughly:

* Modify the library itself (http://dxr.mozilla.org/mozilla-central/source/browser/components/uitour/UITour-lib.js) to allow URL params to be passed.

* Modify the receiver of these messages (https://dxr.mozilla.org/mozilla-central/source/browser/components/uitour/UITour.jsm#645) to accept these params and append them to the about:accounts URL it opens.

* Manually verify that these params are then automagically passed from about:accounts to the accounts.firefox.com iframe (which I believe already happens - there is already an "entrypoint=uitour" param passed for the same basic reason.

There will also need to be a new issue opened (on github?) for bedrock to work out what these params should be and pass them via the above-described facility.

Comment 1

[Jonathan Petto [:jpetto]]

When it comes time to make updates on bedrock, please file a bug and cc myself and :agibson.

Thanks!

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-1244929-Allow-UITour-s-showFirefoxAccounts-comma.patch

This patch seems simple enough and it looks like bug 1162192 has forgotten this ever existed :) Matt, what do you think?

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8721889 [details] [diff] [review]
0001-Bug-1244929-Allow-UITour-s-showFirefoxAccounts-comma.patch

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

Maybe I'm just being paranoid but I think we need to be much stricter in our approach. We've generally tried to treat the UITour inputs as untrusted even though there is a list of allowed origins. I don't think we've given control to arbitrary chrome URL params in UITour before so this is potential for new issues.

I would feel better if we limit the allowed param names to "utm_" prefixed ones and used URLSearchParams to encode the resulting URL. I would feel better if we only allow basic parameter characters e.g. [-_a-zA-Z0-9] as escaping alone won't help potential XSS issues on the about:accounts side.

Nit: there seems to be an unexpected line break in your commit message

::: browser/components/uitour/UITour-lib.js
@@ +234,5 @@
>      });
>    };
>  
> +  Mozilla.UITour.showFirefoxAccounts = function(extraUrlParams) {
> +    // extraUrlParams is a string and will be appended to the FxA's URL params.

Nit: `URL` is generally preferred over `Url` in front-end code (it's also my preference)

@@ +235,5 @@
>    };
>  
> +  Mozilla.UITour.showFirefoxAccounts = function(extraUrlParams) {
> +    // extraUrlParams is a string and will be appended to the FxA's URL params.
> +    // It should not have a leading ? or & (but will have embedded & chars if

Please convert this comment into JSDoc form. I would like to move all the documentation from the bedrock docs to JSDoc and have it auto-generated eventually.
@param {String} extraURLParams …

@@ +236,5 @@
>  
> +  Mozilla.UITour.showFirefoxAccounts = function(extraUrlParams) {
> +    // extraUrlParams is a string and will be appended to the FxA's URL params.
> +    // It should not have a leading ? or & (but will have embedded & chars if
> +    // more than 1 extra param should be sent.)

There is no mention of URL encoding and you're not doing it in UITour.jsm either. I think we should do that for the consumer to also avoid potential security issues but we also need to be careful we don't double-encode to we should tell consumers of this API not to encode. This won't matter if we have an allow-list of characters as I propose in UITour.jsm

::: browser/components/uitour/UITour.jsm
@@ +611,5 @@
>          // 'signup' is the only action that makes sense currently, so we don't
>          // accept arbitrary actions just to be safe...
> +        let url = "about:accounts?action=signup&entrypoint=uitour";
> +        if (typeof data.extraUrlParams == "string") {
> +          url += "&" + data.extraUrlParams;

I think we should be stricter here and only allow "safe" characters since about:accounts has chrome privileges IIUC.

There's also nothing stopping the "untrusted" content from specifying values for `action` and `entrypoint` again which may be bad. I don't know what other URL parameters have meaning to this page but perhaps we should only allow certain param names (e.g. prefixed with "utm_" which is what we used for campaign tracking with Hello before [1]).

You can use URLSearchParams to do the encoding for you btw.:
let p = new URLSearchParams("action=signup&entrypoint=uitour");
p.append("utm_foo", "bar baz");
p.toString(); // action=signup&entrypoint=uitour&utm_foo=bar%20baz

[1] https://mxr.mozilla.org/mozilla-central/search?string=utm_&find=loop&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

::: browser/components/uitour/test/browser_UITour_sync.js
@@ +27,5 @@
>    yield BrowserTestUtils.browserLoaded(gTestTab.linkedBrowser, false,
>                                         "about:accounts?action=signup&entrypoint=uitour");
>  });
> +
> +add_UITour_task(function* test_firefoxAccounts() {

Please use different test function names (x2) since they are output in the logs and are useful for debugging. I wouldn't want someone to accidentally debug the wrong function.

@@ +34,5 @@
> +                                       "about:accounts?action=signup&entrypoint=uitour&foo=bar");
> +});
> +
> +add_UITour_task(function* test_firefoxAccounts() {
> +  yield gContentAPI.showFirefoxAccounts(0); // non-string should be ingored.

Typo: "ignored"

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-1244929-Allow-UITour-s-showFirefoxAccounts-comma.patch

Something like this?  (Note the commit message being wrapped is a side-effect of how I get git to export the patches)

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8722311 [details] [diff] [review]
0001-Bug-1244929-Allow-UITour-s-showFirefoxAccounts-comma.patch

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

Thank Mark.

Please double-check that the "utm_" params are sufficient before landing to avoid churn.

::: browser/components/uitour/UITour.jsm
@@ +632,5 @@
> +          return false;
> +        }
> +        if (campaignParams) {
> +          // The regex that both the name and value of each param must match.
> +          let reSimpleString = /^[-_a-zA-Z0-9]*$/;

This `case` is kinda long now that perhaps from here down should move to its own method but I don't feel super strongly about it.

@@ +633,5 @@
> +        }
> +        if (campaignParams) {
> +          // The regex that both the name and value of each param must match.
> +          let reSimpleString = /^[-_a-zA-Z0-9]*$/;
> +          for (let [name, value] of Iterator(campaignParams)) {

`Iterator` is non-standard (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Iterator) so it may be better to use Object.keys so we don't have to fix this later. Since Object.entries is Nightly-only you can't use it yet.

@@ +638,5 @@
> +            if (typeof name != "string" || typeof value != "string" ||
> +                !name.startsWith("utm_") ||
> +                value.length == 0 ||
> +                name.match(reSimpleString) == null ||
> +                value.match(reSimpleString) == null) {

As Gavin said on IRC, use .test instead

::: browser/components/uitour/test/browser_UITour_sync.js
@@ +42,5 @@
> +      return gBrowser.selectedBrowser.currentURI.spec.startsWith("about:accounts");
> +    }, "Check if about:accounts opened");
> +    ok(false, "No about:accounts tab should have opened");
> +  } catch (ex) {
> +    ok(true, "No about:accounts tab opened: " + ex);

Nit: What does `ex` get stringified to in this success case? If it's more than just "Check if about:accounts opened" then it may be a bit verbose for the common success case.

@@ +47,5 @@
> +  }
> +}
> +
> +add_UITour_task(function* test_firefoxAccountsNonObject() {
> +// non-string should be rejected.

Nit: indent

@@ +54,5 @@
> +});
> +
> +add_UITour_task(function* test_firefoxAccountsNonUtmPrefix() {
> +  // Any non "utm_" name should should be rejected.
> +  yield gContentAPI.showFirefoxAccounts( {utm_foo: "foo", bar: "bar" } );

Nit: No space after `(` and before `)`. I think there is an enabled eslint rule for this but I could be wrong.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Jean and/or Chris,
  This bug was spun out from bug 1162192 to try and help track your campaigns through about:accounts. I've a patch that allows *only* params starting with utm_ to be specified, and each param value must be alpha-numeric + dash and underscore. Can you please confirm that this will meet your requirements, or help me find the correct person to answer that question?

Comment 7

[Chris More [:cmore]]

(In reply to Mark Hammond [:markh] from comment #6)
> Jean and/or Chris,
>   This bug was spun out from bug 1162192 to try and help track your
> campaigns through about:accounts. I've a patch that allows *only* params
> starting with utm_ to be specified, and each param value must be
> alpha-numeric + dash and underscore. Can you please confirm that this will
> meet your requirements, or help me find the correct person to answer that
> question?

Hi Mark. That sounds great and about all we need here. utm_ parameters is suffice with those values mentioned. This will give some insights on how well that page is performing compared to other touchpoints like /firefox/sync/, /firefox/accounts, or /firefox/firstrun/. Especially if we to channel testing on that in-product page compared to websites.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-1244929-Allow-UITour-s-showFirefoxAccounts-comma.patch

Thanks for the review - all comments addressed.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/800b8a648489

Comment 10

[Mark Hammond [:markh] [:mhammond]]

I opened bug 1250758 for the bedrock changes to take advantage of this. I think this patch is also safe to uplift if that's important - Chris, I assume you consider it worth uplifting? If so, could you please give a short description on exactly why so I can include it in an uplift request?

Comment 11

[Chris More [:cmore]]

(In reply to Mark Hammond [:markh] from comment #10)
> I opened bug 1250758 for the bedrock changes to take advantage of this. I
> think this patch is also safe to uplift if that's important - Chris, I
> assume you consider it worth uplifting? If so, could you please give a short
> description on exactly why so I can include it in an uplift request?

What channel would this be uplifted from and to?

Comment 12

[Mark Hammond [:markh] [:mhammond]]

(In reply to Chris More [:cmore] from comment #11)
> What channel would this be uplifted from and to?

It landed on Nightly, so without uplift it would only work on 47 and later. If we uplifted all the way to beta it would be available on 45 - but that might be pushing our luck with the release managers. Aurora would mean it becomes available in 46.

Comment 13

[Chris More [:cmore]]

(In reply to Mark Hammond [:markh] from comment #12)
> (In reply to Chris More [:cmore] from comment #11)
> > What channel would this be uplifted from and to?
> 
> It landed on Nightly, so without uplift it would only work on 47 and later.
> If we uplifted all the way to beta it would be available on 45 - but that
> might be pushing our luck with the release managers. Aurora would mean it
> becomes available in 46.

Let's not push our luck. Let's let it ride the trains for a while and if need an uplift later, we can discuss. Thanks!

Comment 14

[Lina Butler [:lina]]

https://hg.mozilla.org/mozilla-central/rev/800b8a648489