Closed
Bug 773743
Opened 12 years ago
Closed 12 years ago
Add the portrait and user display name to the share button popup
Categories
(Firefox Graveyard :: SocialAPI, defect)
Firefox Graveyard
SocialAPI
Tracking
(firefox17 fixed)
RESOLVED
FIXED
Firefox 17
Tracking | Status | |
---|---|---|
firefox17 | --- | fixed |
People
(Reporter: jaws, Assigned: jaws)
References
Details
Attachments
(1 file, 2 obsolete files)
7.23 KB,
patch
|
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
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 2•12 years ago
|
||
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-
Assignee | ||
Comment 3•12 years ago
|
||
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 4•12 years ago
|
||
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+
Assignee | ||
Comment 5•12 years ago
|
||
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
Assignee | ||
Comment 6•12 years ago
|
||
Pushed to mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/ed67ac9fcef4
Flags: in-testsuite-
Target Milestone: --- → Firefox 17
Assignee | ||
Comment 7•12 years ago
|
||
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?
Comment 8•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ed67ac9fcef4
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 9•12 years ago
|
||
(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.
Updated•12 years ago
|
Whiteboard: [Fx16] → [Fx16][strings:2]
Comment 10•12 years ago
|
||
Adding in Axel, see: potential strings added to Aurora.
Comment 11•12 years ago
|
||
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+
Comment 12•12 years ago
|
||
Is this still needed in FF16? This hasn't landed there yet, as far as I can tell.
tracking-firefox16:
--- → ?
Assignee | ||
Comment 13•12 years ago
|
||
Not needed for Fx16, thanks for checking.
tracking-firefox16:
? → ---
Assignee | ||
Updated•12 years ago
|
Whiteboard: [Fx16][strings:2]
Updated•12 years ago
|
status-firefox17:
--- → fixed
Updated•5 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•