Bug 1524669 - Add protocol checking to link urls for pocket new tab. r=Mardak a=lizzard
authork88hudson <k88hudson@gmail.com>
Sat, 16 Feb 2019 00:49:32 +0200
changeset 515989 d5494d3885c75f0bdde1d08932439961f5300e0e
parent 515988 5d44e50ff0c250f05778c896e2f00387f51b0fe6
child 515990 7f0cb549e3d3f3f532c93898c8177c3d7921cd45
push id1953
push userffxbld-merge
push dateMon, 11 Mar 2019 12:10:20 +0000
treeherdermozilla-release@9c35dcbaa899 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMardak, lizzard
bugs1524669
milestone66.0
Bug 1524669 - Add protocol checking to link urls for pocket new tab. r=Mardak a=lizzard Reviewers: Mardak Reviewed By: Mardak Bug #: 1524669 Differential Revision: https://phabricator.services.mozilla.com/D19971
browser/components/newtab/content-src/components/DiscoveryStreamComponents/DSCard/DSCard.jsx
browser/components/newtab/content-src/components/DiscoveryStreamComponents/Hero/Hero.jsx
browser/components/newtab/content-src/components/DiscoveryStreamComponents/List/List.jsx
browser/components/newtab/content-src/components/DiscoveryStreamComponents/SafeAnchor/SafeAnchor.jsx
browser/components/newtab/data/content/activity-stream.bundle.js
browser/components/newtab/test/unit/content-src/components/DiscoveryStreamComponents/Hero.test.jsx
browser/components/newtab/test/unit/content-src/components/DiscoveryStreamComponents/List.test.jsx
browser/components/newtab/test/unit/content-src/components/DiscoveryStreamComponents/SafeAnchor.test.jsx
--- a/browser/components/newtab/content-src/components/DiscoveryStreamComponents/DSCard/DSCard.jsx
+++ b/browser/components/newtab/content-src/components/DiscoveryStreamComponents/DSCard/DSCard.jsx
@@ -1,10 +1,11 @@
 import {actionCreators as ac} from "common/Actions.jsm";
 import React from "react";
+import {SafeAnchor} from "../SafeAnchor/SafeAnchor";
 import {SpocIntersectionObserver} from "content-src/components/DiscoveryStreamComponents/SpocIntersectionObserver/SpocIntersectionObserver";
 
 export class DSCard extends React.PureComponent {
   constructor(props) {
     super(props);
 
     this.onLinkClick = this.onLinkClick.bind(this);
   }
@@ -23,17 +24,17 @@ export class DSCard extends React.PureCo
         tiles: [{id: this.props.id, pos: this.props.index}],
       }));
     }
   }
 
   render() {
     return (
       <SpocIntersectionObserver campaignId={this.props.campaignId} dispatch={this.props.dispatch}>
-        <a href={this.props.url} className="ds-card" onClick={this.onLinkClick}>
+        <SafeAnchor url={this.props.url} className="ds-card" onLinkClick={this.onLinkClick}>
           <div className="img-wrapper">
             <div className="img" style={{backgroundImage: `url(${this.props.image_src}`}} />
           </div>
           <div className="meta">
             <div className="info-wrap">
               <header className="title">{this.props.title}</header>
               {this.props.excerpt && <p className="excerpt">{this.props.excerpt}</p>}
             </div>
@@ -42,13 +43,13 @@ export class DSCard extends React.PureCo
                 <span>
                   <span className="context">{this.props.context}</span>
                   <br />
                 </span>
               )}
               <span className="source">{this.props.source}</span>
             </p>
           </div>
-        </a>
+        </SafeAnchor>
       </SpocIntersectionObserver>
     );
   }
 }
--- a/browser/components/newtab/content-src/components/DiscoveryStreamComponents/Hero/Hero.jsx
+++ b/browser/components/newtab/content-src/components/DiscoveryStreamComponents/Hero/Hero.jsx
@@ -1,12 +1,13 @@
 import {actionCreators as ac} from "common/Actions.jsm";
 import {DSCard} from "../DSCard/DSCard.jsx";
 import {List} from "../List/List.jsx";
 import React from "react";
