Last Comment Bug 773743 - Add the portrait and user display name to the share button popup
: Add the portrait and user display name to the share button popup
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Jared Wein [:jaws] (please needinfo? me)
Depends on: 776606
  Show dependency treegraph
Reported: 2012-07-13 11:38 PDT by Jared Wein [:jaws] (please needinfo? me)
Modified: 2012-11-03 09:50 PDT (History)
5 users (show)
jaws: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (7.69 KB, patch)
2012-07-13 15:50 PDT, Jared Wein [:jaws] (please needinfo? me) review-
Details | Diff | Review
Patch v2 (7.21 KB, patch)
2012-07-16 15:25 PDT, Jared Wein [:jaws] (please needinfo? me) review+
Details | Diff | Review
Patch for checkin (7.23 KB, patch)
2012-07-17 10:46 PDT, Jared Wein [:jaws] (please needinfo? me)
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description Jared Wein [:jaws] (please needinfo? me) 2012-07-13 11:38:20 PDT
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) 2012-07-13 15:50:02 PDT
Created attachment 642110 [details] [diff] [review]

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:] 2012-07-16 14:10:36 PDT
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!
Comment 3 Jared Wein [:jaws] (please needinfo? me) 2012-07-16 15:25:23 PDT
Created attachment 642756 [details] [diff] [review]
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:] 2012-07-16 16:40:02 PDT
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.
Comment 5 Jared Wein [:jaws] (please needinfo? me) 2012-07-17 10:46:05 PDT
Created attachment 643031 [details] [diff] [review]
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 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.
Comment 6 Jared Wein [:jaws] (please needinfo? me) 2012-07-17 10:50:03 PDT
Pushed to mozilla-inbound:
Comment 7 Jared Wein [:jaws] (please needinfo? me) 2012-07-17 10:52:11 PDT
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] 2012-07-18 05:55:22 PDT
Comment 9 :Gavin Sharp [email:] 2012-07-20 10:40:41 PDT
(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.
Comment 10 Lukas Blakk [:lsblakk] use ?needinfo 2012-07-24 14:41:58 PDT
Adding in Axel, see: potential strings added to Aurora.
Comment 11 Alex Keybl [:akeybl] 2012-07-30 16:52:06 PDT
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] 2012-08-28 08:16:48 PDT
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) 2012-08-28 12:43:40 PDT
Not needed for Fx16, thanks for checking.

Note You need to log in before you can comment on or make changes to this bug.