mdn / kuma

The project that powers MDN.
https://developer.mozilla.org
Mozilla Public License 2.0
1.93k stars 679 forks source link

People are redirected to https://www.mozilla.org/en-US/newsletter/ #5961

Closed peterbe closed 4 years ago

peterbe commented 4 years ago

Originally from: https://bugzilla.mozilla.org/show_bug.cgi?id=1587802

User reported that no matter which page he/she goes to it redirects to https://www.mozilla.org/en-US/newsletter/

schalkneethling commented 4 years ago

It is very strange that this would happen simply by visiting a page. With that said, the newsletter code has the following logic:

If a user completes all required field for the form and the clicks the button to subscribe, the code will:

  1. Attempt to subscribe the user using a fetch call
  2. Should the server respond with a failure, but the errors array is empty(https://github.com/mozilla/kuma/blob/master/kuma/javascript/src/newsletter.jsx#L141),
  3. The code will resubmit the form, but this time, it will not do it via fetch, but just let the form action take over i.e. it send the user to `https://www.mozilla.org/en-US/newsletter

https://github.com/mozilla/kuma/blob/master/kuma/javascript/src/newsletter.jsx#L108

schalkneethling commented 4 years ago

Found this piece of code: https://github.com/mozilla/kuma/blob/master/kuma/javascript/src/newsletter.jsx#L172

useEffect(() => {
        const newsletterForm = newsletterFormRef.current;
        if (submitToServer && newsletterForm) {
            setSubmitToServer(false);
            newsletterForm.submit();
        }
    }, [submitToServer]);

It seems to trigger a form submit onload if both submitToServer and newsletterForm is true. It does however set submitToServer to false, which means it should attempt it using fetch

With all of that said though, I am unsure why this piece of code exists at all @Gregoor any thoughts?

Gregoor commented 4 years ago

Flabbergasted by the original issue atm, here is what's in my mind:

So this code I turned into something effect-y because the previous code relied on storing state in the DOM, which wasn't very React (and imo harder to follow). As Schalk correctly identified this will only be set to true if the form was already submitted once and the server returned an empty errors array, i.e.:

if (errors && errors.length === 0) {
  // resubmit for diagnoses on the server (see skipFetch-
  // comment above)
  setSubmitToServer(true);
}
schalkneethling commented 4 years ago

@Gregoor what would happen if we removed that piece of useEffect code? Will the form still work as expected when a user interacts with it i.e. clicks the subscribe button?

Gregoor commented 4 years ago

So in my mind this can only happen when something makes the form auto-fill and auto-submit. The most likely culprit is a password-manager. It would also need to fill the form and check the box, and on top of that the server needs to respond with an empty-error array. Smells fishy to me.

@pmclanahan Do you know in which case basket would return an empty errors array?

@Gregoor what would happen if we removed that piece of useEffect code? Will the form still work as expected when a user interacts with it i.e. clicks the subscribe button?

Yes! It should still work fine. I'll get on that as a quick-fix!

Gregoor commented 4 years ago

Okay alternative explanation: It's not that piece of code but rather using that form with JS disabled. Then again the privacy checkbox doesn't even show up with JS disabled, which we might want to treat as a separate bug. So something like a password manager would still need to be involved. Without JS the form would straight up redirect to m.o/newsletter

schalkneethling commented 4 years ago

Then again the privacy checkbox doesn't even show up with JS disabled, which we might want to treat as a separate bug.

Agreed. That is why I prefer the pattern of, show everything and then hide things with JS(via classes) when enabled.

Without JS the form would straight up redirect to m.o/newsletter

You would still need to click the button though, especially with JS disabled. A very strange bug indeed.

apboon commented 4 years ago

Hey, original reporter here,

I think I found the problem. Both accounts.firefox.com and developer.mozilla.org are using a <input> with a name attribute set to email (<input name="email" />). I have my LastPass **automatically** log intoaccounts.firefox.com, but this is associated withdeveloper.mozilla.orgtoo. Therefore LastPass automatically fills in my email address into the Newsletter sign up ofdeveloper.mozilla.org`.

Maybe the Newsletter sign up (on developer.mozilla.org) should have a autocomplete=off? Or use unique field names so forms won't get mixed up?

On accounts.firefox.com, my LastPass automatically picked up both username and email fields, so I suspect username was used in the past and changed to the now current email field name. Maybe use username for the login form?

apboon commented 4 years ago

Reading previous comments, I think you guys are on to something regarding the form still able to submit (by a password manager) while the privacy checkbox isn't checked.

Gregoor commented 4 years ago

Thanks for getting back to us! I unfortunately wasn't able to reproduce it in Fx nor Chrome with LastPass enabled. On neither browser auto-fill tried to submit the form and I didn't get a redirect. From your description I was also surprised that accounts.firefox.com would affect MDN, given that they're on separate domains. But maybe I'm just underestimating LastPass. Would you mind sharing which OS and Browser version you're using?

apboon commented 4 years ago

OS: KDE neon 5.16 (User Edition) Browser: Chromium version 77.0.3865.90 (Official Build) built on Ubuntu, running on neon 18.04 (64-bit)

I think LastPass recorded the progression/redirect between the websites and saved a list to which the password entry applies. Although I am not sure..

On neither browser auto-fill tried to submit the form and I didn't get a redirect.

Did you use the Autologin setting within the password entry?

schalkneethling commented 4 years ago

Thank you so much for the additional information @apboon - This is going to be super useful for debugging this issue. The one thing that I find odd is, with autofill, it will most likely fill in the email field but, would it also tick the privacy field checkbox?

If not, the form should not submit unless novalidate is set on the form element. Should it somehow submit without the privacy checkbox checked, the server should respond with an error(as it does), but the error reason array will not be empty.

Thank you again for reporting this problem and your continued feedback. Is it very much appreciated.

apboon commented 4 years ago

would it also tick the privacy field checkbox?

It would not. Sometimes there's a required checkbox on a login form, in such case I always need to manually tell LastPass to check it for Autologin to work.

I am unsure how LastPass submits the form.. if it is even submitting the form and not instead creating a clone of the form (just an example)..

Is validation being checked on form onsubmit or the submit button's onclick? Because I can imagine LastPass ignoring the button and executing form.submit(); instead.

No worries; Happy to help :)

