508112 - CAPTCHA doesn't load when certain extensions are installed

Status: VERIFIED FIXED

Description

[Dan Mills [:thunder]]

The CAPTCHA doesn't load when some extensions (e.g. NoScript, Adblock Plus, etc) are installed.

Weave should figure out how to bypass or add itself to the whitelist so we can side-step this problem.

As a temporary measure, we should add a note to the wizard.

Comment 1

[Anant Narayanan [:anant]]

Created attachment 393723 [details] [diff] [review]
Fallback to <noscript> for Captcha

The patch makes captcha display as-is in an iframe, instead of loading it in a seperate XUL <browser> and extracting the image out of it. This has two effects:
- We now comply with ReCaptcha's terms of service which states that the ReCaptcha wordmark must be present in all deployments.
- We fallback to a plain HTML page in case Javascript is disabled (using the <noscript> tag), which asks the user to perform additional steps to validate.

I looked at how we can circumvent NoScript while the captcha is loading but this proves to be difficult, because:
- Adding recaptcha.net to the whitelist does not help.
- Temporarily disabling NoScript via the preferences requires a browser restart to take effect, and I couldn't find any other way to do it.

In it's current form, the patch will allow users to register even when they have NoScript installed, though the process is a bit cumbersome. AdBlockPlus did not have any effect on the captcha in all my tests.

Comment 2

[Dan Mills [:thunder]]

Comment on attachment 393723 [details] [diff] [review]
Fallback to <noscript> for Captcha

I'm OK with this patch, but we should sync up tomorrow.  The switch to about:weave makes this patch sorta obsolete.

I copied some of it into about:weave, but have other issues with the captcha there (it's so big).

Comment 3

[Ragavan S [:rags]]

We should also try talking to the NoScript developers to see if they can whitelist chrome:// or about: URLs.

Comment 4

[Mike Connor [:mconnor]]

I'm not sure how this patch ever worked.  The manual challenge stuff doesn't seem to work sanely...

Comment 5

[Ed Lee :Mardak (PTO Sep 15 - Oct 7)]

We should contact the NoScript devs to see what they did before and if we can just do that again to avoid it being blocked.

Comment 6

[Mike Connor [:mconnor]]

Discussed with rags, we're going to punt on this for 1.0.  We have a warning for users going into the setup, which seems to have deflected most of this.  It's not awesome, but it's good enough to ship.

cc-ing mao, to see if there's something easy we can do to make noscript ignore this <browser> element.

Comment 7

[Giorgio Maone [:mao]]

I'm checking what can be done, but if you're importing a 3rd party web script over an insecure connection, I guess most paranoid users won't be happy of this potentially hijackable source being hardcoded in the whitelist, possibly without a mean to remove it.

On the other hand, I know first hand (my blog) that recaptcha features a decent noscript fallback. 
Why doesn't it work for you (per comment 4)?

Comment 8

[Giorgio Maone [:mao]]

OK, I checked and I've seen why the scriptless recaptcha is ugly and disfunctional in your wizard.

Good news is that you can easily work around by temporarily whitelisting the following "secure" sources:

https://auth.services.mozilla.com
https://api-secure.recaptcha.net

Comment 9

[Mike Connor [:mconnor]]

Giorgio, is there a simple API we can call to do that in NoScript?  I couldn't find my way through the source to find a way to do that.

Comment 10

[Giorgio Maone [:mao]]

Hope this help:

<SNIP>

var ns = Components.classes["@maone.net/noscript-service;1"]
  .getService().wrappedJSObject;

var disabled = [];
["https://auth.services.mozilla.com", "https://api-secure.recaptcha.net"]
  .forEach(function(site) {
  if (!ns.isJSEnabled(site)) {
    disabled.push(site); // save status
    ns.setJSEnabled(site, true); // allow site
  }
});

// do whatever you need to...

//... and finally reset permissions
disabled.forEach(function(site) {
  ns.setJSEnabled(site, false);
});

</SNIP>

Comment 12

[Mike Connor [:mconnor]]

Created attachment 433408 [details] [diff] [review]
fix

adds the selected weave server + api-secure.recaptcha.net to the whitelist, if it's not added already, and cleans up on successful account creation or wizard cancel.

Comment 13

[Ed Lee :Mardak (PTO Sep 15 - Oct 7)]

Comment on attachment 433408 [details] [diff] [review]
fix

>+++ b/source/chrome/content/preferences/fx-setup.js
>@@ -211,16 +204,53 @@ var gWeaveSetup = {
>+  get _remoteSites() {
>+    let serverURL = Weave.Service.serverURL;
>+    // kinda lame, but NoScript chokes on the trailing slash
>+    if (serverURL[serverURL.length - 1] == "/")
>+      serverURL = serverURL.substr(0, serverURL.length - 1);
serverURL might be a whole path and not just a domain. Does noscript care?

>+  _handleNoScript: function (addExceptions) {
>+    if (addExceptions) {
>+      let self = this;
>+      this._remoteSites.forEach(function(site) {
>+        if (!ns.isJSEnabled(site)) {
>+          self._disabledSites.push(site); // save status
>+          ns.setJSEnabled(site, true); // allow site
>+        }
>+      });
You can pass this as a second argument to forEach and it'll be this inside the function.

>+    }
>+    else {
>+      this._disabledSites.forEach(function(site) {
>+        ns.setJSEnabled(site, false);
>+      });
>+      this._disabledSites = [];
Doesn't matter too much, but why conditionally setJSEnabled(true) and push items into _disabledSites and unconditionally setJSEnabled(false) and set to []?

Comment 14

[Giorgio Maone [:mao]]

(In reply to comment #13)
> >+    if (serverURL[serverURL.length - 1] == "/")
> >+      serverURL = serverURL.substr(0, serverURL.length - 1);
> serverURL might be a whole path and not just a domain. Does noscript care?

Yes it does. A "site" for NoScript must be either a prePath (in nsIURI terms) or a host.

There's an easy way to satisfy this, by removing the serverURL trailing slash "lame" hack and slightly changing the _handleNoScript() code as follows:

    this._remoteSites.forEach(function(site) {
       if (!ns.isJSEnabled(site = ns.getSite(site))) {

Comment 15

[Mike Connor [:mconnor]]

Created attachment 433757 [details] [diff] [review]
fix comments

(In reply to comment #13)
> (From update of attachment 433408 [details] [diff] [review])
> serverURL might be a whole path and not just a domain. Does noscript care?

yes, fixed.

> You can pass this as a second argument to forEach and it'll be this inside the
> function.

right right, fixed

> Doesn't matter too much, but why conditionally setJSEnabled(true) and push
> items into _disabledSites and unconditionally setJSEnabled(false) and set to
> []?

Because if one of the domains is already whitelisted, we don't want to muck with that setting.

Comment 16

[Ed Lee :Mardak (PTO Sep 15 - Oct 7)]

Comment on attachment 433757 [details] [diff] [review]
fix comments

Did you attach the wrong patch? Seems the same as the previous one. ?

Comment 17

[Mike Connor [:mconnor]]

Created attachment 433771 [details] [diff] [review]
fix comments

Bah, yes.

Comment 18

[Mike Connor [:mconnor]]

http://hg.mozilla.org/labs/weave/rev/22d53cec2c35

Comment 19

[Tony Chung [:tchung]]

Verified fix on 1.2b5.  noscript and adblock plus works with captcha