Last Comment Bug 782882 - Restrict pages which can be shared to http and https protocols.
: Restrict pages which can be shared to http and https protocols.
Status: VERIFIED FIXED
[fx17]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 19
Assigned To: Mark Hammond [:markh]
:
Mentors:
: 805257 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-14 23:42 PDT by Mark Hammond [:markh]
Modified: 2012-12-04 13:53 PST (History)
6 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified


Attachments
hide share button for non http/https/ftp/ftps urls (7.75 KB, patch)
2012-10-24 20:46 PDT, Mark Hammond [:markh]
jaws: review+
gavin.sharp: approval‑mozilla‑aurora+
gavin.sharp: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Mark Hammond [:markh] 2012-08-14 23:42:58 PDT
Currently any URL can be shared, including "file://", "chrome://" and "data:" URLs.  We possibly want to restrict what can be shared to, eg, http://, https:// ftp:// and ftps:// URLs.
Comment 1 Matthew N. [:MattN] 2012-10-12 19:02:11 PDT
Sharing of about: URIs (e.g. about:home) should also be prevented.
Comment 2 Mark Hammond [:markh] 2012-10-24 16:24:08 PDT
*** Bug 805257 has been marked as a duplicate of this bug. ***
Comment 3 Mark Hammond [:markh] 2012-10-24 20:46:32 PDT
Created attachment 674968 [details] [diff] [review]
hide share button for non http/https/ftp/ftps urls

This patch keeps the share button hidden if the current URI scheme isn't one of http, https, ftp, ftps (which is the list we used for F1, and seems reasonable to me - any others needed?)
Comment 4 Jared Wein [:jaws] (please needinfo? me) 2012-10-24 22:02:23 PDT
Comment on attachment 674968 [details] [diff] [review]
hide share button for non http/https/ftp/ftps urls

Review of attachment 674968 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-social.js
@@ +508,5 @@
>    dismissUnsharePopup: function SSB_dismissUnsharePopup() {
>      this.unsharePopup.hidePopup();
>    },
>  
> +  canShareCurrentUrl: function SSB_canShareCurrentUrl(aURI) {

s/canShareCurrentUrl/canSharePage/

@@ +510,5 @@
>    },
>  
> +  canShareCurrentUrl: function SSB_canShareCurrentUrl(aURI) {
> +    let tabBrowser = gBrowser.getBrowserForTab(gBrowser.selectedTab);
> +    let uri = tabBrowser.currentURI;

These two lines aren't needed, since you can just use the |aURI| argument of this function :)

@@ +513,5 @@
> +    let tabBrowser = gBrowser.getBrowserForTab(gBrowser.selectedTab);
> +    let uri = tabBrowser.currentURI;
> +    // We only allow sharing of http, https, ftp, ftps.
> +    return (uri && (uri.schemeIs('http') || uri.schemeIs('https') ||
> +                    uri.schemeIs('ftp') || uri.schemeIs('ftps')));

Does FB actually allow you to share ftp/ftps links? I think we should just limit this to http/https. If we're not sure, I think it is better to err on the side of less abilities, since we can add those in the future, but it would be harder to remove ones in the future.

@@ +521,5 @@
>      let shareButton = this.shareButton;
>      if (shareButton)
>        shareButton.hidden = !Social.uiVisible || this.promptImages == null ||
> +                           !SocialUI.haveLoggedInUser() ||
> +                           !this.canShareCurrentUrl();

!this.canSharePage(gBrowser.currentURI);
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-10-25 05:18:57 PDT
https://hg.mozilla.org/mozilla-central/rev/081340dcc074
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-25 12:48:42 PDT
Comment on attachment 674968 [details] [diff] [review]
hide share button for non http/https/ftp/ftps urls

[Triage Comment]
let's get this social-only fix in for 17
Comment 9 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-12-04 13:53:54 PST
Marking verified fixed since this landed with tests. Please add verifyme keyword if there's something you need QA to test.

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