The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

Trunk
Firefox 17
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox17 fixed)

Details

Attachments

(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.
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.
Attachment #642110 - Flags: review?(gavin.sharp)
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!
Attachment #642110 - Flags: review?(gavin.sharp) → review-
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.
Attachment #642110 - Attachment is obsolete: true
Attachment #642756 - Flags: review?(gavin.sharp)
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.
Attachment #642756 - Flags: review?(gavin.sharp) → review+
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.
Attachment #642756 - Attachment is obsolete: true
Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed67ac9fcef4
Flags: in-testsuite-
Target Milestone: --- → Firefox 17
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?
https://hg.mozilla.org/mozilla-central/rev/ed67ac9fcef4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(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.
Whiteboard: [Fx16] → [Fx16][strings:2]
Depends on: 776606
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: ? → ---
Whiteboard: [Fx16][strings:2]
status-firefox17: --- → fixed
You need to log in before you can comment on or make changes to this bug.