Improve URL Auto-Linking In Messages (#2240)

As a user, I’d like the app to autolink as many possible URL formats I write in messages as possible, e.g. - [x] URLs without protocol: `github.com` - [x] URLs with in different languages (Unicode): - [x] `https://zh.wikipedia.org/zh-hans/信号` - [x] `https://ru.wikipedia.org/wiki/Сигнал` - [x] URLs with single quotes: `https://www.example.com/this-couldn't-be-true` - [x] Messages with URLs right after special characters: `wink ;)https://www.youtube.com/watch?v=oHg5SJYRHA0` - [x] URLs with square brackets: `https://www.example.com/test.html?foo=bar&baz[qux]=quux` - [x] **Infrastructure:** Include TypeScript files in build. - [x] **Infrastructure:** Rename `ts/test` to `ts/styleguide`. - [x] **Infrastructure:** Decouple linting from testing. - [x] **Infrastructure:** Run all tests in CI. - [x] **Infrastructure:** Compile TypeScript on CI. ### Dependencies - Forked `link-text` to disable HTML escaping: It only has the minimum required dependencies: - `linkify-it`: Best-in-class link detection library with support for Unicode/IDN. Popular alternative: `linkifyjs`. Doesn’t handle Unicode in URLs. - ~~`escape-html`: Standalone dependency for escaping HTML.~~ - `uc.micro`: Standalone dependency of Unicode data files. ### Known Issues We don’t auto-link trailing exclamation points which in most cases would be expected to be part of the message body rather than the link. **Counterexample:** `https://en.wikipedia.org/wiki/Mother!`. N.B. GitHub doesn’t do this right either. Fixes #598.
2018-04-11 17:31:16 -04:00 · 2018-04-11 17:31:16 -04:00 · 3a05201501
commit 3a05201501
parent 9b22e0667a 55fc21505e
19 changed files with 228 additions and 28 deletions
--- a/.travis.yml
+++ b/.travis.yml
@ -7,12 +7,12 @@ dist: trusty
 install:
  - yarn install --frozen-lockfile
 script:
-  - yarn nsp check
-  - yarn run generate
-  - yarn prepare-beta-build
-  - yarn eslint
-  - yarn test-server
+  - yarn transpile
  - yarn lint
+  - yarn test-node
+  - yarn nsp check
+  - yarn generate
+  - yarn prepare-beta-build
  - $(yarn bin)/build --config.extraMetadata.environment=$SIGNAL_ENV --config.mac.bundleVersion='$TRAVIS_BUILD_NUMBER' --publish=never
  - ./travis.sh
 env:
--- a/Gruntfile.js
+++ b/Gruntfile.js
@ -480,7 +480,8 @@ module.exports = function(grunt) {

  grunt.registerTask('tx', ['exec:tx-pull', 'locale-patch']);
  grunt.registerTask('dev', ['default', 'watch']);
-  grunt.registerTask('test', ['jshint', 'jscs', 'unit-tests', 'lib-unit-tests']);
+  grunt.registerTask('lint', ['jshint', 'jscs']);
+  grunt.registerTask('test', ['unit-tests', 'lib-unit-tests']);
  grunt.registerTask('copy_dist', ['gitinfo', 'copy:res', 'copy:src']);
  grunt.registerTask('date', ['gitinfo', 'getExpireTime']);
  grunt.registerTask('prep-release', ['gitinfo', 'clean-release', 'fetch-release']);
--- a/appveyor.yml
+++ b/appveyor.yml
@ -12,11 +12,11 @@ install:
  - yarn install --frozen-lockfile

 build_script:
-  - yarn nsp check
-  - yarn eslint
-  - yarn test-server
+  - yarn transpile
  - yarn lint
-  - yarn run icon-gen
+  - yarn test-node
+  - yarn nsp check
+  - yarn generate
  - node build\grunt.js
  - type package.json | findstr /v certificateSubjectName > temp.json
  - move temp.json package.json
--- a/js/modules/link_text.d.ts
+++ b/js/modules/link_text.d.ts
@ -0,0 +1,9 @@
+declare namespace LinkText {
+  type Attributes = {
+    [key: string]: string;
+  }
+}
+
+declare function linkText(value: string, attributes: LinkText.Attributes): string;
+
+export = linkText;
--- a/js/modules/link_text.js
+++ b/js/modules/link_text.js
@ -0,0 +1,39 @@
+// Fork of https://github.com/uiureo/link-text with HTML escaping disabled as we leverage
+// jQuery’s escaping mechanism:
+
+const linkify = require('linkify-it')();
+
+function createLink(url, text, attrs = {}) {
+  const html = [];
+  html.push('<a ');
+  html.push(`href="${url}"`);
+  Object.keys(attrs).forEach((key) => {
+    html.push(` ${key}="${attrs[key]}"`);
+  });
+  html.push('>');
+  html.push(decodeURIComponent(text));
+  html.push('</a>');
+
+  return html.join('');
+}
+
+module.exports = (text, attrs = {}) => {
+  const matchData = linkify.match(text) || [];
+
+  const result = [];
+  let last = 0;
+
+  matchData.forEach((match) => {
+    if (last < match.index) {
+      result.push(text.slice(last, match.index));
+    }
+
+    result.push(createLink(match.url, match.text, attrs));
+
+    last = match.lastIndex;
+  });
+
+  result.push(text.slice(last));
+
+  return result.join('');
+};
--- a/js/views/message_view.js
+++ b/js/views/message_view.js
@ -6,6 +6,7 @@
    'use strict';
    window.Whisper = window.Whisper || {};

+    const { HTML } = window.Signal;
    const { Attachment } = window.Signal.Types;
    const { loadAttachmentData } = window.Signal.Migrations;

@ -375,8 +376,8 @@
            emoji_util.parse(body);

            if (body.length > 0) {
-                var escaped = body.html();
-                body.html(escaped.replace(/\n/g, '<br>').replace(URL_REGEX, "$1<a href='$2' target='_blank'>$2</a>"));
+                const escapedBody = body.html();
+                body.html(HTML.render(escapedBody));
            }

            this.renderSent();
--- a/package.json
+++ b/package.json
@ -12,8 +12,6 @@
  "main": "main.js",
  "scripts": {
    "postinstall": "electron-builder install-app-deps && rimraf node_modules/dtrace-provider",
-    "test": "yarn eslint && yarn tslint && yarn test-server && grunt test && yarn test-app && yarn test-modules",
-    "lint": "grunt jshint",
    "start": "electron .",
    "asarl": "asar l release/mac/Signal.app/Contents/Resources/app.asar",
    "icon-gen": "electron-icon-maker --input=images/icon_1024.png --output=./build",
@ -36,12 +34,15 @@
    "release-win": "npm run build-release -- -w --prepackaged release/windows --publish=always",
    "release-lin": "npm run build-release -- -l --prepackaged release/linux && NAME=$npm_package_name VERSION=$npm_package_version ./aptly.sh",
    "release": "npm run release-mac && npm run release-win && npm run release-lin",
-    "test-app": "mocha --recursive test/app",
-    "test-modules": "mocha --recursive test/modules",
-    "test-server": "mocha --recursive test/server",
-    "test-server-coverage": "nyc --reporter=lcov --reporter=text mocha --recursive test/server",
+    "test": "yarn test-node && yarn test-electron",
+    "test-electron": "yarn grunt test",
+    "test-node": "mocha --recursive test/app test/modules ts/test",
+    "test-node-coverage": "nyc --reporter=lcov --reporter=text mocha --recursive test/app test/modules ts/test",
    "eslint": "eslint .",
-    "tslint": "tslint ts/**/*.{ts,tsx}",
+    "jscs": "yarn grunt jscs",
+    "jshint": "yarn grunt jshint",
+    "lint": "yarn eslint && yarn grunt lint && yarn tslint",
+    "tslint": "tslint --config tslint.json --project . --format stylish",
    "transpile": "tsc",
    "clean-transpile": "rimraf ts/**/*.js ts/*.js",
    "open-coverage": "open coverage/lcov-report/index.html",
@ -71,6 +72,7 @@
    "fs-extra": "^5.0.0",
    "google-libphonenumber": "^3.0.7",
    "got": "^8.2.0",
+    "linkify-it": "^2.0.3",
    "lodash": "^4.17.4",
    "mkdirp": "^0.5.1",
    "moment": "^2.21.0",
@ -91,6 +93,9 @@
    "websocket": "^1.0.25"
  },
  "devDependencies": {
+    "@types/chai": "^4.1.2",
+    "@types/lodash": "^4.14.106",
+    "@types/mocha": "^5.0.0",
    "@types/qs": "^6.5.1",
    "@types/react": "^16.3.1",
    "@types/react-dom": "^16.0.4",
@ -223,6 +228,8 @@
      "_locales/**",
      "protos/*",
      "js/**",
+      "ts/**/*.js",
+      "ts/*.js",
      "stylesheets/*.css",
      "!js/register.js",
      "!js/views/standalone_registration_view.js",
--- a/preload.js
+++ b/preload.js
@ -114,7 +114,7 @@ window.ProxyAgent = require('proxy-agent');
 //   two locations:
 //
 //   1) test/styleguide/legacy_bridge.js
-//   2) ts/test/StyleGuideUtil.js
+//   2) ts/styleguide/StyleGuideUtil.js

 window.React = require('react');
 window.ReactDOM = require('react-dom');
@ -158,6 +158,7 @@ window.Signal.Backup = require('./js/modules/backup');
 window.Signal.Crypto = require('./js/modules/crypto');
 window.Signal.Database = require('./js/modules/database');
 window.Signal.Debug = require('./js/modules/debug');
+window.Signal.HTML = require('./ts/html');
 window.Signal.Logs = require('./js/modules/logs');

 window.Signal.Components = {};
--- a/styleguide.config.js
+++ b/styleguide.config.js
@ -20,12 +20,12 @@ module.exports = {
    {
      name: 'Test',
      description: 'Components only used for testing',
-      components: 'ts/test/**/*.tsx',
+      components: 'ts/styleguide/**/*.tsx',
    },
  ],
  context: {
    // Exposes necessary utilities in the global scope for all readme code snippets
-    util: 'ts/test/StyleGuideUtil',
+    util: 'ts/styleguide/StyleGuideUtil',
  },
  // We don't want one long, single page
  pagePerSection: true,
--- a/test/server/app/logging_test.js
+++ b/test/server/app/logging_test.js
@ -13,7 +13,7 @@ const {
  isLineAfterDate,
  fetchLog,
  fetch,
-} = require('../../../app/logging');
+} = require('../../app/logging');

 describe('app/logging', () => {
  let basePath;
--- a/test/modules/debuglogs_test.js
+++ b/test/modules/debuglogs_test.js
@ -12,6 +12,6 @@ describe('debuglogs', () => {

      const { body } = await got.get(url);
      assert.equal(nonce, body);
-    });
+    }).timeout(3000);
  });
 });
