Page MenuHomePhabricator

Security review of banner with FB and Twitter share buttons
Closed, ResolvedPublic


This CN banner includes FB and Twitter share links:
UPDATE: Ooop, previously I put the wrong banner link here. This is the correct one.

At first glance, it seems acceptable, since it doesn't seem to download any images from external sites. Interaction with FB and Twitter only happens when the user clicks.

It also seems that in the referrer header, those sites will only get the domain the banner was seen on ( and not the name of the article the user was browsing, due to referrer policy. However, I'm also not sure if this works everywhere. See T180921.

Any additional feedback would be much appreciated. Many thanks in advance!!!

Event Timeline

AndyRussG created this task.Dec 4 2019, 6:44 AM
AndyRussG created this object with visibility "Custom Policy".
AndyRussG changed the visibility from "Custom Policy" to "Custom Policy".Dec 4 2019, 3:11 PM
AndyRussG updated the task description. (Show Details)Dec 4 2019, 3:44 PM
sbassett added a subscriber: sbassett.EditedDec 4 2019, 6:26 PM

@AndyRussG - I get an error that I'm not auth'd to preview banners when I try to view Not sure if that's critical to this privacy review or not...

sbassett triaged this task as High priority.Dec 4 2019, 6:26 PM
sbassett added a project: Privacy.
sbassett added subscribers: Reedy, JFishback_WMF.
Bawolff added a subscriber: Bawolff.EditedDec 4 2019, 11:05 PM

I'm not the person doing the review, but I did notice there is an XSS in this banner:

Doing regexes on html and then re-inserting that html into the document is very very tricky to get right. I'd suggest instead using .text() on each text node and inserting with .text(), so the regexes get only done over text. If that's a complicated fix, adding an attribute like data-mw-banner to the outer div, and ensuring all selectors only select stuff that is decendents of that div (Since in MW users are not allowed to use attributes starting with data-mw) might be a short term fix to reduce the risk of users being able to set malicious dom elements. But switching to using .text() for the regexes is a much more reliable fix.

sbassett added a comment.EditedDec 5 2019, 3:03 PM

Ugh, thanks for finding that, @Bawolff. I'm going to break that out into a separate high/ubn ticket.

Filed: T239919

@Bawolff thank you so much for finding this!!!! This same code is in live FR banners across enwiki. I've created a separate task: T239922

Thanks again!!!

Ugh, thanks for finding that, @Bawolff. I'm going to break that out into a separate high/ubn ticket.
Filed: T239919


@AndyRussG - I get an error that I'm not auth'd to preview banners when I try to view Not sure if that's critical to this privacy review or not...

That's probably from clicking something in the "Live preview" section? You should be able to preview the saved version of the banner by clicking the "Preview last saved version" at the top?

Jcross lowered the priority of this task from High to Medium.Dec 9 2019, 4:21 PM
sbassett added a comment.EditedDec 16 2019, 9:46 PM

Ok, now that the dust has settled a bit from T239778#5714105 :) , I've had a look at this within the context of the original request:

Regarding the Facebook and Twitter share buttons:

  1. The only code I'm seeing related to the share buttons in the banner referenced within the task description are the two calls to and URLs, as the corresponding SVGs seems fine. Please let me know if there's anything else that I'm missing. I believe these calls should be fine since IIRC, once a user interacts with a banner (including clicking upon or activating a linked URL), they are then covered by the donor privacy policy, specifically the Other service providers sub-section of the Sharing Donor Information section. Though I'm not sure this is even completely relevant since such actions are basically the same as clicking on any external link, such as any article footnote, etc. @JFishback_WMF may have some further clarifying thoughts on this. I'm guessing this has also been at least brought to WMF-Legal's attention?
  2. Regarding the transmission of an HTTP referer header, that isn't ever explicitly mentioned within the current Wikimedia privacy policy, which is more restrictive than the donor privacy policy. While this is obviously something that many WMF and community members care about (as indicated within T180921), I'm not certain any further hardening around referer transmission would be a hard requirement in this case. Again, @JFishback_WMF may have some clarifying thoughts here.

Additional issues:

  1. Given T239922 and its quick resolution (thanks again, everyone) I had a quick look at the JS for the banner again and didn't see anything particularly egregious, though I certainly did not perform any kind of full code review. That might be a part of the additional audit I mentioned at T239922#5719396. I was curious if the JS used for banners like this is purely stored within the MediaWiki namespace without code review or CI checks within gerrit, etc. I know it's probably very convenient to work with such resources in this way, but the importance of CN banners might warrant a different approach more akin with how we treat MW core/extensions and other production code.
sbassett moved this task from Intake to Doing on the Privacy board.Dec 16 2019, 9:46 PM
sbassett moved this task from Incoming to Waiting on the deprecated-security-team-reviews board.
sbassett claimed this task.Dec 16 2019, 9:54 PM

I agree with @sbassett 's analysis here and don't really have anything substantive to add.

  1. I think this is pretty unobjectionable since the user must initiate any action (i.e. we don't automatically load anything from Twitter or FB).
  1. I'm not aware of any written policy specifically addressing this either way. I don't think the current functionality violates the PP. But...

I also think WMF-Legal should sign off on this if they have not already.

sbassett closed this task as Resolved.Jan 6 2020, 5:48 PM
sbassett moved this task from Waiting to Done on the deprecated-security-team-reviews board.

I'm going to call this done from the Security-Team's point-of-view, resolve it and make it public. @AndyRussG - if you had any questions or concerns about mine or @JFishback_WMF's comments above, please feel free to respond here.

sbassett moved this task from Doing to Done on the Privacy board.Jan 6 2020, 5:48 PM
sbassett moved this task from Incoming to Watching on the Privacy Engineering board.
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".

Hey, this is Sam w/ the Online Fundraising banner team - I appreciate everything discussed in this thread, as well as the important offshoot of T239922. Thank you @AndyRussG for starting this! And double thanks to @sbassett and @JFishback_WMF for your review.

I'll make sure this is surfaced to WMF-Legal.

chasemp moved this task from Incoming to Our Part Is Done on the secscrum board.Tue, Mar 10, 8:19 PM