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
Status: RESOLVED FIXED
:
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)
:
: Shane Caraveo (:mixedpuppy)
Mentors:
Depends on: 776606
Blocks:
  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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Patch (7.69 KB, patch)
2012-07-13 15:50 PDT, Jared Wein [:jaws] (please needinfo? me)
gavin.sharp: review-
Details | Diff | Splinter Review
Patch v2 (7.21 KB, patch)
2012-07-16 15:25 PDT, Jared Wein [:jaws] (please needinfo? me)
gavin.sharp: review+
Details | Diff | Splinter 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 | Splinter Review

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

I placed the visitProfile function in SocialUI and not SocialShareButton since the toolbar button will be using the same function.
Comment 2 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-16 14:10:36 PDT
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 User image 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 User image :Gavin Sharp [email: gavin@gavinsharp.com] 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).

>+      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 User image 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 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 User image Jared Wein [:jaws] (please needinfo? me) 2012-07-17 10:50:03 PDT
Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed67ac9fcef4
Comment 7 User image 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 User image Ed Morley [:emorley] 2012-07-18 05:55:22 PDT
https://hg.mozilla.org/mozilla-central/rev/ed67ac9fcef4
Comment 9 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-20 10:40:41 PDT
(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 User image Lukas Blakk [:lsblakk] use ?needinfo 2012-07-24 14:41:58 PDT
Adding in Axel, see: potential strings added to Aurora.
Comment 11 User image 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 User image 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 User image 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.