+import {SafeAnchor} from "../SafeAnchor/SafeAnchor";
 
 export class Hero extends React.PureComponent {
   constructor(props) {
     super(props);
     this.onLinkClick = this.onLinkClick.bind(this);
   }
 
   onLinkClick(event) {
@@ -63,30 +64,30 @@ export class Hero extends React.PureComp
         items={this.props.items - 1}
         type={`Hero`} />
     );
 
     return (
       <div>
         <div className="ds-header">{this.props.title}</div>
         <div className={`ds-hero ds-hero-${this.props.border}`}>
-          <a href={heroRec.url} className="wrapper" onClick={this.onLinkClick}>
+          <SafeAnchor url={heroRec.url} className="wrapper" onLinkClick={this.onLinkClick}>
             <div className="img-wrapper">
               <div className="img" style={{backgroundImage: `url(${heroRec.image_src})`}} />
             </div>
             <div className="meta">
               <header>{heroRec.title}</header>
               <p className="excerpt">{heroRec.excerpt}</p>
               {heroRec.context ? (
                 <p className="context">{heroRec.context}</p>
               ) : (
                 <p className="source">{heroRec.domain}</p>
               )}
             </div>
-          </a>
+          </SafeAnchor>
           <div className={`${this.props.subComponentType}`}>
             { this.props.subComponentType === `cards` ? cards : list }
           </div>
         </div>
       </div>
     );
   }
 }
--- a/browser/components/newtab/content-src/components/DiscoveryStreamComponents/List/List.jsx
+++ b/browser/components/newtab/content-src/components/DiscoveryStreamComponents/List/List.jsx
@@ -1,11 +1,12 @@
 import {actionCreators as ac} from "common/Actions.jsm";
 import {connect} from "react-redux";
 import React from "react";
+import {SafeAnchor} from "../SafeAnchor/SafeAnchor";
 import {SpocIntersectionObserver} from "content-src/components/DiscoveryStreamComponents/SpocIntersectionObserver/SpocIntersectionObserver";
 
 /**
  * @note exported for testing only
  */
 export class ListItem extends React.PureComponent {
   // TODO performance: get feeds to send appropriately sized images rather
   // than waiting longer and scaling down on client?
@@ -29,32 +30,32 @@ export class ListItem extends React.Pure
       }));
     }
   }
 
   render() {
     return (
       <li className="ds-list-item">
         <SpocIntersectionObserver campaignId={this.props.campaignId} dispatch={this.props.dispatch}>
-          <a className="ds-list-item-link" href={this.props.url} onClick={this.onLinkClick}>
+          <SafeAnchor url={this.props.url} className="ds-list-item-link" onLinkClick={this.onLinkClick}>
             <div className="ds-list-item-text">
               <div className="ds-list-item-title">{this.props.title}</div>
               {this.props.excerpt && <div className="ds-list-item-excerpt">{this.props.excerpt}</div>}
               <p>
                 {this.props.context && (
                   <span>
                     <span className="ds-list-item-context">{this.props.context}</span>
                     <br />
                   </span>
                 )}
                 <span className="ds-list-item-info">{this.props.domain}</span>
               </p>
             </div>
             <div className="ds-list-image" style={{backgroundImage: `url(${this.props.image_src})`}} />
-          </a>
+          </SafeAnchor>
         </SpocIntersectionObserver>
       </li>
     );
   }
 }
 
 /**
  * @note exported for testing only
new file mode 100644
--- /dev/null
+++ b/browser/components/newtab/content-src/components/DiscoveryStreamComponents/SafeAnchor/SafeAnchor.jsx
@@ -0,0 +1,29 @@
+import React from "react";
+
+export class SafeAnchor extends React.PureComponent {
+  safeURI(url) {
+    let protocol = null;
+    try {
+      protocol = new URL(url).protocol;
+    } catch (e) { return ""; }
+
+    const isAllowed = [
+      "http:",
+      "https:",
+    ].includes(protocol);
+    if (!isAllowed) {
+      console.warn(`${protocol} is not allowed for anchor targets.`); // eslint-disable-line no-console
+      return "";
+    }
+    return url;
+  }
+
+  render() {
+    const {url, className, onLinkClick} = this.props;
+    return (
+      <a href={this.safeURI(url)} className={className} onClick={onLinkClick}>
+        {this.props.children}
+      </a>
+    );
+  }
+}
--- a/browser/components/newtab/data/content/activity-stream.bundle.js
+++ b/browser/components/newtab/data/content/activity-stream.bundle.js
@@ -7109,24 +7109,54 @@ function enableASRouterContent(store, as
 
 // EXTERNAL MODULE: ./common/Actions.jsm
 var Actions = __webpack_require__(2);
 
 // EXTERNAL MODULE: external "React"
 var external_React_ = __webpack_require__(10);
 var external_React_default = /*#__PURE__*/__webpack_require__.n(external_React_);
 