peterbe commented 4 years ago

Is validation being checked on form onsubmit or the submit button's onclick? Because I can imagine LastPass ignoring the button and executing form.submit(); instead.

To reproduce...

Screen Shot 2019-10-14 at 9 53 36 AM

Type that in the web console and then this happens:

Screen Shot 2019-10-14 at 9 54 50 AM

I think the PR addresses this: https://gist.github.com/peterbe/3d10d5867644c7c8bfbc80c3107aa1f2

peterbe commented 4 years ago

I checked. The problem is not really solved.

Screen Shot 2019-10-14 at 9 57 44 AM

When I hit Enter in the web console ^ it still redirects to www.mozilla.org.

peterbe commented 4 years ago

The problem is that the home page uses newsletter.html which isn't controlled.

peterbe commented 4 years ago

Actually, it's deeper than that!

From https://developer.mozilla.org/en-US/docs/Web/API/HTMLFormElement/submit

No submit event is raised. In particular, the form's onsubmit event handler is not run.

I.e.

<form action="https://www.com" onsubmit="return false">
</form>
document.querySelector('form').submit();

it will still go ahead and submit the form! See: https://whip-nautilus.glitch.me/

peterbe commented 4 years ago

So if a human inside the Web Console or a "weird" browser extension triggers a submission of the form, there is no JavaScript in the world we can use to prevent this form from submitting.

Steps to reproduce:

Write a web extension or grease monkey that does something like...

const postForm = document.querySelector('form[method="post"]');
postForm && postForm.submit();

If I'm not mistaken, @apboon you originally reported that the redirect happens on all pages of developer.mozilla.org. Not just the home page, right? We actually have two newsletter sign-up forms (which needs to be fixed too!). One called newsletter.html and one called newsletter.jsx.

Honestly, having a form on a webpage is totally kosher and nothing we need to change. The browser just shouldn't rather arbitrarily submit the form. LastPass is common and perhaps the combination of LastPass and Firefox Accounts integration is common too so we need to chase that and see if there's an exception/hack we can put in so that those browser extensions don't act like this on our site.

What's still a mystery to me is; why wasn't this happening before we switched the front-end. In fact the home page hasn't changed. It's using the same HTML for the form as before we switched the front-end last week.

peterbe commented 4 years ago

@apboon I've now installed LastPass in both Firefox Nightly and Chrome (77). I've never used these tools before so perhaps I'm not sure how they work. I tried everything to try to trigger the Autologin but no success. I can at least have it form fill my email address in the newsletter form. But for that I have to click the little red browser extension icon and find the right "Fill" button.

In other words, I can't even get the form to auto-fill unless I touch something.

I did find this setting:

Screen Shot 2019-10-14 at 10 52 02 AM

But it doesn't appear to do anything.

Can you safely share how you think your LastPass is configured when it applies to developer.mozilla.org?

peterbe commented 4 years ago

Another thing that "scares" me is that suppose that we set autocomplete="off" on those forms (which I think is wrong anyway!) it probably wouldn't do anything because LastPass users would need to go into their Advanced settings to enable respect for this.:

Screen Shot 2019-10-14 at 11 13 14 AM

The default is that this is off.

peterbe commented 4 years ago

In conclusion...

  1. We have a bug in that we have two separate newsletter sign up forms. We need to consolidate that but perhaps as a separate issue.

  2. Our form implementation is sane. LastPass is wrong in that it submits the newsletter form.

  3. There's no JS we can write to prevent this bug.

  4. We can't edit the default name of the input field to trick LastPass because then the no-JS form submission fallback to www.mozilla.org would break.

  5. Messing with autocomplete="off" might not be an option.

tobinmori commented 4 years ago
tobinmori commented 4 years ago

Closing this because we don't have enough info as to whether this is a major issue.