@Jhernandez, can you link to the repo for this?

Just a few comments:

• Design: just want to verify that you guys are ok with putting internal surveys into eventlogging, and anyone can put garbage into your schema (DoS). Is that OK for what you're doing?

• ./resources/ext.quicksurveys.views/ExternalSurvey.js
  • Line 20 - you're putting a message dirrectly into the href of a link. Please whitelist that it starts with /https?/ so a malicious admin/translator can't inject javascript: or data: urls.
  • Line 29 - you're putting a raw message into the html without escaping, which I try to avoid in mediawiki (there's a task open to eliminate this everywhere... I can't find it at the moment). Can you use wikitext and mw.message.parse() like you do for internal surveys?

Also, not security directly, but it looks like you're assuming that MobileFrontend is loaded to add hogan templates when there's a "mobile" parameter passed. Should you be checking for that dependency specifically?

• /resources/ext.quicksurveys.views/utils.js - really should validate that GeoIP is a sane value. I believe it should always be 2 or 3 characters long, so probably should truncate that before logging it.

Change 239993 had a related patch set uploaded (by Robmoen):
Security improvements

https://gerrit.wikimedia.org/r/239993

Some thoughts for your consideration on the patchset...

@Jdlrobson, comment made. Also, who's the PM for this? I'd like someone to answer,

Design: just want to verify that you guys are ok with putting internal surveys into eventlogging, and anyone can put garbage into your schema (DoS). Is that OK for what you're doing?

Ping @atgo to confirm above ^

That's me! I guess I don't entirely understand the question as I'm not super familiar with eventlogging. @csteipp are you saying that the data could be easily corrupted?

@atgo, it's trivial for anyone to send data into eventlogging for any schema, so there's no guarantee that the survey results you get are from real people, or that one person didn't answer 10M times with their favorite answer, etc.

So as long as you're seeing the results as probably correct, and not using it as the basis of major decision making, you should be fine. If the results are going to be used as, "people voted, and are saying X", then eventlogging is not the right place to put this.

I have to say that does worry me a bit. I guess we can keep it that way for now, but at some point we may need to change how we're logging responses. @leila did you have any thoughts about this when we were reviewing it the other day? See above comments from csteipp

That was a known trade-off with using EventLogging.

To be clear this is an edge case where we have one malicious user who knows what they are doing. Potentially they could create data that looks real but isn't.

Technically they could do the same with every other event (e.g. make it look like edits increased dramatically) we log so this is not a new problem and hasn't been a problem yet.

@atgo there are couple of things to consider:

• A user submitting many answers does not cause a major problem as long as we register ip address and user agent in the EL table (see my email about the list of our requests). We will take that information and match the response with the logs and we will sanity check the data and clean up as needed.
• We need to clearly define how we want to run the first survey. Per our earlier discussion, we want to show the survey question to n% of the traffic once (n to be defined). So, if a user sees the survey, they should not be able to resubmit responses by reloading the page or otherwise. (If this is technically a challenge, we need to know it).

Unsolicited comment from me: I'm inclined to agree with @Jdlrobson and @leila. A particularly malicious user could abuse this, but they'd need to be pretty technically adept and know exactly what to do and when. It's possible, but it strikes me as incredibly unlikely, and as Leila notes there are avenues to detect this kind of deception.

Yes, -ish. There's no anti-csrf token, so a malicious attacker could use
another site to make it appear that many users were submitting.

But again, I'm just making sure this is a conscious decision on the product
owner's part. If you guys are happy with it, it's cool with me. I just
don't want someone asking my team to implement auth / csrf prevention in EL
because we accidentally started using it for high value polls!

@csteipp these are fair points and thanks for bringing them up early in the process. :-)

@atgo I don't know about all the questions you would like to answer with such surveys so I can't speak about whether what @csteipp mentions here can have impact on the decisions the Readership team makes based on those surveys. For the purpose of the survey Bob, Ellery, and I talked to you about, we are aware of this specific potential security issue and would like to continue nevertheless (we have to start from somewhere without putting too much cost on other team's including csteipp's, and we will have ways to exclude outliers based on the type of questions we would like to answer.). I'm happy to chat with you if you want to run other surveys to figure out what we can do about those.

@leila @csteipp I think we're good (and I'm the PO on this). Thanks!

Change 239993 merged by jenkins-bot:
Security improvements

https://gerrit.wikimedia.org/r/239993

I'm pretty happy with this.
In terms of "Also, not security directly, but it looks like you're assuming that MobileFrontend is loaded to add hogan templates when there's a "mobile" parameter passed. Should you be checking for that dependency specifically?", this should be harmless. It will just complain of a missing module in the response but not throw any exceptions.

@csteipp is this resolved to your satisfaction now?