don't render invalid clip image urls

server/src/pages/shot/view.js

@@ -7,6 +7,7 @@ const { ShareButton } = require("../../share-buttons");
 const { TimeDiff } = require("./time-diff");
 const reactruntime = require("../../reactruntime");
 const { Editor } = require("./editor");
+const { isValidClipImageUrl } = require("../../../../shared/shot");
 
 class Clip extends React.Component {
   constructor(props) {
@@ -36,6 +37,9 @@ class Clip extends React.Component {
       console.warn("Somehow there's a shot without an image");
       return null;
     }
+    if (!isValidClipImageUrl(clip.image.url)) {
+      return null;
+    }
     let node = <img id="clipImage" style={{height: "auto", width: clip.image.dimensions.x + "px", maxWidth: "100%" }} ref={clipImage => this.clipImage = clipImage} src={ clip.image.url } alt={ clip.image.text } />;
     return <div ref={clipContainer => this.clipContainer = clipContainer} className="clip-container">
       { this.copyTextContextMenu() }
@@ -139,6 +143,9 @@ class Head extends React.Component {
   }
 
   makeEmbeddedImageUrl(url, type) {
+    if (!isValidClipImageUrl(url)) {
+      return "";
+    }
     if (!url.startsWith("http")) {
       return url;
     }
@@ -362,6 +369,9 @@ class Body extends React.Component {
       let clipId = clipNames[0];
       clip = this.props.shot.getClip(clipId);
       clipUrl = clip.image.url;
+      if (!isValidClipImageUrl(clipUrl)) {
+        clipUrl = "";
+      }
     }
 
     let renderGetFirefox = this.props.userAgent && (this.props.userAgent + "").search(/firefox\/\d{1,255}/i) === -1;

server/src/pages/shotindex/view.js

@@ -6,6 +6,7 @@ const React = require("react");
 const { ShareButton } = require("../../share-buttons");
 const Masonry = require("react-masonry-component");
 const { Localized } = require("fluent-react/compat");
+const { isValidClipImageUrl } = require("../../../shared/shot");
 
 class Head extends React.Component {
 
@@ -221,6 +222,7 @@ class Card extends React.Component {
   }
 
   render() {
+    const defaultImageUrl = this.props.staticLink("img/question-mark.svg");
     let shot = this.props.shot;
     let downloadUrl = this.props.downloadUrl;
     let imageUrl;
@@ -236,8 +238,14 @@ class Card extends React.Component {
     } else if (shot.fullScreenThumbnail) {
       imageUrl = shot.fullScreenThumbnail;
     } else {
+      imageUrl = defaultImageUrl;
+    }
+
+    // fallback to the question mark if the imageUrl is invalid
+    if (!isValidClipImageUrl(imageUrl)) {
       imageUrl = this.props.staticLink("img/question-mark.svg");
     }
+
     let favicon = null;
     if (shot.favicon) {
       // We use background-image so if the image is broken it just doesn't show:
@@ -248,7 +256,7 @@ class Card extends React.Component {
       <div className={`shot ${this.getClipType(clip._image.dimensions)} ${this.state.panelOpen} ${this.isDeleted()}`} key={shot.id}>
         <a href={shot.viewUrl} onClick={this.onOpen.bind(this, shot.viewUrl)}>
           <div className="shot-image-container" style={{
-            backgroundImage: `url(${imageUrl})`
+            backgroundImage: `url("${imageUrl}")`
           }}>
           </div>
           <div className="shot-info">

server/src/proxy-url.js

@@ -1,5 +1,6 @@
 const dbschema = require("./dbschema");
 const config = require("./config").getProperties();
+const { isValidClipImageUrl } = require("../../shared/shot");
 
 exports.createProxyUrl = function(req, url, hash) {
   let base = `${req.protocol}://${config.contentOrigin}`;
@@ -13,5 +14,8 @@ exports.createProxyUrl = function(req, url, hash) {
 
 exports.createDownloadUrl = function(url, filename) {
   const sig = dbschema.getKeygrip().sign(new Buffer(filename, 'utf8'));
+  if (!isValidClipImageUrl(url)) {
+      return "";
+  }
   return `${url}?download=${encodeURIComponent(filename)}&sig=${encodeURIComponent(sig)}`;
 };

shared/shot.js

@@ -32,6 +32,10 @@ function isUrl(url) {
   return (/^https?:\/\/[a-z0-9.-]{1,8000}[a-z0-9](:[0-9]{1,8000})?\/?/i).test(url);
 }
 
+function isValidClipImageUrl(url) {
+    return isUrl(url) && !(url.indexOf(')') > -1);
+}
+
 function assertUrl(url) {
   if (!url) {
     throw new Error("Empty value is not URL");
@@ -743,4 +747,5 @@ AbstractShot.prototype.Clip = _Clip;
 if (typeof exports != "undefined") {
   exports.AbstractShot = AbstractShot;
   exports.originFromUrl = originFromUrl;
+  exports.isValidClipImageUrl = isValidClipImageUrl;
 }

test/server/test_shotindex_image_url.py

@@ -0,0 +1,40 @@
+from urlparse import urljoin
+from clientlib import (
+    make_example_shot,
+    make_random_id,
+    screenshots_session,
+)
+import random
+
+
+# Hack to make this predictable:
+random.seed(0)
+
+
+def test_shotindex_image_url():  # bug 1389705
+    with screenshots_session() as user:
+        shot_id = make_random_id() + "/test.com"
+        shot_url = urljoin(user.backend, shot_id)
+        shot_data = urljoin(user.backend, "data/" + shot_id)
+
+        shot_json = make_example_shot(user.deviceId)
+        test_url = "https://example.com/?aaA=bbb=\"); background-color: red;"
+        for clip_id in shot_json['clips']:
+            shot_json['clips'][clip_id]['image']['url'] = test_url
+            break
+
+        resp = user.session.put(
+            shot_data,
+            json=shot_json,
+        )
+        resp.raise_for_status()
+
+        shot_page = user.read_shot(urljoin(user.backend, '/' + shot_id))['page']
+        model_json_start = shot_page.find("<script id=\"json-data\" type=\"data\">")
+        assert '); background-color: red' not in shot_page[:model_json_start]
+
+        user.delete_shot(shot_url)
+
+
+if __name__ == "__main__":
+    test_shotindex_image_url()