Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Topics get mis-unescaped when narrowing through a recipient bar #4338

Closed
gnprice opened this issue Dec 14, 2020 · 0 comments · Fixed by #4339
Closed

Topics get mis-unescaped when narrowing through a recipient bar #4338

gnprice opened this issue Dec 14, 2020 · 0 comments · Fixed by #4339
Assignees
@gnprice

Comments

@gnprice
Copy link
Member

gnprice commented Dec 14, 2020

Repro:

  • 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.

I discovered this while working on this code for #4333, and I have a fix which I'll send shortly.
@gnprice gnprice self-assigned this Dec 14, 2020
gnprice added a commit to gnprice/zulip-mobile that referenced this issue Dec 14, 2020
@gnprice
narrow: Stop mis-unescaping narrows from recipient bars.
de26b93 
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: zulip#4338
@gnprice gnprice mentioned this issue Dec 14, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gnprice gnprice

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

narrow: Encode our own structured keys instead of JSON. gnprice/zulip-mobile
1 participant
@gnprice