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

RESOLVED FIXED in Firefox 17


7 years ago
29 days ago


(Reporter: jaws, Assigned: jaws)


Firefox 17
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox17 fixed)



(1 attachment, 2 obsolete attachments)

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.
Posted patch Patch (obsolete) — Splinter Review
I placed the visitProfile function in SocialUI and not SocialShareButton since the toolbar button will be using the same function.
Attachment #642110 - Flags: review?(
Comment on attachment 642110 [details] [diff] [review]

>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");
>+ = 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!
Attachment #642110 - Flags: review?( → review-
Posted patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #642110 - Attachment is obsolete: true
Attachment #642756 - Flags: review?(
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).

>+ = 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.
Attachment #642756 - Flags: review?( → review+
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 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 that states that this type of attack only works in IE. = "javascript:alert(1)";
// invalid syntax = "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.
Attachment #642756 - Attachment is obsolete: true
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
Attachment #643031 - Flags: approval-mozilla-aurora?
Last Resolved: 7 years ago
Resolution: --- → FIXED
(In reply to Jared Wein [:jaws] from comment #5)
> = "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.
Whiteboard: [Fx16] → [Fx16][strings:2]
Adding in Axel, see: potential strings added to Aurora.
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.
Attachment #643031 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Is this still needed in FF16? This hasn't landed there yet, as far as I can tell.
tracking-firefox16: --- → ?
Not needed for Fx16, thanks for checking.
tracking-firefox16: ? → ---
status-firefox17: --- → fixed


29 days ago
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.