From bee997a100b0aaeb3c9b4da71871175a8e9e0530 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 2 Nov 2020 13:00:58 -0800 Subject: [PATCH] narrow: Stop mis-unescaping narrows from recipient bars. Here's a bug: * Send a message to a topic with a name like "this & that". * Look at it in the whole-stream view, or another interleaved view. The recipient header will show it nice and correctly: "this & that". * Tap the recipient header. * Expected: We narrow to topic "this & that". * Actual: We narrow to a different topic, "this & that". The cause of the bug is that we're trying to "unescape" a string that hasn't been "escaped" in the first place -- it's already simply the string we wanted, and "unescaping" can only corrupt it. Specifically: in `webViewEventHandlers.js` when we get the relevant outbound message-list event, it has the target narrow as a property, encoded as a string; we pass it to this helper `parseNarrowString`, and instead of simply decoding that string to a narrow, it calls `lodash.unescape` on it first. It looks like this bug has been here since the beginning of the webview-based message list. The location of it changed in commit b5f2e5353, in 2018-02: before then, the narrow was encoded wrong in the HTML in the first place. That commit fixed that problem (well, part of it -- see 994145383) but introduced this spurious unescaping. This helper has no other call sites which might be relying on its current behavior, so we can just fix it directly. There was a test that gestured in the direction of this kind of question... but it actually just exercised the broken behavior, because it wasn't paired with the use of keyFromNarrow. Its useful content is covered by the round-trip tests we just added, so delete it; then add a bunch more round-trip tests that specifically probe for this class of bug. Fixes: #4338 --- package.json | 1 - src/__tests__/lib/exampleData.js | 17 +++++++++++++++ src/utils/__tests__/narrow-test.js | 33 ++++++++++++++++++++++-------- src/utils/narrow.js | 3 +-- 4 files changed, 42 insertions(+), 12 deletions(-) diff --git a/package.json b/package.json index 97a00a5a1a3..096d6d3c60b 100644 --- a/package.json +++ b/package.json @@ -55,7 +55,6 @@ "lodash.isequal": "^4.4.0", "lodash.isplainobject": "^4.0.6", "lodash.omit": "^4.5.0", - "lodash.unescape": "^4.0.1", "lodash.union": "^4.6.0", "lodash.uniqby": "^4.4.0", "react": "16.11.0", diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index 5eb81dcce71..4439e0d8679 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -80,6 +80,23 @@ const makeUniqueRandInt = (itemsType: string, end: number): (() => number) => { /** Return a string that's almost surely different every time. */ export const randString = () => randInt(2 ** 54).toString(36); +const intRange = (start, len) => Array.from({ length: len }, (k, i) => i + start); + +/** A string with diverse characters to exercise encoding/decoding bugs. */ +/* eslint-disable prefer-template */ +export const diverseCharacters = + // The whole range of lowest code points, including control codes + // and ASCII punctuation like `"` and `&` used in various syntax... + String.fromCharCode(...intRange(0, 0x100)) + // ... some characters from other scripts... + + 'いい文字🎇' + // ... some unpaired surrogates, which JS strings can have... + + String.fromCharCode(...intRange(0xdbf0, 0x20)) + // ... some characters beyond the BMP (≥ U+10000)... + + '𐂷𠂢' + // ... and some code points at the very end of the Unicode range. + + String.fromCodePoint(...intRange(0x10fff0, 0x10)); + /* ======================================================================== * Users and bots */ diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js index ba65920b66d..05f34029074 100644 --- a/src/utils/__tests__/narrow-test.js +++ b/src/utils/__tests__/narrow-test.js @@ -420,15 +420,8 @@ describe('isSameNarrow', () => { }); }); -describe('parseNarrow', () => { - test('straightforward arrays are parsed', () => { - expect(parseNarrow('[]')).toEqual([]); - expect(parseNarrow('[{"operator":"hey"}]')).toEqual([{ operator: 'hey' }]); - }); -}); - describe('keyFromNarrow+parseNarrow', () => { - const narrows = [ + const baseNarrows = [ ['whole stream', streamNarrow(eg.stream.name)], ['stream conversation', topicNarrow(eg.stream.name, 'a topic')], ['1:1 PM conversation, non-self', pm1to1NarrowFromUser(eg.otherUser)], @@ -440,8 +433,30 @@ describe('keyFromNarrow+parseNarrow', () => { ['all-PMs', ALL_PRIVATE_NARROW], ['search narrow', SEARCH_NARROW('a query')], ]; + + // The only character not allowed in Zulip stream names is '\x00'. + // (See `check_stream_name` in zulip.git:zerver/lib/streams.py.) + // Try approximately everything else. + /* eslint-disable no-control-regex */ + const diverseCharacters = eg.diverseCharacters.replace(/\x00/g, ''); + const htmlEntities = 'h & t & &lquo;ml"'; + const awkwardNarrows = [ + ['whole stream about awkward characters', streamNarrow(diverseCharacters)], + ['whole stream about HTML entities', streamNarrow(htmlEntities)], + [ + 'stream conversation about awkward characters', + topicNarrow(diverseCharacters, `regarding ${diverseCharacters}`), + ], + [ + 'stream conversation about HTML entities', + topicNarrow(htmlEntities, `regarding ${htmlEntities}`), + ], + ['search narrow for awkward characters', SEARCH_NARROW(diverseCharacters)], + ['search narrow for HTML entities', SEARCH_NARROW(htmlEntities)], + ]; + describe('round-trips', () => { - for (const [description, narrow] of narrows) { + for (const [description, narrow] of [...baseNarrows, ...awkwardNarrows]) { test(description, () => { expect(parseNarrow(keyFromNarrow(narrow))).toEqual(narrow); }); diff --git a/src/utils/narrow.js b/src/utils/narrow.js index a3ab7ab13bd..d7be54294d0 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -1,6 +1,5 @@ /* @flow strict-local */ import isEqual from 'lodash.isequal'; -import unescape from 'lodash.unescape'; import type { Narrow, Message, Outbox, User, UserOrBot } from '../types'; import { @@ -18,7 +17,7 @@ export const isSameNarrow = (narrow1: Narrow, narrow2: Narrow): boolean => /** * Parse a narrow previously encoded with keyFromNarrow. */ -export const parseNarrow = (narrowStr: string): Narrow => JSON.parse(unescape(narrowStr)); +export const parseNarrow = (narrowStr: string): Narrow => JSON.parse(narrowStr); /** * The key we use for this narrow in our Redux data structures.