--- a/travis.sh
+++ b/travis.sh
@ -8,6 +8,6 @@ if [[ "$TRAVIS_OS_NAME" == "linux" ]]; then
  sleep 3
 fi

-grunt test
+yarn test-electron

-NODE_ENV=production grunt test-release:$TRAVIS_OS_NAME
+NODE_ENV=production yarn grunt test-release:$TRAVIS_OS_NAME
--- a/ts/html/index.ts
+++ b/ts/html/index.ts
@ -0,0 +1,12 @@
+import linkTextInternal from '../../js/modules/link_text';
+
+
+export const linkText = (value: string): string =>
+  linkTextInternal(value, { target: '_blank' });
+
+export const replaceLineBreaks = (value: string): string =>
+  value.replace(/\r?\n/g, '<br>');
+
+// NOTE: How can we use `lodash/fp` `compose` with type checking?
+export const render = (value: string): string =>
+  replaceLineBreaks(linkText(value));
--- a/ts/styleguide/ConversationContext.md
+++ b/ts/styleguide/ConversationContext.md
--- a/ts/styleguide/ConversationContext.tsx
+++ b/ts/styleguide/ConversationContext.tsx
--- a/ts/styleguide/StyleGuideUtil.ts
+++ b/ts/styleguide/StyleGuideUtil.ts
--- a/ts/test/html/index_test.ts
+++ b/ts/test/html/index_test.ts
@ -0,0 +1,96 @@
+import 'mocha';
+import { assert } from 'chai';
+
+import * as HTML from '../../html';
+
+interface Test {
+  input: string;
+  name: string;
+  output?: string;
+  outputHref?: string;
+  outputLabel?: string;
+  postText?: string;
+  preText?: string;
+  skipped?: boolean;
+}
+
+describe('HTML', () => {
+  describe('linkText', () => {
+    const TESTS: Array<Test> = [
+      {
+        name: 'square brackets',
+        input: 'https://www.example.com/test.html?foo=bar&baz[qux]=quux',
+        output: 'https://www.example.com/test.html?foo=bar&baz[qux]=quux',
+      },
+      {
+        name: 'Chinese characters',
+        input: 'https://zh.wikipedia.org/zh-hans/信号',
+        output: 'https://zh.wikipedia.org/zh-hans/信号',
+      },
+      {
+        name: 'Cyrillic characters',
+        input: 'https://ru.wikipedia.org/wiki/Сигнал',
+        output: 'https://ru.wikipedia.org/wiki/Сигнал',
+      },
+      {
+        skipped: true,
+        name: 'trailing exclamation points',
+        input: 'https://en.wikipedia.org/wiki/Mother!',
+        output: 'https://en.wikipedia.org/wiki/Mother!',
+      },
+      {
+        name: 'single quotes',
+        input: "https://www.example.com/this-couldn't-be-true",
+        output: "https://www.example.com/this-couldn't-be-true",
+      },
+      {
+        name: 'special characters before URL begins',
+        preText: 'wink ;)',
+        input: 'https://www.youtube.com/watch?v=oHg5SJYRHA0',
+        output: 'https://www.youtube.com/watch?v=oHg5SJYRHA0',
+      },
+      {
+        name: 'URLs without protocols',
+        input: 'github.com',
+        outputHref: 'http://github.com',
+        outputLabel: 'github.com',
+      },
+    ];
+
+    TESTS.forEach((test) => {
+      (test.skipped ? it.skip : it)(`should handle ${test.name}`, () => {
+        const preText = test.preText || 'Hello ';
+        const postText = test.postText || ' World!';
+        const input: string = `${preText}${test.input}${postText}`;
+        const expected: string = [
+          preText,
+          `<a href="${test.outputHref || test.output}" target="_blank">`,
+          test.outputLabel || test.output,
+          '</a>',
+          postText,
+        ].join('');
+
+        const actual = HTML.linkText(input);
+        assert.equal(actual, expected);
+      });
+    });
+  });
+
+  describe('render', () => {
+    it('should preserve line breaks', () => {
+      const input: string = 'Hello\n\n\nWorld!';
+      const expected: string = 'Hello<br><br><br>World!';
+
+      const actual = HTML.render(input);
+      assert.equal(actual, expected);
+    });
+
+    it('should not escape HTML', () => {
+      const input: string = "Hello\n<script>alert('evil');</script>World!";
+      const expected: string = "Hello<br><script>alert('evil');</script>World!";
+
+      const actual = HTML.render(input);
+      assert.equal(actual, expected);
+    });
+  });
+});
--- a/tslint.json
+++ b/tslint.json
@ -6,9 +6,21 @@
  ],
  "jsRules": {},
  "rules": {
-    "quotemark": [true, "single", "jsx-double", "avoid-template", "avoid-escape"],
+    "array-type": [true, "generic"],
+    "interface-name": [true, "never-prefix"],
    "no-consecutive-blank-lines": [true, 2],
-    "interface-name": [true, "never-prefix"]
+    "object-literal-key-quotes": [true, "as-needed"],
+    "object-literal-sort-keys": false,
+
+    // Ignore import sources order until we can specify that we want ordering
+    // based on import name vs module name:
+    "ordered-imports": [true, {
+      "import-sources-order": "any",
+      "named-imports-order": "case-insensitive"
+    }],
+
+    "quotemark": [true, "single", "jsx-double", "avoid-template", "avoid-escape"]
+
  },
  "rulesDirectory": []
 }
