773743 - Add the portrait and user display name to the share button popup

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Bug 765874 implemented the recommend/share button and added a reduced implementation of the details popup.

This bug is to track adding in the users portrait and display name to the popup.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

I placed the visitProfile function in SocialUI and not SocialShareButton since the toolbar button will be using the same function.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 642110 [details] [diff] [review]
Patch

>diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js

>   _providerReady: function SocialUI_providerReady() {

>+    let portrait = document.getElementById("socialUserPortrait");
>+    portrait.style.listStyleImage = Social.provider.profile.portrait;
>+    let displayName = document.getElementById("socialUserDisplayName");
>+    displayName.setAttribute("label", displayName);

You need to handle provider.profile being null. And setting the displayName to "[object XULElement]" isn't going to work well :)

Otherwise looks good!

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

facepalm on the XUL element assignment there. thanks for the review.

in this version i also hide the row if the profile isn't set so we don't have to worry about determining defaults for the user.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 642756 [details] [diff] [review]
Patch v2

>diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js

> let SocialShareButton = {
>   init: function SSB_init() {

>+    if (Social.provider && Social.provider.profile) {

Social.provider is garanteed to be non-null here, no need to check it. But you do need to check .displayName, since it can also be null (in the case where users log out).

>+      portrait.style.listStyleImage = Social.provider.profile.portrait;

We should file a bug to sanitize the input given by providers. Wouldn't want to set this to a javascript: URI.

This UI is kind of redundant with the toolbar button dropdown, but I guess that's OK? We might want to revisit when we have more UI pieces landed and have a better look at the overall picture.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for checkin

I updated the patch to check for a valid portrait and displayName in order to display the profile row.

As for the security implications of setting element.style.listStyleImage to a user-supplied value, I'm pretty confident we don't have anything to worry about here. I asked in #security, and this is what we came up with:

albino found this page http://ha.ckers.org/blog/20060815/list-style-image-xss/ that states that this type of attack only works in IE.

element.style.listStyleImage = "javascript:alert(1)";
// invalid syntax
element.style.listStyleImage = "url('javascript:alert(1)')";
// alert is not defined

The first case simply won't work, and the second case appears to be executing the javascript but it looks like it doesn't have access to |window|.

decoder thinks that we could try to validate the value. We could check to see if the value begins with "url(data:", "http:", or "https:". However, if there is no attack vector there, then the extra work may not be worth it.

albino offered that we could prepend "url(data:image/png" but I think that requiring providers to send us mangled data URIs wouldn't be a good direction to take.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed67ac9fcef4

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 643031 [details] [diff] [review]
Patch for checkin

[Approval Request Comment]
Bug caused by (feature/regressing bug #): didn't catch the 16 train

User impact if declined: the social api feature will have a partial share popup

Testing completed (on m-c, etc.): locally, just pushed to mozilla-inbound

Risk to taking this patch (and alternatives if risky): this is a very low risk patch, as the social api features are preffed off by default.

String or UUID changes made by this patch: yes, two strings are added in this patch

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ed67ac9fcef4

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to Jared Wein [:jaws] from comment #5)
> element.style.listStyleImage = "url('javascript:alert(1)')";
> // alert is not defined
> 
> The first case simply won't work, and the second case appears to be
> executing the javascript but it looks like it doesn't have access to
> |window|.

Executing the JS in a window-less context is still a security risk, because it will run with chrome privileges. We need to validate the value. Let's fix this before uplifting to Aurora.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Adding in Axel, see: potential strings added to Aurora.

Comment 11

[Alex Keybl [:akeybl]]

Comment on attachment 643031 [details] [diff] [review]
Patch for checkin

[Triage Comment]
We've communicated the necessity for this late string change to the l10n community, and we're ready to move forward with the uplift. Approved for Aurora 16.

Comment 12

[Alex Keybl [:akeybl]]

Is this still needed in FF16? This hasn't landed there yet, as far as I can tell.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Not needed for Fx16, thanks for checking.