Skip to content

Commit

Permalink
narrow: Stop mis-unescaping narrows from recipient bars.
Browse files Browse the repository at this point in the history
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
b5f2e53, 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 9941453) 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
  • Loading branch information
gnprice committed Dec 15, 2020
1 parent 81feef4 commit bee997a
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 12 deletions.
1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
17 changes: 17 additions & 0 deletions src/__tests__/lib/exampleData.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down
33 changes: 24 additions & 9 deletions src/utils/__tests__/narrow-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)],
Expand All @@ -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);
});
Expand Down
3 changes: 1 addition & 2 deletions src/utils/narrow.js
Original file line number Diff line number Diff line change
@@ -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 {
Expand All @@ -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.
Expand Down

0 comments on commit bee997a

Please sign in to comment.