Skip to content
This repository has been archived by the owner on Jun 25, 2020. It is now read-only.

Newsletter form not sending proper content-type #210

Open
pmac opened this issue Jun 30, 2016 · 14 comments
Open

Newsletter form not sending proper content-type #210

pmac opened this issue Jun 30, 2016 · 14 comments

Comments

@pmac
Copy link
Member

pmac commented Jun 30, 2016

Basket is receiving posts from the site with a text/plain content type and is thus doing a poor job of parsing the request. It should send application/x-www-form-urlencoded like it does in the gaia basket client.

@pmac
Copy link
Member Author

pmac commented Jul 12, 2016

@alicoding any chance you can look at this? Looks like you were last to touch the code. Basket is handling it okay, but it would be much better if these submissions could follow the standard code path.

@alicoding
Copy link
Collaborator

Will take a look at this ASAP.

alicoding added a commit to alicoding/advocacy.mozilla.org that referenced this issue Jul 12, 2016
ScottDowne added a commit that referenced this issue Jul 12, 2016
Fix issue #210 - Set header form submission
@ScottDowne
Copy link
Contributor

I had to revert this. Our submissions started to fail.

Reopening. I'll add some more info about how it was failing in a minute.

@ScottDowne ScottDowne reopened this Jul 27, 2016
@ScottDowne
Copy link
Contributor

During form submission, it would fail with a "InvalidStateError: An attempt was made to use an object that is not, or is no longer, usable" in the console, which at that point, I tracked down the patch that caused it, then reverted it.

Need to fix it again, but figure out why the form doesn't submit.

@pmac
Copy link
Member Author

pmac commented Jul 27, 2016

Very strange. I'm not sure why adding a content type would cause any problems. All I can really do is link to a submission form that is working well and hopefully we can figure it out from there. The subscription JS on the viewsourceconf.org site seems to be working well. It submits to an endpoint on www.mozilla.org though, which in turn calls basket, so maybe that is more reliable or better for AJAX submission?

@ScottDowne
Copy link
Contributor

I am a bit curious what the repercussions are for reverting this. In other words, what's the current issue with text/plain? and what kind of urgency is on this again now that it's backed out?

@pmac
Copy link
Member Author

pmac commented Jul 28, 2016

Basket relies on Django's request parsing to get the data you send. When the content type is wrong that parsing doesn't happen. Normally we'd have rejected all of these, but we had to hack around it to make FxOS work as it shipped initially with the same problem. We'd very much like to remove that hack as it's no longer needed for FxOS and is not nearly as robust as requests that are handled the correct way. As they're being sent the requests are actually invalid. We only did the hack because the buggy FxOS was on devices and couldn't be fixed in a timely manner.

@pmac
Copy link
Member Author

pmac commented Jul 28, 2016

One thing I noticed looking at the code here and that in viewsourceconf is that the headers were set on the xhr object after the call to open(). Perhaps that's significant?

Also maybe @stephaniehobson can help as I think she ran into some similar issues submitting directly to basket. This is part of the reason we switched to the submission to bedrock.

@pmac
Copy link
Member Author

pmac commented Jul 28, 2016

For example, I've gotten 3 new request parse errors recently in basket with the referrer https://advocacy.mozilla.org/en-US/encrypt/codemoji/2.

The raw request params were:

'newsletters=mozilla-foundation&source_url=https%3A%2F%2Fadvocacy.mozilla.org%2Fencrypt&lang=en&email=a@b."><img src=x onerror=prompt(1);>com&trigger_welcome=N&country=A"><img src=x onerror=prompt(1);>F&first_name=dasdsd'

Clearly someone doing pen-testing. I believe this would have been handled appropriately (mostly ignored I think) had it been sent with the appropriate content-type header.

@ScottDowne
Copy link
Contributor

ScottDowne commented Jul 28, 2016

I tried a few random submissions to try to test a few forms to see if I can get it working with the fixed header. What you're probably seeing is me just ensuring it still works without the header.

Still not able to get it working yet, but I keep getting pulled into other tasks, so it's slow going.

@ScottDowne
Copy link
Contributor

This seems to do the trick: #223

I can confirm it works without that added line, with, but as soon as I move it above open, it fails, making your suspicion true. :)

@pmac
Copy link
Member Author

pmac commented Jul 28, 2016

I can confirm it works without that added line, with, but as soon as I move it above open, it fails, making your suspicion true. :)

Oh interesting. I was sure that wouldn't turn out to be the deal... Nice work!

@ScottDowne
Copy link
Contributor

I'll merge, get it on production, and ping you here, just in case you want to try a submission and see it go through correctly.

I can only see that the UI pops up with a submission complete page, and only assume it's working on the other end :)

@ScottDowne
Copy link
Contributor

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants