Merge pull request #2533 from hypothesis/9sqXWtNi-fix-title-for-local-file-pdf-annotations

Fix document title and domain for local file PDF annotations

h/static/scripts/app.coffee

@@ -152,6 +152,8 @@ module.exports = angular.module('h', [
 .filter('moment', require('./filter/moment'))
 .filter('persona', require('./filter/persona'))
 .filter('urlencode', require('./filter/urlencode'))
+.filter('documentTitle', require('./filter/document-title'))
+.filter('documentDomain', require('./filter/document-domain'))
 
 .provider('identity', require('./identity'))
 

h/static/scripts/directive/test/annotation-test.coffee

@@ -18,6 +18,8 @@ describe 'annotation', ->
   fakeMomentFilter = null
   fakePermissions = null
   fakePersonaFilter = null
+  fakeDocumentTitleFilter = null
+  fakeDocumentDomainFilter = null
   fakeSession = null
   fakeStore = null
   fakeTags = null
@@ -65,6 +67,8 @@ describe 'annotation', ->
       private: sandbox.stub().returns({read: ['justme']})
     }
     fakePersonaFilter = sandbox.stub().returnsArg(0)
+    fakeDocumentTitleFilter = (arg) -> ''
+    fakeDocumentDomainFilter = (arg) -> ''
 
     fakeSession =
       state:
@@ -92,6 +96,8 @@ describe 'annotation', ->
     $provide.value 'momentFilter', fakeMomentFilter
     $provide.value 'permissions', fakePermissions
     $provide.value 'personaFilter', fakePersonaFilter
+    $provide.value('documentTitleFilter', fakeDocumentTitleFilter)
+    $provide.value('documentDomainFilter', fakeDocumentDomainFilter)
     $provide.value 'session', fakeSession
     $provide.value 'store', fakeStore
     $provide.value 'tags', fakeTags
@@ -564,7 +570,8 @@ describe("AnnotationController", ->
   createAnnotationDirective = ({annotation, personaFilter, momentFilter,
                                 urlencodeFilter, drafts, flash,
                                 permissions, session, tags, time, annotationUI,
-                                annotationMapper, groups}) ->
+                                annotationMapper, groups,
+                                documentTitleFilter, documentDomainFilter}) ->
     locals = {
       personaFilter: personaFilter or ->
       momentFilter: momentFilter or {}
@@ -594,6 +601,8 @@ describe("AnnotationController", ->
         get: ->
         focused: -> {}
       }
+      documentTitleFilter: documentTitleFilter or -> ''
+      documentDomainFilter: documentDomainFilter or -> ''
     }
     module(($provide) ->
       $provide.value("personaFilter", locals.personaFilter)
@@ -608,6 +617,8 @@ describe("AnnotationController", ->
       $provide.value("annotationUI", locals.annotationUI)
       $provide.value("annotationMapper", locals.annotationMapper)
       $provide.value("groups", locals.groups)
+      $provide.value("documentTitleFilter", locals.documentTitleFilter)
+      $provide.value("documentDomainFilter", locals.documentDomainFilter)
       return
     )
 

h/static/scripts/filter/document-domain.js

+'use strict';
+
+var escapeHtml = require('escape-html');
+
+module.exports = function() {
+  /**
+   * Return a nice displayable string representation of a document's domain.
+   *
+   * @returns {String} The document's domain in braces, e.g. '(example.com)'.
+   *   Returns '' if the document has no domain or if the document's domain is
+   *   the same as its title (because we assume that the title is already
+   *   displayed elsewhere and displaying it twice would be redundant).
+   *
+   */
+  function documentDomainFilter(document) {
+    var domain = escapeHtml(document.domain || '');
+    var title = escapeHtml(document.title || '');
+
+    if (domain && domain !== title) {
+      return '(' + domain + ')';
+    } else {
+      return '';
+    }
+  }
+  return documentDomainFilter;
+};

h/static/scripts/filter/document-title.js

+'use strict';
+
+var escapeHtml  = require('escape-html');
+
+module.exports = function() {
+  /**
+   * Return a nice displayable string representation of a document's title.
+   *
+   * @returns {String} The document's title preceded on "on " and hyperlinked
+   *   to the document's URI. If the document has no http(s) URI then don't
+   *   hyperlink the title. If the document has no title then return ''.
+   *
+   */
+  function documentTitleFilter(document) {
+    var title = escapeHtml(document.title || '');
+    var uri = escapeHtml(document.uri || '');
+
+    if (uri && !(uri.startsWith('http://') || uri.startsWith('https://'))) {
+      // We only link to http(s) URLs.
+      uri = null;
+    }
+
+    if (title && uri) {
+      return ('on &ldquo;<a target="_blank" href="' + uri + '">' + title +
+              '</a>&rdquo;');
+    } else if (title) {
+      return 'on &ldquo;' + title + '&rdquo;';
+    } else {
+      return '';
+    }
+  }
+  return documentTitleFilter;
+};

h/static/scripts/filter/test/document-domain-test.js

+'use strict';
+
+var documentDomainFilterProvider = require('../document-domain');
+
+describe('documentDomain', function() {
+
+  it('returns the domain in braces', function() {
+    var domain = documentDomainFilterProvider()({
+      domain: 'example.com'
+    });
+
+    assert(domain === '(example.com)');
+  });
+
+  it('returns an empty string if domain and title are the same', function() {
+    var domain = documentDomainFilterProvider()({
+      domain: 'example.com',
+      title: 'example.com'
+    });
+
+    assert(domain === '');
+  });
+
+  it('returns an empty string if the document has no domain', function() {
+    var domain = documentDomainFilterProvider()({
+      title: 'example.com'
+    });
+
+    assert(domain === '');
+  });
+
+  it('escapes HTML in the document domain', function() {
+    var spamLink = '<a href="http://example.com/rubies">Buy rubies!!!</a>';
+
+    var domain = documentDomainFilterProvider()({
+      title: 'title',
+      domain: '</a>' + spamLink
+    });
+
+    assert(domain.indexOf(spamLink) === -1);
+  });
+});

h/static/scripts/filter/test/document-title-test.js

+'use strict';
+
+var documentTitleFilterProvider = require('../document-title');
+
+describe('documentTitle', function() {
+
+  it('returns the title linked if the document has title and uri', function() {
+    var title = documentTitleFilterProvider()({
+      title: 'title',
+      uri: 'http://example.com/example.html'
+    });
+
+    assert(title === 'on &ldquo;<a target="_blank" ' +
+                     'href="http://example.com/example.html">' +
+                     'title</a>&rdquo;');
+  });
+
+  it('returns the title linked if the document has an https uri', function() {
+    var title = documentTitleFilterProvider()({
+      title: 'title',
+      uri: 'https://example.com/example.html'
+    });
+
+    assert(title === 'on &ldquo;<a target="_blank" '+
+                     'href="https://example.com/example.html">' +
+                     'title</a>&rdquo;');
+  });
+
+  it('returns the title unlinked if doc has title but no uri', function() {
+    var title = documentTitleFilterProvider()({
+      title: 'title',
+    });
+
+    assert(title === 'on &ldquo;title&rdquo;');
+  });
+
+  it('returns the title unlinked if doc has non-http uri', function() {
+    var title = documentTitleFilterProvider()({
+      title: 'title',
+      uri: 'file:///home/bob/Documents/example.pdf'
+    });
+
+    assert(title === 'on &ldquo;title&rdquo;');
+  });
+
+  it('returns the title linked if the document has title and uri', function() {
+    var title = documentTitleFilterProvider()({
+      title: 'title',
+      uri: 'http://example.com/example.html'
+    });
+
+    assert(title === 'on &ldquo;<a target="_blank" ' +
+                     'href="http://example.com/example.html">' +
+                     'title</a>&rdquo;');
+  });
+
+  it('returns an empty string if the document has no title', function() {
+    var title = documentTitleFilterProvider()({
+      uri: 'http://example.com/example.html'
+    });
+
+    assert(title === '');
+  });
+
+  it('escapes HTML in the document title', function() {
+    var spamLink = '<a href="http://example.com/rubies">Buy rubies!!!</a>';
+
+    var title = documentTitleFilterProvider()({
+      title: '</a>' + spamLink,
+      uri: 'http://example.com/example.html'
+    });
+
+    assert(title.indexOf(spamLink) === -1);
+  });
+
+  it('escapes HTML in the document URI', function() {
+    var spamLink = '<a href="http://example.com/rubies">Buy rubies!!!</a>';
+
+    var title = documentTitleFilterProvider()({
+      uri: 'http://</a>' + spamLink,
+      title: 'title'
+    });
+
+    assert(title.indexOf(spamLink) === -1);
+  });
+});

h/templates/client/annotation.html

@@ -25,15 +25,16 @@
             title="This annotation is visible only to you.">
         <i class="h-icon-lock"></i> Only Me
       </span>
+
       <span class="annotation-citation"
-            ng-if="!vm.embedded"
-            ng-show="vm.document.title">
-        on &ldquo;<a href="{{vm.document.uri}}" target="_blank"
-                     >{{vm.document.title}}</a>&rdquo;
+            ng-bind-html="vm.document | documentTitle"
+            ng-if="!vm.embedded">
+      </span>
       <span class="annotation-citation-domain"
-              ng-show="vm.document.domain != vm.document.title"
-              >({{vm.document.domain}})</span>
+            ng-bind-html="vm.document | documentDomain"
+            ng-if="!vm.embedded">
       </span>
+
       <!-- Editing controls -->
       <aside class="pull-right" ng-if="vm.editing">
         <privacy ng-click="$event.stopPropagation()"

package.json

@@ -29,6 +29,7 @@
     "dom-anchor-text-position": "^2.0.0",
     "dom-anchor-text-quote": "^2.0.0",
     "dom-seek": "^1.0.1",
+    "escape-html": "^1.0.3",
     "extend": "^2.0.0",
     "frame-rpc": "^1.3.1",
     "hammerjs": "^2.0.4",