--- a/yarn.lock
+++ b/yarn.lock
@ -36,6 +36,18 @@
  dependencies:
    samsam "1.3.0"

+"@types/chai@^4.1.2":
+  version "4.1.2"
+  resolved "https://registry.yarnpkg.com/@types/chai/-/chai-4.1.2.tgz#f1af664769cfb50af805431c407425ed619daa21"
+
+"@types/lodash@^4.14.106":
+  version "4.14.106"
+  resolved "https://registry.yarnpkg.com/@types/lodash/-/lodash-4.14.106.tgz#6093e9a02aa567ddecfe9afadca89e53e5dce4dd"
+
+"@types/mocha@^5.0.0":
+  version "5.0.0"
+  resolved "https://registry.yarnpkg.com/@types/mocha/-/mocha-5.0.0.tgz#a3014921991066193f6c8e47290d4d598dfd19e6"
+
 "@types/node@*":
  version "9.6.1"
  resolved "https://registry.yarnpkg.com/@types/node/-/node-9.6.1.tgz#e2d374ef15b315b48e7efc308fa1a7cd51faa06c"
@ -5151,6 +5163,12 @@ lie@*:
  dependencies:
    immediate "~3.0.5"

+linkify-it@^2.0.3:
+  version "2.0.3"
+  resolved "https://registry.yarnpkg.com/linkify-it/-/linkify-it-2.0.3.tgz#d94a4648f9b1c179d64fa97291268bdb6ce9434f"
+  dependencies:
+    uc.micro "^1.0.1"
+
 listify@^1.0.0:
  version "1.0.0"
  resolved "https://registry.yarnpkg.com/listify/-/listify-1.0.0.tgz#03ca7ba2d150d4267773f74e57558d1053d2bee3"
@ -8937,6 +8955,10 @@ ua-parser-js@^0.7.9:
  version "0.7.17"
  resolved "https://registry.yarnpkg.com/ua-parser-js/-/ua-parser-js-0.7.17.tgz#e9ec5f9498b9ec910e7ae3ac626a805c4d09ecac"

+uc.micro@^1.0.1:
+  version "1.0.5"
+  resolved "https://registry.yarnpkg.com/uc.micro/-/uc.micro-1.0.5.tgz#0c65f15f815aa08b560a61ce8b4db7ffc3f45376"
+
 uglify-es@^3.3.4:
  version "3.3.9"
  resolved "https://registry.yarnpkg.com/uglify-es/-/uglify-es-3.3.9.tgz#0c1c4f0700bed8dbc124cdb304d2592ca203e677"