+// CONCATENATED MODULE: ./content-src/components/DiscoveryStreamComponents/SafeAnchor/SafeAnchor.jsx
+
+
+class SafeAnchor_SafeAnchor extends external_React_default.a.PureComponent {
+  safeURI(url) {
+    let protocol = null;
+    try {
+      protocol = new URL(url).protocol;
+    } catch (e) {
+      return "";
+    }
+
+    const isAllowed = ["http:", "https:"].includes(protocol);
+    if (!isAllowed) {
+      console.warn(`${protocol} is not allowed for anchor targets.`); // eslint-disable-line no-console
+      return "";
+    }
+    return url;
+  }
+
+  render() {
+    const { url, className, onLinkClick } = this.props;
+    return external_React_default.a.createElement(
+      "a",
+      { href: this.safeURI(url), className: className, onClick: onLinkClick },
+      this.props.children
+    );
+  }
+}
 // EXTERNAL MODULE: ./content-src/components/DiscoveryStreamComponents/SpocIntersectionObserver/SpocIntersectionObserver.jsx
 var SpocIntersectionObserver = __webpack_require__(29);
 
 // CONCATENATED MODULE: ./content-src/components/DiscoveryStreamComponents/DSCard/DSCard.jsx
 
 
 
 
+
 class DSCard_DSCard extends external_React_default.a.PureComponent {
   constructor(props) {
     super(props);
 
     this.onLinkClick = this.onLinkClick.bind(this);
   }
 
   onLinkClick(event) {
@@ -7145,18 +7175,18 @@ class DSCard_DSCard extends external_Rea
     }
   }
 
   render() {
     return external_React_default.a.createElement(
       SpocIntersectionObserver["SpocIntersectionObserver"],
       { campaignId: this.props.campaignId, dispatch: this.props.dispatch },
       external_React_default.a.createElement(
-        "a",
-        { href: this.props.url, className: "ds-card", onClick: this.onLinkClick },
+        SafeAnchor_SafeAnchor,
+        { url: this.props.url, className: "ds-card", onLinkClick: this.onLinkClick },
         external_React_default.a.createElement(
           "div",
           { className: "img-wrapper" },
           external_React_default.a.createElement("div", { className: "img", style: { backgroundImage: `url(${this.props.image_src}` } })
         ),
         external_React_default.a.createElement(
           "div",
           { className: "meta" },
@@ -7297,16 +7327,17 @@ class DSMessage_DSMessage extends extern
   }
 }
 // CONCATENATED MODULE: ./content-src/components/DiscoveryStreamComponents/List/List.jsx
 
 
 
 
 
+
 /**
  * @note exported for testing only
  */
 class List_ListItem extends external_React_default.a.PureComponent {
   // TODO performance: get feeds to send appropriately sized images rather
   // than waiting longer and scaling down on client?
   constructor(props) {
     super(props);
@@ -7332,18 +7363,18 @@ class List_ListItem extends external_Rea
   render() {
     return external_React_default.a.createElement(
       "li",
       { className: "ds-list-item" },
       external_React_default.a.createElement(
         SpocIntersectionObserver["SpocIntersectionObserver"],
         { campaignId: this.props.campaignId, dispatch: this.props.dispatch },
         external_React_default.a.createElement(
-          "a",
-          { className: "ds-list-item-link", href: this.props.url, onClick: this.onLinkClick },
+          SafeAnchor_SafeAnchor,
+          { url: this.props.url, className: "ds-list-item-link", onLinkClick: this.onLinkClick },
           external_React_default.a.createElement(
             "div",
             { className: "ds-list-item-text" },
             external_React_default.a.createElement(
               "div",
               { className: "ds-list-item-title" },
               this.props.title
             ),
@@ -7428,16 +7459,17 @@ function _List(props) {
 
 const List = Object(external_ReactRedux_["connect"])(state => ({ DiscoveryStream: state.DiscoveryStream }))(_List);
 // CONCATENATED MODULE: ./content-src/components/DiscoveryStreamComponents/Hero/Hero.jsx
 
 
 
 
 
+
 class Hero_Hero extends external_React_default.a.PureComponent {
   constructor(props) {
     super(props);
     this.onLinkClick = this.onLinkClick.bind(this);
   }
 
   onLinkClick(event) {
     if (this.props.dispatch) {
@@ -7495,18 +7527,18 @@ class Hero_Hero extends external_React_d
         "div",
         { className: "ds-header" },
         this.props.title
       ),
       external_React_default.a.createElement(
         "div",
         { className: `ds-hero ds-hero-${this.props.border}` },
         external_React_default.a.createElement(
-          "a",
-          { href: heroRec.url, className: "wrapper", onClick: this.onLinkClick },
+          SafeAnchor_SafeAnchor,
+          { url: heroRec.url, className: "wrapper", onLinkClick: this.onLinkClick },
           external_React_default.a.createElement(
             "div",
             { className: "img-wrapper" },
             external_React_default.a.createElement("div", { className: "img", style: { backgroundImage: `url(${heroRec.image_src})` } })
           ),
           external_React_default.a.createElement(
             "div",
             { className: "meta" },
new file mode 100644
--- /dev/null
+++ b/browser/components/newtab/test/unit/content-src/components/DiscoveryStreamComponents/Hero.test.jsx
@@ -0,0 +1,68 @@
+import {DSCard} from "content-src/components/DiscoveryStreamComponents/DSCard/DSCard";
+import {Hero} from "content-src/components/DiscoveryStreamComponents/Hero/Hero";
+import {List} from "content-src/components/DiscoveryStreamComponents/List/List";
+import React from "react";
+import {shallow} from "enzyme";
+
+describe("<Hero>", () => {
+  let DEFAULT_PROPS;
+  beforeEach(() => {
+    DEFAULT_PROPS = {
+      data: {
+        recommendations: [
+          {url: 1},
+          {url: 2},
+          {url: 3},
+        ],
+      },
+    };
+  });
+
+  it("should render with nothing", () => {
+    const wrapper = shallow(<Hero />);
+
+    assert.lengthOf(wrapper.find("a"), 0);
+  });
+
+  it("should render a hero link with expected url", () => {
+    const wrapper = shallow(<Hero {...DEFAULT_PROPS} />);
+
+    assert.equal(wrapper.find("SafeAnchor").prop("url"), DEFAULT_PROPS.data.recommendations[0].url);
+  });
+
+  describe("subComponent: cards", () => {
+    beforeEach(() => {
+      DEFAULT_PROPS.subComponentType = "cards";
+    });
+
+    it("should render no cards for 1 hero item", () => {
+      const wrapper = shallow(<Hero {...DEFAULT_PROPS} items={1} />);
+
+      assert.lengthOf(wrapper.find(DSCard), 0);
+    });
+
+    it("should render 1 card with expected url for 2 hero items", () => {
+      const wrapper = shallow(<Hero {...DEFAULT_PROPS} items={2} />);
+
+      assert.equal(wrapper.find(DSCard).prop("url"), DEFAULT_PROPS.data.recommendations[1].url);
+    });
+  });
+
+  describe("subComponent: list", () => {
+    beforeEach(() => {
+      DEFAULT_PROPS.subComponentType = "list";
+    });
+
+    it("should render list with no items for 1 hero item", () => {
+      const wrapper = shallow(<Hero {...DEFAULT_PROPS} items={1} />);
+
+      assert.equal(wrapper.find(List).prop("items"), 0);
+    });
+
+    it("should render list with 1 item for 2 hero items", () => {
+      const wrapper = shallow(<Hero {...DEFAULT_PROPS} items={2} />);
+
+      assert.equal(wrapper.find(List).prop("items"), 1);
+    });
+  });
+});
--- a/browser/components/newtab/test/unit/content-src/components/DiscoveryStreamComponents/List.test.jsx
+++ b/browser/components/newtab/test/unit/content-src/components/DiscoveryStreamComponents/List.test.jsx
@@ -96,17 +96,17 @@ describe("<ListItem> presentation compon
   afterEach(() => {
     globals.sandbox.restore();
   });
 
   it("should contain 'a.ds-list-item-link' with the props.url set", () => {
     const wrapper = shallow(<ListItem {...ValidListItemProps} />);
 
     const anchors = wrapper.find(
-      `a.ds-list-item-link[href="${ValidListItemProps.url}"]`);
+      `SafeAnchor.ds-list-item-link[url="${ValidListItemProps.url}"]`);
     assert.lengthOf(anchors, 1);
   });
 
   it("should include an background image of props.image_src", () => {
     const wrapper = shallow(<ListItem {...ValidListItemProps} />);
 
     const imageStyle = wrapper.find(".ds-list-image").prop("style");
     assert.propertyVal(imageStyle, "backgroundImage", `url(${ValidListItemProps.image_src})`);
new file mode 100644
--- /dev/null
+++ b/browser/components/newtab/test/unit/content-src/components/DiscoveryStreamComponents/SafeAnchor.test.jsx
@@ -0,0 +1,35 @@
+import React from "react";
+import {SafeAnchor} from "content-src/components/DiscoveryStreamComponents/SafeAnchor/SafeAnchor";
+import {shallow} from "enzyme";
+
+describe("Discovery Stream <SafeAnchor>", () => {
+  let warnStub;
+  beforeEach(() => {
+    warnStub = sinon.stub(console, "warn");
+  });
+  afterEach(() => {
+    warnStub.restore();
+  });
+  it("should render with anchor", () => {
+    const wrapper = shallow(<SafeAnchor />);
+    assert.lengthOf(wrapper.find("a"), 1);
+  });
+  it("should render with anchor target for http", () => {
+    const wrapper = shallow(<SafeAnchor url="http://example.com" />);
+    assert.equal(wrapper.find("a").prop("href"), "http://example.com");
+  });
+  it("should render with anchor target for https", () => {
+    const wrapper = shallow(<SafeAnchor url="https://example.com" />);
+    assert.equal(wrapper.find("a").prop("href"), "https://example.com");
+  });
+  it("should not allow javascript: URIs", () => {
+    const wrapper = shallow(<SafeAnchor url="javascript:foo()" />); // eslint-disable-line no-script-url
+    assert.equal(wrapper.find("a").prop("href"), "");
+    assert.calledOnce(warnStub);
+  });
+  it("should not warn if the URL is falsey ", () => {
+    const wrapper = shallow(<SafeAnchor url="" />);
+    assert.equal(wrapper.find("a").prop("href"), "");
+    assert.notCalled(warnStub);
+  });
+});