782882 - Restrict pages which can be shared to http and https protocols.

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

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]]

Sharing of about: URIs (e.g. about:home) should also be prevented.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attachment: 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)]

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 5

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/081340dcc074

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/081340dcc074

Comment 7

[:Gavin Sharp [email: [e-mail]]]

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 8

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ffee7552efec
https://hg.mozilla.org/releases/mozilla-beta/rev/3999273bf1ca

Comment 9

[u279076]

Marking verified fixed since this landed with tests. Please add verifyme keyword if there's something you need QA to test.